cc: Add SwapPromiseManager and SurfaceSequenceGenerator.

cc: Add SwapPromiseManager and SurfaceSequenceGenerator.

Move SwapPromise tracking to SwapPromiseManager and SurfaceSequnce
creation to SurfaceSequenceGenerator.

BUG=625283
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/8297ae9916ffbacac9a30107eb54e0950e9bdb3f
Cr-Commit-Position: refs/heads/master@{#418666}

[Khushal, 2016-09-09 19:42:29]

Description was changed from

==========
cc: Add SwapPromiseManager and SurfaceSequenceGenerator.

Move SwapPromise tracking to SwapPromiseManager and SurfaceSequnce
creation to SurfaceSequenceGenerator.

BUG=625283
==========

to

==========
cc: Add SwapPromiseManager and SurfaceSequenceGenerator.

Move SwapPromise tracking to SwapPromiseManager and SurfaceSequnce
creation to SurfaceSequenceGenerator.

BUG=625283
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Khushal, 2016-09-09 19:46:50]

khushalsagar@chromium.org changed reviewers:
+ vmpstr@chromium.org

[Khushal, 2016-09-09 19:46:51]

Felt cleaner to compose all these elements into the LayerTreeHost rather than
stashing everything into the host and adding many many virtual methods. PTAL.

[Khushal, 2016-09-09 19:47:33]

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

[commit-bot: I haz the power, 2016-09-09 19:48:12]

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, 2016-09-09 19:53:38]

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

[commit-bot: I haz the power, 2016-09-09 19:53:39]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vmpstr, 2016-09-09 20:45:45]

https://codereview.chromium.org/2323423002/diff/1/cc/trees/swap_promise_manag...
File cc/trees/swap_promise_manager.cc (right):

https://codereview.chromium.org/2323423002/diff/1/cc/trees/swap_promise_manag...
cc/trees/swap_promise_manager.cc:46: return std::move(swap_promise_list_);
This leaves swap_promise_list_ in an unspecified state. Although this very
likely leaves the source vector empty (due to the complexity guarantee of
vector(vector&&)), it's not guaranteed in all cases...

I wonder if we should do something like

std::vector<std::unique_ptr<SwapPromise>> result;
result.swap(swap_promise_list_);
return result;

(this is just me being pedantic, so I can be convinced that this is fine)

[Khushal, 2016-09-09 21:09:35]

I moved the SurfaceSequenceGenerator to cc itself. Looks like in general cc does
not want to depend on the Surfaces target. The few things we have are
identifiers that are probably necessary.

https://codereview.chromium.org/2323423002/diff/1/cc/trees/swap_promise_manag...
File cc/trees/swap_promise_manager.cc (right):

https://codereview.chromium.org/2323423002/diff/1/cc/trees/swap_promise_manag...
cc/trees/swap_promise_manager.cc:46: return std::move(swap_promise_list_);
On 2016/09/09 20:45:45, vmpstr wrote:
> This leaves swap_promise_list_ in an unspecified state. Although this very
> likely leaves the source vector empty (due to the complexity guarantee of
> vector(vector&&)), it's not guaranteed in all cases...
> 
> I wonder if we should do something like
> 
> std::vector<std::unique_ptr<SwapPromise>> result;
> result.swap(swap_promise_list_);
> return result;
> 
> (this is just me being pedantic, so I can be convinced that this is fine)

Thanks for pointing this out. It was quite subtle, sorry I missed it earlier.
Done.

[Khushal, 2016-09-09 21:09:41]

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

[commit-bot: I haz the power, 2016-09-09 21:10:35]

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, 2016-09-09 21:28:24]

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

[commit-bot: I haz the power, 2016-09-09 21:28:24]

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

[Khushal, 2016-09-09 21:40:15]

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

[commit-bot: I haz the power, 2016-09-09 21:40:45]

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, 2016-09-09 23:01:20]

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

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

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-09-09 23:30:27]

