Noissue - Adapt check for devtools panel support for Firefox

messageResponder.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 24 matching lines @@
   const {Filter, BlockingFilter, RegExpFilter} = require("filterClasses");
   const {Synchronizer} = require("synchronizer");
 
   const info = require("info");
   const {
     Subscription,
     DownloadableSubscription,
     SpecialSubscription
   } = require("subscriptionClasses");
 
+  const {Services} = Cu.import("resource://gre/modules/Services.jsm");
+
   // Some modules doesn't exist on Firefox. Moreover,
   // require() throws an exception on Firefox in that case.
   // However, try/catch causes the whole function to to be
   // deoptimized on V8. So we wrap it into another function.
   function tryRequire(module)
   {
     try
     {
       return require(module);
     }
@@ skipping 122 matching lines @@
       }
       else
         bidiDir = Utils.readingDirection;
 
       return {locale: Utils.appLocale, bidiDir};
     }
 
     if (message.what == "features")
     {
       return {
-        devToolsPanel: info.platform == "chromium"
+        devToolsPanel: info.platform == "chromium" ||
+                       info.application == "firefox" &&
+                       Services.vc.compare(info.applicationVersion, "54") >= 0

[Manish Jethani, 2017/09/15 22:10:21]

Since we're going to be getting rid of these Gecko APIs anyway, I wonder if we
shouldn't just use parseInt here?

This implementation of Services.vc.compare seems to be different from the Gecko
version. The Gecko version will return -1 for "54.0b1", "54" (beta vs. final
release), which is correct, whereas this one will return 0. Interestingly the
Gecko version also returns -1 for "52.0esr", "52" while I would expect it to
return 1, since ESR definitely comes later. Anyway, as we're only interested in
the major version number, we should probably just use parseInt and call it a
day.

We're using parseInt in one more place, for what it's worth:

https://hg.adblockplus.org/adblockpluschrome/file/5180860aff78/ext/background...

[Sebastian Noack, 2017/09/15 22:33:22]

Probably we won't get rid of the implementation of Services.vc.compare in
lib/compat.js, but rather move it into a module in adblockpluscore, using a more
sane calling convention, since there are more places where it is needed, and I'm
not sure whether we are only interested in the major version there as well. But
yeah, we'd have to adapt the code here as well then.

The inconsistency should be fine. The code in adblockplusui/background.js isn't
meant to be feature complete, but rather minimal, as it is only used when
running the UI standalone from within the adblockplusui repository, during
development, where this limitation is fine.

Then again, simply using parseInt() here would reduce the code in adblockplusui,
and avoids unnecessary changes here when we clean up these APIs.

Thomas, what do you think?

[Thomas Greiner, 2017/09/18 16:20:02]

I'd say it's probably best to stick to what we've been using so far so that we
can easily identify and update all the places later on when we replace
`Services.vc.compare` with whichever solution we think works best.

[Sebastian Noack, 2017/09/18 18:17:28]

I've submitted a patch to adblockpluscore, replacing Services.vc.compare, and
updated this patch to use the new API right away.

[Manish Jethani, 2017/09/19 06:06:59]

LGTM, provided we also update the core dependency once that patch has landed.

[Manish Jethani, 2017/09/19 06:06:59]

Sorry about going off on a bit of a tangent here, but I tried to test this and
of course it gave an error because compareVersion is still not available. But
the options page still showed the checkbox for devtools panel.

  getInfo("features", features =>
  {
    if (!features.devToolsPanel)
      document.getElementById("showDevtoolsPanelContainer").hidden = true;
  });

features is undefined here in this case so the checkbox is never hidden.

This makes me wonder if the elements on the page should be changed to be hidden
by default and shown only when it is confirmed that they have to be shown.

Anyway, that would be a separate exercise, it's a relatively minor thing.

       };
     }
 
     return info[message.what];
   });
 
   port.on("app.listen", (message, sender) =>
   {
     getListenerFilters(sender.page).app = message.filter;
   });
@@ skipping 236 matching lines @@
     if (message.url)
       subscriptions = [Subscription.fromURL(message.url)];
 
     for (let subscription of subscriptions)
     {
       if (subscription instanceof DownloadableSubscription)
         Synchronizer.execute(subscription, true);
     }
   });
 })(this);