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

Issue 6286955629248512: disable acceptable ads from first run page (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 9 months ago by saroyanm
Modified:
5 years, 9 months ago
Reviewers:
Wladimir Palant
CC:
Philip Hill
Visibility:
Public.

Description

This Review is related to current issue: https://issues.adblockplus.org/ticket/255

Patch Set 1 #

Total comments: 1

Patch Set 2 : trigger popstate event #

Total comments: 9

Patch Set 3 : Fixes after Wladimir comments #

Total comments: 4

Patch Set 4 : state declaration in runasync function #

Patch Set 5 : call getAddonsByTypes method #

Total comments: 5

Patch Set 6 : Description to getAddonsByTypes method and minore changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -0 lines) Patch
M lib/appSupport.js View 1 2 3 4 5 2 chunks +26 lines, -0 lines 0 comments Download

Messages

Total messages: 12
saroyanm
Wladimir can you please have a look on this review, I've updated the issue with ...
5 years, 9 months ago (2014-04-30 15:21:47 UTC) #1
Wladimir Palant
I don't think this is the right approach - we should override UI.openFiltersDialogs() in appSupport.js ...
5 years, 9 months ago (2014-05-01 18:00:32 UTC) #2
saroyanm
Thanks for your comment Wladimir, I really like your solution, the only thing that I ...
5 years, 9 months ago (2014-05-02 11:18:30 UTC) #3
Wladimir Palant
http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/appSupport.js File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/appSupport.js#newcode922 lib/appSupport.js:922: let window = this.currentWindow; Please use UI.currentWindow explicitly - ...
5 years, 9 months ago (2014-05-02 11:50:00 UTC) #4
saroyanm
Thanks for your notes Wladimir, New patch uploaded. http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/appSupport.js File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5639274879778816/lib/appSupport.js#newcode922 lib/appSupport.js:922: let ...
5 years, 9 months ago (2014-05-02 14:07:27 UTC) #5
Wladimir Palant
http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/appSupport.js File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/appSupport.js#newcode936 lib/appSupport.js:936: }, this, {id: require("info").addonID}); Passing in this and the ...
5 years, 9 months ago (2014-05-02 16:36:47 UTC) #6
saroyanm
New patch uploaded. Sorry for late answer. http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/appSupport.js File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/appSupport.js#newcode936 lib/appSupport.js:936: }, this, ...
5 years, 9 months ago (2014-05-03 08:02:05 UTC) #7
Wladimir Palant
http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/appSupport.js File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5693417237512192/lib/appSupport.js#newcode936 lib/appSupport.js:936: }, this, {id: require("info").addonID}); On 2014/05/03 08:02:06, saroyanm wrote: ...
5 years, 9 months ago (2014-05-03 17:54:18 UTC) #8
saroyanm
Wladimir I've updated the patch with your suggested solution which works more reliable (Replied also ...
5 years, 9 months ago (2014-05-04 14:05:08 UTC) #9
Wladimir Palant
http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/appSupport.js File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/appSupport.js#newcode933 lib/appSupport.js:933: AddonManager.getAddonsByTypes(["extension", "theme", "locale"], function() Please add a comment on ...
5 years, 9 months ago (2014-05-08 13:38:00 UTC) #10
saroyanm
Uploaded new patch, hope we are ready to go.. http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/appSupport.js File lib/appSupport.js (right): http://codereview.adblockplus.org/6286955629248512/diff/5643440998055936/lib/appSupport.js#newcode933 lib/appSupport.js:933: ...
5 years, 9 months ago (2014-05-08 14:42:11 UTC) #11
Wladimir Palant
5 years, 9 months ago (2014-05-08 18:03:09 UTC) #12
LGTM
Sign in to reply to this message.

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