[subresource_filter] Add debugging trace events around activation

[subresource_filter] Add debugging trace events around activation

This is the first of a few CLs adding helpful traces to the feature.

BUG=721512
Review-Url: https://codereview.chromium.org/2875033004
Cr-Commit-Position: refs/heads/master@{#471788}
Committed: https://chromium.googlesource.com/chromium/src/+/8e6d3357dce6196a843b7967695e16a4ae82c663

[Charlie Harrison, 2017-05-11 20:29:59]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 20:30:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 22:29:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 22:29:30]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-05-11 23:31:49]

csharrison@chromium.org changed reviewers:
+ engedy@chromium.org, shivanisha@chromium.org

[Charlie Harrison, 2017-05-11 23:31:50]

engedy, shivanisha: PTAL? I asked loading team if we could use their tracing
category and they were supportive.

[shivanisha, 2017-05-12 17:09:06]

On 2017/05/11 at 23:31:50, csharrison wrote:
> engedy, shivanisha: PTAL? I asked loading team if we could use their tracing
category and they were supportive.

lgtm

[engedy, 2017-05-15 12:10:38]

LGTM % nits.

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:124:
bool matched_config = highest_priority_activated_config !=
nit: s//has_activated_config/ for consistency with above.

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:126:
TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("loading"),
nit: The TRACE_EVENT in document_subresource_filter.cc:67 uses the category
"loader". This is intended, right?

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:357:
value->SetValue("Configuration", ConfigToDictionaryValue(*this));
nit: Note that this method is deprecated. Might want to discuss with primiano@.

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:112:
std::unique_ptr<base::trace_event::TracedValue> GetTracedValue() const;
nit: {As|To}TracedValue

[Charlie Harrison, 2017-05-15 15:14:11]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[Charlie Harrison, 2017-05-15 15:14:23]

engedy, can you take another look?

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:124:
bool matched_config = highest_priority_activated_config !=
On 2017/05/15 12:10:38, engedy wrote:
> nit: s//has_activated_config/ for consistency with above.

Done.

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:126:
TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("loading"),
On 2017/05/15 12:10:37, engedy wrote:
> nit: The TRACE_EVENT in document_subresource_filter.cc:67 uses the category
> "loader". This is intended, right?

I think "loading" is the category we should be using, and "loader" is
deprecated, though I don't think it is a very big deal. I can look into changing
that one in a followup after clarification with loading team.

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:357:
value->SetValue("Configuration", ConfigToDictionaryValue(*this));
On 2017/05/15 12:10:38, engedy wrote:
> nit: Note that this method is deprecated. Might want to discuss with
primiano@.

Discussed with Primiano who recommended we use the TracedValue::ToBaseValue to
generate the logging string. I've done that and moved operator<< to the
test_support file.

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2875033004/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:112:
std::unique_ptr<base::trace_event::TracedValue> GetTracedValue() const;
On 2017/05/15 12:10:38, engedy wrote:
> nit: {As|To}TracedValue

Renamed ToTracedValue.

[commit-bot: I haz the power, 2017-05-15 15:14:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-05-15 15:15:59]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 15:16:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-05-15 15:34:57]

LGTM, thanks!

[Charlie Harrison, 2017-05-15 15:36:13]

On 2017/05/15 15:34:57, engedy wrote:
> LGTM, thanks!

Thank you!

[Charlie Harrison, 2017-05-15 15:36:23]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-05-15 15:36:23]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-05-15 15:36:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
shivanisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2875033004/#ps60001
(title: "quick fix")

[commit-bot: I haz the power, 2017-05-15 15:36:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 16:33:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494862583312990,
"parent_rev": "5e7df37046f536af318339ae62570394a83c4abf", "commit_rev":
"8e6d3357dce6196a843b7967695e16a4ae82c663"}

[commit-bot: I haz the power, 2017-05-15 16:33:51]

Description was changed from

==========
[subresource_filter] Add debugging trace events around activation

This is the first of a few CLs adding helpful traces to the feature.

BUG=721512
==========

to

==========
[subresource_filter] Add debugging trace events around activation

This is the first of a few CLs adding helpful traces to the feature.

BUG=721512

Review-Url: https://codereview.chromium.org/2875033004
Cr-Commit-Position: refs/heads/master@{#471788}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e6d3357dce6196a843b7967695e...
==========

[commit-bot: I haz the power, 2017-05-15 16:33:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8e6d3357dce6196a843b7967695e...