Remove CompositorObserver::OnCompositingEnded()

Remove CompositorObserver::OnCompositingEnded()

OnCompistingEnded() is only used in tests. By replacing usages of
OnCompositingEnded() with OnCompositingStarted(), we are able to remove
the call to OnCompositingEnded() in
ui::Compositor::DidReceiveCompositorFrameAck() without making the tests less
effective.

This is the first step in simplifying CompositorFrameAcks.

BUG=671202
Review-Url: https://codereview.chromium.org/2776973004
Cr-Commit-Position: refs/heads/master@{#460409}
Committed: https://chromium.googlesource.com/chromium/src/+/ce75f9388f73820bc5bb31401bcc75520f467a3f

[Alex Z., 2017-03-27 18:20:21]

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

[commit-bot: I haz the power, 2017-03-27 18:21:21]

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

[Alex Z., 2017-03-27 18:32:13]

staraz@chromium.org changed reviewers:
+ danakj@chromium.org, fsamuel@chromium.org, varkha@chromium.org

[Alex Z., 2017-03-27 18:32:13]

fsamuel@chromium.org: PTAL before I send this CL to owners.

danakj@chromium.org & varkha@: please review changes in
ui/compositor/compositor*. Specifically, is it okay to move
++committed_frame_number_; to Compositor::DidCommit()?

[Fady Samuel, 2017-03-27 18:36:18]

Ahh, this is for latency tracking? Well moving it to DidCommit will certainly
change the latency metric because it doesn't measure round trip time to the
display compositor and back.

https://codereview.chromium.org/2776973004/diff/1/ui/compositor/layer_unittes...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/2776973004/diff/1/ui/compositor/layer_unittes...
ui/compositor/layer_unittest.cc:247: // void WaitForSwap() {
Delete this?

[commit-bot: I haz the power, 2017-03-27 18:38:31]

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

[commit-bot: I haz the power, 2017-03-27 18:38:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alex Z., 2017-03-27 18:52:38]

https://codereview.chromium.org/2776973004/diff/1/ui/compositor/layer_unittes...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/2776973004/diff/1/ui/compositor/layer_unittes...
ui/compositor/layer_unittest.cc:247: // void WaitForSwap() {
On 2017/03/27 18:36:18, Fady Samuel wrote:
> Delete this?

Done.

[varkha, 2017-03-27 19:41:52]

https://codereview.chromium.org/2776973004/diff/20001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2776973004/diff/20001/ui/compositor/composito...
ui/compositor/compositor.cc:528: ++committed_frame_number_;
Yea, this I think could change the metric as measured. The intention was to see
how many frame commits actually make it to the hardware and are visible. It is
used in the metric that measures how smooth the animations are by keeping track
of how many frames are presented between the start and the end of a given
animation.

[Alex Z., 2017-03-27 21:11:29]

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

[commit-bot: I haz the power, 2017-03-27 21:12:14]

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

[Fady Samuel, 2017-03-27 21:15:49]

If all we care about is frame count then this seems reasonable to me. LGTM.

You'll need piman@ for content changes.

[commit-bot: I haz the power, 2017-03-27 21:23:12]

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

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

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[varkha, 2017-03-27 21:31:28]

Maybe it is possible to try and see how much would the metric be impacted. It is
easy to get the Overview into relatively junky (just open 5-10 windows). So I
thought it is possible to put separate counting in both the new suggested place
and the old (DidReceiveCompositorFrameAcq) and see if the counts diverge on a
device under memory pressure.

[danakj, 2017-03-28 16:04:55]

https://codereview.chromium.org/2776973004/diff/40001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2776973004/diff/40001/ui/compositor/composito...
ui/compositor/compositor.cc:536: ++committed_frame_number_;
This would change the result of Layer::GetFrameNumber. I started trying to track
down all callers of that to see if any would happen after
DidSubmitCompositorFrame and before DidReceiveCompositorFrameAck, but there's
many callers, so can you do that to convince yourself and me that it's okay? In
the end its about animation ticking afaict. If you're stuck ajuma can help.

[Fady Samuel, 2017-03-28 17:35:05]

https://codereview.chromium.org/2776973004/diff/40001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2776973004/diff/40001/ui/compositor/composito...
ui/compositor/compositor.cc:536: ++committed_frame_number_;
On 2017/03/28 16:04:54, danakj wrote:
> This would change the result of Layer::GetFrameNumber. I started trying to
track
> down all callers of that to see if any would happen after
> DidSubmitCompositorFrame and before DidReceiveCompositorFrameAck, but there's
> many callers, so can you do that to convince yourself and me that it's okay?
In
> the end its about animation ticking afaict. If you're stuck ajuma can help.

As I said offline, we can undo this change and land the rest of the patch then
figure out the right way to measure animation jank separately.

[Alex Z., 2017-03-28 18:01:12]

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

[commit-bot: I haz the power, 2017-03-28 18:01:45]

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

[Alex Z., 2017-03-28 18:06:32]

staraz@chromium.org changed reviewers:
+ asvitkine@chromium.org, sky@chromium.org

[Alex Z., 2017-03-28 18:06:33]

sky@chromium.org: Please review changes in ash/ and ui/

asvitkine@: Please review changes in 
browser_compositor_view_mac.mm

[Alex Z., 2017-03-28 18:08:27]

https://codereview.chromium.org/2776973004/diff/40001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2776973004/diff/40001/ui/compositor/composito...
ui/compositor/compositor.cc:536: ++committed_frame_number_;
On 2017/03/28 16:04:54, danakj wrote:
> This would change the result of Layer::GetFrameNumber. I started trying to
track
> down all callers of that to see if any would happen after
> DidSubmitCompositorFrame and before DidReceiveCompositorFrameAck, but there's
> many callers, so can you do that to convince yourself and me that it's okay?
In
> the end its about animation ticking afaict. If you're stuck ajuma can help.

This change has been reverted in the latest patch.

[Alexei Svitkine (slow), 2017-03-28 18:37:09]

lgtm

[danakj, 2017-03-28 18:42:15]

https://codereview.chromium.org/2776973004/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2776973004/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:528: ++committed_frame_number_;
Did you mean to put this back in DidReceiveCompositorFrameAck?

[Alex Z., 2017-03-28 19:23:38]

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

[Alex Z., 2017-03-28 19:24:23]

https://codereview.chromium.org/2776973004/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2776973004/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:528: ++committed_frame_number_;
On 2017/03/28 18:42:15, danakj wrote:
> Did you mean to put this back in DidReceiveCompositorFrameAck?

Yes. Sorry about this.

[commit-bot: I haz the power, 2017-03-28 19:24:46]

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-28 20:21:00]

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

