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

Issue 29367031: Issue 4684 - Allow { and } in attribute and property selectors (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 11 months ago by Felix Dahlke
Modified:
2 years, 11 months ago
Reviewers:
Wladimir Palant
Base URL:
https://bitbucket.org/fhd/adblockpluscore
Visibility:
Public.

Description

Issue 4684 - Allow { and } in attribute and property selectors

Patch Set 1 #

Total comments: 1

Patch Set 2 : #

Total comments: 5

Patch Set 3 : Add a space after the escape sequence for { and } #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -3 lines) Patch
M chrome/content/elemHideEmulation.js View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M lib/filterClasses.js View 1 2 2 chunks +4 lines, -2 lines 0 comments Download
M test/filterClasses.js View 1 2 1 chunk +17 lines, -0 lines 0 comments Download

Messages

Total messages: 7
Felix Dahlke
I feel mildly bad about making that regexp more complex than it was. OTOH, this ...
2 years, 11 months ago (2016-12-08 10:13:36 UTC) #1
Wladimir Palant
We shouldn't risk introducing security issues here. I think the secure approach would be accepting ...
2 years, 11 months ago (2016-12-08 12:36:19 UTC) #2
Felix Dahlke
Not sure I like that approach better, dealing with braces could get pretty complex in ...
2 years, 11 months ago (2016-12-09 14:09:06 UTC) #3
Wladimir Palant
https://codereview.adblockplus.org/29367031/diff/29367165/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367031/diff/29367165/chrome/content/elemHideEmulation.js#newcode141 chrome/content/elemHideEmulation.js:141: .replace("\\x7B", "{").replace("\\x7D", "}"); This will incorrectly replace \x7B1234 which ...
2 years, 11 months ago (2016-12-13 14:32:16 UTC) #4
Felix Dahlke
Add a space after the escape sequence for { and }
2 years, 11 months ago (2016-12-13 16:01:47 UTC) #5
Felix Dahlke
New patch set is up. https://codereview.adblockplus.org/29367031/diff/29367165/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367031/diff/29367165/chrome/content/elemHideEmulation.js#newcode141 chrome/content/elemHideEmulation.js:141: .replace("\\x7B", "{").replace("\\x7D", "}"); On ...
2 years, 11 months ago (2016-12-13 16:04:48 UTC) #6
Wladimir Palant
2 years, 11 months ago (2016-12-13 16:29:13 UTC) #7
LGTM

https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.j...
lib/filterClasses.js:831: this.selector = selector.replace("{",
"\\x7B").replace("}", "\\x7D");
On 2016/12/13 16:04:48, Felix Dahlke wrote:
> Done. I gotta admit this is feeling hacky.

This is exactly how EHH is doing it - took a while to arrive at a reliable
solution.
Sign in to reply to this message.

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