Separate Settings from the background page, and use it directly in options.html/popup.html #1599
Conversation
|
Thanks @mrmr1993. I'll look at this soon, once the 1.50 issues calm down. |
mrmr1993
force-pushed
the
settings-refactor
branch
from
d217526
to
b6fa65a
Compare
|
Rebased onto master. |
mrmr1993
force-pushed
the
settings-refactor
branch
from
b6fa65a
to
f4954fc
Compare
|
Rebased onto master. |
mrmr1993
force-pushed
the
settings-refactor
branch
from
f4954fc
to
e6ae179
Compare
|
Rebased onto master. |
|
@mrmr1993. I looked at this a while ago (but forgot to comment -- sorry!). Thoughts... Does this go far enough? It would be good to:
|
I was already planning to do all of those things, this is just a step along the way. The diff is already unweildy, so I decided not to make it worse by touching all the frontend code that uses If there are no issues, it would be great to see this merged so I have a base to start working on unified settings from. |
This function does nothing related to Sync, and only affects Settings.
This stops Sync from being referred to from anywhere except settings.coffee and settings_test.coffee.
This completely decouples settings.coffee from all other background source files, so that it can (eventually) also be used in the frontend.
The Settings object used by the background page now uses 1 of 3 caches,
depending on the context it is available in:
* localStorage - in the background page
* a copy of localStorage - in non-background extension pages
(options.html, popup.html, etc.)
* an empty object - in all other pages (where localStorage doesn't point
to the extension's localStorage object).
For any extension page which is *not* the background page, a copy of
localStorage is used instead of true localStorage:
* Once localStorage is updated by one background page, the others can
only see the updated copy.
- Pages with an updated cache can't tell which changes are new, and so
don't know which postUpdateHooks to run.
* By copying localStorage's contents into a new object, extension pages
can still access settings synchronously.
- This is especially important to options.html and popup.html; they
will not work without it.
Instead of directly accessing the background page's Settings object, the options page and the page popup now have their own.
mrmr1993
force-pushed
the
settings-refactor
branch
from
e6ae179
to
ea53567
Compare
|
Rebased onto master. |
|
|
||
| root.Settings = Settings = | ||
| cache: settingsCache | ||
| init: -> Sync.init() |
There was a problem hiding this comment.
Should we only initialise Sync on the background page?
There was a problem hiding this comment.
No. Starting to use chrome.storage (via Sync) everywhere is the point of this PR, so we can stop moving settings data between contexts manually.
|
I can see the elegance of what you're getting at with this. It's not too far from the three "should-we-not-go-further" issues I mentioned above. Idea:
|
Separate Settings from the background page, and use it directly in options.html/popup.html
|
Thanks, @mrmr1993. It would be great if you could push on with the next steps. |
This PR decouples the
Settingsobject from the background page, with a view to merging it withsettingsin the front-end, creating a unifiedchrome.storage-backed implementation.It also uses the decoupled
Settingsto provide settings information directly to the options page and the browser icon popup, instead of via the background page.The diff here is really quite unfriendly, but I've tried to keep every commit so that it
master.