Add locationSiteKeyCache and use for siteUtil.isBookmarked #8894
Conversation
ayumi
force-pushed
the
fix/url-bar-is-bookmarked-cache
branch
from
916bb43 to
1584019
Compare
js/state/siteUtil.js
Outdated
|
|
||
| /** | ||
| * Checks if a siteDetail has the specified tag. | ||
| * O(n) warning; this iterates through sites. |
There was a problem hiding this comment.
I think this should either be removed or it should just access in O(1) to check via site key.
| const site = sites.get(key) | ||
| if (!site) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
some will still iterate over everything, I think it would be best to just make the cache related to bookmarks, and if the key is in there then return in O(1) if it is bookmarked.
There was a problem hiding this comment.
This should only iterate over a median of 1 or 2 keys.
We could have a location-to-bookmarked cache if we assume a location can be bookmarked only once.
app/browser/reducers/tabsReducer.js
Outdated
| } | ||
| const bookmarked = isLocationBookmarked(state, url) | ||
| tabValue = tabValue.set('bookmarked', bookmarked) | ||
| state = tabState.updateTabValue(state, tabValue) |
There was a problem hiding this comment.
I think this should all be removed and then just add the bookmarked property in getTabValue and it'll work better.
There was a problem hiding this comment.
bookmarking a page doesn't seem to trigger this function. how would that state change get reflected here?
There was a problem hiding this comment.
I think I'd handle appConstants.APP_ADD_SITE in the tabsReducer, the only problem is that doesn't tell you which tab to update. You could either find the tabs with that location or pass in the info with ADD_SITE, that might be better if you can fix all the callers.
Then you could just callupdateTab after getting the tab by tabId in tabs.js
ayumi
force-pushed
the
fix/url-bar-is-bookmarked-cache
branch
6 times, most recently
from
fecb5d3 to
ffe78cf
Compare
ayumi
force-pushed
the
fix/url-bar-is-bookmarked-cache
branch
from
ffe78cf to
980a855
Compare
| @@ -96,6 +96,7 @@ class NewTabPage extends React.Component { | |||
| }).size > 0 | |||
| } | |||
| isBookmarked (siteProps) { | |||
| // XXX: Fixme, not passing state in! | |||
There was a problem hiding this comment.
Could you check how often this is called? If it is often could you put this in a different component passing in only the URL? It would ensure each is only rendered / called once.
There was a problem hiding this comment.
i think it's called once per site icon shown on newTab, when you open a new tab.
it seems to be pretty fast.
| const assert = require('assert') | ||
| const Immutable = require('immutable') | ||
| // const mockery = require('mockery') | ||
| // const settings = require('../../../js/constants/settings') |
There was a problem hiding this comment.
nit: these can be removed in a follow up
| // const mockery = require('mockery') | ||
| // const settings = require('../../../js/constants/settings') | ||
|
|
||
| describe('siteCache', function () { |
| const siteUtil = require('./siteUtil') | ||
| const UrlUtil = require('../lib/urlutil') | ||
|
|
||
| const createLocationSiteKeysCache = (state) => { |
There was a problem hiding this comment.
In a follow up after this is merged, please move this to app/common/state. Basically no files should be in js/* anymore and we're migrating away from that.
| @@ -0,0 +1,145 @@ | |||
| /* global describe, it */ | |||
There was a problem hiding this comment.
Please also move this test to test/unit/app/common/state/
|
Some minor follow ups to do in a new commit but merging this. |
Add locationSiteKeyCache and use for siteUtil.isBookmarked
Auditors: @bbondy Test Plan: Check that automated tests pass.
Fix #8703
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests