Issue 29544706: Noissue - Adapt check for devtools panel support for Firefox (Closed)
|
Created:
Sept. 14, 2017, 11:07 p.m. by Sebastian Noack Modified:
Sept. 25, 2017, 5:51 p.m. Visibility:
Public. |
DescriptionNoissue - Adapt check for devtools panel support for Firefox
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Use Services.vc.compare() #
Total comments: 7
Patch Set 3 : Rebase #Patch Set 4 : Use compareVersion from coreUtils #Patch Set 5 : Rebase #Patch Set 6 : Use parseInt() #MessagesTotal messages: 13
Without this additional logic the option to disable the "Adblock Plus" devtools panel wouldn't show up on Firefox, neither with the old nor with the new options page. Unfortunately, feature detection doesn't seem to be possible (which is the reason we were checking for the platform here in the first place). The chrome.devtools API is not available from the background page, but only from the devtools page. https://codereview.adblockplus.org/29544706/diff/29544718/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29544706/diff/29544718/messageResponder.js... messageResponder.js:188: info.application == "firefox" && I explicitly check for `application == "firefox" here, and not for `platform == "gecko"`, since the devtools panel API is not supported on Firefox Mobile.
https://codereview.adblockplus.org/29544706/diff/29544718/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29544706/diff/29544718/messageResponder.js... messageResponder.js:188: info.application == "firefox" && On 2017/09/14 23:30:49, Sebastian Noack wrote: > I explicitly check for `application == "firefox" here, and not for `platform == > "gecko"`, since the devtools panel API is not supported on Firefox Mobile. Acknowledged. https://codereview.adblockplus.org/29544706/diff/29544718/messageResponder.js... messageResponder.js:188: info.application == "firefox" && We'll have to use parseInt here since the application version can be something like "54.0.1", which becomes NaN when converted to a number simply. I would also use parentheses around the two checks to group them for clarity, but it's your call.
https://codereview.adblockplus.org/29544706/diff/29544718/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29544706/diff/29544718/messageResponder.js... messageResponder.js:188: info.application == "firefox" && On 2017/09/15 11:37:14, Manish Jethani wrote: > We'll have to use parseInt here since the application version can be something > like "54.0.1", which becomes NaN when converted to a number simply. Well spotted. Note that we usually use `Services.vc.compare()` to compare version strings.
https://codereview.adblockplus.org/29544706/diff/29544718/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29544706/diff/29544718/messageResponder.js... messageResponder.js:188: info.application == "firefox" && On 2017/09/15 12:20:04, Thomas Greiner wrote: > On 2017/09/15 11:37:14, Manish Jethani wrote: > > We'll have to use parseInt here since the application version can be something > > like "54.0.1", which becomes NaN when converted to a number simply. > > Well spotted. Note that we usually use `Services.vc.compare()` to compare > version strings. Done. https://codereview.adblockplus.org/29544706/diff/29545676/background.js File background.js (left): https://codereview.adblockplus.org/29544706/diff/29545676/background.js#oldco... background.js:448: window.Services = { This has been dead code. There is no global Services object in adblockpluschrome anymore.
https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js... messageResponder.js:191: Services.vc.compare(info.applicationVersion, "54") >= 0 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...
https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js... messageResponder.js:191: Services.vc.compare(info.applicationVersion, "54") >= 0 On 2017/09/15 22:10:21, Manish Jethani wrote: > 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... 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?
https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js... messageResponder.js:191: Services.vc.compare(info.applicationVersion, "54") >= 0 On 2017/09/15 22:33:22, Sebastian Noack wrote: > On 2017/09/15 22:10:21, Manish Jethani wrote: > > 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... > > 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? 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.
https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js... messageResponder.js:191: Services.vc.compare(info.applicationVersion, "54") >= 0 On 2017/09/18 16:20:02, Thomas Greiner wrote: > On 2017/09/15 22:33:22, Sebastian Noack wrote: > > On 2017/09/15 22:10:21, Manish Jethani wrote: > > > 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... > > > > 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? > > 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. I've submitted a patch to adblockpluscore, replacing Services.vc.compare, and updated this patch to use the new API right away.
https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js... messageResponder.js:191: Services.vc.compare(info.applicationVersion, "54") >= 0 On 2017/09/18 18:17:28, Sebastian Noack wrote: > I've submitted a patch to adblockpluscore, replacing Services.vc.compare, and > updated this patch to use the new API right away. 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. https://codereview.adblockplus.org/29544706/diff/29545676/messageResponder.js... messageResponder.js:191: Services.vc.compare(info.applicationVersion, "54") >= 0 On 2017/09/18 18:17:28, Sebastian Noack wrote: > I've submitted a patch to adblockpluscore, replacing Services.vc.compare, and > updated this patch to use the new API right away. LGTM, provided we also update the core dependency once that patch has landed.
LGTM
As discussed in the review of the adblockpluscore change, core will no longer export a version compare function. So back to parseInt(). As for consistency, this is how we already compare versions in similar scenarios in adblockpluschrome.
LGTM
LGTM 
|
