Issue 6403 - Updated adblockplusui dependency

See also https://codereview.adblockplus.org/29680702/ for related changes to adblockplusui.

[Thomas Greiner, 2018-01-12 16:33:40]

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:204: * The version of major updates that the user is aware of. If
it's too low,
I'm aware that "version of major updates" might sound a bit confusing so if you
have any suggestions, please let me know.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:233: browser.tabs.create({url:
browser.extension.getURL(page)});
Note that in later versions we can simply use `Prefs.lastUpdatesVersion` in
combination with `updatesVersion` to determine which parts of the updates page
should be shown.

At the moment this is not necessary yet since this is the first version of the
updates page.

[kzar, 2018-01-15 11:58:36]

> (other than the updates page there were merely style changes to the options
page)

Please could you update the issue to mention those, and add a Hints for Testers
section which explains what parts of the new options page changed and therefore
needs testing?

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:158: *
Could you do me a favour and delete the trailing whitespace here as well? I just
spotted it whilst reviewing your changes.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:186: * Whether to suppress the first run page. This preference
isn't
Mind updating this comment since the preference also suppresses the updates page
now as well. Also please could you update the issue to mention this change and
the Hints for testers section to add a note to test that the preference works.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:204: * The version of major updates that the user is aware of. If
it's too low,
On 2018/01/12 16:33:39, Thomas Greiner wrote:
> I'm aware that "version of major updates" might sound a bit confusing so if
you
> have any suggestions, please let me know.

How about something like this? "With some major releases an update page is
featured which explains any new functionality to the user. This preferences
contains the most recent version of the extension for which the user has seen an
update page."

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:209: defaults.lastUpdatesVersion = 0;
Other preferences names seem to mostly use snake casing (well 4 out of 28 use
camelCasing), also I think the name is a little confusing. How about
`updates_page_seen` or `update_page_seen_version`?

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:209: defaults.lastUpdatesVersion = 0;
I guess the default value should be "0.0.0" with my other suggestions.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:38: const updatesVersion = 1;
I figured we should instead reuse the extension's version `info.addonVersion`.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:224: // Chromium-based browsers to gage its impact
That's not mentioned in the issue, would you mind updating the description?

Also it seems kind of weird to me to restrict to "only" our most popular
browser, wouldn't we normally test on a less popular one if we're not sure?

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion)
I guess this comparison gets a little trickier with my suggestion to use the
addon version, I've had a go:

/**
 * Compare two semantic (or similiar) version strings.
 * @param {string} The first version string
 * @param {string} The second version string
 * @return {number}
 *   1 if the first version is larger, -1 if the second is and
 *   0 if they are the same.
 */
function compareVersions(v1, v2)
{
  let parseDecimal = s => parseInt(s, 10);
  let padWithZeros = (a, length) =>
  {
    let oldLength = a.length;
    a.length = length;
    a.fill(0, oldLength, length);
  };

  v1 = v1.split(".").map(parseDecimal);
  v2 = v2.split(".").map(parseDecimal);
  if (v1.length < v2.length)
    padWithZeros(v1, v2.length);
  else if (v2.length < v1.length)
    padWithZeros(v2, v1.length);

  for (let i = 0; i < v1.length; i++)
  {
    if (v1[i] > v2[i])
      return 1;
    if (v1[i] < v2[i])
      return -1;
  }
  return 0;
}

[kzar, 2018-01-15 12:17:20]

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion)
On 2018/01/15 11:58:35, kzar wrote:
> I guess this comparison gets a little trickier with my suggestion to use the
> addon version, I've had a go:
> 
> /**
>  * Compare two semantic (or similiar) version strings.
>  * @param {string} The first version string
>  * @param {string} The second version string
>  * @return {number}
>  *   1 if the first version is larger, -1 if the second is and
>  *   0 if they are the same.
>  */
> function compareVersions(v1, v2)
> {
>   let parseDecimal = s => parseInt(s, 10);
>   let padWithZeros = (a, length) =>
>   {
>     let oldLength = a.length;
>     a.length = length;
>     a.fill(0, oldLength, length);
>   };
> 
>   v1 = v1.split(".").map(parseDecimal);
>   v2 = v2.split(".").map(parseDecimal);
>   if (v1.length < v2.length)
>     padWithZeros(v1, v2.length);
>   else if (v2.length < v1.length)
>     padWithZeros(v2, v1.length);
> 
>   for (let i = 0; i < v1.length; i++)
>   {
>     if (v1[i] > v2[i])
>       return 1;
>     if (v1[i] < v2[i])
>       return -1;
>   }
>   return 0;
> }

In fact if you like I'll add this to adblockpluschrome/lib/utils.js for you
ready for use here? (Or perhaps adblockpluscore/lib/coreUtils.js is better hmmm,
what do you think?)

[Thomas Greiner, 2018-01-17 18:00:07]

