tools/android/loading: ContentClassificationLens, ads and tracking requests.

tools/android/loading: ContentClassificationLens, ads and tracking requests.

Adds a lens leveraging a rules file to tag whether a given request is
related to Ads and/or tracking/analytics. This CL also displays this in
the PNG output of the dependency graph.

Committed: https://crrev.com/4208a052792bde3b964aa443201ee6c66906f423
Cr-Commit-Position: refs/heads/master@{#371504}

[Benoit L, 2016-01-25 15:09:24]

lizeb@chromium.org changed reviewers:
+ blundell@chromium.org, mattcary@chromium.org

[mattcary, 2016-01-25 16:08:58]

lgtm

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/c...
File tools/android/loading/content_classification_lens.py (right):

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/c...
tools/android/loading/content_classification_lens.py:5: """Labels requests
according to the type of content they represent."""
Include instructions about where to get adblockparser from in case its not
installed?

If a user doesn't have it installed, it will cause an error in the imports of
this file, so a module comment should be a quick & easy way to make that
eventuality less painful.

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/c...
tools/android/loading/content_classification_lens.py:47: ans it's not the main
frame."""
ans -> and

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/c...
tools/android/loading/content_classification_lens.py:53: return
(float(ad_requests_count) / frame_requests_count) > .5
Is this 50% threshold necessary? I would think that all of the resources would
be ad-related.

At any rate, arbitrary thresholds always make me nervous.

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/l...
File tools/android/loading/loading_model.py (right):

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/l...
tools/android/loading/loading_model.py:500: node_info = self._NodeInfo(node,
request, is_ad, is_tracking)
I think it would be nicer to add setters to NodeInfo for tracking (or maybe even
a single SetRequestContent(is_ad, is_tracking) method, have NodeInfo default
those members to false, and then conditionally set them if we have a
content_lens.

IMHO the pattern of infinitely extending constructor parameters gets
unmaintainable, particularly in tests (true, that doesn't matter here, but
what's a code review if not a chance to preach general principles that don't
apply to the code in question?).

[blundell, 2016-01-25 16:33:33]

Please don't land this before we sort out the dependency issue.

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/i...
File tools/android/loading/install-deps.sh (right):

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/i...
tools/android/loading/install-deps.sh:7: sudo apt-get install libre2-1v5
libre2-dev
Hmm. Does this fall under the umbrella of adding third-party code in Chromium?

https://www.chromium.org/developers/adding-3rd-party-libraries

[chromium-reviews, 2016-01-26 08:22:11]

I think we need to evaluate how useful these libraries are before starting
the process to add them to 3rd party. As long as we all recognize that it's
a temporary situation--either we'll decide to use these and put them into
3rd party, or drop them--I think this is a necessary step. There has to be
an evaluation period.

On Mon, Jan 25, 2016 at 5:33 PM, <blundell@chromium.org> wrote:

> Please don't land this before we sort out the dependency issue.
>
>
>
>
https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/i...
> File tools/android/loading/install-deps.sh (right):
>
>
>
https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/i...
> tools/android/loading/install-deps.sh:7: sudo apt-get install libre2-1v5
> libre2-dev
> Hmm. Does this fall under the umbrella of adding third-party code in
> Chromium?
>
> https://www.chromium.org/developers/adding-3rd-party-libraries
>
> https://codereview.chromium.org/1626393002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Benoit L, 2016-01-26 13:31:13]

All done, thanks.

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/c...
File tools/android/loading/content_classification_lens.py (right):

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/c...
tools/android/loading/content_classification_lens.py:5: """Labels requests
according to the type of content they represent."""
On 2016/01/25 16:08:58, mattcary wrote:
> Include instructions about where to get adblockparser from in case its not
> installed?
> 
> If a user doesn't have it installed, it will cause an error in the imports of
> this file, so a module comment should be a quick & easy way to make that
> eventuality less painful.

Done.

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/c...
tools/android/loading/content_classification_lens.py:47: ans it's not the main
frame."""
On 2016/01/25 16:08:58, mattcary wrote:
> ans -> and

Done.

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/c...
tools/android/loading/content_classification_lens.py:53: return
(float(ad_requests_count) / frame_requests_count) > .5
On 2016/01/25 16:08:58, mattcary wrote:
> Is this 50% threshold necessary? I would think that all of the resources would
> be ad-related.
> 
> At any rate, arbitrary thresholds always make me nervous.

Changed to a configurable threshold.
In practice, not all requests from ad frames are marked as ads. I guess that
makes sense, since an ad frame can still use JQuery, for instance.

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/l...
File tools/android/loading/loading_model.py (right):

https://codereview.chromium.org/1626393002/diff/20001/tools/android/loading/l...
tools/android/loading/loading_model.py:500: node_info = self._NodeInfo(node,
request, is_ad, is_tracking)
On 2016/01/25 16:08:58, mattcary wrote:
> I think it would be nicer to add setters to NodeInfo for tracking (or maybe
even
> a single SetRequestContent(is_ad, is_tracking) method, have NodeInfo default
> those members to false, and then conditionally set them if we have a
> content_lens.
> 
> IMHO the pattern of infinitely extending constructor parameters gets
> unmaintainable, particularly in tests (true, that doesn't matter here, but
> what's a code review if not a chance to preach general principles that don't
> apply to the code in question?).

Absolutely, thanks for the suggestion.
Done.

[blundell, 2016-01-26 13:37:29]

lgtm

[Benoit L, 2016-01-26 13:40:58]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-01-26 13:40:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattcary@chromium.org
Link to the patchset: https://codereview.chromium.org/1626393002/#ps40001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-01-26 13:41:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1626393002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1626393002/40001

[commit-bot: I haz the power, 2016-01-26 13:59:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-26 14:00:44]

Description was changed from

==========
tools/android/loading: ContentClassificationLens, ads and tracking requests.

Adds a lens leveraging a rules file to tag whether a given request is
related to Ads and/or tracking/analytics. This CL also displays this in
the PNG output of the dependency graph.
==========

to

==========
tools/android/loading: ContentClassificationLens, ads and tracking requests.

Adds a lens leveraging a rules file to tag whether a given request is
related to Ads and/or tracking/analytics. This CL also displays this in
the PNG output of the dependency graph.

Committed: https://crrev.com/4208a052792bde3b964aa443201ee6c66906f423
Cr-Commit-Position: refs/heads/master@{#371504}
==========

[commit-bot: I haz the power, 2016-01-26 14:00:45]

Patchset 3 (id:??) landed as
https://crrev.com/4208a052792bde3b964aa443201ee6c66906f423
Cr-Commit-Position: refs/heads/master@{#371504}