[subresource_filter] Use the SubresourceFilterObserver for PageLoadMetrics

[subresource_filter] Use the SubresourceFilterObserver for PageLoadMetrics

This was the final non-test client of the ActivationDecision persisted in the
driver factory. Follow up CLs will remove that member.

BUG=717590
Review-Url: https://codereview.chromium.org/2893183002
Cr-Commit-Position: refs/heads/master@{#477507}
Committed: https://chromium.googlesource.com/chromium/src/+/ded86c6fc10f0fde6d3dba373fffc002289e3fbd

[Charlie Harrison, 2017-05-21 21:19:55]

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

[commit-bot: I haz the power, 2017-05-21 21:20:03]

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-21 21:21:20]

Description was changed from

==========
[subresource_filter] Use the SubresourceFilterObserver for PageLoadMetrics

BUG=717590
==========

to

==========
[subresource_filter] Use the SubresourceFilterObserver for PageLoadMetrics

This was the final non-test client of the ActivationDecision persisted in the
driver factory. Follow up CLs will remove that member.

BUG=717590
==========

[commit-bot: I haz the power, 2017-05-21 22:02:51]

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

[commit-bot: I haz the power, 2017-05-21 22:02:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-05-22 15:03:02]

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

[commit-bot: I haz the power, 2017-05-22 15:03:20]

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-22 15:06:34]

csharrison@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Charlie Harrison, 2017-05-22 15:06:34]

bmcquade: here's the CL hooking up the metrics observer to the subresource
filter observer. We also change the semantics of the observer to notify even for
ACTIVATION_DISABLED, which the throttle manager is already robust to.

[commit-bot: I haz the power, 2017-05-22 15:50:51]

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

[commit-bot: I haz the power, 2017-05-22 15:50:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-05-22 16:19:52]

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

[commit-bot: I haz the power, 2017-05-22 16:20:13]

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-22 17:55:15]

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

[commit-bot: I haz the power, 2017-05-22 17:55:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bryan McQuade, 2017-05-23 19:31:58]

Thanks! A couple comments / questions.

https://codereview.chromium.org/2893183002/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2893183002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:162:
if (observer_manager)
what are the cases where observer manager won't exist? will it not exist if
filtering is disabled for instance?

https://codereview.chromium.org/2893183002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:168:
SubresourceFilterMetricsObserver::OnCommit(
i think we discussed this, but the activation decision will have always been
reported before commit, is that right?

is there any way we can audit this? perhaps if we're registered as a listener,
we can dcheck that activation_decision_.has_value()?

[Charlie Harrison, 2017-05-23 19:58:28]

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

[Charlie Harrison, 2017-05-23 19:58:51]

Thanks, I also added some comments to the observer

https://codereview.chromium.org/2893183002/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2893183002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:162:
if (observer_manager)
On 2017/05/23 19:31:57, Bryan McQuade wrote:
> what are the cases where observer manager won't exist? will it not exist if
> filtering is disabled for instance?

Only tests :/ I can add a comment.

https://codereview.chromium.org/2893183002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:168:
SubresourceFilterMetricsObserver::OnCommit(
On 2017/05/23 19:31:57, Bryan McQuade wrote:
> i think we discussed this, but the activation decision will have always been
> reported before commit, is that right?
> 
> is there any way we can audit this? perhaps if we're registered as a listener,
> we can dcheck that activation_decision_.has_value()?

Done.

[commit-bot: I haz the power, 2017-05-23 19:59:27]

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-23 20:04:07]

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

[commit-bot: I haz the power, 2017-05-23 20:04:08]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-05-24 01:46:40]

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

[commit-bot: I haz the power, 2017-05-24 01:48:27]

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-24 03:58:25]

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

[commit-bot: I haz the power, 2017-05-24 03:58:51]

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-24 04:13:47]

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

[commit-bot: I haz the power, 2017-05-24 04:13:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2017-06-02 20:51:45]

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

[commit-bot: I haz the power, 2017-06-02 20:52:34]

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-06-02 20:52:41]

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

[commit-bot: I haz the power, 2017-06-02 20:53:29]

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-06-02 22:10:46]

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

[commit-bot: I haz the power, 2017-06-02 22:10:47]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Bryan McQuade, 2017-06-03 01:51:08]