It's important to discuss the possible implications of tying the updates version
to the addon version so I'm glad you brought that point up. The reason why I was
making it explicit when the updates page should and shouldn't be shown was to
not cause any unintended side-effects.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:158: *
On 2018/01/15 11:58:35, kzar wrote:
> Could you do me a favour and delete the trailing whitespace here as well? I
just
> spotted it whilst reviewing your changes.

Done.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:186: * Whether to suppress the first run page. This preference
isn't
On 2018/01/15 11:58:35, kzar wrote:
> Mind updating this comment since the preference also suppresses the updates
page
> now as well. Also please could you update the issue to mention this change and
> the Hints for testers section to add a note to test that the preference works.

Done. Good point.

Posted it as a comment in the ticket but since nobody replied there I must've
missed to update the ticket description.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:204: * The version of major updates that the user is aware of. If
it's too low,
On 2018/01/15 11:58:35, kzar wrote:
> On 2018/01/12 16:33:39, Thomas Greiner wrote:
> > I'm aware that "version of major updates" might sound a bit confusing so if
> you
> > have any suggestions, please let me know.
> 
> How about something like this? "With some major releases an update page is
> featured which explains any new functionality to the user. This preferences
> contains the most recent version of the extension for which the user has seen
an
> update page."

Problem is it doesn't reflect the "most recent version of the extension" but the
version of the updates page which is independent of the extension version.

But I guess its meaning depends on the outcome of the discussion we're having in
the other comment.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:209: defaults.lastUpdatesVersion = 0;
On 2018/01/15 11:58:35, kzar wrote:
> I guess the default value should be "0.0.0" with my other suggestions.

Yep, it would need to be. Let's come back to this when we concluded the
discussion there.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/prefs.js#newcod...
lib/prefs.js:209: defaults.lastUpdatesVersion = 0;
On 2018/01/15 11:58:35, kzar wrote:
> Other preferences names seem to mostly use snake casing (well 4 out of 28 use
> camelCasing), also I think the name is a little confusing. How about
> `updates_page_seen` or `update_page_seen_version`?

It'd be great if we could make those consistent in the future but yeah, let's
strive towards the most commonly used format. Done.

Note that I also removed the "last" prefix because it seemed unnecessary.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:38: const updatesVersion = 1;
On 2018/01/15 11:58:35, kzar wrote:
> I figured we should instead reuse the extension's version `info.addonVersion`.

I'm wondering whether we (a) will always increment the extension's major version
when we want to show the updates page and (b) will always show the updates page
when we want to increment the extension's major version.

So do we want to directly tie the updates page to the extension version? For
instance, currently the stable version is already on 3.0.2 even though we
haven't included the new options page yet.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:224: // Chromium-based browsers to gage its impact
On 2018/01/15 11:58:35, kzar wrote:
> That's not mentioned in the issue, would you mind updating the description?

Weird, I actually thought it'd be in the spec since we've agreed on that in our
meetings. I'll check back with Jeen on that.

> Also it seems kind of weird to me to restrict to "only" our most popular
> browser, wouldn't we normally test on a less popular one if we're not sure?

The plan is to have a staged rollout on Chrome. Apparently, Winsley and
Sebastian have a plan about how they intend to do that but I'm not aware of the
details.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion)
We used to use Firefox' `Services.vc.compare()` for that which we used to
polyfill for use in other browsers. There is a `compareVersions()` function
already included in adblockpluscore/lib/notification.js so it seems like we're
simply not exposing it globally anymore.
It'd be great if we could move that to adblockpluscore/lib/coreUtils.js then to
avoid unnecessarily duplicating this logic.

Anyway, using this here would mean that we'd end up showing the updates page for
every single extension update rather than only for ones that are meaningful to
the user. That's not what the updates page is meant for though.

[Sebastian Noack, 2018-01-17 22:14:43]

Note that since the update page will announce the new options page, we have to
enable the new options page on Chrome in the same go, we start showing the
update page, i.e. with the same patch.

[kzar, 2018-01-18 10:53:14]

What do you think Sebastian?

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:224: // Chromium-based browsers to gage its impact
On 2018/01/17 18:00:06, Thomas Greiner wrote:
> On 2018/01/15 11:58:35, kzar wrote:
> > That's not mentioned in the issue, would you mind updating the description?
> 
> Weird, I actually thought it'd be in the spec since we've agreed on that in
our
> meetings. I'll check back with Jeen on that.
> 
> > Also it seems kind of weird to me to restrict to "only" our most popular
> > browser, wouldn't we normally test on a less popular one if we're not sure?
> 
> The plan is to have a staged rollout on Chrome. Apparently, Winsley and
> Sebastian have a plan about how they intend to do that but I'm not aware of
the
> details.

