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

Issue 5138680696012800: Issue 616 - Enforce $generichide and $genericblock in Chrome (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 6 months ago by kzar
Modified:
3 years, 11 months ago
CC:
Wladimir Palant
Visibility:
Public.

Description

Issue 616 - Enforce $generichide and $genericblock in Chrome Depends on these changes to adblockplus: http://codereview.adblockplus.org/5840485868371968/ http://codereview.adblockplus.org/5991150368325632/ and these changes to adblockplustests: https://codereview.adblockplus.org/6439460933730304/

Patch Set 1 #

Total comments: 4

Patch Set 2 : Consider $genericblock when blocking popups. #

Patch Set 3 : Removed redundant logic in popup blocking changes. #

Total comments: 4

Patch Set 4 : Rebased. #

Patch Set 5 : Addressed feedback. #

Patch Set 6 : Rebased onto typeMask changes #

Patch Set 7 : Obey 80 character line limit #

Total comments: 3

Patch Set 8 : Rebased and updated adblockplus dependency #

Patch Set 9 : Updated adblockplustests dependency #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -9 lines) Patch
M background.js View 1 2 3 4 5 6 2 chunks +4 lines, -1 line 0 comments Download
M dependencies View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M popupBlocker.js View 1 2 3 4 5 6 2 chunks +16 lines, -5 lines 0 comments Download
M webrequest.js View 1 2 3 4 5 6 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 19
kzar
4 years, 6 months ago (2015-03-11 17:11:26 UTC) #1
Sebastian Noack
http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/background.js File background.js (right): http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/background.js#newcode474 background.js:474: var specificOnly = isFrameWhitelisted(sender.page, sender.frame, "GENERICHIDE"); I'm concerned about ...
4 years, 6 months ago (2015-03-12 12:36:12 UTC) #2
kzar
Patch Set 2 : Consider $genericblock when blocking popups. http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/background.js File background.js (right): http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/background.js#newcode474 background.js:474: ...
4 years, 6 months ago (2015-03-12 19:22:45 UTC) #3
kzar
Patch Set 3 : Removed redundant logic in popup blocking changes.
4 years, 6 months ago (2015-03-12 19:47:27 UTC) #4
Sebastian Noack
http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popupBlocker.js File popupBlocker.js (right): http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popupBlocker.js#newcode62 popupBlocker.js:62: function checkPotentialPopup(tabId, url, specificOnly, documentHost) Nit: I'd put the ...
4 years, 6 months ago (2015-03-12 19:59:36 UTC) #5
kzar
Patch Set 5 : Addressed feedback. http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popupBlocker.js File popupBlocker.js (right): http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popupBlocker.js#newcode62 popupBlocker.js:62: function checkPotentialPopup(tabId, url, ...
4 years, 6 months ago (2015-03-12 20:42:12 UTC) #6
Sebastian Noack
Looks good, beside my performance concerns of course.
4 years, 6 months ago (2015-03-12 20:44:28 UTC) #7
kzar
Patch Set 6 : Rebased onto typeMask changes
4 years, 2 months ago (2015-07-14 16:56:28 UTC) #8
Felix Dahlke
Same as in https://codereview.adblockplus.org/5840485868371968/ - moving Wladimir to CC, adding Thomas and me as reviewers.
4 years, 1 month ago (2015-08-20 10:15:07 UTC) #9
kzar
Patch Set 7 : Obey 80 character line limit
4 years ago (2015-08-26 10:21:16 UTC) #10
Thomas Greiner
https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocker.js File popupBlocker.js (right): https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocker.js#newcode54 popupBlocker.js:54: var source = tabsLoading[tabId]; Would make sense to check ...
4 years ago (2015-09-02 16:28:21 UTC) #11
kzar
https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocker.js File popupBlocker.js (right): https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocker.js#newcode54 popupBlocker.js:54: var source = tabsLoading[tabId]; On 2015/09/02 16:28:20, Thomas Greiner ...
4 years ago (2015-09-05 14:53:44 UTC) #12
Thomas Greiner
LGTM https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocker.js File popupBlocker.js (right): https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocker.js#newcode54 popupBlocker.js:54: var source = tabsLoading[tabId]; On 2015/09/05 14:53:44, kzar ...
4 years ago (2015-09-18 17:41:06 UTC) #13
Sebastian Noack
LGTM
3 years, 12 months ago (2015-09-21 14:25:09 UTC) #14
Felix Dahlke
LGTM
3 years, 11 months ago (2015-09-28 08:28:52 UTC) #15
kzar
Patch Set 8 : Rebased and updated adblockplus dependency We still need to wait for ...
3 years, 11 months ago (2015-09-28 17:21:49 UTC) #16
kzar
Patch Set 9 : Updated adblockplustests dependency (Hopefully the last change! If this gets the ...
3 years, 11 months ago (2015-10-05 09:56:57 UTC) #17
Sebastian Noack
LGTM
3 years, 11 months ago (2015-10-05 10:44:12 UTC) #18
Felix Dahlke
3 years, 11 months ago (2015-10-05 11:37:31 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