Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(244)

Issue 29511561: Issue 5347 - Check for contextMenus API support (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by Manish Jethani
Modified:
2 years, 1 month ago
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -1 line) Patch
M ext/background.js View 1 2 3 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 11
Manish Jethani
2 years, 1 month ago (2017-08-10 07:34:41 UTC) #1
Manish Jethani
Patch Set 1 Firefox for Android does not support context menus, the chrome.contextMenus API is ...
2 years, 1 month ago (2017-08-10 07:40:06 UTC) #2
Manish Jethani
Patch Set 2: Use consistent style for feature detection
2 years, 1 month ago (2017-08-15 16:39:16 UTC) #3
Wladimir Palant
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#newcode79 ext/background.js:79: if ("contextMenus" in chrome) Please add a comment explaining ...
2 years, 1 month ago (2017-08-16 11:04:00 UTC) #4
Manish Jethani
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#newcode79 ext/background.js:79: if ...
2 years, 1 month ago (2017-08-16 12:09:45 UTC) #5
Sebastian Noack
LGTM
2 years, 1 month ago (2017-08-16 12:14:15 UTC) #6
Wladimir Palant
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#newcode425 ext/background.js:425: if ("contextMenus" in chrome) On 2017/08/16 12:09:45, Manish Jethani ...
2 years, 1 month ago (2017-08-16 13:21:57 UTC) #7
Manish Jethani
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#newcode425 ext/background.js:425: if ("contextMenus" in ...
2 years, 1 month ago (2017-08-16 19:39:09 UTC) #8
Wladimir Palant
LGTM
2 years, 1 month ago (2017-08-17 08:42:24 UTC) #9
Manish Jethani
On 2017/08/17 08:42:24, Wladimir Palant wrote: > LGTM Thanks, Wladimir. Sebastian, I'll wait for a ...
2 years, 1 month ago (2017-08-17 10:22:23 UTC) #10
Sebastian Noack
2 years, 1 month ago (2017-08-17 12:45:49 UTC) #11
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5