Opened on 04/08/2019 at 03:19:37 PM

Closed on 04/10/2019 at 01:08:38 PM

Last modified on 07/29/2019 at 01:15:53 PM

#7449 closed change (fixed)

Move compareVersion function into separate module and export

Reported by: kzar Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords: goodfirstbug
Cc: mjethani, sebastian, hfiguiere Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

Description (last modified by mjethani)


To implement a workaround for #7430 I would like to emit an event, but only for users running Firefox versions 66 or 67. To do that I need a way to compare info.platformVersion to those numbers. This logic has already been implemented in adblockpluscore/lib/notification.js, but is not exported for use elsewhere.

What to change

  • Move the compareVersion function into its own module lib/versions.js, rename it to compareVersions and export.
  • Add unit tests for the compareVersions function.
  • Import that function from lib/notification.js and adjust the calling code to use the new name.

Hints for testers

Set up a notification.json on your test server:

  "notifications": [
      "type": "critical",
      "title": "Hello",
      "message": "I just want to say hello.",
      "targets": [
          "applicationMaxVersion": "75.0.3759.4"
  "version": "201904101149"

(Note: I'm not sure how you normally test notifications, but I imagine you redirect the request to to your own server somehow.)

The value of applicationMaxVersion should be set to be greater than or equal to the version number you see in the About dialog of your browser.

Now do a fresh install of the extension, wait for a minute, and see that you get notified with the message I just want to say hello.

Next, edit the notification.json to make the value of applicationMaxVersion less than your browser's version number; then do a fresh install of the extension again, wait for a minute, and see that you do not get notified with the same message.

This should be enough, but you could also experiment with applicationMinVersion in the same way if you like, try different values (less than the browser version, greater than the browser version, exactly equal to the browser version), values with only the major version number, values with only the major and minor version numbers, and so on.

It would be a good idea to test this on all supported platforms since the formats of the version numbers may differ.

Attachments (0)

Change History (14)

comment:1 Changed on 04/08/2019 at 05:50:28 PM by mjethani

  • Priority changed from Unknown to P2
  • Ready set

comment:2 Changed on 04/08/2019 at 05:53:36 PM by mjethani

I wouldn't mind if this went into its own lib/versions.js module with an accompanying test/versions.js test file.

While we're at it, maybe it's a good opportunity to rename the function to compareVersions() since it actually compares two versions against each other.

comment:3 Changed on 04/08/2019 at 05:55:14 PM by mjethani

  • Cc hfiguiere added

comment:4 Changed on 04/09/2019 at 09:35:44 AM by kzar

  • Description modified (diff)
  • Keywords goodfirstbug added
  • Summary changed from Move compareVersion function into coreUtils module and export to Move compareVersion function into separate module and export

Sounds good to me, updated the description.

comment:5 Changed on 04/09/2019 at 12:27:12 PM by kzar

  • Description modified (diff)

comment:6 Changed on 04/09/2019 at 01:16:47 PM by mjethani

  • Owner set to mjethani

comment:7 Changed on 04/09/2019 at 05:11:39 PM by mjethani

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:8 Changed on 04/09/2019 at 10:20:28 PM by sebastian

Note that the compareVersion() function were previously part of the public API, but we stopped exporting it, in order to simplify things. I don't think that at the time we need it for anything outside of the notifications module.

For the workaround mentioned (#7430) where we want to target only specific Firefox versions, we will be just using parseInt() which is good enough when only checking for the major version.

comment:9 Changed on 04/10/2019 at 09:39:23 AM by mjethani

Does this mean that #7430 does not depend on this change?

comment:10 Changed on 04/10/2019 at 10:57:49 AM by kzar

Yea, since we could go ahead with #7430 without this change I didn't mark this issue as blocking it and I added the keyword "goodfirstbug" in case a new contributor wanted to pick it up.

Anyway, you already did a pretty good job of exporting the function and I'm in favour of the change. Even if we don't use it for #7430 it's the kind of function that will surely be useful in the future IMO. Even better now that you've added a bunch of great tests too, surely this is a good thing?

comment:11 Changed on 04/10/2019 at 11:11:25 AM by mjethani

Yes, I agree it's a good idea to move the function to its own module and add some tests. For reference, we might even need it for snippets (#7451).

comment:12 Changed on 04/10/2019 at 11:45:21 AM by abpbot

A commit referencing this issue has landed:
Issue 7449 - Move compareVersion() to lib/versions.js

comment:13 Changed on 04/10/2019 at 01:08:38 PM by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:14 Changed on 07/29/2019 at 01:15:53 PM by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described. Tested with a variety of targets and version numbers.

Microsoft Edge 44.17763.1.0 / Windows 10 1809

Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from mjethani.
Note: See TracTickets for help on using tickets.