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

Issue 29490698: Issue 5422 - Properly build props selector (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by hub
Modified:
2 years, 6 months ago
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 5422 - Properly build props selector

Patch Set 1 #

Patch Set 2 : Rebased on master #

Total comments: 4

Patch Set 3 : Changes following review. #

Total comments: 2

Patch Set 4 : Updated following review. #

Total comments: 2

Patch Set 5 : Another nit #

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

Messages

Total messages: 10
hub
2 years, 7 months ago (2017-07-17 17:07:09 UTC) #1
hub
this is on top of the patches for issues 5395 and 5404 on master. I'm ...
2 years, 7 months ago (2017-07-17 17:09:04 UTC) #2
hub
this is now on top of master.
2 years, 7 months ago (2017-07-19 14:36:12 UTC) #3
Wladimir Palant
This needs at least one unit test, one that fails without your changes. I wouldn't ...
2 years, 6 months ago (2017-08-16 08:37:02 UTC) #4
hub
https://codereview.adblockplus.org/29490698/diff/29492588/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29490698/diff/29492588/chrome/content/elemHideEmulation.js#newcode277 chrome/content/elemHideEmulation.js:277: prefix + " " : prefix; On 2017/08/16 08:37:02, ...
2 years, 6 months ago (2017-08-16 19:50:32 UTC) #5
Wladimir Palant
Unfortunately, this isn't going to be the end of it. Looking through https://www.w3.org/TR/css3-selectors/, `foo[bar]:-abp-properties(...)` will ...
2 years, 6 months ago (2017-08-17 08:53:08 UTC) #6
hub
there is also the case if subSelector starts with a letter it while concatenate it ...
2 years, 6 months ago (2017-08-19 02:12:51 UTC) #7
Wladimir Palant
https://codereview.adblockplus.org/29490698/diff/29519689/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29490698/diff/29519689/chrome/content/elemHideEmulation.js#newcode285 chrome/content/elemHideEmulation.js:285: subSelector = subSelector.substr(1); Nit: we require brackets around the ...
2 years, 6 months ago (2017-08-19 07:39:13 UTC) #8
hub
https://codereview.adblockplus.org/29490698/diff/29519689/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29490698/diff/29519689/chrome/content/elemHideEmulation.js#newcode285 chrome/content/elemHideEmulation.js:285: subSelector = subSelector.substr(1); On 2017/08/19 07:39:13, Wladimir Palant wrote: ...
2 years, 6 months ago (2017-08-21 15:36:04 UTC) #9
Wladimir Palant
2 years, 6 months ago (2017-08-21 19:19:45 UTC) #10
LGTM
Sign in to reply to this message.

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