Pass collected frame timing events to Blink

Pass collected frame timing events to Blink

Related Blink change: https://codereview.chromium.org/908453003/
Related cc change: https://codereview.chromium.org/914403003/

BUG=441552
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ed24c2ca28e1f77957f63f6406f86a047a77b8b1
Cr-Commit-Position: refs/heads/master@{#333128}

[MikeB, 2015-05-08 22:39:47]

mpb@chromium.org changed reviewers:
+ enne@chromium.org, vmpstr@chromium.org

[vmpstr, 2015-05-12 23:48:26]

https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:803:
begin_main_frame_state->frame_composite_counts.get());
Should we be passing ownership here instead? It's a bit weird to just give a raw
pointer, since we don't know from where what LTH plans to do with it, and as far
as i can tell we don't really want to own the data anymore.

https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.h...
cc/trees/thread_proxy.h:58: scoped_ptr<FrameTimingTracker::CompositeTimingSet>
frame_composite_counts;
I would name this something like frame_composite_events and frame_render_events.

https://codereview.chromium.org/1137823002/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1137823002/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:887: static std::tuple<int,
double, double> CompositeTimingEventToDouble(
I don't think you can use a tuple unless we have full c++11 support. You can use
a pair<int, pair<double, double>> or maybe a struct or something? Also, prefer
to put free functions like that in anonymous namespace instead of making them
static.

https://codereview.chromium.org/1137823002/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:896: for (const auto& record :
*composites ) {
Can you give names (variables) to record.first and record.second so it's clear
what they are?

[MikeB, 2015-05-21 21:53:08]

https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:803:
begin_main_frame_state->frame_composite_counts.get());
On 2015/05/12 23:48:25, vmpstr wrote:
> Should we be passing ownership here instead? It's a bit weird to just give a
raw
> pointer, since we don't know from where what LTH plans to do with it, and as
far
> as i can tell we don't really want to own the data anymore.

That's how ApplyScrollAndScale does it, and that's what I was basing my change
on. Is your suggestion to just have LTH take a scoped_ptr and thread_proxy calls
.pass?

https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.h...
cc/trees/thread_proxy.h:58: scoped_ptr<FrameTimingTracker::CompositeTimingSet>
frame_composite_counts;
On 2015/05/12 23:48:25, vmpstr wrote:
> I would name this something like frame_composite_events and
frame_render_events.

Done.

https://codereview.chromium.org/1137823002/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1137823002/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:887: static std::tuple<int,
double, double> CompositeTimingEventToDouble(
On 2015/05/12 23:48:26, vmpstr wrote:
> I don't think you can use a tuple unless we have full c++11 support. You can
use
> a pair<int, pair<double, double>> or maybe a struct or something? Also, prefer
> to put free functions like that in anonymous namespace instead of making them
> static.

Done.

https://codereview.chromium.org/1137823002/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:896: for (const auto& record :
*composites ) {
On 2015/05/12 23:48:26, vmpstr wrote:
> Can you give names (variables) to record.first and record.second so it's clear
> what they are?

Done.

[vmpstr, 2015-05-22 23:58:07]

I wrote up this https://codereview.chromium.org/1152153003/ which looks a lot
like what you have here, so feel free to just gut my patch and implement it in
here. The difference in the idea is that it doesn't use a commit, but instead
implements a separate post task to the main thread, which aligns a bit better
with the discussion on crbug.com/488647.

Wdyt?

On 2015/05/21 21:53:08, MikeB wrote:
> https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.c...
> cc/trees/thread_proxy.cc:803:
> begin_main_frame_state->frame_composite_counts.get());
> On 2015/05/12 23:48:25, vmpstr wrote:
> > Should we be passing ownership here instead? It's a bit weird to just give a
> raw
> > pointer, since we don't know from where what LTH plans to do with it, and as
> far
> > as i can tell we don't really want to own the data anymore.
> 
> That's how ApplyScrollAndScale does it, and that's what I was basing my change
> on. Is your suggestion to just have LTH take a scoped_ptr and thread_proxy
calls
> .pass?

