Issue 29516684: Issue 5347 - Open options page on Firefox for Android when browser action is clicked (Closed)
|
Created:
Aug. 16, 2017, 12:38 a.m. by Manish Jethani Modified:
Aug. 21, 2017, 2:04 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5347 - Open options page on Firefox for Android when browser action is clicked
Patch Set 1 #
Total comments: 9
Patch Set 2 : Check specifically for "fennec" #
Total comments: 2
Patch Set 3 : Remove unnecessary variable and add link to bug #MessagesTotal messages: 11
Patch Set 1 This needs to be combined with 29516569 (chrome.windows fixes). https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js#n... ext/background.js:322: if (!("getPopup" in chrome.browserAction)) We're checking for browserAction.getPopup, i.e. whether the popup is supported. If there's no popup, we open the options page directly. https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js#n... ext/background.js:678: ext.showOptions = callback => Note that now it's one function with the if...else moved inside. We have to do this because info.application becomes available only later. Also require isn't available at the time of defining this function, therefore we have to call it within the function body. https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js#n... ext/background.js:733: if (tabs && tabs.length > 0) Firefox returns null instead of an empty array.
https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js#n... ext/background.js:686: application != "fennec") This value comes from browser's internal data, not from user agent parsing or such. So you can actually rely on it saying "fennec" for Firefox Mobile, no need to test the other values.
https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js#n... ext/background.js:686: application != "fennec") On 2017/08/17 12:21:21, Wladimir Palant wrote: > This value comes from browser's internal data, not from user agent parsing or > such. So you can actually rely on it saying "fennec" for Firefox Mobile, no need > to test the other values. This information is fetched asynchronously, it may not be available when this function is called. https://hg.adblockplus.org/buildtools/file/ef4ad61830eb/templates/geckoInfo.j...
https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js#n... ext/background.js:686: application != "fennec") On 2017/08/17 13:21:26, Manish Jethani wrote: > On 2017/08/17 12:21:21, Wladimir Palant wrote: > > This value comes from browser's internal data, not from user agent parsing or > > such. So you can actually rely on it saying "fennec" for Firefox Mobile, no > need > > to test the other values. > > This information is fetched asynchronously, it may not be available when this > function is called. > > https://hg.adblockplus.org/buildtools/file/ef4ad61830eb/templates/geckoInfo.j... The functionality here is triggered by user interaction, and it seems practically impossible that the user triggers the option page through the user interface before initialization of info.application completes. On the other hand there are potentially other scenarios (e.g. Chromium based browsers with quirky user agent strings) where it will be set to "unknown".
Patch Set 2: Check specifically for "fennec" https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js#n... ext/background.js:686: application != "fennec") On 2017/08/18 10:13:47, Sebastian Noack wrote: > On 2017/08/17 13:21:26, Manish Jethani wrote: > > On 2017/08/17 12:21:21, Wladimir Palant wrote: > > > This value comes from browser's internal data, not from user agent parsing > or > > > such. So you can actually rely on it saying "fennec" for Firefox Mobile, no > > need > > > to test the other values. > > > > This information is fetched asynchronously, it may not be available when this > > function is called. > > > > > https://hg.adblockplus.org/buildtools/file/ef4ad61830eb/templates/geckoInfo.j... > > The functionality here is triggered by user interaction, and it seems > practically impossible that the user triggers the option page through the user > interface before initialization of info.application completes. On the other hand > there are potentially other scenarios (e.g. Chromium based browsers with quirky > user agent strings) where it will be set to "unknown". This can be mitigated by adding an additional check for info.platform == "gecko" Anyway, the worst that could happen is that it's a no-op on Android, that's no so bad especially because the chance of it happening is so miniscule. I've made the change. https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js#n... ext/background.js:696: if (chrome.runtime.lastError) Shouldn't this still call the callback even though there's an error? In general the caller should be allowed to deal with the error (reset state, update the UI, etc.). It's not a problem in this particular case though.
https://codereview.adblockplus.org/29516684/diff/29519618/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516684/diff/29519618/ext/background.js#n... ext/background.js:680: let {application} = require("info"); The temporary variable is redundant now, you could just use require("info").application, below.
LGTM but you probably want to address Sebastian's nit still. https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516684/diff/29516685/ext/background.js#n... ext/background.js:696: if (chrome.runtime.lastError) On 2017/08/18 14:20:30, Manish Jethani wrote: > Shouldn't this still call the callback even though there's an error? > > In general the caller should be allowed to deal with the error (reset state, > update the UI, etc.). Yes, we could easily do that if we start returning promises here. As things are now, the caller is only expecting to be notified on success.
Patch Set 3: Remove unnecessary variable and add link to bug https://codereview.adblockplus.org/29516684/diff/29519618/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516684/diff/29519618/ext/background.js#n... ext/background.js:680: let {application} = require("info"); On 2017/08/18 17:46:24, Sebastian Noack wrote: > The temporary variable is redundant now, you could just use > require("info").application, below. Done.
LGTM
LGTM 
|