Acknowledged.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion)
On 2018/01/17 18:00:06, Thomas Greiner wrote:
> We used to use Firefox' `Services.vc.compare()` for that which we used to
> polyfill for use in other browsers. There is a `compareVersions()` function
> already included in adblockpluscore/lib/notification.js so it seems like we're
> simply not exposing it globally anymore.
> It'd be great if we could move that to adblockpluscore/lib/coreUtils.js then
to
> avoid unnecessarily duplicating this logic.

Good point, yes we should reuse that instead of rewriting it. (I thought it was
weird we didn't implement such a function yet!)

> Anyway, using this here would mean that we'd end up showing the updates page
for
> every single extension update rather than only for ones that are meaningful to
> the user. That's not what the updates page is meant for though.

Oh yea, good point. We need to store some version number like updatesVersion, I
agree. Otherwise if version 1.0.0 has an updates page, it'll get shown again for
the 1.0.1, 1.0.2 etc releases until it was replaced with a new one. I think that
updatesVersion should really live in adblockplusui however, along with the
updates page itself.

As for using a separate number VS the extension version I think you're right as
well, if updatesVersion is stored in adblockplusui it seems wrong to rely on the
release version numbering of adblockpluschrome.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:233: browser.tabs.create({url:
browser.extension.getURL(page)});
On 2018/01/12 16:33:39, Thomas Greiner wrote:
> Note that in later versions we can simply use `Prefs.lastUpdatesVersion` in
> combination with `updatesVersion` to determine which parts of the updates page
> should be shown.

I'm not quite sure how that would work. So the plan isn't to simply replace the
existing updates page with the latest one and to increment the updatesVersion?
(Is it really useful to say "hey we released a new options page 2 years ago" to
a new user?)

[Thomas Greiner, 2018-01-18 11:33:50]

On 2018/01/17 22:14:43, Sebastian Noack wrote:
> Note that since the update page will announce the new options page, we have to
> enable the new options page on Chrome in the same go, we start showing the
> update page, i.e. with the same patch.

There are also two other options to tackle this:
- Push the options page before pushing the updates page
- Set `Prefs.updates_version = 0` for now so that the updates page isn't shown
yet and increment it to `1` in the same patch which also includes the options
page

On 2018/01/18 10:53:14, kzar wrote:
> Oh yea, good point. We need to store some version number like updatesVersion,
I
> agree. Otherwise if version 1.0.0 has an updates page, it'll get shown again
for
> the 1.0.1, 1.0.2 etc releases until it was replaced with a new one. I think
that
> updatesVersion should really live in adblockplusui however, along with the
> updates page itself.

I'll check out what options we have to achieve that.

> I'm not quite sure how that would work. So the plan isn't to simply replace
the
> existing updates page with the latest one and to increment the updatesVersion?

There is no fixed plan yet on how we want to progress with the updates page in
the future but if you were to ask me, I'd be in favor of reusing the current
one, adding new content to it and making sure that only the relevant content is
shown.

> (Is it really useful to say "hey we released a new options page 2 years ago"
to
> a new user?)

No, but I'd consider it to be a good way to apologize to our Firefox users for
not telling them what, why and how things have changed.

[kzar, 2018-01-18 11:38:24]

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:233: browser.tabs.create({url:
browser.extension.getURL(page)});
On 2018/01/18 10:53:13, kzar wrote:
> On 2018/01/12 16:33:39, Thomas Greiner wrote:
> > Note that in later versions we can simply use `Prefs.lastUpdatesVersion` in
> > combination with `updatesVersion` to determine which parts of the updates
page
> > should be shown.
> 
> I'm not quite sure how that would work. So the plan isn't to simply replace
the
> existing updates page with the latest one and to increment the updatesVersion?
> (Is it really useful to say "hey we released a new options page 2 years ago"
to
> a new user?)

Hmm, I suppose we need to make sure the updates page gets access to the current
version of Prefs.lastUpdatesVersion before it is updated. That way it'll know
the delta and can decide for itself what should be displayed to the user.

[Thomas Greiner, 2018-01-18 11:50:27]

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:233: browser.tabs.create({url:
browser.extension.getURL(page)});
On 2018/01/18 11:38:24, kzar wrote:
> On 2018/01/18 10:53:13, kzar wrote:
> > On 2018/01/12 16:33:39, Thomas Greiner wrote:
> > > Note that in later versions we can simply use `Prefs.lastUpdatesVersion`
in
> > > combination with `updatesVersion` to determine which parts of the updates
> page
> > > should be shown.
> > 
> > I'm not quite sure how that would work. So the plan isn't to simply replace
> the
> > existing updates page with the latest one and to increment the
updatesVersion?
> > (Is it really useful to say "hey we released a new options page 2 years ago"
> to
> > a new user?)
> 
> Hmm, I suppose we need to make sure the updates page gets access to the
current
> version of Prefs.lastUpdatesVersion before it is updated. That way it'll know
> the delta and can decide for itself what should be displayed to the user.

Yep, the simplest way would be to pass it as a GET parameter when opening the
page here but we can find other ways for that if we don't like doing it this
way.

However, since we're not making use of it at the moment, I didn't see the need
to implement this feature yet.

[kzar, 2018-01-18 12:01:40]

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:233: browser.tabs.create({url:
browser.extension.getURL(page)});
On 2018/01/18 11:50:26, Thomas Greiner wrote:
> On 2018/01/18 11:38:24, kzar wrote:
> > On 2018/01/18 10:53:13, kzar wrote:
> > > On 2018/01/12 16:33:39, Thomas Greiner wrote:
> > > > Note that in later versions we can simply use `Prefs.lastUpdatesVersion`
> in
> > > > combination with `updatesVersion` to determine which parts of the
updates
> > page
> > > > should be shown.
> > > 
> > > I'm not quite sure how that would work. So the plan isn't to simply
replace
> > the
> > > existing updates page with the latest one and to increment the
> updatesVersion?
> > > (Is it really useful to say "hey we released a new options page 2 years
ago"
> > to
> > > a new user?)
> > 
> > Hmm, I suppose we need to make sure the updates page gets access to the
> current
> > version of Prefs.lastUpdatesVersion before it is updated. That way it'll
know
> > the delta and can decide for itself what should be displayed to the user.
> 
> Yep, the simplest way would be to pass it as a GET parameter when opening the
> page here but we can find other ways for that if we don't like doing it this
> way.
> 
> However, since we're not making use of it at the moment, I didn't see the need
> to implement this feature yet.

Generally I agree about not implementing stuff before it's needed, but I think
it's better to start passing the lastUpdatesVersion through now. The updates
page can simple ignore lastUpdatesVersion for now, but it'll be there ready.

[kzar, 2018-01-18 12:07:07]

Just a thought but we should probably be careful to set the lastUpdateVersion
when the first run page is displayed too. Otherwise new users will be shown a
bunch of random update news the first time they receive a minor update.

[Sebastian Noack, 2018-01-18 12:46:48]

On 2018/01/18 11:33:50, Thomas Greiner wrote:
> On 2018/01/17 22:14:43, Sebastian Noack wrote:
> > Note that since the update page will announce the new options page, we have
to
> > enable the new options page on Chrome in the same go, we start showing the
> > update page, i.e. with the same patch.
> 
> There are also two other options to tackle this:
> - Push the options page before pushing the updates page

I don't mind separating these changes, as long as they land with no delay in
between.

> - Set `Prefs.updates_version = 0` for now so that the updates page isn't shown
> yet and increment it to `1` in the same patch which also includes the options
> page

Yes, if we add separate versioning for the update page, this would be an option
too.

BTW, I just noticed that there are no translations for the updates page yet?

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:38: const updatesVersion = 1;
On 2018/01/17 18:00:06, Thomas Greiner wrote:
> currently the stable version is already on 3.0.2 even though we
> haven't included the new options page yet.

This is not true. We only advanced the version to 3.* for Firefox, where we have
the new options page. The release version of Adblock Plus for Chrome is 1.13.4
(and we won't advance it to 3.* before adding the new options page).

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:224: // Chromium-based browsers to gage its impact
On 2018/01/17 18:00:06, Thomas Greiner wrote:
> On 2018/01/15 11:58:35, kzar wrote:
> > Also it seems kind of weird to me to restrict to "only" our most popular
> > browser, wouldn't we normally test on a less popular one if we're not sure?

Well, the current update page, announces the new options page, which has already
been rolled on firefox. So it makes sense to not show the update page to Firefox
users. As for Microsoft Edge, the new options page isn't compatible there yet,
so we won't roll it out at the same time there. Also since Microsoft Edge is
still in beta, it is probably not necessary to show them an update page. That
leaves us with only Chromium-based browsers, where the update page is relevant.
Of course, if we later update the update page to announce other features, that
are simultaneously rolled out on multiple platforms, we have to adapt the logic
here, accordingly.

> The plan is to have a staged rollout on Chrome. Apparently, Winsley and
> Sebastian have a plan about how they intend to do that but I'm not aware of
the
> details.

It is a feature of the Chrome Web Store. We can say, to only update n% of users
to a new version of the extension. Of course, this means, while we are running
this test, we cannot push any other updates. So we should keep the test short,
and then either roll the update out to all users, or release a new version with
the new options page reverted. This is what we have discussed so far.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion)
On 2018/01/18 10:53:13, kzar wrote:
> Good point, yes we should reuse that instead of rewriting it. (I thought it
was
> weird we didn't implement such a function yet!)
> 
> [...]
>
> As for using a separate number VS the extension version I think you're right
as
> well, if updatesVersion is stored in adblockplusui it seems wrong to rely on
the
> release version numbering of adblockpluschrome.

For reference, originally the version comparison logic was provided by Firefox
(only for legacy extensions), and we had a polyfill in adblockpluschrome, and
another (inconsistent) polyfill for the notification tests in adblockpluscore.
When we dropped the legacy Firefox extension, it turned out that we only still
rely on that logic for the notifications, so we moved the logic in there.

If we also need that logic here, now, we can move it to
adblockpluscore/lib/coreUtils.js. But then we should also add separate tests
(which was the main reason we didn't export that function in the first place).
However, this only seems to make sense if we rely on the version number of the
extension. If we use separate versioning for the update page, there is no need
to support complex version schemes, but we can just stick to a single integer.

However, I don't see (yet), why we would want to introduce a separate version
number just for the update page. I'd rather show the update page based on the
extension's version (as outlined below).

On 2018/01/17 18:00:06, Thomas Greiner wrote:
> Anyway, using this here would mean that we'd end up showing the updates page
for
> every single extension update rather than only for ones that are meaningful to
> the user. That's not what the updates page is meant for though.

Sure, we should only show the update once, after updating to a version that
includes the features announced in the update page. So for example, the new
options page has been added in 3.0. So the logic should simply be to show the
update page if the previous version is smaller than 3.0.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:233: browser.tabs.create({url:
browser.extension.getURL(page)});
On 2018/01/12 16:33:39, Thomas Greiner wrote:
> Note that in later versions we can simply use `Prefs.lastUpdatesVersion` in
> combination with `updatesVersion` to determine which parts of the updates page
> should be shown.
> 
> At the moment this is not necessary yet since this is the first version of the
> updates page.

FWIW, I think, in the (rather theoretical) scenario when some updates with
update information have been skipped, it is good enough, to just show the latest
update information. I expect this scenario to be extremely rare, and otherwise
it will also make testing a pain (and unlikely to happen).

[Thomas Greiner, 2018-01-18 13:36:02]

On 2018/01/18 12:07:07, kzar wrote:
> Just a thought but we should probably be careful to set the lastUpdateVersion
> when the first run page is displayed too. Otherwise new users will be shown a
> bunch of random update news the first time they receive a minor update.

The purpose of setting `Prefs.update_version` on first-run is to exactly avoid
the scenario that users would be presented with the updates page on the first
update they receive since they've already had those features included when they
installed the extension.

On 2018/01/18 12:46:48, Sebastian Noack wrote:
> BTW, I just noticed that there are no translations for the updates page yet?

Yep, Winsley also brought that up on IRC so we will wait for adblockplusui to be
ready for release before doing the dependency update.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:38: const updatesVersion = 1;
On 2018/01/18 12:46:48, Sebastian Noack wrote:
> This is not true. We only advanced the version to 3.* for Firefox, where we
have
> the new options page. The release version of Adblock Plus for Chrome is 1.13.4
> (and we won't advance it to 3.* before adding the new options page).

My bad, I looked at a local build of mine which I confused with the stable
version since it didn't have "development build" in the name. Sorry about that.

So it's only the development build that's already on 3.x without having the
options page yet. That's not as big of an issue though but my argument still
stands.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:224: // Chromium-based browsers to gage its impact
On 2018/01/18 12:46:48, Sebastian Noack wrote:
> Well, the current update page, announces the new options page, which has
already
> been rolled on firefox. So it makes sense to not show the update page to
Firefox
> users. As for Microsoft Edge, the new options page isn't compatible there yet,
> so we won't roll it out at the same time there. Also since Microsoft Edge is
> still in beta, it is probably not necessary to show them an update page. That
> leaves us with only Chromium-based browsers, where the update page is
relevant.
> Of course, if we later update the update page to announce other features, that
> are simultaneously rolled out on multiple platforms, we have to adapt the
logic
> here, accordingly.

I haven't heard back yet on whether we want to mention that restriction in the
spec. But besides that, should we explain our reasoning in an inline comment
here?

> It is a feature of the Chrome Web Store. We can say, to only update n% of
users
> to a new version of the extension. Of course, this means, while we are running
> this test, we cannot push any other updates. So we should keep the test short,
> and then either roll the update out to all users, or release a new version
with
> the new options page reverted. This is what we have discussed so far.

Cool, I didn't know about that feature yet.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion)
On 2018/01/18 12:46:48, Sebastian Noack wrote:
> Sure, we should only show the update once, after updating to a version that
> includes the features announced in the update page. So for example, the new
> options page has been added in 3.0. So the logic should simply be to show the
> update page if the previous version is smaller than 3.0.

That means that none of our devbuild users will receive the updates page since
they're already on 3.x. Furthermore, it'd make it a bit more complicated to show
the same updates page to Firefox users later on since they are also already on
3.x.

Those are just two examples of edge cases which we may encounter more of in the
future due to the addon version and the updates page  version being not quite
the same.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:233: browser.tabs.create({url:
browser.extension.getURL(page)});
On 2018/01/18 12:46:48, Sebastian Noack wrote:
> FWIW, I think, in the (rather theoretical) scenario when some updates with
> update information have been skipped, it is good enough, to just show the
latest
> update information. I expect this scenario to be extremely rare, and otherwise
> it will also make testing a pain (and unlikely to happen).

Ok, so if Dave agrees with that as well I'd not implement this feature at this
point.

If we want to we can continue this discussion when we start working on the next
version of the updates page.

[Sebastian Noack, 2018-01-18 16:23:12]

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:38: const updatesVersion = 1;
On 2018/01/18 13:36:01, Thomas Greiner wrote:
> My bad, I looked at a local build of mine which I confused with the stable
> version since it didn't have "development build" in the name. Sorry about
that.
> 
> So it's only the development build that's already on 3.x without having the
> options page yet. That's not as big of an issue though but my argument still
> stands.

Oh, that was a mistake. We should not have advanced the version number in
metadata.chrome, but override it in metadata.gecko.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion)
On 2018/01/18 13:36:01, Thomas Greiner wrote:
> That means that none of our devbuild users will receive the updates page since
> they're already on 3.x. Furthermore, it'd make it a bit more complicated to
show
> the same updates page to Firefox users later on since they are also already on
> 3.x.

Well, we have to special case either Firefox or Chromium, either way. But you
have a point, that if we rely on the extension version, the behavior on the
development builds, would be flawed (and difficult to test). Even if we wouldn't
have accidentally advanced the devbuilds to 3.x, the devbuild users (and
testers) still wouldn't see the update page before the release.

[kzar, 2018-01-19 11:05:24]

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:226: updatesVersion > Prefs.lastUpdatesVersion)
On 2018/01/18 16:23:12, Sebastian Noack wrote:
> On 2018/01/18 13:36:01, Thomas Greiner wrote:
> > That means that none of our devbuild users will receive the updates page
since
> > they're already on 3.x. Furthermore, it'd make it a bit more complicated to
> show
> > the same updates page to Firefox users later on since they are also already
on
> > 3.x.
> 
> Well, we have to special case either Firefox or Chromium, either way. But you
> have a point, that if we rely on the extension version, the behavior on the
> development builds, would be flawed (and difficult to test). Even if we
wouldn't
> have accidentally advanced the devbuilds to 3.x, the devbuild users (and
> testers) still wouldn't see the update page before the release.

Yea, I'm coming around to the idea of having the separate updatesVersion number
as well, I think Thomas has a point.

https://codereview.adblockplus.org/29664623/diff/29664624/lib/subscriptionIni...
lib/subscriptionInit.js:233: browser.tabs.create({url:
browser.extension.getURL(page)});
On 2018/01/18 13:36:01, Thomas Greiner wrote:
> On 2018/01/18 12:46:48, Sebastian Noack wrote:
> > FWIW, I think, in the (rather theoretical) scenario when some updates with
> > update information have been skipped, it is good enough, to just show the
> latest
> > update information. I expect this scenario to be extremely rare, and
otherwise
> > it will also make testing a pain (and unlikely to happen).
> 
> Ok, so if Dave agrees with that as well I'd not implement this feature at this
> point.
> 
> If we want to we can continue this discussion when we start working on the
next
> version of the updates page.

Alright then, seems I'm outnumbered.

[Thomas Greiner, 2018-01-22 17:10:22]

In my latest patch I've moved the `updatesVersion` constant into adblockplusui
by introducing a lib/ui.js module. We may want to revisit this later on when we
get a better grasp of how we want to do UI releases going forward but for now
this seems sufficient.

Obviously, this requires a corresponding change in adblockplusui so the
dependency still needs to be updated.

[kzar, 2018-01-23 10:07:14]

I'm much happier with this now, thanks for those changes. A few nits remain.

https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js#newcod...
lib/prefs.js:209: defaults.updates_version = 0;
I think this preference name is kind of confusing, especially since we call the
version of the current update's page updatesVersion. Without the (very useful)
comment above I would be quite confused about what the preference did. How about
we call it last_update_page_displayed and the other variable updatePageVersion?

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
lib/subscriptionInit.js:219: updatesVersion > Prefs.updates_version)
Nit: This indentation is quite unusual, mind putting the line below the one
above? E.g.

else if (info.platform == "chromium" &&
         updatesVersion > Prefs.updates_version)

instead of

else if (info.platform == "chromium" &&
    updatesVersion > Prefs.updates_version)

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
lib/subscriptionInit.js:227: Prefs.updates_version = updatesVersion;
Mind adding a short comment here explaining that we do this to prevent the
update page showing again either after it already was shown, but also for new
installs?

[Thomas Greiner, 2018-01-25 11:09:45]

https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js#newcod...
lib/prefs.js:209: defaults.updates_version = 0;
On 2018/01/23 10:07:13, kzar wrote:
> I think this preference name is kind of confusing, especially since we call
the
> version of the current update's page updatesVersion. Without the (very useful)
> comment above I would be quite confused about what the preference did. How
about
> we call it last_update_page_displayed and the other variable
updatePageVersion?

I see your point. Just a bit hesitant to make it too specific to the updates
page or otherwise we'd end up with the same issue as with
"suppress_first_run_page" which we're now using also for the updates page. If we
had used a more generic name (e.g. "suppress_info_pages") would've helped us in
that regard.

So what about something along the lines of "updates_awareness" instead to make
it a bit clearer what it's supposed to represent?

Anyway, if you don't think that being too specific to the updates page is an
issue, I'd be happy to change it to "last_update_page_displayed" as you
suggested.

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
lib/subscriptionInit.js:219: updatesVersion > Prefs.updates_version)
On 2018/01/23 10:07:13, kzar wrote:
> Nit: This indentation is quite unusual, mind putting the line below the one
> above? E.g.
> 
> else if (info.platform == "chromium" &&
>          updatesVersion > Prefs.updates_version)
> 
> instead of
> 
> else if (info.platform == "chromium" &&
>     updatesVersion > Prefs.updates_version)