Yeah that's what I meant.

> 
> https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.h
> File cc/trees/thread_proxy.h (right):
> 
>
https://codereview.chromium.org/1137823002/diff/20001/cc/trees/thread_proxy.h...
> cc/trees/thread_proxy.h:58: scoped_ptr<FrameTimingTracker::CompositeTimingSet>
> frame_composite_counts;
> On 2015/05/12 23:48:25, vmpstr wrote:
> > I would name this something like frame_composite_events and
> frame_render_events.
> 
> Done.
> 
>
https://codereview.chromium.org/1137823002/diff/20001/content/renderer/gpu/re...
> File content/renderer/gpu/render_widget_compositor.cc (right):
> 
>
https://codereview.chromium.org/1137823002/diff/20001/content/renderer/gpu/re...
> content/renderer/gpu/render_widget_compositor.cc:887: static std::tuple<int,
> double, double> CompositeTimingEventToDouble(
> On 2015/05/12 23:48:26, vmpstr wrote:
> > I don't think you can use a tuple unless we have full c++11 support. You can
> use
> > a pair<int, pair<double, double>> or maybe a struct or something? Also,
prefer
> > to put free functions like that in anonymous namespace instead of making
them
> > static.
> 
> Done.
> 
>
https://codereview.chromium.org/1137823002/diff/20001/content/renderer/gpu/re...
> content/renderer/gpu/render_widget_compositor.cc:896: for (const auto& record
:
> *composites ) {
> On 2015/05/12 23:48:26, vmpstr wrote:
> > Can you give names (variables) to record.first and record.second so it's
clear
> > what they are?
> 
> Done.

[MikeB, 2015-06-01 22:48:31]

Merged in vmpstr's changes. I kept the actual communication with Blink in
content/renderer/gpu/render_widget_compositor.cc but everything else is from
https://codereview.chromium.org/1152153003/
PTAL.

[vmpstr, 2015-06-02 17:40:09]

https://codereview.chromium.org/1137823002/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1137823002/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:983:
CompositeTimingEventToDouble);
I kind of weakly prefer an explicit loop here, but if you're going to keep this,
maybe use a lambda instead of CompositeTimingEventToDouble (and similar comment
for the second loop)

[enne (OOO), 2015-06-02 18:13:38]

lgtm % vmpstr

https://codereview.chromium.org/1137823002/diff/80001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1137823002/diff/80001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1426: void ThreadProxy::PostFrameTimingEvents(
DCHECK(IsMainThread()) here?

[MikeB, 2015-06-02 21:59:36]

https://codereview.chromium.org/1137823002/diff/80001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/1137823002/diff/80001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1426: void ThreadProxy::PostFrameTimingEvents(
On 2015/06/02 18:13:38, enne wrote:
> DCHECK(IsMainThread()) here?

Good catch!

https://codereview.chromium.org/1137823002/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1137823002/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:983:
CompositeTimingEventToDouble);
On 2015/06/02 17:40:08, vmpstr wrote:
> I kind of weakly prefer an explicit loop here, but if you're going to keep
this,
> maybe use a lambda instead of CompositeTimingEventToDouble (and similar
comment
> for the second loop)

Done.

[MikeB, 2015-06-02 22:03:10]

mpb@chromium.org changed reviewers:
+ danakj@chromium.org

[vmpstr, 2015-06-02 22:05:19]

lgtm % nits

