Issue 6403 - Updated adblockplusui dependency

lib/subscriptionInit.js

Index: lib/subscriptionInit.js
===================================================================
--- a/lib/subscriptionInit.js
+++ b/lib/subscriptionInit.js
@@ -23,11 +23,20 @@
        SpecialSubscription} = require("subscriptionClasses");
 const {FilterStorage} = require("filterStorage");
 const {FilterNotifier} = require("filterNotifier");
+const info = require("info");
 const {Prefs} = require("prefs");
 const {Synchronizer} = require("synchronizer");
 const {Utils} = require("utils");
 const {initNotifications} = require("notificationHelper");
 
+/**
+ * The version of major updates that the user should be aware of.
+ * See also Prefs.lastUpdatesVersion
+ *
+ * @type {number}
+ */
+const updatesVersion = 1;

[kzar, 2018/01/15 11:58:35]

I figured we should instead reuse the extension's version `info.addonVersion`.

[Thomas Greiner, 2018/01/17 18:00:06]

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.

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

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).

[Thomas Greiner, 2018/01/18 13:36:01]

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.

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

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

+
 let firstRun;
 let subscriptionsCallback = null;
 
@@ -48,7 +57,7 @@
   if (firstRun && (!FilterStorage.firstRun || Prefs.currentVersion))
     exports.reinitialized = true;
 
-  Prefs.currentVersion = require("info").addonVersion;
+  Prefs.currentVersion = info.addonVersion;
 }
 
 /**
@@ -204,8 +213,27 @@
       Synchronizer.execute(subscription);
   }
 
-  if (firstRun && !Prefs.suppress_first_run_page)
-    browser.tabs.create({url: browser.extension.getURL("firstRun.html")});
+  if (!Prefs.suppress_first_run_page)
+  {
+    let page = null;
+    if (firstRun)
+    {
+      page = "firstRun.html";
+    }
+    // For now we're limiting the updates page to users of
+    // Chromium-based browsers to gage its impact

[kzar, 2018/01/15 11:58:35]

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?

[Thomas Greiner, 2018/01/17 18:00:06]

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.
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.

[kzar, 2018/01/18 10:53:13]

Acknowledged.

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

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.
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.

[Thomas Greiner, 2018/01/18 13:36:01]

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?
Cool, I didn't know about that feature yet.

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

[kzar, 2018/01/15 11:58:35]

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]

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:06]

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.

[kzar, 2018/01/18 10:53:13]

Good point, yes we should reuse that instead of rewriting it. (I thought it was
weird we didn't implement such a function yet!)
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.

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

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:
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.

[Thomas Greiner, 2018/01/18 13:36:01]

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.

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

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:23]

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

+    {
+      page = "updates.html";
+    }
+
+    if (page)
+    {
+      browser.tabs.create({url: browser.extension.getURL(page)});

[Thomas Greiner, 2018/01/12 16:33:39]

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/18 10:53:13]

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?)

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

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:26]

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:39]

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.

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

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:01]

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.

[kzar, 2018/01/19 11:05:23]

Alright then, seems I'm outnumbered.

+      Prefs.lastUpdatesVersion = updatesVersion;
+    }
+  }
 
   initNotifications();
 }