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

Issue 4559243822759936: Issue 431/432 - Remove special handling for the $sitekey option (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 7 months ago by Thomas Greiner
Modified:
5 years, 5 months ago
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

This review addresses both issues #431 and #432 due to them being somewhat intertwined. I'll create a separate review for #698 as soon as there's consent about the interfaces.

Patch Set 1 #

Total comments: 16

Patch Set 2 : #

Patch Set 3 : #

Total comments: 34

Patch Set 4 : #

Total comments: 2

Patch Set 5 : Rebased and added comment #

Patch Set 6 : Made changes for WebKit bug 132872 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+137 lines, -116 lines) Patch
M lib/contentPolicy.js View 1 2 3 7 chunks +54 lines, -30 lines 0 comments Download
M lib/filterClasses.js View 1 2 3 4 5 10 chunks +70 lines, -32 lines 0 comments Download
M lib/matcher.js View 1 12 chunks +13 lines, -54 lines 0 comments Download

Messages

Total messages: 11
Thomas Greiner
5 years, 7 months ago (2014-07-28 14:51:26 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/contentPolicy.js File lib/contentPolicy.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/contentPolicy.js#newcode175 lib/contentPolicy.js:175: match = Policy.isWhitelisted(testWndLocation, parentWndLocation, siteKey); The logic here seems ...
5 years, 6 months ago (2014-08-01 10:50:21 UTC) #2
Thomas Greiner
http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/contentPolicy.js File lib/contentPolicy.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/contentPolicy.js#newcode175 lib/contentPolicy.js:175: match = Policy.isWhitelisted(testWndLocation, parentWndLocation, siteKey); On 2014/08/01 10:50:21, Wladimir ...
5 years, 6 months ago (2014-08-11 17:18:15 UTC) #3
Thomas Greiner
The changes to appSupport.js were missing in the previous patch so I uploaded them now.
5 years, 6 months ago (2014-08-12 09:33:52 UTC) #4
Wladimir Palant
http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/appSupport.js File lib/appSupport.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/appSupport.js#newcode865 lib/appSupport.js:865: if (Policy.isWindowWhitelisted(exports.getBrowser(window).contentWindow)) Please revert the changes to this file. ...
5 years, 6 months ago (2014-08-14 19:58:15 UTC) #5
Thomas Greiner
http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/appSupport.js File lib/appSupport.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/appSupport.js#newcode865 lib/appSupport.js:865: if (Policy.isWindowWhitelisted(exports.getBrowser(window).contentWindow)) On 2014/08/14 19:58:15, Wladimir Palant wrote: > ...
5 years, 6 months ago (2014-08-29 15:57:29 UTC) #6
Wladimir Palant
Look good, only one issue below. http://codereview.adblockplus.org/4559243822759936/diff/5121099960418304/lib/filterClasses.js File lib/filterClasses.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5121099960418304/lib/filterClasses.js#newcode397 lib/filterClasses.js:397: if (this.sitekeys && ...
5 years, 6 months ago (2014-08-29 20:13:59 UTC) #7
Thomas Greiner
http://codereview.adblockplus.org/4559243822759936/diff/5121099960418304/lib/filterClasses.js File lib/filterClasses.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5121099960418304/lib/filterClasses.js#newcode397 lib/filterClasses.js:397: if (this.sitekeys && (!sitekey || this.sitekeys.indexOf(sitekey.toUpperCase()) < 0)) On ...
5 years, 5 months ago (2014-09-01 09:51:46 UTC) #8
Wladimir Palant
LGTM
5 years, 5 months ago (2014-09-01 11:49:53 UTC) #9
Thomas Greiner
I noticed that WebKit bug 132872 also affects this issue so I added the necessary ...
5 years, 5 months ago (2014-09-01 15:31:14 UTC) #10
Wladimir Palant
5 years, 5 months ago (2014-09-01 17:59:04 UTC) #11
Not sure whether this was really necessary, definitely won't hurt however. LGTM
Sign in to reply to this message.

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