https://codereview.chromium.org/1137823002/diff/100001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1137823002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:966: webEvents[i] =
blink::WebFrameTimingEvent(
nit: just create a default vector above and .push_back. If performance becomes a
concern, we typically do vector::reserve.

https://codereview.chromium.org/1137823002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:979: webEvents[i] =
blink::WebFrameTimingEvent(
nit: just create a default vector above and .push_back. If performance becomes a
concern, we typically do vector::reserve.

[MikeB, 2015-06-03 20:19:20]

https://codereview.chromium.org/1137823002/diff/100001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1137823002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:966: webEvents[i] =
blink::WebFrameTimingEvent(
On 2015/06/02 22:05:19, vmpstr wrote:
> nit: just create a default vector above and .push_back. If performance becomes
a
> concern, we typically do vector::reserve.

Done.

https://codereview.chromium.org/1137823002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:979: webEvents[i] =
blink::WebFrameTimingEvent(
On 2015/06/02 22:05:19, vmpstr wrote:
> nit: just create a default vector above and .push_back. If performance becomes
a
> concern, we typically do vector::reserve.

Done.

[vmpstr, 2015-06-03 21:49:38]

(+danakj for owner's review)

[danakj, 2015-06-03 21:50:03]

ui LGTM

[MikeB, 2015-06-04 16:56:41]

The CQ bit was checked by mpb@chromium.org

[MikeB, 2015-06-04 16:56:42]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1137823002/#ps120001
(title: "Code review nits.")

[commit-bot: I haz the power, 2015-06-04 16:57:16]

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

[commit-bot: I haz the power, 2015-06-04 17:13:44]

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

[commit-bot: I haz the power, 2015-06-04 17:13:46]

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

[MikeB, 2015-06-04 17:33:40]

The CQ bit was checked by mpb@chromium.org

[MikeB, 2015-06-04 17:33:41]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
vmpstr@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1137823002/#ps140001
(title: "Missed a pure virtual function on one of the test-only classes.")

[commit-bot: I haz the power, 2015-06-04 17:35:11]

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

[commit-bot: I haz the power, 2015-06-04 17:44:09]

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

[commit-bot: I haz the power, 2015-06-04 17:44:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[MikeB, 2015-06-04 17:55:01]

mpb@chromium.org changed reviewers:
+ sky@chromium.org

[MikeB, 2015-06-04 17:55:38]

+sky for content/test/web_layer_tree_view_impl_for_testing.h

[sky, 2015-06-04 19:27:27]

LGTM

[MikeB, 2015-06-04 20:08:39]

The CQ bit was checked by mpb@chromium.org

[commit-bot: I haz the power, 2015-06-04 20:09:27]

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

[commit-bot: I haz the power, 2015-06-04 20:25:25]

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

[commit-bot: I haz the power, 2015-06-04 20:25:28]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[MikeB, 2015-06-04 20:30:15]

The CQ bit was checked by mpb@chromium.org

[MikeB, 2015-06-04 20:30:15]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org, sky@chromium.org, vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1137823002/#ps160001
(title: "Use full namespace.")

[commit-bot: I haz the power, 2015-06-04 20:32:03]

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

[MikeB, 2015-06-04 20:36:09]

The CQ bit was checked by mpb@chromium.org

[MikeB, 2015-06-04 20:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org, sky@chromium.org, vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1137823002/#ps180001
(title: "Missed virtual function in android-only test.")

[commit-bot: I haz the power, 2015-06-04 20:37:40]

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

[commit-bot: I haz the power, 2015-06-04 20:48:22]

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

[commit-bot: I haz the power, 2015-06-04 20:48:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[MikeB, 2015-06-04 21:07:07]

mpb@chromium.org changed reviewers:
+ sievers@chromium.org

[MikeB, 2015-06-04 21:09:39]

+sievers for  compositor_impl_android.h

[no sievers, 2015-06-04 22:51:25]

lgtm

[MikeB, 2015-06-04 23:10:20]

The CQ bit was checked by mpb@chromium.org

[commit-bot: I haz the power, 2015-06-04 23:10:43]

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

[commit-bot: I haz the power, 2015-06-04 23:26:23]

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

[commit-bot: I haz the power, 2015-06-04 23:26:25]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[MikeB, 2015-06-05 19:47:32]

The CQ bit was checked by mpb@chromium.org

[MikeB, 2015-06-05 19:47:33]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org, vmpstr@chromium.org, sievers@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1137823002/#ps200001
(title: "Missed implementing virtual function on Android html_viewer.")

[commit-bot: I haz the power, 2015-06-05 19:48:52]

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

[commit-bot: I haz the power, 2015-06-05 20:57:24]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-06-05 20:58:19]

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