Adding UMA on page load size for pages with Media

Adding UMA on page load size for pages with Media

This CL plumbs a renderer behavior (MediaPlayed) from blink/ when a
media element is played. An observer records page size UMA on pages that
saw at least one media element get played.

BUG=698348
Review-Url: https://codereview.chromium.org/2732293003
Cr-Commit-Position: refs/heads/master@{#457275}
Committed: https://chromium.googlesource.com/chromium/src/+/09fb15d21c53769e466ea5a80bd3a170082e7bfe

[RyanSturm, 2017-03-07 22:09:20]

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

[commit-bot: I haz the power, 2017-03-07 22:10:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-03-07 23:49:27]

ryansturm@chromium.org changed reviewers:
+ csharrison@chromium.org, japhet@chromium.org, mlamouri@chromium.org,
rkaplow@chromium.org

[RyanSturm, 2017-03-07 23:49:28]

mlamouri: PTAL @ WebKit/Source/core/html/HTMLMediaElement.cpp

csharrison: PTAL @ page_load_metrics/ (or the entire CL)

rkaplow: PTAL @ histograms.xml

japhet:PTAL @ WebKit/public/platform/WebLoadingBehaviorFlag.h

Thanks!

https://codereview.chromium.org/2732293003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2732293003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:29: #include <limits>
git cl format keeps moving this header like this. Let me know if I should leave
it at the bottom of the list (I'm not sure what blink style says).

[commit-bot: I haz the power, 2017-03-07 23:58:29]

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

[commit-bot: I haz the power, 2017-03-07 23:58:30]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-03-08 05:39:53]

lgtm

[Charlie Harrison, 2017-03-08 14:33:58]

Generally looks good, just a few comments.

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.cc:47:
RecordByteHistograms();
Don't we want to Record only if the page commit?

Optional:
I think it would simplify the flow if this method was rewritten:

if (played_media_ && info.did_commit)
  RecordByteHistograms()
return STOP_OBSERVING;

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h:43:
bool played_media_;
nit: can you put the bool at the end to avoid padding?

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc:43:
void RunTest(bool simulate_play_media) {
nit: I would prefer if this method was renamed "SimulatePageLoad", and the
histogram testers were moved to the the actual tests.

https://codereview.chromium.org/2732293003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2732293003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:29: #include <limits>
On 2017/03/07 23:49:28, Ryan Sturm wrote:
> git cl format keeps moving this header like this. Let me know if I should
leave
> it at the bottom of the list (I'm not sure what blink style says).

Personally I am happy doing whatever git cl format thinks is right. At least
that's what I've been doing in Blink.

[RyanSturm, 2017-03-08 17:18:40]

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

[commit-bot: I haz the power, 2017-03-08 17:19:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-03-08 17:45:36]

csharrison: PTAL, thanks for spotting the info.did_commit error!

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.cc:47:
RecordByteHistograms();
On 2017/03/08 14:33:58, Charlie Harrison wrote:
> Don't we want to Record only if the page commit?
> 
> Optional:
> I think it would simplify the flow if this method was rewritten:
> 
> if (played_media_ && info.did_commit)
>   RecordByteHistograms()
> return STOP_OBSERVING;

Wow. Thanks for finding that. Added a test case.

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h:43:
bool played_media_;
On 2017/03/08 14:33:58, Charlie Harrison wrote:
> nit: can you put the bool at the end to avoid padding?

Done.

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2732293003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc:43:
void RunTest(bool simulate_play_media) {
On 2017/03/08 14:33:58, Charlie Harrison wrote:
> nit: I would prefer if this method was renamed "SimulatePageLoad", and the
> histogram testers were moved to the the actual tests.

Done.

[Charlie Harrison, 2017-03-08 17:55:29]

OK LGTM!

[Bryan McQuade, 2017-03-08 18:03:21]

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

[Bryan McQuade, 2017-03-08 18:03:22]

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2322:
document().loader()->didObserveLoadingBehavior(
one thing to note - this may prevent you from discovering loading behaviors in
child iframes (e.g. youtube iframe embeds). you may want to report the loading
behavior on the top-level document if you want to know about media loads in
child frames (and update the comment in the enum to note that this is reported
for media played in child frames)

[Bryan McQuade, 2017-03-08 18:10:11]

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2322:
document().loader()->didObserveLoadingBehavior(
On 2017/03/08 at 18:03:22, Bryan McQuade wrote:
> one thing to note - this may prevent you from discovering loading behaviors in
child iframes (e.g. youtube iframe embeds). you may want to report the loading
behavior on the top-level document if you want to know about media loads in
child frames (and update the comment in the enum to note that this is reported
for media played in child frames)

charles and i just chatted - due to OOPIF, we think the best way forward here is
to send IPCs for child frames, and then join things up browser-side. turns out
this bug affects my subresource filter page load metrics as well (i want to
track cases where subresources are filtered in child frames) so i'll take a look
at fixing this now.

[RyanSturm, 2017-03-08 18:17:11]

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2322:
document().loader()->didObserveLoadingBehavior(
On 2017/03/08 18:10:11, Bryan McQuade wrote:
> On 2017/03/08 at 18:03:22, Bryan McQuade wrote:
> > one thing to note - this may prevent you from discovering loading behaviors
in
> child iframes (e.g. youtube iframe embeds). you may want to report the loading
> behavior on the top-level document if you want to know about media loads in
> child frames (and update the comment in the enum to note that this is reported
> for media played in child frames)
> 
> charles and i just chatted - due to OOPIF, we think the best way forward here
is
> to send IPCs for child frames, and then join things up browser-side. turns out
> this bug affects my subresource filter page load metrics as well (i want to
> track cases where subresources are filtered in child frames) so i'll take a
look
> at fixing this now.

Agreed. I made the bad assumption that this was already happening due to the
other places that use this. Keep more on board with the solution that you are
considering for tracking child frames in the browser MetricsWCO.

[Bryan McQuade, 2017-03-13 13:18:08]

On 2017/03/08 at 18:17:11, ryansturm wrote:
>
https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2322:
document().loader()->didObserveLoadingBehavior(
> On 2017/03/08 18:10:11, Bryan McQuade wrote:
> > On 2017/03/08 at 18:03:22, Bryan McQuade wrote:
> > > one thing to note - this may prevent you from discovering loading
behaviors in
> > child iframes (e.g. youtube iframe embeds). you may want to report the
loading
> > behavior on the top-level document if you want to know about media loads in
> > child frames (and update the comment in the enum to note that this is
reported
> > for media played in child frames)
> > 
> > charles and i just chatted - due to OOPIF, we think the best way forward
here is
> > to send IPCs for child frames, and then join things up browser-side. turns
out
> > this bug affects my subresource filter page load metrics as well (i want to
> > track cases where subresources are filtered in child frames) so i'll take a
look
> > at fixing this now.
> 
> Agreed. I made the bad assumption that this was already happening due to the
other places that use this. Keep more on board with the solution that you are
considering for tracking child frames in the browser MetricsWCO.

The change to allow tracking behaviors in child frames has landed. There's a
helper method DidObserveLoadingBehaviorInAnyFrame that may be useful for you
here.

[mlamouri (slow - plz ping), 2017-03-13 18:38:27]

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

https://codereview.chromium.org/2732293003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.cc:25:
MediaPageLoadMetricsObserver::~MediaPageLoadMetricsObserver() {}
nit: = default;

https://codereview.chromium.org/2732293003/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2732293003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h:18:
// Observer responsible for recording metrics on pages that load at least one
By load, you really mean play I think.

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

https://codereview.chromium.org/2732293003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc:29:
~MediaPageLoadMetricsObserverTest() override {}
double nit: = default

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2322:
document().loader()->didObserveLoadingBehavior(
On 2017/03/08 at 18:17:11, Ryan Sturm wrote:
> On 2017/03/08 18:10:11, Bryan McQuade wrote:
> > On 2017/03/08 at 18:03:22, Bryan McQuade wrote:
> > > one thing to note - this may prevent you from discovering loading
behaviors in
> > child iframes (e.g. youtube iframe embeds). you may want to report the
loading
> > behavior on the top-level document if you want to know about media loads in
> > child frames (and update the comment in the enum to note that this is
reported
> > for media played in child frames)
> > 
> > charles and i just chatted - due to OOPIF, we think the best way forward
here is
> > to send IPCs for child frames, and then join things up browser-side. turns
out
> > this bug affects my subresource filter page load metrics as well (i want to
> > track cases where subresources are filtered in child frames) so i'll take a
look
> > at fixing this now.
> 
> Agreed. I made the bad assumption that this was already happening due to the
other places that use this. Keep more on board with the solution that you are
considering for tracking child frames in the browser MetricsWCO.

Instead of adding more plumbing, why not re-using
WebContentsObserver::MediaStartedPlaying? It should be fairly similar to what
you are doing except that you know the media actually loaded (ie. a 404 media
would likely hit your new plumbing).

[RyanSturm, 2017-03-13 22:02:01]

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

[commit-bot: I haz the power, 2017-03-13 22:03:36]

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-03-13 23:21:56]

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

[commit-bot: I haz the power, 2017-03-13 23:21:57]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-03-13 23:38:23]

ryansturm@chromium.org changed reviewers:
- japhet@chromium.org, mlamouri@chromium.org

[RyanSturm, 2017-03-13 23:38:24]

mlamouri: Thanks for the suggestion to use WCO::MediaStartedPlaying, I moved you
to cc

-japhet


bmcquade, csharrison: review the new approach please. Thanks!

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

https://codereview.chromium.org/2732293003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.cc:25:
MediaPageLoadMetricsObserver::~MediaPageLoadMetricsObserver() {}
On 2017/03/13 18:38:27, mlamouri wrote:
> nit: = default;

Done. I've only rarely seen "default" for destructors in chromium code (copy and
move I've seen a bunch of), and the style guide doesn't really say much about
it. Is this a new style point?

https://codereview.chromium.org/2732293003/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2732293003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h:18:
// Observer responsible for recording metrics on pages that load at least one
On 2017/03/13 18:38:27, mlamouri wrote:
> By load, you really mean play I think.

Done.

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

https://codereview.chromium.org/2732293003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc:29:
~MediaPageLoadMetricsObserverTest() override {}
On 2017/03/13 18:38:27, mlamouri wrote:
> double nit: = default

Done.

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2732293003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2322:
document().loader()->didObserveLoadingBehavior(
On 2017/03/13 18:38:27, mlamouri wrote:
> On 2017/03/08 at 18:17:11, Ryan Sturm wrote:
> > On 2017/03/08 18:10:11, Bryan McQuade wrote:
> > > On 2017/03/08 at 18:03:22, Bryan McQuade wrote:
> > > > one thing to note - this may prevent you from discovering loading
> behaviors in
> > > child iframes (e.g. youtube iframe embeds). you may want to report the
> loading
> > > behavior on the top-level document if you want to know about media loads
in
> > > child frames (and update the comment in the enum to note that this is
> reported
> > > for media played in child frames)
> > > 
> > > charles and i just chatted - due to OOPIF, we think the best way forward
> here is
> > > to send IPCs for child frames, and then join things up browser-side. turns
> out
> > > this bug affects my subresource filter page load metrics as well (i want
to
> > > track cases where subresources are filtered in child frames) so i'll take
a
> look
> > > at fixing this now.
> > 
> > Agreed. I made the bad assumption that this was already happening due to the
> other places that use this. Keep more on board with the solution that you are
> considering for tracking child frames in the browser MetricsWCO.
> 
> Instead of adding more plumbing, why not re-using
> WebContentsObserver::MediaStartedPlaying? It should be fairly similar to what
> you are doing except that you know the media actually loaded (ie. a 404 media
> would likely hit your new plumbing).

Thanks for the pointer. That looks like a better approach.

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

https://codereview.chromium.org/2732293003/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):

https://codereview.chromium.org/2732293003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:68: void
MediaStartedPlaying(
do we know if this gets invoked for child frames? :)

[Bryan McQuade, 2017-03-14 00:31:31]

On 2017/03/14 at 00:24:07, Bryan McQuade wrote:
>
https://codereview.chromium.org/2732293003/diff/60001/chrome/browser/page_loa...
> File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):
> 
>
https://codereview.chromium.org/2732293003/diff/60001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.h:68: void
MediaStartedPlaying(
> do we know if this gets invoked for child frames? :)

i traced the calls and indeed it looks like it is used in child frames. great!
glad this exists. will review shortly.

[Bryan McQuade, 2017-03-15 01:59:46]

LGTM, thanks for adding this!

https://codereview.chromium.org/2732293003/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2732293003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:110: if
(committed_load_)
we should just make sure that the top-level frame matches the currently
committed main frame first, similar to what we do in OnTimingUpdated. something
like

if (id.first>GetRenderViewHost()->GetMainFrame() !=
          web_contents()->GetMainFrame()) {
  // received a media notification for a different frame.
  // this may happen in cases where media starts playing in a document
  // that was navigated away from. ignore it.
  return;
}

[RyanSturm, 2017-03-15 21:42:23]

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

[commit-bot: I haz the power, 2017-03-15 21:42:50]

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-03-15 23:09:24]

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

[commit-bot: I haz the power, 2017-03-15 23:09:25]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-03-16 00:06:24]

https://codereview.chromium.org/2732293003/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):

https://codereview.chromium.org/2732293003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:68: void
MediaStartedPlaying(
On 2017/03/14 00:24:06, Bryan McQuade wrote:
> do we know if this gets invoked for child frames? :)

Acknowledged.

[RyanSturm, 2017-03-16 00:06:31]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2017-03-16 00:06:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, csharrison@chromium.org, bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2732293003/#ps80001
(title: "bmcquade comment")

[commit-bot: I haz the power, 2017-03-16 00:07:02]

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-03-16 00:15:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489622791325360,
"parent_rev": "966d0cf62d587368c674f2eafd37739431e45c43", "commit_rev":
"09fb15d21c53769e466ea5a80bd3a170082e7bfe"}

[commit-bot: I haz the power, 2017-03-16 00:16:28]

Description was changed from

==========
Adding UMA on page load size for pages with Media

This CL plumbs a renderer behavior (MediaPlayed) from blink/ when a
media element is played. An observer records page size UMA on pages that
saw at least one media element get played.

BUG=698348
==========

to

==========
Adding UMA on page load size for pages with Media

This CL plumbs a renderer behavior (MediaPlayed) from blink/ when a
media element is played. An observer records page size UMA on pages that
saw at least one media element get played.

BUG=698348

Review-Url: https://codereview.chromium.org/2732293003
Cr-Commit-Position: refs/heads/master@{#457275}
Committed:
https://chromium.googlesource.com/chromium/src/+/09fb15d21c53769e466ea5a80bd3...
==========

[commit-bot: I haz the power, 2017-03-16 00:16:30]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/09fb15d21c53769e466ea5a80bd3...