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

Issue 29516684: Issue 5347 - Open options page on Firefox for Android when browser action is clicked (Closed)

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

Description

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

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

Messages

Total messages: 11
Manish Jethani
2 years, 1 month ago (2017-08-16 00:38:46 UTC) #1
Manish Jethani
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 ...
2 years, 1 month ago (2017-08-16 00:44:45 UTC) #2
Wladimir Palant
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#newcode686 ext/background.js:686: application != "fennec") This value comes from browser's internal ...
2 years, 1 month ago (2017-08-17 12:21:22 UTC) #3
Manish Jethani
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#newcode686 ext/background.js:686: application != "fennec") On 2017/08/17 12:21:21, Wladimir Palant wrote: ...
2 years, 1 month ago (2017-08-17 13:21:26 UTC) #4
Sebastian Noack
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#newcode686 ext/background.js:686: application != "fennec") On 2017/08/17 13:21:26, Manish Jethani wrote: ...
2 years, 1 month ago (2017-08-18 10:13:47 UTC) #5
Manish Jethani
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#newcode686 ext/background.js:686: application != ...
2 years, 1 month ago (2017-08-18 14:20:30 UTC) #6
Sebastian Noack
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#newcode680 ext/background.js:680: let {application} = require("info"); The temporary variable is redundant ...
2 years, 1 month ago (2017-08-18 17:46:25 UTC) #7
Wladimir Palant
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#newcode696 ...
2 years, 1 month ago (2017-08-18 21:03:36 UTC) #8
Manish Jethani
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): ...
2 years, 1 month ago (2017-08-19 03:43:42 UTC) #9
Wladimir Palant
LGTM
2 years, 1 month ago (2017-08-19 07:37:50 UTC) #10
Sebastian Noack
2 years ago (2017-08-21 12:20:24 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