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

Issue 29564735: Issue 5587 - Use options module (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
Reviewers:
Thomas Greiner
CC:
Sebastian Noack
Base URL:
https://hg.adblockplus.org/adblockplusui/
Visibility:
Public.

Description

Issue 5587 - Use options module

Patch Set 1 #

Total comments: 4

Patch Set 2 : Include check for mobile-options.html #

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

Messages

Total messages: 5
Manish Jethani
2 years, 1 month ago (2017-10-04 21:13:11 UTC) #1
Manish Jethani
Patch Set 1 Since we're moving ext.showOptions into a module, we need to update adblockplusui ...
2 years, 1 month ago (2017-10-04 21:15:34 UTC) #2
Thomas Greiner
https://codereview.adblockplus.org/29564735/diff/29564736/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29564735/diff/29564736/ext/background.js#oldcode83 ext/background.js:83: if (!/\/(?:mobile-)?options\.html/.test(top.location.href)) On 2017/10/04 21:15:33, Manish Jethani wrote: > ...
2 years, 1 month ago (2017-10-05 11:58:16 UTC) #3
Manish Jethani
Patch Set 2: Include check for mobile-options.html https://codereview.adblockplus.org/29564735/diff/29564736/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29564735/diff/29564736/ext/background.js#oldcode83 ext/background.js:83: if (!/\/(?:mobile-)?options\.html/.test(top.location.href)) ...
2 years, 1 month ago (2017-10-05 12:09:25 UTC) #4
Thomas Greiner
2 years, 1 month ago (2017-10-05 12:25:54 UTC) #5
LGTM

https://codereview.adblockplus.org/29564735/diff/29564736/ext/background.js
File ext/background.js (left):

https://codereview.adblockplus.org/29564735/diff/29564736/ext/background.js#o...
ext/background.js:83: if
(!/\/(?:mobile-)?options\.html/.test(top.location.href))
On 2017/10/05 12:09:25, Manish Jethani wrote:
> On 2017/10/05 11:58:16, Thomas Greiner wrote:
> > On 2017/10/04 21:15:33, Manish Jethani wrote:
> > > In the new version I've removed the "mobile-" part since we're planning to
> use
> > > options.html as the top page always.
> > 
> > We're only using options.html as the top page inside the extension. For the
> test
> > environment this top page doesn't exist though which is why this check is
> > necessary.
> 
> Ah, I see.
> 
> Made the change.

Thanks.
Sign in to reply to this message.

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