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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 3 months ago by Sebastian Noack
Modified:
4 years, 11 months ago
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
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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: > > ...
5 years, 3 months ago (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: > > ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (2014-06-24 11:51:34 UTC) #7
Sebastian Noack
5 years, 3 months ago (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: > > ...
5 years ago (2014-09-25 08:47:36 UTC) #9
Sebastian Noack
4 years, 12 months ago (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 ...
4 years, 11 months ago (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; ...
4 years, 11 months ago (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++) ...
4 years, 11 months ago (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++) ...
4 years, 11 months ago (2014-11-04 15:15:53 UTC) #14
Sebastian Noack
4 years, 11 months ago (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
Sign in to reply to this message.

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