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

Issue 29345639: Issue 3969 - Hits are not counted for custom CSS property rules (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by Wladimir Palant
Modified:
3 years, 9 months ago
Reviewers:
saroyanm
Visibility:
Public.

Description

Issue 3969 - Hits are not counted for custom CSS property rules Repository: hg.adblockplus.org/adblockplus

Patch Set 1 #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+61 lines, -3 lines) Patch
M chrome/content/ui/sidebar.js View 1 chunk +1 line, -1 line 0 comments Download
M lib/child/cssProperties.js View 2 chunks +31 lines, -2 lines 8 comments Download
A lib/cssProperties.js View 1 chunk +28 lines, -0 lines 0 comments Download
M lib/main.js View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 4
Wladimir Palant
3 years, 10 months ago (2016-06-08 13:42:32 UTC) #1
saroyanm
https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProperties.js File lib/child/cssProperties.js (right): https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProperties.js#newcode85 lib/child/cssProperties.js:85: // Invalid URL? Detail: I'm not sure if this ...
3 years, 9 months ago (2016-06-10 08:55:12 UTC) #2
Wladimir Palant
https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProperties.js File lib/child/cssProperties.js (right): https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProperties.js#newcode85 lib/child/cssProperties.js:85: // Invalid URL? On 2016/06/10 08:55:11, saroyanm wrote: > ...
3 years, 9 months ago (2016-06-10 14:59:54 UTC) #3
saroyanm
3 years, 9 months ago (2016-06-10 17:00:24 UTC) #4
LGTM

https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProper...
File lib/child/cssProperties.js (right):

https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProper...
lib/child/cssProperties.js:94: // TODO: Show the actual matching selector here?
On 2016/06/10 14:59:54, Wladimir Palant wrote:
> On 2016/06/10 08:55:11, saroyanm wrote:
> > Detail: Feels like you forgot "TODO" Smth, or maybe remove the comment
> 
> No, I meant to have this TODO comment here. Addressing it requires changes to
> adblockpluscore meaning that I cannot do it right now.

Acknowledged.

https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProper...
lib/child/cssProperties.js:95: location: filter.replace(/^.*?##/, ""),
On 2016/06/10 14:59:54, Wladimir Palant wrote:
> On 2016/06/10 08:55:12, saroyanm wrote:
> > Why do we remove the URL part from the filter to assign to location ?
> Shouldn't
> > we assigne location/URL ? Currently in the "Address" column we are showing
the
> > "Property selector" part of the filter.
> 
> hit.location is normally the address that the filter matched, not the domain
of
> the document (we have hit.docDomain for that already). For element hiding
> filters there is no address however which is why we show the selector part of
> the filter instead (as it represents the matching element in a way). For CSS
> property filters we duplicated this behavior for now, we could improve it
> however by showing the *actual* selector rather than the selector part of the
> filter - hence the TODO comment.

Acknowledged.
Sign in to reply to this message.

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