Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4829486721794048: Issue 700 - Generate filters based on the style attribute as last resort (Closed)

Created:
June 24, 2014, 9:05 a.m. by Sebastian Noack
Modified:
Nov. 4, 2014, 3:44 p.m.
Visibility:
Public.

Description

Issue 700 - Make the "Block Element" dialog generate filters based on arbitrary attributes

Patch Set 1 #

Patch Set 2 : Apply fallback only on style and only if at least 2 rules are given #

Patch Set 3 : Added escaping again #

Patch Set 4 : Rebased #

Patch Set 5 : Rebased and escaped backslashes #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -15 lines) Patch
M include.postload.js View 1 2 3 4 3 chunks +37 lines, -15 lines 5 comments Download

Messages

Total messages: 15
Sebastian Noack
June 24, 2014, 9:08 a.m. (2014-06-24 09:08:09 UTC) #1
Wladimir Palant
This is a footgun. The "Block Element" dialog already is dangerous as it is, the ...
June 24, 2014, 10:46 a.m. (2014-06-24 10:46:35 UTC) #2
Sebastian Noack
On 2014/06/24 10:46:35, Wladimir Palant wrote: > This is a footgun. The "Block Element" dialog ...
June 24, 2014, 11 a.m. (2014-06-24 11:00:04 UTC) #3
Wladimir Palant
On 2014/06/24 11:00:04, Sebastian Noack wrote: > However, given that we only generate the fallback ...
June 24, 2014, 11:10 a.m. (2014-06-24 11:10:51 UTC) #4
Sebastian Noack
On 2014/06/24 11:10:51, Wladimir Palant wrote: > On 2014/06/24 11:00:04, Sebastian Noack wrote: > > ...
June 24, 2014, 11:25 a.m. (2014-06-24 11:25:18 UTC) #5
Sebastian Noack
On 2014/06/24 11:25:18, Sebastian Noack wrote: > On 2014/06/24 11:10:51, Wladimir Palant wrote: > > ...
June 24, 2014, 11:33 a.m. (2014-06-24 11:33:26 UTC) #6
Wladimir Palant
On 2014/06/24 11:33:26, Sebastian Noack wrote: > How about making the fallback less generic, by ...
June 24, 2014, 11:51 a.m. (2014-06-24 11:51:34 UTC) #7
Sebastian Noack
June 24, 2014, 12:52 p.m. (2014-06-24 12:52:37 UTC) #8
Sebastian Noack
On 2014/06/24 11:51:34, Wladimir Palant wrote: > On 2014/06/24 11:33:26, Sebastian Noack wrote: > > ...
Sept. 25, 2014, 8:47 a.m. (2014-09-25 08:47:36 UTC) #9
Sebastian Noack
Oct. 23, 2014, 10:38 a.m. (2014-10-23 10:38:16 UTC) #10
Wladimir Palant
LGTM http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/include.postload.js#newcode404 include.postload.js:404: selector += "." + elt.classList[i].replace(/([^\w-])/, "\\$1"); Somehow I ...
Nov. 3, 2014, 6:11 p.m. (2014-11-03 18:11:33 UTC) #11
Thomas Greiner
Just one minor issue http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/include.postload.js#newcode403 include.postload.js:403: for (var i = 0; ...
Nov. 4, 2014, 9:53 a.m. (2014-11-04 09:53:21 UTC) #12
Sebastian Noack
http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/include.postload.js#newcode403 include.postload.js:403: for (var i = 0; i < elt.classList.length; i++) ...
Nov. 4, 2014, 2:51 p.m. (2014-11-04 14:51:07 UTC) #13
Thomas Greiner
http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/include.postload.js#newcode403 include.postload.js:403: for (var i = 0; i < elt.classList.length; i++) ...
Nov. 4, 2014, 3:15 p.m. (2014-11-04 15:15:53 UTC) #14
Sebastian Noack
Nov. 4, 2014, 3:44 p.m. (2014-11-04 15:44:54 UTC) #15
http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
include.postload.js:403: for (var i = 0; i < elt.classList.length; i++)
On 2014/11/04 15:15:53, Thomas Greiner wrote:
> On 2014/11/04 14:51:07, Sebastian Noack wrote:
> > On 2014/11/04 09:53:21, Thomas Greiner wrote:
> > > It's certainly not wrong to use elt.classList here but it's inconsistent
> since
> > > we've been using elementClasses in all of the lines above.
> > 
> > The patch has already been merged, but I might file a separate issue.
However,
> > I'm in the favor of getting rid of elementClasses (which is initialized by
> > splitting elt.className with two regexes) and using .classList everywhere
> > instead. Does anybody know why the original code did it that way in the
first
> > place?
> 
> That's totally fine with me. Looks like the origin of this code can be traced
> back to AdThwart when there was no support for classList yet.

For reference, https://issues.adblockplus.org/ticket/1522

Powered by Google App Engine
This is Rietveld