Done.

I don't mind making such adjustments but it'd be great if we could stick to the
coding style. Otherwise we'll end up with every project team having undocumented
rules.

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
lib/subscriptionInit.js:227: Prefs.updates_version = updatesVersion;
On 2018/01/23 10:07:13, kzar wrote:
> Mind adding a short comment here explaining that we do this to prevent the
> update page showing again either after it already was shown, but also for new
> installs?

Done.

[kzar, 2018-01-25 11:46:26]

https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js#newcod...
lib/prefs.js:209: defaults.updates_version = 0;
On 2018/01/25 11:09:45, Thomas Greiner wrote:
> On 2018/01/23 10:07:13, kzar wrote:
> > I think this preference name is kind of confusing, especially since we call
> the
> > version of the current update's page updatesVersion. Without the (very
useful)
> > comment above I would be quite confused about what the preference did. How
> about
> > we call it last_update_page_displayed and the other variable
> updatePageVersion?
> 
> I see your point. Just a bit hesitant to make it too specific to the updates
> page or otherwise we'd end up with the same issue as with
> "suppress_first_run_page" which we're now using also for the updates page. If
we
> had used a more generic name (e.g. "suppress_info_pages") would've helped us
in
> that regard.
> 
> So what about something along the lines of "updates_awareness" instead to make
> it a bit clearer what it's supposed to represent?
> 
> Anyway, if you don't think that being too specific to the updates page is an
> issue, I'd be happy to change it to "last_update_page_displayed" as you
> suggested.

