Issue 29511561: Issue 5347 - Check for contextMenus API support (Closed)
|
Created:
Aug. 10, 2017, 7:34 a.m. by Manish Jethani Modified:
Aug. 17, 2017, 2:09 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5347 - Check for contextMenus API support
Firefox for Android does not support context menus, the chrome.contextMenus API is missing. We can just check for the presence of this API and avoid all the unnecessary code (event handlers and so on) if the feature is not supported.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use consistent style for feature detection #
Total comments: 6
Patch Set 3 : Minimize changes and add comments #
Total comments: 4
Patch Set 4 : Remove unnecessary checks #MessagesTotal messages: 11
Patch Set 1 Firefox for Android does not support context menus, the chrome.contextMenus API is missing. We can just check for the presence of this API and avoid all the unnecessary code (event handlers and so on) if the feature is not supported. https://codereview.adblockplus.org/29511561/diff/29511562/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29511561/diff/29511562/ext/background.js#n... ext/background.js:425: if (chrome.contextMenus) Rietveld seems to have messed up the diff. I've only wrapped this code in "if (chrome.contextMenus)". https://codereview.adblockplus.org/29511561/diff/29511562/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29511561/diff/29511562/lib/filterComposer.... lib/filterComposer.js:187: if (chrome.contextMenus) Again, I've only wrapped this code in "if (chrome.contextMenus)".
Patch Set 2: Use consistent style for feature detection
https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js#n... ext/background.js:79: if ("contextMenus" in chrome) Please add a comment explaining which platform this check is needed for, linking to https://bugzilla.mozilla.org/show_bug.cgi?id=1269062. This will help knowing when the check can be removed in future. https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js#n... ext/background.js:425: if ("contextMenus" in chrome) If I see it correctly, the point of this check is merely avoiding to run too much code? Doesn't seem worth it, particularly given that it will only affect Firefox on Android (and even that probably not for long). I'd rather go for a smaller change here, only prevent errors but nothing beyond that. It seems that updateContextMenu is the only place where chrome.contextMenus is actually used - we can have that function return early if chrome.contextMenus doesn't exist. Other changes seem unnecessary.
Patch Set 3: Minimize changes and add comments https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js#n... ext/background.js:79: if ("contextMenus" in chrome) On 2017/08/16 11:04:00, Wladimir Palant wrote: > Please add a comment explaining which platform this check is needed for, linking > to https://bugzilla.mozilla.org/show_bug.cgi?id=1269062. This will help knowing > when the check can be removed in future. Done. https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js#n... ext/background.js:425: if ("contextMenus" in chrome) On 2017/08/16 11:04:00, Wladimir Palant wrote: > If I see it correctly, the point of this check is merely avoiding to run too > much code? Doesn't seem worth it, particularly given that it will only affect > Firefox on Android (and even that probably not for long). I'd rather go for a > smaller change here, only prevent errors but nothing beyond that. It seems that > updateContextMenu is the only place where chrome.contextMenus is actually used - > we can have that function return early if chrome.contextMenus doesn't exist. > Other changes seem unnecessary. Right, I was going for optimizing for mobile, but I agree that that should probably be a separate exercise. I think there's a good case for optimizing for mobile because of limited resources (battery, network, and so on). Done.
LGTM
https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js#n... ext/background.js:425: if ("contextMenus" in chrome) On 2017/08/16 12:09:45, Manish Jethani wrote: > Right, I was going for optimizing for mobile, but I agree that that should > probably be a separate exercise. > > I think there's a good case for optimizing for mobile because of limited > resources (battery, network, and so on). Respectfully, I disagree. Limited resources or not, this isn't a hot path, with it only being triggered when the user switches tabs. Considering that Mozilla is likely to add context menu soon, we should do as little work and keep the changes as localized as possible. https://codereview.adblockplus.org/29511561/diff/29517583/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29511561/diff/29517583/ext/background.js#n... ext/background.js:81: if ("contextMenus" in chrome) I don't think that this change is necessary. It should be ok to create this instance regardless, even if it isn't used. https://codereview.adblockplus.org/29511561/diff/29517583/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29511561/diff/29517583/lib/filterComposer.... lib/filterComposer.js:182: return; This change won't be necessary any more if my previous comment is addressed.
Patch Set 4: Remove unnecessary checks https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29511561/diff/29516661/ext/background.js#n... ext/background.js:425: if ("contextMenus" in chrome) On 2017/08/16 13:21:56, Wladimir Palant wrote: > On 2017/08/16 12:09:45, Manish Jethani wrote: > > > > I think there's a good case for optimizing for mobile because of limited > > resources (battery, network, and so on). > > Respectfully, I disagree. Limited resources or not, this isn't a hot path, with > it only being triggered when the user switches tabs. Yeah, I meant optimizing for mobile in general, not this specific piece of code. It's not just performance in terms of CPU/memory, it's also storage, network usage, the user experience, etc. For example Firefox for Android has the navigator.connection API [1] which tells you if you're on cellular or on Wi-Fi, so maybe use that to avoid updating the filter lists on cellular. Again, may not be the best example (EasyList is really like ~2.8 MB, even smaller compressed), but my point is that there may be things to do specifically for mobile besides just checking for API support. [1]: http://wicg.github.io/netinfo/#-dfn-connectiontype-dfn-enum https://codereview.adblockplus.org/29511561/diff/29517583/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29511561/diff/29517583/ext/background.js#n... ext/background.js:81: if ("contextMenus" in chrome) On 2017/08/16 13:21:56, Wladimir Palant wrote: > I don't think that this change is necessary. It should be ok to create this > instance regardless, even if it isn't used. Done. https://codereview.adblockplus.org/29511561/diff/29517583/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29511561/diff/29517583/lib/filterComposer.... lib/filterComposer.js:182: return; On 2017/08/16 13:21:56, Wladimir Palant wrote: > This change won't be necessary any more if my previous comment is addressed. Done.
LGTM
On 2017/08/17 08:42:24, Wladimir Palant wrote: > LGTM Thanks, Wladimir. Sebastian, I'll wait for a go-ahead from you before I commit and push (also on 29509573).
LGTM 
|
