cc: Add frame timing request plumbing through the layers.

cc: Add frame timing request plumbing through the layers.

This patch adds frame timing request class, which contains a rect and
an id for that rect. Furthermore, it adds plumbing so that the requests
get propagated from the original Layer into both pending and active
LayerImpls. Verified using tracing that the requests appear correctly.

R=danakj, michaelblain@chromium.org
BUG=441555
Committed: https://crrev.com/3d1d72c397c947b7d3ac6390b2a0c2b3a859b094
Cr-Commit-Position: refs/heads/master@{#313093}

[vmpstr, 2015-01-20 18:55:03]

Please take a look

[Mike B, 2015-01-21 17:56:07]

michaelblain@google.com changed reviewers:
+ mpb@chromium.org
- michaelblain@chromium.org

[MikeB, 2015-01-21 18:12:27]

lgtm

Looks good to me!

[danakj, 2015-01-23 18:08:45]

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer.cc#newcode1020
cc/layers/layer.cc:1020: if (frame_timing_requests_dirty_) {
what's the reason for these dirty flags?

in case we commit twice in a row before consuming the requests on the other end?
we could do that if dirty was true twice too.

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer_impl.cc#newc...
cc/layers/layer_impl.cc:1637: void LayerImpl::SetFrameTimingRequests(
same order as in the header please

[vmpstr, 2015-01-23 18:41:57]

PTAL

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer.cc#newcode1020
cc/layers/layer.cc:1020: if (frame_timing_requests_dirty_) {
On 2015/01/23 18:08:45, danakj wrote:
> what's the reason for these dirty flags?
> 
> in case we commit twice in a row before consuming the requests on the other
end?
> we could do that if dirty was true twice too.

It's just so we don't keep copying the same vector. Once we set some requests,
those requests are in effect until something else is set, so this avoids
propagating the same thing on each commit/activation.

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer_impl.cc#newc...
cc/layers/layer_impl.cc:1637: void LayerImpl::SetFrameTimingRequests(
On 2015/01/23 18:08:45, danakj wrote:
> same order as in the header please

Done.

[danakj, 2015-01-23 18:43:29]

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer.cc#newcode1020
cc/layers/layer.cc:1020: if (frame_timing_requests_dirty_) {
On 2015/01/23 18:41:57, vmpstr wrote:
> On 2015/01/23 18:08:45, danakj wrote:
> > what's the reason for these dirty flags?
> > 
> > in case we commit twice in a row before consuming the requests on the other
> end?
> > we could do that if dirty was true twice too.
> 
> It's just so we don't keep copying the same vector. Once we set some requests,
> those requests are in effect until something else is set, so this avoids
> propagating the same thing on each commit/activation.

I see, you need them to stick around for multiple frames? Do you need them to
stick around on all of the layers? Could you just swap() them thru to the active
layer?

[vmpstr, 2015-01-23 18:45:58]

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/834343004/diff/1/cc/layers/layer.cc#newcode1020
cc/layers/layer.cc:1020: if (frame_timing_requests_dirty_) {
On 2015/01/23 18:43:29, danakj wrote:
> On 2015/01/23 18:41:57, vmpstr wrote:
> > On 2015/01/23 18:08:45, danakj wrote:
> > > what's the reason for these dirty flags?
> > > 
> > > in case we commit twice in a row before consuming the requests on the
other
> > end?
> > > we could do that if dirty was true twice too.
> > 
> > It's just so we don't keep copying the same vector. Once we set some
requests,
> > those requests are in effect until something else is set, so this avoids
> > propagating the same thing on each commit/activation.
> 
> I see, you need them to stick around for multiple frames? Do you need them to
> stick around on all of the layers? Could you just swap() them thru to the
active
> layer?

They really only need to be on the active layer.. Swapping would only work with
the dirty flag, since otherwise we'd swap back the next time, but other than
that it could work. Any suggestions for a function name? Liek
layer->TakeFrameTimingRequests(&frame_timing_requests_) or something?

[danakj, 2015-01-23 18:47:45]

On Fri, Jan 23, 2015 at 10:45 AM, <vmpstr@chromium.org> wrote:

>
> https://codereview.chromium.org/834343004/diff/1/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
>
> https://codereview.chromium.org/834343004/diff/1/cc/
> layers/layer.cc#newcode1020
> cc/layers/layer.cc:1020: if (frame_timing_requests_dirty_) {
> On 2015/01/23 18:43:29, danakj wrote:
>
>> On 2015/01/23 18:41:57, vmpstr wrote:
>> > On 2015/01/23 18:08:45, danakj wrote:
>> > > what's the reason for these dirty flags?
>> > >
>> > > in case we commit twice in a row before consuming the requests on
>>
> the other
>
>> > end?
>> > > we could do that if dirty was true twice too.
>> >
>> > It's just so we don't keep copying the same vector. Once we set some
>>
> requests,
>
>> > those requests are in effect until something else is set, so this
>>
> avoids
>
>> > propagating the same thing on each commit/activation.
>>
>
>  I see, you need them to stick around for multiple frames? Do you need
>>
> them to
>
>> stick around on all of the layers? Could you just swap() them thru to
>>
> the active
>
>> layer?
>>
>
> They really only need to be on the active layer.. Swapping would only
> work with the dirty flag, since otherwise we'd swap back the next time,
> but other than that it could work. Any suggestions for a function name?
> Liek layer->TakeFrameTimingRequests(&frame_timing_requests_) or
> something?
>

Match this perhaps?
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...


>
> https://codereview.chromium.org/834343004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2015-01-23 19:01:09]

PTAL

[danakj, 2015-01-23 19:15:37]

https://codereview.chromium.org/834343004/diff/20001/cc/debug/frame_timing_re...
File cc/debug/frame_timing_request.h (right):

https://codereview.chromium.org/834343004/diff/20001/cc/debug/frame_timing_re...
cc/debug/frame_timing_request.h:12: class FrameTimingRequest {
can you add a class level comment saying what this is? Maybe what the id and
rect are (what space is it?) etc..

https://codereview.chromium.org/834343004/diff/20001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/834343004/diff/20001/cc/layers/layer.h#newcod...
cc/layers/layer.h:511: void
PassFrameTimingRequests(std::vector<FrameTimingRequest>* requests);
Sorry I like the Pass on the layerimpl but I'm not so sure about it on the
public API. It seems a bit more fickle to expose it this way, and I think I
prefer a Set method on this class.

[vmpstr, 2015-01-23 19:35:27]

PTAL

https://codereview.chromium.org/834343004/diff/20001/cc/debug/frame_timing_re...
File cc/debug/frame_timing_request.h (right):

https://codereview.chromium.org/834343004/diff/20001/cc/debug/frame_timing_re...
cc/debug/frame_timing_request.h:12: class FrameTimingRequest {
On 2015/01/23 19:15:37, danakj wrote:
> can you add a class level comment saying what this is? Maybe what the id and
> rect are (what space is it?) etc..

Done.

https://codereview.chromium.org/834343004/diff/20001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/834343004/diff/20001/cc/layers/layer.h#newcod...
cc/layers/layer.h:511: void
PassFrameTimingRequests(std::vector<FrameTimingRequest>* requests);
On 2015/01/23 19:15:37, danakj wrote:
> Sorry I like the Pass on the layerimpl but I'm not so sure about it on the
> public API. It seems a bit more fickle to expose it this way, and I think I
> prefer a Set method on this class.

Done.

[danakj, 2015-01-23 19:40:18]

LGTM

https://codereview.chromium.org/834343004/diff/40001/cc/debug/frame_timing_re...
File cc/debug/frame_timing_request.h (right):

https://codereview.chromium.org/834343004/diff/40001/cc/debug/frame_timing_re...
cc/debug/frame_timing_request.h:12: // This class represents a request from
Blink to record frame timing information
nit: Mentioning Blink is a layering violation, but I'm not sure how to better
word this without it. If you can think of something that'd be good.

[MikeB, 2015-01-23 19:45:22]

On 2015/01/23 19:40:18, danakj wrote:
> LGTM
> 
>
https://codereview.chromium.org/834343004/diff/40001/cc/debug/frame_timing_re...
> File cc/debug/frame_timing_request.h (right):
> 
>
https://codereview.chromium.org/834343004/diff/40001/cc/debug/frame_timing_re...
> cc/debug/frame_timing_request.h:12: // This class represents a request from
> Blink to record frame timing information
> nit: Mentioning Blink is a layering violation, but I'm not sure how to better
> word this without it. If you can think of something that'd be good.

It seems to make sense without directly calling it Blink...

// This class represents a request to record frame timing information
// about the given rect (in layer space) and an associated request id. When
// this request is propagated to the active LayerImpl, it will cause events to
// be saved in FrameTimingTracker, which in turn will be consumed by the
requester.

[vmpstr, 2015-01-23 21:01:13]

https://codereview.chromium.org/834343004/diff/40001/cc/debug/frame_timing_re...
File cc/debug/frame_timing_request.h (right):

https://codereview.chromium.org/834343004/diff/40001/cc/debug/frame_timing_re...
cc/debug/frame_timing_request.h:12: // This class represents a request from
Blink to record frame timing information
On 2015/01/23 19:40:18, danakj wrote:
> nit: Mentioning Blink is a layering violation, but I'm not sure how to better
> word this without it. If you can think of something that'd be good.

Done.

[vmpstr, 2015-01-23 21:01:17]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:02:09]

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

[commit-bot: I haz the power, 2015-01-23 21:23:40]

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

[commit-bot: I haz the power, 2015-01-23 21:23:41]

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

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

[MikeB, 2015-01-26 16:46:44]

https://codereview.chromium.org/834343004/diff/60001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/834343004/diff/60001/cc/BUILD.gn#newcode84
cc/BUILD.gn:84: "debug/frame_Timing_request.h",
"timing" should be lower case.

[vmpstr, 2015-01-26 17:34:22]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-01-26 17:35:33]

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

[commit-bot: I haz the power, 2015-01-26 18:28:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-26 18:29:10]

Patchset 5 (id:??) landed as
https://crrev.com/3d1d72c397c947b7d3ac6390b2a0c2b3a859b094
Cr-Commit-Position: refs/heads/master@{#313093}