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

Issue 5225119261655040: Issue 1282 - Don't generate filters conflicting with existing exception rules (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 9 months ago by Sebastian Noack
Modified:
4 years, 6 months ago
Reviewers:
kzar, Wladimir Palant
Visibility:
Public.

Patch Set 1 : #

Total comments: 2

Patch Set 2 : Renamed addElemHideFilter() and simplified addStyleAttributeFilter() #

Patch Set 3 : Rebased #

Patch Set 4 : Rebased #

Patch Set 5 : Rebased #

Total comments: 15

Patch Set 6 : Rebased and addressed comments #

Patch Set 7 : Addressed comment #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+159 lines, -103 lines) Patch
M background.js View 1 2 3 4 5 6 1 chunk +12 lines, -4 lines 0 comments Download
M include.postload.js View 1 2 3 4 5 6 6 chunks +79 lines, -57 lines 0 comments Download
M include.preload.js View 1 chunk +4 lines, -1 line 0 comments Download
M lib/filterComposer.js View 1 2 3 4 5 6 2 chunks +64 lines, -41 lines 3 comments Download

Messages

Total messages: 17
Sebastian Noack
4 years, 9 months ago (2014-12-13 14:19:19 UTC) #1
Sebastian Noack
http://codereview.adblockplus.org/5225119261655040/diff/5629499534213120/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5629499534213120/include.postload.js#newcode552 include.postload.js:552: if (!whitelisted[url] && /^https?:/i.test(url)) The code below this line ...
4 years, 9 months ago (2014-12-13 14:32:37 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5225119261655040/diff/5724160613416960/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5724160613416960/include.postload.js#newcode548 include.postload.js:548: if (!whitelisted[url] && /^https?:/i.test(url)) The code below this line ...
4 years, 9 months ago (2014-12-13 18:29:00 UTC) #3
kzar
I guess your comment about indentation changes no longer apply? I couldn't see the changes ...
4 years, 9 months ago (2014-12-15 15:45:40 UTC) #4
Sebastian Noack
On 2014/12/15 15:45:40, kzar wrote: > I guess your comment about indentation changes no longer ...
4 years, 9 months ago (2014-12-15 15:58:42 UTC) #5
Sebastian Noack
I've uploaded a new patch set based on #1853. Having the filter generation in the ...
4 years, 8 months ago (2015-01-22 15:40:28 UTC) #6
kzar
A little hard to review since the rebase but that's more a fault of rietveld. ...
4 years, 7 months ago (2015-01-26 17:13:15 UTC) #7
kzar
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/include.postload.js#newcode465 include.postload.js:465: if (!clickHide_activated || currentElement != e.target) Wouldn't e.target not ...
4 years, 6 months ago (2015-03-02 18:30:08 UTC) #8
Sebastian Noack
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/include.postload.js#newcode465 include.postload.js:465: if (!clickHide_activated || currentElement != e.target) On 2015/03/02 18:30:08, ...
4 years, 6 months ago (2015-03-02 18:40:48 UTC) #9
kzar
LGTM http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/include.postload.js#newcode465 include.postload.js:465: if (!clickHide_activated || currentElement != e.target) On 2015/03/02 ...
4 years, 6 months ago (2015-03-02 18:42:28 UTC) #10
Wladimir Palant
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/background.js File background.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/background.js#newcode559 background.js:559: msg.baseURL, sender.page, sender.frame Ten positional parameters in a function ...
4 years, 6 months ago (2015-03-02 20:14:19 UTC) #11
Sebastian Noack
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/background.js File background.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/background.js#newcode559 background.js:559: msg.baseURL, sender.page, sender.frame On 2015/03/02 20:14:19, Wladimir Palant wrote: ...
4 years, 6 months ago (2015-03-03 14:28:59 UTC) #12
Wladimir Palant
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/background.js File background.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/background.js#newcode559 background.js:559: msg.baseURL, sender.page, sender.frame On 2015/03/03 14:29:00, Sebastian Noack wrote: ...
4 years, 6 months ago (2015-03-03 14:40:31 UTC) #13
Sebastian Noack
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/background.js File background.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/background.js#newcode559 background.js:559: msg.baseURL, sender.page, sender.frame On 2015/03/03 14:40:31, Wladimir Palant wrote: ...
4 years, 6 months ago (2015-03-03 14:59:27 UTC) #14
Wladimir Palant
LGTM http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/filterComposer.js File lib/filterComposer.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/filterComposer.js#newcode73 lib/filterComposer.js:73: * @param {Frame} details.frame The frame containing the ...
4 years, 6 months ago (2015-03-03 19:17:01 UTC) #15
Sebastian Noack
http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/filterComposer.js File lib/filterComposer.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/filterComposer.js#newcode73 lib/filterComposer.js:73: * @param {Frame} details.frame The frame containing the element ...
4 years, 6 months ago (2015-03-03 19:18:53 UTC) #16
Wladimir Palant
4 years, 6 months ago (2015-03-03 20:11:47 UTC) #17
http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/...
File lib/filterComposer.js (right):

http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/...
lib/filterComposer.js:73: * @param {Frame}    details.frame    The frame
containing the element
On 2015/03/03 19:18:54, Sebastian Noack wrote:
> Why not? This is official JSDoc syntax:
> http://usejsdoc.org/tags-param.html#parameters-with-properties

Yes, seems to be supported by our version as well:
https://code.google.com/p/jsdoc-toolkit/wiki/TagParam
Sign in to reply to this message.

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