SetPreviousFrameSurface should copy latency info to the new surface

In an effort to simplify DelegatedFrameHost, this CL moves the code for
copying latency info from old surface to new surface to cc.

This needs to be done because the code for capturing screenshots depends
on the fact that that a particular latency info reaches cc::Display. When
resizing, the latency info can remain in the old surface which is not
drawn, and screenshot times out. A previous CL of mine had this problem
and got reverted. 

BUG=696199
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2721763002
Cr-Commit-Position: refs/heads/master@{#454101}
Committed: https://chromium.googlesource.com/chromium/src/+/71ca55f2b59fe14c1a803f00c2c219502fd5d9bb

[Saman Sami, 2017-02-27 23:48:21]

Description was changed from

==========
Test
==========

to

==========
Test
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-02-27 23:48:42]

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

[commit-bot: I haz the power, 2017-02-27 23:49:21]

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

[Saman Sami, 2017-02-28 00:09:29]

Description was changed from

==========
Test
==========

to

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.
==========

[commit-bot: I haz the power, 2017-02-28 00:50:47]

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

[commit-bot: I haz the power, 2017-02-28 00:50:47]

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

[Saman Sami, 2017-02-28 01:16:25]

Description was changed from

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.
==========

to

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-02-28 01:16:37]

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

[commit-bot: I haz the power, 2017-02-28 01:17:14]

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-02-28 01:52:35]

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

[commit-bot: I haz the power, 2017-02-28 01:52:36]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Saman Sami, 2017-02-28 02:25:42]

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

[commit-bot: I haz the power, 2017-02-28 02:26:32]

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-02-28 04:07:42]

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

[commit-bot: I haz the power, 2017-02-28 04:07:43]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-02-28 16:00:02]

Description was changed from

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-02-28 16:00:02]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org, jbauman@chromium.org, tdresser@chromium.org

[Saman Sami, 2017-02-28 18:53:26]

jbauman@: Can you please take a look since this involves DelegatedFrameHost and
Surface stuff.

tdresser@: fsamuel@ told me you know about latency stuff. Can you take a look
please?

[Fady Samuel, 2017-02-28 18:54:24]

Please add a BUG=bugnumber.

[Saman Sami, 2017-02-28 19:00:14]

Description was changed from

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.

This needs to be done because the code for capturing screenshots depends
on the fact that that a particular latency info reaches cc::Display. When
resizing, the latency info can remain in the old surface which is not
drawn, and screenshot times out. A previous CL of mine had this problem
and got reverted. 

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-02-28 19:01:04]

Description was changed from

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.

This needs to be done because the code for capturing screenshots depends
on the fact that that a particular latency info reaches cc::Display. When
resizing, the latency info can remain in the old surface which is not
drawn, and screenshot times out. A previous CL of mine had this problem
and got reverted. 

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.

This needs to be done because the code for capturing screenshots depends
on the fact that that a particular latency info reaches cc::Display. When
resizing, the latency info can remain in the old surface which is not
drawn, and screenshot times out. A previous CL of mine had this problem
and got reverted. 

BUG=696199
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[tdresser, 2017-02-28 20:25:45]

https://codereview.chromium.org/2721763002/diff/30001/content/browser/rendere...
File content/browser/renderer_host/delegated_frame_host.cc (left):

https://codereview.chromium.org/2721763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:465:
frame.metadata.latency_info.clear();
I have context on LatencyInfo, but not much context on this part of the code.

The CL description makes it sound like this is just a refactor, without any
behavior change, but this looks like it's changing behavior.

Is it? If we are changing behavior, the CL description should make it clear, and
we should have a test.

[Saman Sami, 2017-02-28 20:33:44]

On 2017/02/28 20:25:45, tdresser wrote:
>
https://codereview.chromium.org/2721763002/diff/30001/content/browser/rendere...
> File content/browser/renderer_host/delegated_frame_host.cc (left):
> 
>
https://codereview.chromium.org/2721763002/diff/30001/content/browser/rendere...
> content/browser/renderer_host/delegated_frame_host.cc:465:
> frame.metadata.latency_info.clear();
> I have context on LatencyInfo, but not much context on this part of the code.
> 
> The CL description makes it sound like this is just a refactor, without any
> behavior change, but this looks like it's changing behavior.
> 
> Is it? If we are changing behavior, the CL description should make it clear,
and
> we should have a test.

