Issue 5593 - Wait for prefs before updating popup UI

Issue 5593 - Wait for prefs before updating popup UI

This is part of the work to move away from overwriting the window.ext of the background page and the popup having its own window.ext.

We'll still need a minimal lib/adblockplus.js in the popup. At the very least, we'll need the Prefs object (lib/prefs.js). We can do this by updating metadata.chrome.

If we do that, the prefs aren't yet loaded when the onLoad handler is called. We need to wait for the prefs to be loaded. Once they're loaded, we can update the UI. The delay in loading is not visible to the human eye, but it's slow enough that the onLoad handler will get the default value rather than the actual value that was previously set.

[Manish Jethani, 2017-08-31 15:56:57]

Patch Set 1

Please see the description.

https://codereview.adblockplus.org/29532763/diff/29532764/popup.html
File popup.html (right):

https://codereview.adblockplus.org/29532763/diff/29532764/popup.html#newcode73
popup.html:73: <li id="stats-container" class="collapsed">
This should be collapsed by default.

https://codereview.adblockplus.org/29532763/diff/29532764/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29532763/diff/29532764/popup.js#newcode83
popup.js:83: Prefs.untilLoaded.then(() =>
Wait for the prefs to load.

https://codereview.adblockplus.org/29532763/diff/29532764/popup.js#newcode89
popup.js:89: if (Prefs[collapser.dataset.option])
Since it's now collapsed by default, uncollapse it if the option is true.

https://codereview.adblockplus.org/29532763/diff/29532764/popup.js#newcode152
popup.js:152: Prefs[collapser.dataset.option] =
!collapsable.classList.toggle("collapsed");
The pref value should follow the UI so they never go out of sync. The toggle
function will return a boolean value indicating whether it's collapsed, simply
set the "show" value to the opposite.

[Thomas Greiner, 2017-08-31 16:39:09]

LGTM

Just a typo and an optional enhancement.

https://codereview.adblockplus.org/29532763/diff/29532764/popup.html
File popup.html (right):

https://codereview.adblockplus.org/29532763/diff/29532764/popup.html#newcode73
popup.html:73: <li id="stats-container" class="collapsed">
On 2017/08/31 15:56:57, Manish Jethani wrote:
> This should be collapsed by default.

Acknowledged.

https://codereview.adblockplus.org/29532763/diff/29532764/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29532763/diff/29532764/popup.js#newcode94
popup.js:94: }
FYI: IIRC we've avoided using `Element.classList.toggle(string, boolean)` until
now due to cross-browser compatibility issues but according to
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_co...
it seems like all browsers we need to support are covered. So if you want you
can use it instead of `Element.classList.remove()` here.

https://codereview.adblockplus.org/29532763/diff/29532764/popup.js#newcode151
popup.js:151: let collapsable =
document.getElementById(collapser.dataset.collapsable);
Typo: Replace "collapsable" with "collapsible"

https://codereview.adblockplus.org/29532763/diff/29532764/popup.js#newcode152
popup.js:152: Prefs[collapser.dataset.option] =
!collapsable.classList.toggle("collapsed");
On 2017/08/31 15:56:57, Manish Jethani wrote:
> The pref value should follow the UI so they never go out of sync. The toggle
> function will return a boolean value indicating whether it's collapsed, simply
> set the "show" value to the opposite.

Usually we do it the other way around to ensure that the UI always reflects the
current state of the extension. This is helpful, for instance, to avoid
inconsistency if an error occurs during a state change.

For that we use `Prefs.on()` to keep the UI up-to-date irregardless of where the
preference was changed. The messaging API equivalent of that is "prefs.listen"
which is what we've already been using in all other UIs.

Feel free to change that later on though if there are reasons that neither
`Prefs.on()` nor "prefs.listen" are feasible at the moment.

[Sebastian Noack, 2017-08-31 18:05:46]

NOT LGTM. I don't think that we should include lib/prefs.js in the popup, but
rather use messaging, in the same fashion the options page does.

[Manish Jethani, 2017-09-01 10:27:57]

On 2017/08/31 18:05:46, Sebastian Noack wrote:
> NOT LGTM. I don't think that we should include lib/prefs.js in the popup, but
> rather use messaging, in the same fashion the options page does.

Agreed, this is not necessary.