On 2017/05/23 at 19:58:51, csharrison wrote:
> Thanks, I also added some comments to the observer
> 
>
https://codereview.chromium.org/2893183002/diff/40001/chrome/browser/page_loa...
> File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):
> 
>
https://codereview.chromium.org/2893183002/diff/40001/chrome/browser/page_loa...
>
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:162:
if (observer_manager)
> On 2017/05/23 19:31:57, Bryan McQuade wrote:
> > what are the cases where observer manager won't exist? will it not exist if
> > filtering is disabled for instance?
> 
> Only tests :/ I can add a comment.
> 
>
https://codereview.chromium.org/2893183002/diff/40001/chrome/browser/page_loa...
>
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:168:
SubresourceFilterMetricsObserver::OnCommit(
> On 2017/05/23 19:31:57, Bryan McQuade wrote:
> > i think we discussed this, but the activation decision will have always been
> > reported before commit, is that right?
> > 
> > is there any way we can audit this? perhaps if we're registered as a
listener,
> > we can dcheck that activation_decision_.has_value()?
> 
> Done.

Thanks again for making this change! Looks like an assertion is still firing
somewhere so causing tests to go red so I will hold off on reviewing until
that's fixed. Happy to take a look on Monday.

[Charlie Harrison, 2017-06-04 03:47:39]

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

[commit-bot: I haz the power, 2017-06-04 03:47:47]

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-06-04 05:43:46]

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

[commit-bot: I haz the power, 2017-06-04 05:43:47]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Charlie Harrison, 2017-06-04 14:17:07]

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

[commit-bot: I haz the power, 2017-06-04 14:17:12]

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-06-04 14:49:50]

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

[commit-bot: I haz the power, 2017-06-04 14:49:51]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-06-04 22:05:36]

Sorry about that, bad rebase. Things should be working now.

[Charlie Harrison, 2017-06-05 12:32:10]

csharrison@chromium.org changed reviewers:
+ pkasting@chromium.org

[Charlie Harrison, 2017-06-05 12:32:11]

+pkasting for tab_helpers. Previously the component would be initialized in a
weird non-functioning state if the feature is not enabled. This change prevents
the tab helper from even being created which is a bit more consistent.

[Bryan McQuade, 2017-06-05 13:47:04]

Thanks!

https://codereview.chromium.org/2893183002/diff/140001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2893183002/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:176:
DCHECK(!scoped_observer_.IsObservingSources() ||
if we get to the commit and we're no longer observing, is it possible for
subresource filtering to be active for the current load?

if so, maybe add a comment to explain that case since i was under the impression
that could not happen.

if not, should we just stop observing at this point? are there any metrics we
want to log in this observer, in cases where we don't have an activation
decision?

maybe we could do:

// If |this| is no longer observing, subresource filtering can't activate for
this load,
// so stop observing.
if (!scoped_observer_.IsObservingSources())
  return STOP_OBSERVING;

LogActivationDecisionMetrics(navigation_handle, *activation_decision_);
return CONTINUE_OBSERVING;

https://codereview.chromium.org/2893183002/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:320:
scoped_observer_.RemoveAll();
does it make sense to move this logic into the base interface? are are there
some cases where an implementer wouldn't want this implementation?

https://codereview.chromium.org/2893183002/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:327:
activation_decision_ = activation_decision;
can we add a DCHECK(!did_commit_); here (and set the bool to true in the
OnCommit callback)? I want to make sure that we can never invoke this method
after the nav has committed. It looks like the current arch prevents that, but
the path to invoke this callback is nontrivial with various callbacks being
invoked, and it's not impossible that a refactor in the future could lead us to
a place where the ordering breaks. My guess is that's unlikely, but I'd feel
better knowing we DCHECK it here.

and while we are in here, let's also DCHECK(!activation_decision_); just to
verify that this only gets invoked once per page load.

[Charlie Harrison, 2017-06-05 15:33:23]

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

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

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-06-05 16:26:13]

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

[commit-bot: I haz the power, 2017-06-05 16:26:14]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Charlie Harrison, 2017-06-05 17:23:36]

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

[commit-bot: I haz the power, 2017-06-05 17:24:01]

Dry run: CQ is trying da patch.

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

[Peter Kasting, 2017-06-05 18:30:02]

LGTM

[Charlie Harrison, 2017-06-05 18:32:38]

Thanks Peter,
Bryan, would you take another look?