Yes, the behavior is not exactly the same. So far only DelegatedFrameHost used
to do this, but from now every client of SurfaceFactory will have its latency
infos carried over to the new surface. Also, I can think of scenarios that
DelegatedFrameHost would not copy latency infos but after this CL they will be.
I don't know a lot about LatencyInfo, that's why we thought it'll be a good idea
to ask you to comment on this change. Could this change have any unwanted
side-effects? When do we want and when do we not want to copy the latency info
to the new surface? If this is the right behaviour, I can write a unit test to
ensure it doesn't change in the future.

[tdresser, 2017-02-28 20:49:45]

Always copying the LatencyInfo should be correct.

[jbauman, 2017-02-28 23:11:09]

https://codereview.chromium.org/2721763002/diff/30001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2721763002/diff/30001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:55: active_frame_->metadata.latency_info.insert(
Also check whether active_frame_ exists for the new surface.

[Fady Samuel, 2017-03-01 04:30:56]

https://codereview.chromium.org/2721763002/diff/30001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2721763002/diff/30001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:55: active_frame_->metadata.latency_info.insert(
On 2017/02/28 23:11:09, jbauman wrote:
> Also check whether active_frame_ exists for the new surface.

I actually think that we might want to place the LatencyInfo on the pending
frame if there's no active frame or else it will get lost?

[Saman Sami, 2017-03-01 15:34:06]

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

[commit-bot: I haz the power, 2017-03-01 15:34:25]

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

[Saman Sami, 2017-03-01 16:19:44]

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

[commit-bot: I haz the power, 2017-03-01 16:19:56]

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

[Saman Sami, 2017-03-01 16:20:00]

Patchset #5 (id:70001) has been deleted

[commit-bot: I haz the power, 2017-03-01 16:21:55]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Saman Sami, 2017-03-01 16:27:27]

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

[commit-bot: I haz the power, 2017-03-01 16:27:46]

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

[Saman Sami, 2017-03-01 16:29:16]

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

[commit-bot: I haz the power, 2017-03-01 16:29:30]

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

[Saman Sami, 2017-03-01 16:33:21]

Description was changed from

==========
Currently, DelegatedFrameHost deals with copying latency info from the
old surface to the new surface on resize. This is something that cc
code should handle. From now on, SetPreviousFrameSurface takes care of
this.

This needs to be done because the code for capturing screenshots depends
on the fact that that a particular latency info reaches cc::Display. When
resizing, the latency info can remain in the old surface which is not
drawn, and screenshot times out. A previous CL of mine had this problem
and got reverted. 

BUG=696199
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
In an effort to simplify DelegatedFrameHost, this CL moves the code for
copying latency info from old surface to new surface to cc.

This needs to be done because the code for capturing screenshots depends
on the fact that that a particular latency info reaches cc::Display. When
resizing, the latency info can remain in the old surface which is not
drawn, and screenshot times out. A previous CL of mine had this problem
and got reverted. 

BUG=696199
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-03-01 16:34:31]

PTAL

I added a unit test and also added support for pending frame.

[Saman Sami, 2017-03-01 16:35:20]

https://codereview.chromium.org/2721763002/diff/30001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2721763002/diff/30001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:55: active_frame_->metadata.latency_info.insert(
On 2017/02/28 23:11:09, jbauman wrote:
> Also check whether active_frame_ exists for the new surface.

A frame was just submitted, so either pending or active frame should exist.

[Fady Samuel, 2017-03-01 16:48:24]

https://codereview.chromium.org/2721763002/diff/60003/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory_unittest.cc (right):

https://codereview.chromium.org/2721763002/diff/60003/cc/surfaces/surface_fac...
cc/surfaces/surface_factory_unittest.cc:826: TEST_F(SurfaceFactoryTest,
LatencyInfoCarriedOverOnResize) {
Could you move this to compositor_frame_sink_support_unittest and verify as well
that LatencyInfo doesn't get lost if we have a pending frame?

[Saman Sami, 2017-03-01 19:01:18]

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

[commit-bot: I haz the power, 2017-03-01 19:01:49]

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

[Saman Sami, 2017-03-01 19:30:09]

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

[commit-bot: I haz the power, 2017-03-01 19:30:21]

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

[Saman Sami, 2017-03-01 19:30:50]

PTAL

I fixed the logic for pending frames and added new unit tests.

[Fady Samuel, 2017-03-01 19:35:19]

lgtm + nits

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:773: std::move(frame2));
nit: Add an expectation that the surface has a pending frame.

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:781: Surface* surface =
surface_manager().GetSurfaceForId(parent_id2);
ASSERT_NE(nullptr, surface)? Also, given this is the current surface, simply use
parent_surface() and maybe verify the surface ID?

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:831: std::move(frame2));
Verify there's a pending frame.

