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

Issue 29478597: Issue 5326 - General tab (HTML, strings and functionality) (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 2 weeks ago by saroyanm
Modified:
1 month, 4 weeks ago
Reviewers:
Thomas Greiner
Visibility:
Public.

Description

This review contains HTML/Semantics, strings and functional implementation excluding "Acceptable Ads notification" of General tab according to the specification ( https://bitbucket.org/adblockplus/spec/src/6eb0fc6e906a60ac76d93cf1ba9ae770e01f88df/spec/abp/options-page.md?at=master&fileviewer=file-view-default#markdown-header-general-tab ).

Patch Set 1 : #

Total comments: 109

Patch Set 2 : #

Total comments: 36

Patch Set 3 : #

Total comments: 13

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+398 lines, -444 lines) Patch
M background.js View 1 2 3 chunks +31 lines, -8 lines 0 comments Download
M locale/en-US/new-options.json View 1 2 3 4 7 chunks +85 lines, -89 lines 0 comments Download
M new-options.html View 1 2 3 4 3 chunks +72 lines, -99 lines 0 comments Download
M new-options.js View 1 2 3 4 19 chunks +142 lines, -130 lines 0 comments Download
M skin/new-options.css View 1 14 chunks +68 lines, -106 lines 0 comments Download
R skin/tooltips/acceptable-ads.png View Binary file 0 comments Download
R skin/tooltips/block.png View Binary file 0 comments Download
R skin/tooltips/more.png View Binary file 0 comments Download
M subscriptions.xml View 1 2 1 chunk +0 lines, -12 lines 0 comments Download

Messages

Total messages: 19
saroyanm
Ready for review https://codereview.adblockplus.org/29478597/diff/29498701/background.js File background.js (right): https://codereview.adblockplus.org/29478597/diff/29498701/background.js#newcode106 background.js:106: subscriptions_exceptionsurl_privacy: "https://easylist-downloads.adblockplus.org/exceptionrules-privacy.txt" The URL is subject ...
2 months, 3 weeks ago (2017-07-26 20:56:50 UTC) #1
Thomas Greiner
My main focus was on the JavaScript so I only did a rough review of ...
2 months, 1 week ago (2017-08-09 18:14:51 UTC) #2
saroyanm
https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-options.json#newcode58 locale/en-US/new-options.json:58: "options_aa_header": { On 2017/08/09 18:14:47, Thomas Greiner wrote: > ...
2 months, 1 week ago (2017-08-14 14:00:11 UTC) #3
Thomas Greiner
https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newcode90 new-options.js:90: if (item.url === acceptableAdsUrl) On 2017/08/14 14:00:10, saroyanm wrote: ...
2 months ago (2017-08-15 17:10:30 UTC) #4
saroyanm
New patch uploaded. Ready for review. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-options.json#newcode58 locale/en-US/new-options.json:58: "options_aa_header": { On ...
2 months ago (2017-08-16 14:17:39 UTC) #5
Thomas Greiner
https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-options.json#newcode58 locale/en-US/new-options.json:58: "options_aa_header": { On 2017/08/16 14:17:35, saroyanm wrote: > On ...
2 months ago (2017-08-16 17:57:11 UTC) #6
saroyanm
Thanks for reviewing Thomas, please see my comments. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newcode223 new-options.js:223: for ...
2 months ago (2017-08-17 11:21:19 UTC) #7
Thomas Greiner
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newcode394 new-options.js:394: collection = collections.custom; On 2017/08/17 11:21:19, saroyanm wrote: > ...
2 months ago (2017-08-17 12:06:06 UTC) #8
saroyanm
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newcode394 new-options.js:394: collection = collections.custom; On 2017/08/17 12:06:05, Thomas Greiner wrote: ...
2 months ago (2017-08-17 12:32:08 UTC) #9
Thomas Greiner
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newcode394 new-options.js:394: collection = collections.custom; On 2017/08/17 12:32:08, saroyanm wrote: > ...
2 months ago (2017-08-17 12:49:06 UTC) #10
saroyanm
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newcode394 new-options.js:394: collection = collections.custom; On 2017/08/17 12:49:06, Thomas Greiner wrote: ...
2 months ago (2017-08-17 14:06:18 UTC) #11
Thomas Greiner
On 2017/08/17 14:06:18, saroyanm wrote: > Thanks for clarifying that, in this case whole logic ...
2 months ago (2017-08-17 14:53:11 UTC) #12
saroyanm
On 2017/08/17 14:53:11, Thomas Greiner wrote: > On 2017/08/17 14:06:18, saroyanm wrote: > > Thanks ...
2 months ago (2017-08-17 15:03:03 UTC) #13
saroyanm
New patch ready for the review. I do have one small issue, which I want ...
2 months ago (2017-08-17 21:24:09 UTC) #14
Thomas Greiner
Unfortunately, I didn't have time yet to review your latest patch set but I quickly ...
2 months ago (2017-08-18 16:27:14 UTC) #15
saroyanm
https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-options.json#newcode133 locale/en-US/new-options.json:133: "options_more_malware_tooltip": { On 2017/08/18 16:27:14, Thomas Greiner wrote: > ...
2 months ago (2017-08-18 16:44:04 UTC) #16
Thomas Greiner
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newcode1261 new-options.js:1261: delete subscriptionsMap[subscription.url]; On 2017/08/17 21:24:07, saroyanm wrote: > On ...
1 month, 4 weeks ago (2017-08-22 16:08:59 UTC) #17
saroyanm
Hope it's closer to be ready, or in the best scenario ready. https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-options.json File locale/en-US/new-options.json ...
1 month, 4 weeks ago (2017-08-22 17:00:10 UTC) #18
Thomas Greiner
1 month, 4 weeks ago (2017-08-22 17:42:01 UTC) #19
LGTM
Sign in to reply to this message.

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