[commit-bot: I haz the power, 2017-03-28 20:21:01]

Dry run: This issue passed the CQ dry run.

[varkha, 2017-03-28 20:30:57]

PS5 lgtm.

[danakj, 2017-03-28 20:43:01]

ui/compositor LGTM

[sky, 2017-03-29 16:00:42]

LGTM

[Alex Z., 2017-03-29 16:03:41]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2017-03-29 16:03:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2776973004/#ps80001
(title: "Rename commited_frame_number_ to activated_frame_count_ and move
increments back to DidReceiveCompo…")

[commit-bot: I haz the power, 2017-03-29 16:04:37]

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-29 16:12:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490803421939810,
"parent_rev": "08a71580c7d69cf13bb7228e032f817d9a9c1bd0", "commit_rev":
"c8a8999a9176c92cd2959cb0a949be01d533231f"}

[commit-bot: I haz the power, 2017-03-29 16:17:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490803421939810,
"parent_rev": "1be12c0d8a2399ff96e2189919ac13f547ecf7d0", "commit_rev":
"ce75f9388f73820bc5bb31401bcc75520f467a3f"}

[commit-bot: I haz the power, 2017-03-29 16:17:59]

Description was changed from

==========
Remove CompositorObserver::OnCompositingEnded()

OnCompistingEnded() is only used in tests. By replacing usages of
OnCompositingEnded() with OnCompositingStarted(), we are able to remove
the call to OnCompositingEnded() in
ui::Compositor::DidReceiveCompositorFrameAck() without making the tests less
effective.

This is the first step in simplifying CompositorFrameAcks.

BUG=671202
==========

to

==========
Remove CompositorObserver::OnCompositingEnded()

OnCompistingEnded() is only used in tests. By replacing usages of
OnCompositingEnded() with OnCompositingStarted(), we are able to remove
the call to OnCompositingEnded() in
ui::Compositor::DidReceiveCompositorFrameAck() without making the tests less
effective.

This is the first step in simplifying CompositorFrameAcks.

BUG=671202

Review-Url: https://codereview.chromium.org/2776973004
Cr-Commit-Position: refs/heads/master@{#460409}
Committed:
https://chromium.googlesource.com/chromium/src/+/ce75f9388f73820bc5bb31401bcc...
==========

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

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

[Alex Z., 2017-04-12 12:27:01]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2815073002/ by staraz@chromium.org.

The reason for reverting is: Cause LayerWithRealCompositorTest.ReportMetrics to 
be flaky on Windows.

We don't need this change anyway now that 
MojoCompositorFrameSinkClient::WillDrawSurface() is 
removed..