Well it was unfortunate about the "suppress_first_run_page" name you're right
but I think being vague when naming this preference as a response is a bad idea.
I'd rather we went with something like "last_update_page_displayed" that was
easy to understand.

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
lib/subscriptionInit.js:219: updatesVersion > Prefs.updates_version)
On 2018/01/25 11:09:45, Thomas Greiner wrote:
> On 2018/01/23 10:07:13, kzar wrote:
> > Nit: This indentation is quite unusual, mind putting the line below the one
> > above? E.g.
> > 
> > else if (info.platform == "chromium" &&
> >          updatesVersion > Prefs.updates_version)
> > 
> > instead of
> > 
> > else if (info.platform == "chromium" &&
> >     updatesVersion > Prefs.updates_version)
> 
> Done.
> 
> I don't mind making such adjustments but it'd be great if we could stick to
the
> coding style. Otherwise we'll end up with every project team having
undocumented
> rules.

Yes, it would be great if ESLint could capture all our rules and conventions,
but it wasn't possible without false positives in practice. We tried our best
and it was a big improvement over no linting at least.

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
lib/subscriptionInit.js:227: Prefs.updates_version = updatesVersion;
On 2018/01/25 11:09:45, Thomas Greiner wrote:
> On 2018/01/23 10:07:13, kzar wrote:
> > Mind adding a short comment here explaining that we do this to prevent the
> > update page showing again either after it already was shown, but also for
new
> > installs?
> 
> Done.

