Measure the number of frame loads with Flash.

Measure the number of frame loads with Flash.

Also measure the number of frame loads for comparison, and the number of Flash instances created while we are at it.

BUG=403800
Committed: https://crrev.com/f00bafe1a7f9a3047e1c2abb41d3b1350d45d8c2
Cr-Commit-Position: refs/heads/master@{#302522}

[Lei Zhang, 2014-10-30 01:36:46]

thestig@chromium.org changed reviewers:
+ creis@chromium.org

[Lei Zhang, 2014-10-30 01:36:46]

This is roughly correlated with the PageLoad metric which in WebContentsImpl.

[Lei Zhang, 2014-10-30 01:37:04]

thestig@chromium.org changed reviewers:
+ isherman@chromium.org

[Lei Zhang, 2014-10-30 01:37:15]

+isherman for metrics.

[Ilya Sherman, 2014-10-30 03:28:30]

lgtm

[Charlie Reis, 2014-10-30 18:46:26]

https://codereview.chromium.org/687273004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/687273004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:644:
RecordAction(base::UserMetricsAction("PageLoadWithFlash"));
Can this be called more than once per page load?  (e.g., multiple Flash movies
on a page, or add one + remove it + add another)

Just wondering if the stat name might be misleading.

[Lei Zhang, 2014-10-30 22:14:40]

+dmichael/raymes in case they have additional comments.

https://codereview.chromium.org/687273004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/687273004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:644:
RecordAction(base::UserMetricsAction("PageLoadWithFlash"));
On 2014/10/30 18:46:26, Charlie Reis wrote:
> Can this be called more than once per page load?  (e.g., multiple Flash movies
> on a page, or add one + remove it + add another)
> 
> Just wondering if the stat name might be misleading.

The plugin is only created once here, and each Flash embed is a separate
PepperPluginInstance.

[Charlie Reis, 2014-10-30 22:25:39]

Thanks.  Should be good, but one last question.

https://codereview.chromium.org/687273004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/687273004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:644:
RecordAction(base::UserMetricsAction("PageLoadWithFlash"));
On 2014/10/30 22:14:40, Lei Zhang wrote:
> On 2014/10/30 18:46:26, Charlie Reis wrote:
> > Can this be called more than once per page load?  (e.g., multiple Flash
movies
> > on a page, or add one + remove it + add another)
> > 
> > Just wondering if the stat name might be misleading.
> 
> The plugin is only created once here, and each Flash embed is a separate
> PepperPluginInstance.

Cool.  Seems like it could happen in multiple RenderFrames within the same page,
though, possibly across multiple processes (with out-of-process iframes).  In
that sense, is it a document-level count rather than a page-level count?

[dmichael (off chromium), 2014-10-30 22:54:10]

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

[dmichael (off chromium), 2014-10-30 22:54:10]

(I don't care a bunch about the name, but I think creis is right)

https://codereview.chromium.org/687273004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/687273004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:644:
RecordAction(base::UserMetricsAction("PageLoadWithFlash"));
On 2014/10/30 22:25:39, Charlie Reis wrote:
> On 2014/10/30 22:14:40, Lei Zhang wrote:
> > On 2014/10/30 18:46:26, Charlie Reis wrote:
> > > Can this be called more than once per page load?  (e.g., multiple Flash
> movies
> > > on a page, or add one + remove it + add another)
> > > 
> > > Just wondering if the stat name might be misleading.
> > 
> > The plugin is only created once here, and each Flash embed is a separate
> > PepperPluginInstance.
> 
> Cool.  Seems like it could happen in multiple RenderFrames within the same
page,
> though, possibly across multiple processes (with out-of-process iframes).  In
> that sense, is it a document-level count rather than a page-level count?
Right, it looks like it's 1 per-RenderFrame-per-plugin. Maybe
"DocumentLoadWithFlash" or "FrameLoadWithFlash" would be a better name.

[Lei Zhang, 2014-10-30 23:52:47]

I want to test, but running with --site-per-process is very crashy in my ToT
debug build. :\ I'll update the CL shortly and ping everyone.

[Lei Zhang, 2014-10-31 00:45:01]

Ok, I now have:

FrameLoad (done on the browser side to save an IPC)
FrameLoadWithFlash (whether a frame loads Flash)
Flash.PluginInstanceCreated (number of Flash instances)

[Lei Zhang, 2014-10-31 00:45:01]

Ok, I now have:

FrameLoad (done on the browser side to save an IPC)
FrameLoadWithFlash (whether a frame loads Flash)
Flash.PluginInstanceCreated (number of Flash instances)

[Charlie Reis, 2014-10-31 16:55:46]

I'm ok with those names, if you're able to process that data.  (For example, you
won't be able to see the breakdown of pages with lots of Flash vs no Flash, just
an overall count of frame loads vs an overall count of Flash usage.)

One concern about the FrameLoad metric, though.

https://codereview.chromium.org/687273004/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/687273004/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2749:
RecordAction(base::UserMetricsAction("FrameLoad"));
DidStartLoading isn't the right place for this-- the navigation could fail.

I'm surprised if we don't have something like this already.  If not, we should
probably watch for it at commit time, similar to
NavigatorImpl::RecordNavigationMetrics.  I'm guessing you also don't want to
count in-page navigations, but I'm not sure.

https://codereview.chromium.org/687273004/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/687273004/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:2662: <description>Count of frame loads that
also loads PPAPI Flash.</description>
nit: also load

https://codereview.chromium.org/687273004/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:2957: <description>Count of frame loads that
also loads PPAPI Flash.</description>
nit: also load

[Charlie Reis, 2014-10-31 16:59:27]

On 2014/10/30 23:52:47, Lei Zhang wrote:
> I want to test, but running with --site-per-process is very crashy in my ToT
> debug build. :\ I'll update the CL shortly and ping everyone.

Yeah, I expect you're hitting http://crbug.com/426658, if not related issues. 
We haven't gotten to plugins yet.  :)

[Lei Zhang, 2014-11-01 00:51:47]

https://codereview.chromium.org/687273004/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/687273004/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2749:
RecordAction(base::UserMetricsAction("FrameLoad"));
On 2014/10/31 16:55:45, Charlie Reis wrote:
> DidStartLoading isn't the right place for this-- the navigation could fail.
> 
> I'm surprised if we don't have something like this already.  If not, we should
> probably watch for it at commit time, similar to
> NavigatorImpl::RecordNavigationMetrics.  I'm guessing you also don't want to
> count in-page navigations, but I'm not sure.

FWIW, the PageLoad metric that's used as part of "NN foo per million page loads"
also doesn't care about failed navigation and navigation to anchors. But I agree
it's odd.

Successful, not in-page frame loads seem more accurate, so that's what I tried
to do.

https://codereview.chromium.org/687273004/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/687273004/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:2662: <description>Count of frame loads that
also loads PPAPI Flash.</description>
On 2014/10/31 16:55:45, Charlie Reis wrote:
> nit: also load

Done.

https://codereview.chromium.org/687273004/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:2957: <description>Count of frame loads that
also loads PPAPI Flash.</description>
On 2014/10/31 16:55:45, Charlie Reis wrote:
> nit: also load

Done.

[Charlie Reis, 2014-11-03 18:27:50]

Thanks, and sorry for all the questions!  LGTM.

[Lei Zhang, 2014-11-03 23:00:09]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-11-03 23:02:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/687273004/140001

[commit-bot: I haz the power, 2014-11-03 23:55:19]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-03 23:56:37]

Patchset 8 (id:??) landed as
https://crrev.com/f00bafe1a7f9a3047e1c2abb41d3b1350d45d8c2
Cr-Commit-Position: refs/heads/master@{#302522}