Issue 5728 - Replace Services.vc.compare

lib/notification.js

Index: lib/notification.js
===================================================================
--- a/lib/notification.js
+++ b/lib/notification.js
@@ -21,8 +21,6 @@
  * @fileOverview Handles notifications.
  */
 
-const {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
-
 const {Prefs} = require("prefs");
 const {Downloader, Downloadable,
        MILLIS_IN_MINUTE, MILLIS_IN_HOUR, MILLIS_IN_DAY} = require("downloader");
@@ -69,6 +67,30 @@
   return translations[defaultLocale];
 }
 
+function compareVersion(v1, v2)
+{
+  let regexp = /^(.*?)([a-z].*)?$/i;

[Manish Jethani, 2017/09/22 23:28:51]

If "57.0a9" vs "57.0a10" matters, we could handle it with a little extra effort:

  const re = /([\d*]+\.?[\d*]*)?([a-zA-Z]+)?(\d+)?/;

  let [, numA1, strB1, numC1, strD1] = re.exec(v1);
  let [, numA2, strB2, numC2, strD2] = re.exec(v2);

Then we can simply repeat the number and string comparisons for the C and D
components respectively.

[Sebastian Noack, 2017/09/23 00:32:50]

This would add notable complexity to this implementation, this regular
expression alone is rather complex, and incorrect (the 4th group is missing).

+  let [, head1, tail1] = regexp.exec(v1);
+  let [, head2, tail2] = regexp.exec(v2);
+  let components1 = head1.split(".");
+  let components2 = head2.split(".");
+
+  for (let i = 0; i < components1.length ||
+                  i < components2.length; i++)
+  {
+    let comp1 = components1[i];
+    let comp2 = components2[i];
+
+    let result = (comp1 == "*" ? Infinity : parseInt(comp1, 10) || 0) -
+                 (comp2 == "*" ? Infinity : parseInt(comp2, 10) || 0) || 0;

[Wladimir Palant, 2017/09/22 10:11:49]

The readability of this statement is very suboptimal. In particular, I didn't
immediately realize that "1.*" and "1.*" will compare correctly (`|| 0` converts
NaN to zero). I suggest adding an inner function to parse components:

  function parseComponent(comp)
  {
    if (comp == "*")
      return Infinity;
    return parseInt(comp, 10) || 0;
  }

Then you can change this into:

  let result = (parseComponent(components1[i]) - parseComponent(components2[i]))
|| 0;

[Sebastian Noack, 2017/09/23 00:32:50]

I kinda liked the one-liner, but what I like about your suggestion is that we
don't need those two temporary variables. Done.

+
+    if (result != 0)
+      return result;
+  }
+
+  return tail1 == tail2 ? 0 : !tail1 || tail2 && tail1 > tail2 ? 1 : -1;

[Wladimir Palant, 2017/09/22 10:11:49]

This part isn't covered by unit tests (so far these were assuming that version
comparison is tested elsewhere), so you need to add some. At the very least, you
need to verify that "27.0b1" < "27.0" and "27.0" < "27.1a1".

Also, this might be valid for compilers, but this statement is very hard to
read. I suggest that you change at least one conditional into a proper if
statement (better both) and add parentheses:

  if (tail1 == tail2)
    return 0;
  if (!tail1 || (tail2 && tail1 > tail2))
    return 1;
  return -1;

For reference, this code won't compare "57.0a9" and "57.0a10" correctly. We can
live with that I think, but you should add a comment noting that.

[Manish Jethani, 2017/09/22 23:23:41]

FWIW "tail2 && tail1 > tail2" can be just "tail1 > tail2" as tail2 will never be
an empty string, just undefined, in which case the comparison will evaluate to
false anyway.

[Sebastian Noack, 2017/09/23 00:32:50]

Done.

[Sebastian Noack, 2017/09/23 00:32:50]

This is actually true (it wasn't in earlier when I extracted the tails
differently). But I have my doubts that Wladimir would approve a simplification,
relying on the fact that any compassion with undefined returns false. ;)

FWIW, I'm not sure either, how great an idea that is. If we'd ever touch this
code again, and some change causes tailN to be an empty string instead again, it
wouldn't be very obvious what implication this has on the logic here.

+}
+
 /**
  * The object providing actual downloading functionality.
  * @type {Downloader}
@@ -217,19 +239,19 @@
     let targetChecks = {
       extension: v => v == addonName,
       extensionMinVersion:
-        v => Services.vc.compare(addonVersion, v) >= 0,
+        v => compareVersion(addonVersion, v) >= 0,
       extensionMaxVersion:
-        v => Services.vc.compare(addonVersion, v) <= 0,
+        v => compareVersion(addonVersion, v) <= 0,
       application: v => v == application,
       applicationMinVersion:
-        v => Services.vc.compare(applicationVersion, v) >= 0,
+        v => compareVersion(applicationVersion, v) >= 0,
       applicationMaxVersion:
-        v => Services.vc.compare(applicationVersion, v) <= 0,
+        v => compareVersion(applicationVersion, v) <= 0,
       platform: v => v == platform,
       platformMinVersion:
-        v => Services.vc.compare(platformVersion, v) >= 0,
+        v => compareVersion(platformVersion, v) >= 0,
       platformMaxVersion:
-        v => Services.vc.compare(platformVersion, v) <= 0,
+        v => compareVersion(platformVersion, v) <= 0,
       blockedTotalMin: v => Prefs.show_statsinpopup &&
         Prefs.blocked_total >= v,
       blockedTotalMax: v => Prefs.show_statsinpopup &&