Issue 3969 - Hits are not counted for custom CSS property rules

Issue 3969 - Hits are not counted for custom CSS property rules

Repository: hg.adblockplus.org/adblockplus

[saroyanm, 2016-06-10 08:55:12]

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:85: // Invalid URL?
Detail: I'm not sure if this comment provides any value.

https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProper...
lib/child/cssProperties.js:94: // TODO: Show the actual matching selector here?
Detail: Feels like you forgot "TODO" Smth, or maybe remove the comment

https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProper...
lib/child/cssProperties.js:95: location: filter.replace(/^.*?##/, ""),
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.

[Wladimir Palant, 2016-06-10 14:59:54]

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:85: // Invalid URL?
On 2016/06/10 08:55:11, saroyanm wrote:
> Detail: I'm not sure if this comment provides any value.

Well, eslint objects to empty catch clauses. In fact, the comment explains the
error condition that we are ignoring here.

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 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.

https://codereview.adblockplus.org/29345639/diff/29345640/lib/child/cssProper...
lib/child/cssProperties.js:95: location: filter.replace(/^.*?##/, ""),
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.

[saroyanm, 2016-06-10 17:00:24]

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.