Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4864767881641984: Issue 1528 - Implemented backend for general tab of new options page (Closed)

Created:
Jan. 21, 2015, 6:04 p.m. by Thomas Greiner
Modified:
June 9, 2015, 2:48 p.m.
Reviewers:
Felix Dahlke, saroyanm
Visibility:
Public.

Description

See http://codereview.adblockplus.org/6088024630755328/ for the UI review for the general tab. There are two methods not implemented which do exist in the current options page because they were not included in the design: - `Utils.checkLocalePrefixMatch` which is used for highlighting the subscription matching the browser locale - `Synchronizer.isExecuting` which is used as part of the download progress indicator in the current options page

Patch Set 1 #

Total comments: 4

Patch Set 2 : Fixed filter management in background.js #

Patch Set 3 : Added app.get/addonVersion and fixed app.listen/addSubscription #

Patch Set 4 : Added addSubscription querystring parameter for testing #

Total comments: 20

Patch Set 5 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+242 lines, -50 lines) Patch
M README.md View 1 2 3 1 chunk +13 lines, -0 lines 0 comments Download
M background.js View 1 2 3 4 6 chunks +100 lines, -21 lines 1 comment Download
M ext/background.js View 1 2 3 4 1 chunk +6 lines, -2 lines 0 comments Download
M messageResponder.js View 1 2 3 4 8 chunks +123 lines, -27 lines 0 comments Download

Messages

Total messages: 8
Thomas Greiner
Jan. 21, 2015, 6:51 p.m. (2015-01-21 18:51:00 UTC) #1
saroyanm
http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/background.js File background.js (right): http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/background.js#newcode129 background.js:129: var subscription = Subscription.fromURL("~user~786254"); Subscription is not defined here, ...
Jan. 26, 2015, 7:44 p.m. (2015-01-26 19:44:06 UTC) #2
Thomas Greiner
I noticed that I forgot to test the filter management within the test environment so ...
Jan. 27, 2015, 1:01 p.m. (2015-01-27 13:01:07 UTC) #3
saroyanm
On 2015/01/27 13:01:07, Thomas Greiner wrote: > I noticed that I forgot to test the ...
Jan. 27, 2015, 3:37 p.m. (2015-01-27 15:37:12 UTC) #4
Thomas Greiner
Updated the review to add the "app.get/addonVersion" endpoint (as promised at http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/options.html#newcode42). I also fixed ...
Jan. 30, 2015, 6:15 p.m. (2015-01-30 18:15:17 UTC) #5
Felix Dahlke
Looks good all in all, a few comments. I have yet to look more closely ...
May 28, 2015, 8:35 p.m. (2015-05-28 20:35:41 UTC) #6
Thomas Greiner
http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/background.js File background.js (right): http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/background.js#newcode77 background.js:77: SpecialSubscription: function(url) { On 2015/05/28 20:35:41, Felix H. Dahlke ...
June 8, 2015, 4:12 p.m. (2015-06-08 16:12:43 UTC) #7
Felix Dahlke
June 9, 2015, 1:04 p.m. (2015-06-09 13:04:46 UTC) #8
LGTM

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:29: var filterClasses = require("filterClasses");
On 2015/06/08 16:12:43, Thomas Greiner wrote:
> On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> > Nit: adblockpluschrome typically uses `with` for this IIRC, nicer IMO. But
> it's
> > fine by me as it is.
> 
> "Using with is not recommended, and is forbidden in ECMAScript 5 strict mode.
> The recommended alternative is to assign the object whose properties you want
to
> access to a temporary variable."
> 
> see
> https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Statements/with
> 
> Note that the "need for `with`" will be gone as soon as Chrome/Opera/Safari
> implement destructuring assignments which will allow us to write:
> 
> var {Filter, BlockingFilter} = require("filterClasses");

Oh, I wasn't aware that with isn't working in strict mode - then never mind of
course.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:180: case "filters.blocked":
On 2015/06/08 16:12:43, Thomas Greiner wrote:
> On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> > Nit: `filters.blocking` maybe?
> 
> This part was added by Wladimir and would require firstRun.js to be adapted
> accordingly. I just moved it to have the cases be sorted alphabetically. :)

Na, let's leave it alone then, not really worth changing existing code over.

Powered by Google App Engine
This is Rietveld