[Fady Samuel, 2017-03-01 19:35:20]

lgtm + nits

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:773: std::move(frame2));
nit: Add an expectation that the surface has a pending frame.

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:781: Surface* surface =
surface_manager().GetSurfaceForId(parent_id2);
ASSERT_NE(nullptr, surface)? Also, given this is the current surface, simply use
parent_surface() and maybe verify the surface ID?

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:831: std::move(frame2));
Verify there's a pending frame.

[Saman Sami, 2017-03-01 19:48:44]

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

[commit-bot: I haz the power, 2017-03-01 19:49:24]

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

[Saman Sami, 2017-03-01 19:49:30]

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:773: std::move(frame2));
On 2017/03/01 19:35:19, Fady Samuel wrote:
> nit: Add an expectation that the surface has a pending frame.

Done.

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:781: Surface* surface =
surface_manager().GetSurfaceForId(parent_id2);
On 2017/03/01 19:35:19, Fady Samuel wrote:
> ASSERT_NE(nullptr, surface)? Also, given this is the current surface, simply
use
> parent_surface() and maybe verify the surface ID?

I prefer the assert.

https://codereview.chromium.org/2721763002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:831: std::move(frame2));
On 2017/03/01 19:35:19, Fady Samuel wrote:
> Verify there's a pending frame.

I do it below.

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

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

[commit-bot: I haz the power, 2017-03-01 20:55:02]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-03-01 22:11:08]

lgtm with one nit.

https://codereview.chromium.org/2721763002/diff/200001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2721763002/diff/200001/cc/surfaces/surface.h#...
cc/surfaces/surface.h:137: void TakeLatencyInfoFromFrame(CompositorFrame* frame,
Make this static.

[Fady Samuel, 2017-03-01 22:17:43]

https://codereview.chromium.org/2721763002/diff/200001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2721763002/diff/200001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:272: void
Surface::TakeLatencyInfo(std::vector<ui::LatencyInfo>* latency_info) {
nit: TakeLatencyInfoFromActiveFrame?

[Saman Sami, 2017-03-01 22:32:23]

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

[commit-bot: I haz the power, 2017-03-01 22:33:10]

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

[Saman Sami, 2017-03-01 22:35:05]

https://codereview.chromium.org/2721763002/diff/200001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2721763002/diff/200001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:272: void
Surface::TakeLatencyInfo(std::vector<ui::LatencyInfo>* latency_info) {
On 2017/03/01 22:17:43, Fady Samuel wrote:
> nit: TakeLatencyInfoFromActiveFrame?

We're gonna leave this for another CL as we discussed.

https://codereview.chromium.org/2721763002/diff/200001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2721763002/diff/200001/cc/surfaces/surface.h#...
cc/surfaces/surface.h:137: void TakeLatencyInfoFromFrame(CompositorFrame* frame,
On 2017/03/01 22:11:08, jbauman wrote:
> Make this static.

Done.

[Saman Sami, 2017-03-01 23:41:05]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2017-03-01 23:41:11]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-03-01 23:41:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2721763002/#ps220001
(title: "c")

[commit-bot: I haz the power, 2017-03-01 23:41:22]

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-01 23:50:12]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1488411671069670,
"parent_rev": "0a07691703b543c6c6f2652b708fb18c86f0f617", "commit_rev":
"71ca55f2b59fe14c1a803f00c2c219502fd5d9bb"}

[commit-bot: I haz the power, 2017-03-01 23:50:48]

Description was changed from

==========
In an effort to simplify DelegatedFrameHost, this CL moves the code for
copying latency info from old surface to new surface to cc.

This needs to be done because the code for capturing screenshots depends
on the fact that that a particular latency info reaches cc::Display. When
resizing, the latency info can remain in the old surface which is not
drawn, and screenshot times out. A previous CL of mine had this problem
and got reverted. 

BUG=696199
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
In an effort to simplify DelegatedFrameHost, this CL moves the code for
copying latency info from old surface to new surface to cc.

This needs to be done because the code for capturing screenshots depends
on the fact that that a particular latency info reaches cc::Display. When
resizing, the latency info can remain in the old surface which is not
drawn, and screenshot times out. A previous CL of mine had this problem
and got reverted. 

BUG=696199
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2721763002
Cr-Commit-Position: refs/heads/master@{#454101}
Committed:
https://chromium.googlesource.com/chromium/src/+/71ca55f2b59fe14c1a803f00c2c2...
==========

[commit-bot: I haz the power, 2017-03-01 23:50:50]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/71ca55f2b59fe14c1a803f00c2c2...