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

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

Created:
July 17, 2017, 5:07 p.m. by hub
Modified:
Aug. 21, 2017, 7:59 p.m.
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
July 17, 2017, 5:07 p.m. (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 ...
July 17, 2017, 5:09 p.m. (2017-07-17 17:09:04 UTC) #2
hub
this is now on top of master.
July 19, 2017, 2:36 p.m. (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 ...
Aug. 16, 2017, 8:37 a.m. (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, ...
Aug. 16, 2017, 7:50 p.m. (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 ...
Aug. 17, 2017, 8:53 a.m. (2017-08-17 08:53:08 UTC) #6
hub
there is also the case if subSelector starts with a letter it while concatenate it ...
Aug. 19, 2017, 2:12 a.m. (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 ...
Aug. 19, 2017, 7:39 a.m. (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: ...
Aug. 21, 2017, 3:36 p.m. (2017-08-21 15:36:04 UTC) #9
Wladimir Palant
Aug. 21, 2017, 7:19 p.m. (2017-08-21 19:19:45 UTC) #10
LGTM

Powered by Google App Engine
This is Rietveld