Thanks but the main thing I wanted to mention was that for new users the updates
page would be suppressed too, since I initially thought this line was mistakenly
in the wrong place. Maybe something like this?

// For new users and users that have already seen this updates page we want to
// avoid it showing it the next time the extension is updated.

[Thomas Greiner, 2018-01-25 13:09:04]

https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29664623/diff/29676735/lib/prefs.js#newcod...
lib/prefs.js:209: defaults.updates_version = 0;
On 2018/01/25 11:46:25, kzar wrote:
> On 2018/01/25 11:09:45, Thomas Greiner wrote:
> > On 2018/01/23 10:07:13, kzar wrote:
> > > I think this preference name is kind of confusing, especially since we
call
> > the
> > > version of the current update's page updatesVersion. Without the (very
> useful)
> > > comment above I would be quite confused about what the preference did. How
> > about
> > > we call it last_update_page_displayed and the other variable
> > updatePageVersion?
> > 
> > I see your point. Just a bit hesitant to make it too specific to the updates
> > page or otherwise we'd end up with the same issue as with
> > "suppress_first_run_page" which we're now using also for the updates page.
If
> we
> > had used a more generic name (e.g. "suppress_info_pages") would've helped us
> in
> > that regard.
> > 
> > So what about something along the lines of "updates_awareness" instead to
make
> > it a bit clearer what it's supposed to represent?
> > 
> > Anyway, if you don't think that being too specific to the updates page is an
> > issue, I'd be happy to change it to "last_update_page_displayed" as you
> > suggested.
> 
> Well it was unfortunate about the "suppress_first_run_page" name you're right
> but I think being vague when naming this preference as a response is a bad
idea.
> I'd rather we went with something like "last_update_page_displayed" that was
> easy to understand.