https://codereview.chromium.org/2893183002/diff/140001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2893183002/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:176:
DCHECK(!scoped_observer_.IsObservingSources() ||
On 2017/06/05 13:47:04, Bryan McQuade wrote:
> if we get to the commit and we're no longer observing, is it possible for
> subresource filtering to be active for the current load?
> 
> if so, maybe add a comment to explain that case since i was under the
impression
> that could not happen.
> 
> if not, should we just stop observing at this point? are there any metrics we
> want to log in this observer, in cases where we don't have an activation
> decision?
> 
> maybe we could do:
> 
> // If |this| is no longer observing, subresource filtering can't activate for
> this load,
> // so stop observing.
> if (!scoped_observer_.IsObservingSources())
>   return STOP_OBSERVING;
> 
> LogActivationDecisionMetrics(navigation_handle, *activation_decision_);
> return CONTINUE_OBSERVING;

As discussed offline there is some confusion here. We should never stop
observing once we've started (unless we are tearing down the web contents). We
should also never really be in a non-observing state. Currently we are for unit
tests but I've updated the CL so we can immediately stop observing if there
isn't a SubresourceFilterObserverManager.
Does this make sense?

https://codereview.chromium.org/2893183002/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:320:
scoped_observer_.RemoveAll();
On 2017/06/05 13:47:04, Bryan McQuade wrote:
> does it make sense to move this logic into the base interface? are are there
> some cases where an implementer wouldn't want this implementation?

I'm not sure. I think most implementations will want this but I'd like to keep
this interface abstract if possible to reduce multiple inheritance. I also don't
want to force observers into using scoped_observer.

https://codereview.chromium.org/2893183002/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:327:
activation_decision_ = activation_decision;
On 2017/06/05 13:47:04, Bryan McQuade wrote:
> can we add a DCHECK(!did_commit_); here (and set the bool to true in the
> OnCommit callback)? I want to make sure that we can never invoke this method
> after the nav has committed. It looks like the current arch prevents that, but
> the path to invoke this callback is nontrivial with various callbacks being
> invoked, and it's not impossible that a refactor in the future could lead us
to
> a place where the ordering breaks. My guess is that's unlikely, but I'd feel
> better knowing we DCHECK it here.
> 
> and while we are in here, let's also DCHECK(!activation_decision_); just to
> verify that this only gets invoked once per page load.

Done, great idea.

[commit-bot: I haz the power, 2017-06-05 18:34:20]

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

[commit-bot: I haz the power, 2017-06-05 18:34:21]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-06-07 00:03:24]

Thanks! LGTM - just one question / idea (optional).

https://codereview.chromium.org/2893183002/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2893183002/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:323:
scoped_observer_.RemoveAll();
can this ever be invoked before commit, in which case we might not get an
activation decision? should we guard against this possibility in OnCommit?

[Charlie Harrison, 2017-06-07 00:26:40]

https://codereview.chromium.org/2893183002/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2893183002/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:323:
scoped_observer_.RemoveAll();
On 2017/06/07 00:03:24, Bryan McQuade wrote:
> can this ever be invoked before commit, in which case we might not get an
> activation decision? should we guard against this possibility in OnCommit?

It shouldn't happen but we can guard against it in case the API changes. I've
added an early return if we don't have an activation_decision_.

[Charlie Harrison, 2017-06-07 00:27:22]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-06-07 00:27:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2893183002/#ps200001
(title: "bmcquade review")

[commit-bot: I haz the power, 2017-06-07 00:27:45]

CQ is trying da patch.

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

[Bryan McQuade, 2017-06-07 00:28:00]

LGTM, thanks!

https://codereview.chromium.org/2893183002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2893183002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:185:
DCHECK(scoped_observer_.IsObservingSources());
this DCHECK could probably be removed, but it's optional

[Charlie Harrison, 2017-06-07 01:15:10]

https://codereview.chromium.org/2893183002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2893183002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:185:
DCHECK(scoped_observer_.IsObservingSources());
On 2017/06/07 00:28:00, Bryan McQuade wrote:
> this DCHECK could probably be removed, but it's optional

Yeah I decided to keep it, just to enforce that we shouldn't have stopped
observing even if we've seen the activation decision callback.

[commit-bot: I haz the power, 2017-06-07 01:39:20]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496795242081020,
"parent_rev": "193094009c210267cfa2c194ec9875711ccaf647", "commit_rev":
"ded86c6fc10f0fde6d3dba373fffc002289e3fbd"}

[commit-bot: I haz the power, 2017-06-07 01:39:28]

Description was changed from

==========
[subresource_filter] Use the SubresourceFilterObserver for PageLoadMetrics

This was the final non-test client of the ActivationDecision persisted in the
driver factory. Follow up CLs will remove that member.

BUG=717590
==========

to

==========
[subresource_filter] Use the SubresourceFilterObserver for PageLoadMetrics

This was the final non-test client of the ActivationDecision persisted in the
driver factory. Follow up CLs will remove that member.

BUG=717590

Review-Url: https://codereview.chromium.org/2893183002
Cr-Commit-Position: refs/heads/master@{#477507}
Committed:
https://chromium.googlesource.com/chromium/src/+/ded86c6fc10f0fde6d3dba373fff...
==========

[commit-bot: I haz the power, 2017-06-07 01:39:31]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/ded86c6fc10f0fde6d3dba373fff...