cc: Record frame timing composite events in the tracker.

cc: Record frame timing composite events in the tracker.

This patch adds composite event recording for requested rects into the
frame timing tracker.

The current approach is to record all rects that intersect the visible
rect for that layer.

R=danakj, mpb@chromium.org
BUG=441555
Committed: https://crrev.com/99aa853cae0e968f54c15bbbd33c9abcc344d133
Cr-Commit-Position: refs/heads/master@{#314398}

[vmpstr, 2015-01-28 23:40:05]

vmpstr@chromium.org changed reviewers:
+ danakj@chromium.org, michaelblain@chromium.org

[vmpstr, 2015-01-28 23:40:41]

vmpstr@chromium.org changed reviewers:
+ mpb@chromium.org
- michaelblain@chromium.org

[vmpstr, 2015-01-28 23:41:41]

Please take a look. I'll work on the tests, but let me know if there are any
objections to the approach.

[vmpstr, 2015-02-02 19:06:29]

Added a test, PTAL.

[MikeB, 2015-02-02 19:25:31]

LGTM

Test looks good too, thanks.

[danakj, 2015-02-02 21:19:06]

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:810: for (const auto& request :
frame_timing_requests) {
you could just : it->frame_timing_requests())  ? and avoid the temp var?

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:811: if
(request.rect().Intersects(it->visible_content_rect())) {
you need to map this VCR to layer space

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1065: current_composite_events_.clear();
if you make this a member of the |frame| then you don't have to worry about
clearing it after each frame?

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1455:
frame_timing_tracker_->SaveTimeStamps(base::TimeTicks::Now(),
should we use the frame time instead of now?

[vmpstr, 2015-02-02 22:32:05]

PTAL

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:810: for (const auto& request :
frame_timing_requests) {
On 2015/02/02 21:19:06, danakj wrote:
> you could just : it->frame_timing_requests())  ? and avoid the temp var?

Done.

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:811: if
(request.rect().Intersects(it->visible_content_rect())) {
On 2015/02/02 21:19:06, danakj wrote:
> you need to map this VCR to layer space

Done.

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1065: current_composite_events_.clear();
On 2015/02/02 21:19:06, danakj wrote:
> if you make this a member of the |frame| then you don't have to worry about
> clearing it after each frame?

Done.

https://codereview.chromium.org/884243004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1455:
frame_timing_tracker_->SaveTimeStamps(base::TimeTicks::Now(),
On 2015/02/02 21:19:06, danakj wrote:
> should we use the frame time instead of now?

Done. I think that works. According to the spec, we want the "time of the
event", the event being the composite event, which I think the impl frame time
captures.

[danakj, 2015-02-02 23:10:09]

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:193:
std::vector<FrameTimingTracker::FrameAndRectIds> composite_events;
nit: stick this up with the other vectors?

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6130:
client_.set_fill_with_nonsolid_color(true);
why?

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6146:
layer_tree_host()->SetViewportSize(gfx::Size(16, 16));
this is kinda weird, i think you should make the root layer 16x16 and just make
a child that's 1500x1500 with the requests.

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6154: FrameTimingTracker* tracker =
host_impl->frame_timing_tracker();
Why is this checking on the next commit instead of just checking in
DrawLayersOnThread. Can you add comments to explain that?

[vmpstr, 2015-02-02 23:32:29]

PTAL

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:193:
std::vector<FrameTimingTracker::FrameAndRectIds> composite_events;
On 2015/02/02 23:10:09, danakj wrote:
> nit: stick this up with the other vectors?

Done.

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6130:
client_.set_fill_with_nonsolid_color(true);
On 2015/02/02 23:10:09, danakj wrote:
> why?

Removed.

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6146:
layer_tree_host()->SetViewportSize(gfx::Size(16, 16));
On 2015/02/02 23:10:09, danakj wrote:
> this is kinda weird, i think you should make the root layer 16x16 and just
make
> a child that's 1500x1500 with the requests.

Done. (made it 200x200 since 16 is kinda of arbitrary, yet seems specific)

https://codereview.chromium.org/884243004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6154: FrameTimingTracker* tracker =
host_impl->frame_timing_tracker();
On 2015/02/02 23:10:09, danakj wrote:
> Why is this checking on the next commit instead of just checking in
> DrawLayersOnThread. Can you add comments to explain that?

Added a comment. There's no real reason except that I want to test that the
timestamps survive correctly until the next commit, which is when the main
thread will read off the values.

[danakj, 2015-02-02 23:57:29]

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6156: // Since in reality, the events will
be read by LayerTreeHost during commit,
Then it would be even better if you did this in DidCommit() and read them off
the LTH. Or is that not possible yet?

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6163: auto rect_1_it = timing_set->find(1);
const auto&?

[vmpstr, 2015-02-03 00:10:41]

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6156: // Since in reality, the events will
be read by LayerTreeHost during commit,
On 2015/02/02 23:57:29, danakj wrote:
> Then it would be even better if you did this in DidCommit() and read them off
> the LTH. Or is that not possible yet?

DidCommit doesn't have a host_impl... I think this would happen on BeginCommit
anyway. Do you think this is OK or do you still prefer oh DidCommit? I can
probably grab lthi from somewhere.

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6163: auto rect_1_it = timing_set->find(1);
On 2015/02/02 23:57:29, danakj wrote:
> const auto&?

We typically do iterators by value. I can make an explicit type if that's
clearer?

[danakj, 2015-02-03 00:21:28]

LGTM

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6156: // Since in reality, the events will
be read by LayerTreeHost during commit,
On 2015/02/03 00:10:41, vmpstr wrote:
> On 2015/02/02 23:57:29, danakj wrote:
> > Then it would be even better if you did this in DidCommit() and read them
off
> > the LTH. Or is that not possible yet?
> 
> DidCommit doesn't have a host_impl... I think this would happen on BeginCommit
> anyway. Do you think this is OK or do you still prefer oh DidCommit? I can
> probably grab lthi from somewhere.

Poking LTHI in DidCommit would be unthreadsafe. So then this is the right place
after all.

Sounds like you'll be adding some main thread logic, which you can then test
with this test instead of poking the LTHI directly. Add a TODO here for that?

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6163: auto rect_1_it = timing_set->find(1);
On 2015/02/03 00:10:41, vmpstr wrote:
> On 2015/02/02 23:57:29, danakj wrote:
> > const auto&?
> 
> We typically do iterators by value. I can make an explicit type if that's
> clearer?

Hm.. ya okay nvm!

[vmpstr, 2015-02-03 00:26:08]

Thanks!

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/884243004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:6156: // Since in reality, the events will
be read by LayerTreeHost during commit,
On 2015/02/03 00:21:28, danakj wrote:
> On 2015/02/03 00:10:41, vmpstr wrote:
> > On 2015/02/02 23:57:29, danakj wrote:
> > > Then it would be even better if you did this in DidCommit() and read them
> off
> > > the LTH. Or is that not possible yet?
> > 
> > DidCommit doesn't have a host_impl... I think this would happen on
BeginCommit
> > anyway. Do you think this is OK or do you still prefer oh DidCommit? I can
> > probably grab lthi from somewhere.
> 
> Poking LTHI in DidCommit would be unthreadsafe. So then this is the right
place
> after all.
> 
> Sounds like you'll be adding some main thread logic, which you can then test
> with this test instead of poking the LTHI directly. Add a TODO here for that?

Done.

[vmpstr, 2015-02-03 00:26:15]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-02-03 00:28:00]

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

[commit-bot: I haz the power, 2015-02-03 00:51:33]

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

[commit-bot: I haz the power, 2015-02-03 00:51:34]

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

[vmpstr, 2015-02-03 18:56:43]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-02-03 18:57:13]

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

[commit-bot: I haz the power, 2015-02-03 20:31:54]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-03 20:33:06]

Patchset 7 (id:??) landed as
https://crrev.com/99aa853cae0e968f54c15bbbd33c9abcc344d133
Cr-Commit-Position: refs/heads/master@{#314398}