Done.

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
lib/subscriptionInit.js:219: updatesVersion > Prefs.updates_version)
On 2018/01/25 11:46:25, kzar wrote:
> Yes, it would be great if ESLint could capture all our rules and conventions,
> but it wasn't possible without false positives in practice. We tried our best
> and it was a big improvement over no linting at least.

Just to clarify: I'm not talking about linting rules but about what we define in
our coding style.

https://codereview.adblockplus.org/29664623/diff/29676735/lib/subscriptionIni...
lib/subscriptionInit.js:227: Prefs.updates_version = updatesVersion;
On 2018/01/25 11:46:25, kzar wrote:
> Thanks but the main thing I wanted to mention was that for new users the
updates
> page would be suppressed too, since I initially thought this line was
mistakenly
> in the wrong place. Maybe something like this?
> 
> // For new users and users that have already seen this updates page we want to
> // avoid it showing it the next time the extension is updated.

Done. My bad.

Note that I replaced "the next time the extension is updated" with "subsequent
updates".

[kzar, 2018-01-25 13:13:26]

Once the dependency update is sorted this LGTM

[Thomas Greiner, 2018-01-31 12:03:26]

Updated dependency to reflect changes from
https://codereview.adblockplus.org/29680702/.

We're still waiting for community translations though (see also #6336).