khushalsagar@chromium.org changed reviewers:
+ nyquist@chromium.org, piman@chromium.org

[Khushal, 2016-09-09 23:30:27]

+piman for stamping content and ui
+nyquist for blimp

[Khushal, 2016-09-12 14:15:10]

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

[commit-bot: I haz the power, 2016-09-12 14:15:19]

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, 2016-09-12 15:38:38]

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

[commit-bot: I haz the power, 2016-09-12 15:38:39]

Dry run: This issue passed the CQ dry run.

[nyquist, 2016-09-12 19:02:43]

blimp lgtm

[piman, 2016-09-12 20:35:44]

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

https://codereview.chromium.org/2323423002/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:638:
layer_tree_host_->GetSwapPromiseManager()->QueueSwapPromise(
I have a mild preference to keeping a QueueSwapPromise and SetSurfaceClientId
APIs on LTH, that would forward to their individual tear-off. 2 reasons:
1- They both seem like legitimate operations one would do on LTH
2- Law of Demeter. The fact that those go to tear-off classes is an
implementation detail of LTH, and the client shouldn't have to know. It also
increases coupling.

[vmpstr, 2016-09-12 20:44:31]

cc lgtm % piman

https://codereview.chromium.org/2323423002/diff/80001/cc/trees/swap_promise_m...
File cc/trees/swap_promise_manager.cc (right):

https://codereview.chromium.org/2323423002/diff/80001/cc/trees/swap_promise_m...
cc/trees/swap_promise_manager.cc:35: for (; it != swap_promise_monitor_.end();
it++)
for (auto* swap_promise_monitor : swap_promise_monitor_)

https://codereview.chromium.org/2323423002/diff/80001/cc/trees/swap_promise_m...
File cc/trees/swap_promise_manager.h (right):

https://codereview.chromium.org/2323423002/diff/80001/cc/trees/swap_promise_m...
cc/trees/swap_promise_manager.h:51: std::set<SwapPromiseMonitor*>
swap_promise_monitor_;
swap_promise_monitors_?

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

https://codereview.chromium.org/2323423002/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:631: return
std::unique_ptr<cc::SwapPromiseMonitor>(
does base::MakeUnique<cc::LatencyInfoSwapPromiseMonitor> work here?

[Khushal, 2016-09-12 22:53:03]

https://codereview.chromium.org/2323423002/diff/80001/cc/trees/swap_promise_m...
File cc/trees/swap_promise_manager.cc (right):

https://codereview.chromium.org/2323423002/diff/80001/cc/trees/swap_promise_m...
cc/trees/swap_promise_manager.cc:35: for (; it != swap_promise_monitor_.end();
it++)
On 2016/09/12 20:44:31, vmpstr wrote:
> for (auto* swap_promise_monitor : swap_promise_monitor_)

Done.

https://codereview.chromium.org/2323423002/diff/80001/cc/trees/swap_promise_m...
File cc/trees/swap_promise_manager.h (right):

https://codereview.chromium.org/2323423002/diff/80001/cc/trees/swap_promise_m...
cc/trees/swap_promise_manager.h:51: std::set<SwapPromiseMonitor*>
swap_promise_monitor_;
On 2016/09/12 20:44:31, vmpstr wrote:
> swap_promise_monitors_?

Done.

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

https://codereview.chromium.org/2323423002/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:631: return
std::unique_ptr<cc::SwapPromiseMonitor>(
On 2016/09/12 20:44:31, vmpstr wrote:
> does base::MakeUnique<cc::LatencyInfoSwapPromiseMonitor> work here?

Done.

https://codereview.chromium.org/2323423002/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:638:
layer_tree_host_->GetSwapPromiseManager()->QueueSwapPromise(
On 2016/09/12 20:35:44, piman OOO back 2016-09-12 wrote:
> I have a mild preference to keeping a QueueSwapPromise and SetSurfaceClientId
> APIs on LTH, that would forward to their individual tear-off. 2 reasons:
> 1- They both seem like legitimate operations one would do on LTH
> 2- Law of Demeter. The fact that those go to tear-off classes is an
> implementation detail of LTH, and the client shouldn't have to know. It also
> increases coupling.

Done. Retained those 2 APIs.

[Khushal, 2016-09-12 23:05:07]

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

[commit-bot: I haz the power, 2016-09-12 23:05:26]

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, 2016-09-13 00:18:21]

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

[commit-bot: I haz the power, 2016-09-13 00:18:22]

Dry run: This issue passed the CQ dry run.

[piman, 2016-09-13 00:19:35]

lgtm

[Khushal, 2016-09-13 17:31:11]

https://codereview.chromium.org/2323423002/diff/140001/cc/layers/layer.h
File cc/layers/layer.h (left):

https://codereview.chromium.org/2323423002/diff/140001/cc/layers/layer.h#oldc...
cc/layers/layer.h:395: // http://crbug.com/628683
When I actually got around to having Layers go through the tree to use the host,
it was adding an unnecessary layer of wrapping, since Layers use everything from
the host anyway. https://codereview.chromium.org/2331983002/ will change it to a
protected member but I think we can leave everything as-is for Layer and
friends.
vmpstr@, could you take another look?

[vmpstr, 2016-09-13 18:39:44]

https://codereview.chromium.org/2323423002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/2323423002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:352: std::unique_ptr<SwapPromiseManager>
swap_promise_manager_;
If this is tied to the lifetime of the enclosing class, please don't use a
pointer.

https://codereview.chromium.org/2323423002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_serialization.cc (left):

https://codereview.chromium.org/2323423002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_serialization.cc:212:
EXPECT_EQ(layer_tree_host_src_->surface_client_id_,
Is this just gone? Should be add unittests for the new classes you've added?

[Khushal, 2016-09-14 19:29:12]

https://codereview.chromium.org/2323423002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/2323423002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:352: std::unique_ptr<SwapPromiseManager>
swap_promise_manager_;
On 2016/09/13 18:39:44, vmpstr wrote:
> If this is tied to the lifetime of the enclosing class, please don't use a
> pointer.

Done.

https://codereview.chromium.org/2323423002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_serialization.cc (left):

https://codereview.chromium.org/2323423002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_serialization.cc:212:
EXPECT_EQ(layer_tree_host_src_->surface_client_id_,
On 2016/09/13 18:39:44, vmpstr wrote:
> Is this just gone? Should be add unittests for the new classes you've added?

Blimp doesn't need to care about SurfaceLayers. I'll add unittests for the new
classes. Good idea!

[Khushal, 2016-09-14 19:29:19]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-09-14 19:29:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, vmpstr@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2323423002/#ps160001
(title: "tests")

[commit-bot: I haz the power, 2016-09-14 19:29:56]

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, 2016-09-14 20:51:48]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-14 20:53:59]

Description was changed from

==========
cc: Add SwapPromiseManager and SurfaceSequenceGenerator.

Move SwapPromise tracking to SwapPromiseManager and SurfaceSequnce
creation to SurfaceSequenceGenerator.

BUG=625283
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Add SwapPromiseManager and SurfaceSequenceGenerator.

Move SwapPromise tracking to SwapPromiseManager and SurfaceSequnce
creation to SurfaceSequenceGenerator.

BUG=625283
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/8297ae9916ffbacac9a30107eb54e0950e9bdb3f
Cr-Commit-Position: refs/heads/master@{#418666}
==========

[commit-bot: I haz the power, 2016-09-14 20:54:00]

Patchset 9 (id:??) landed as
https://crrev.com/8297ae9916ffbacac9a30107eb54e0950e9bdb3f
Cr-Commit-Position: refs/heads/master@{#418666}