[Thomas Greiner, 2018-02-22 17:28:31]

I've updated the dependency so that it includes the translations.

As mentioned in #6403 this also required the following changes:
- Added skin/fonts.css
- Added skin/fonts/Source-Sans-Pro/*
- Removed skin/fonts/SourceSansPro-*
- Moved mapping related to desktop options page from metadata.gecko to
metadata.chrome

That's why I also added Manvel to this review.

[kzar, 2018-02-23 10:17:47]

https://codereview.adblockplus.org/29664623/diff/29705802/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29664623/diff/29705802/dependencies#newcode5
dependencies:5: adblockplusui = adblockplusui hg:2a04df826943 git:04b38a9
There have been more changes since, including updated translations. Shouldn't we
update to the latest revision (08bd84805764) instead?

[Thomas Greiner, 2018-02-23 11:57:15]

https://codereview.adblockplus.org/29664623/diff/29705802/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29664623/diff/29705802/dependencies#newcode5
dependencies:5: adblockplusui = adblockplusui hg:2a04df826943 git:04b38a9
On 2018/02/23 10:17:46, kzar wrote:
> There have been more changes since, including updated translations. Shouldn't
we
> update to the latest revision (08bd84805764) instead?

Done. Don't know why this part of the change wasn't included in the last patch
together with the other ones. Glad you spotted that.

[kzar, 2018-02-23 15:14:32]

So just to confirm this now includes all the translations for the new UI(s) that
we were waiting on before updating the adblockplusui dependency right? Also
you've tested the changes again with the latest patchset? If so this LGTM.

[saroyanm, 2018-02-23 15:48:01]

On 2018/02/23 15:14:32, kzar wrote:
> So just to confirm this now includes all the translations for the new UI(s)
that
> we were waiting on before updating the adblockplusui dependency right? Also
> you've tested the changes again with the latest patchset? If so this LGTM.

Yes, it contains latest translations.
All Mappings looks to be in place as well.
LGTM

[Thomas Greiner, 2018-02-23 16:34:41]

On 2018/02/23 15:14:32, kzar wrote:
> Also you've tested the changes again with the latest patchset? If so this
LGTM.

I did some rough testing, yes. The first-run page, updates page, desktop options
page as well as mobile options page all load without issues on both Chrome and
Firefox.

[kzar, 2018-02-23 17:05:20]

On 2018/02/23 16:34:41, Thomas Greiner wrote:

> I did some rough testing, yes. The first-run page, updates page, desktop
options
> page as well as mobile options page all load without issues on both Chrome and
> Firefox.

Cool OK, thanks sounds good to me.