Splitting DidSwap in cc::SwapPromise into WillSwap and DidSwap

Splitting DidSwap in cc::SwapPromise into WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that runs after it. DidSwap is also not the best name because
it implies it'll run after the swap, so we believe the best course of
action is to rename DidSwap to WillSwap and create a separate DidSwap
method that is run after the swap.

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

Committed: https://crrev.com/e7345c8c5ec3831fb46788c903ea62b67a7a4174
Cr-Commit-Position: refs/heads/master@{#438993}

[Saman Sami, 2016-12-14 21:46:41]

Description was changed from

==========
WillSwap

BUG=
==========

to

==========
WillSwap

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

[Saman Sami, 2016-12-14 21:46:53]

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

[commit-bot: I haz the power, 2016-12-14 21:47:22]

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

[Saman Sami, 2016-12-14 22:01:30]

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

[Saman Sami, 2016-12-14 22:01:44]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-12-14 22:02:54]

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-12-14 22:21:02]

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

[commit-bot: I haz the power, 2016-12-14 22:21:03]

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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2016-12-14 22:23:36]

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

[Saman Sami, 2016-12-14 22:23:40]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-12-14 22:24:07]

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

[Saman Sami, 2016-12-14 23:16:45]

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

[commit-bot: I haz the power, 2016-12-14 23:17:43]

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

[Saman Sami, 2016-12-15 00:23:25]

Description was changed from

==========
WillSwap

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

to

==========
WillSwap

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

[Saman Sami, 2016-12-15 00:23:25]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org

[commit-bot: I haz the power, 2016-12-15 00:35:28]

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

[commit-bot: I haz the power, 2016-12-15 00:35:29]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-15 21:07:11]

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

[commit-bot: I haz the power, 2016-12-15 21:07:57]

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

[Fady Samuel, 2016-12-15 21:08:41]

overall looks good, but a couple of things.

https://codereview.chromium.org/2580493002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2580493002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1669: active_tree()->ClearSwapPromises();
Is this necessary? Why was this added?

https://codereview.chromium.org/2580493002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1686: 
Remove blank line?

[Saman Sami, 2016-12-15 21:10:41]

Description was changed from

==========
WillSwap

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

to

==========
Splitting DidSwap in cc::SwapPromise to WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that is run after it. DidSwap is also not the best name because
it's called before the swap is done, so we believe WillSwap will
be a better name for it. So DidSwap will be renamed to WillSwap
and we will create a new DidSwap method that is run after the swap.

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

[Saman Sami, 2016-12-15 21:14:33]

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

[commit-bot: I haz the power, 2016-12-15 21:14:58]

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

[Saman Sami, 2016-12-15 21:17:22]

samans@chromium.org changed reviewers:
+ jam@chromium.org

[Saman Sami, 2016-12-15 21:20:55]

https://codereview.chromium.org/2580493002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2580493002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1669: active_tree()->ClearSwapPromises();
On 2016/12/15 21:08:40, Fady Samuel wrote:
> Is this necessary? Why was this added?

This calls DidSwap, and clears the swap promise list.

https://codereview.chromium.org/2580493002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1686: 
On 2016/12/15 21:08:40, Fady Samuel wrote:
> Remove blank line?

Done.

[Saman Sami, 2016-12-15 21:23:23]

samans@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Saman Sami, 2016-12-15 21:26:48]

Description was changed from

==========
Splitting DidSwap in cc::SwapPromise to WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that is run after it. DidSwap is also not the best name because
it's called before the swap is done, so we believe WillSwap will
be a better name for it. So DidSwap will be renamed to WillSwap
and we will create a new DidSwap method that is run after the swap.

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

to

==========
Splitting DidSwap in cc::SwapPromise to WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that is run after it. DidSwap is also not the best name because
it's called before the swap is done, so we believe WillSwap will
be a better name for it. So DidSwap will be renamed to WillSwap
and we will create a new DidSwap method that is run after the swap.

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

[David Trainor- moved to gerrit, 2016-12-15 21:29:37]

blimp/ lgtm

[Saman Sami, 2016-12-15 21:32:19]

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

[vmpstr, 2016-12-15 21:54:32]

Overall looks fine, but all of the DidSwap calls are empty (unless I missed
one). I assume there are plans to use this somewhere?

https://codereview.chromium.org/2580493002/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2580493002/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1669: active_tree()->ClearSwapPromises();
Add a comment here please

https://codereview.chromium.org/2580493002/diff/100001/content/renderer/gpu/q...
File content/renderer/gpu/queue_message_swap_promise_unittest.cc (right):

https://codereview.chromium.org/2580493002/diff/100001/content/renderer/gpu/q...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:123:
(*i)->WillSwap(NULL);
Does this need a DidSwap as well?

https://codereview.chromium.org/2580493002/diff/100001/content/renderer/gpu/q...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:153:
promises_[0]->WillSwap(NULL);
DidSwap as well?

https://codereview.chromium.org/2580493002/diff/100001/content/renderer/gpu/q...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:257:
promises_[0]->WillSwap(NULL);
DidSwap as well?

[Saman Sami, 2016-12-15 22:09:47]

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

[commit-bot: I haz the power, 2016-12-15 22:10:32]

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

[Saman Sami, 2016-12-15 22:11:59]

PTAL

Yes, most of DidSwaps are empty right now, but that's gonna change in future
CLs. Specifically, SatisfySwapPromise is going to need it. The SatisfyPromise in
blimp currently uses it as well.

https://codereview.chromium.org/2580493002/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2580493002/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1669: active_tree()->ClearSwapPromises();
On 2016/12/15 21:54:31, vmpstr wrote:
> Add a comment here please

Done.

https://codereview.chromium.org/2580493002/diff/100001/content/renderer/gpu/q...
File content/renderer/gpu/queue_message_swap_promise_unittest.cc (right):

https://codereview.chromium.org/2580493002/diff/100001/content/renderer/gpu/q...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:123:
(*i)->WillSwap(NULL);
On 2016/12/15 21:54:32, vmpstr wrote:
> Does this need a DidSwap as well?

Done.

https://codereview.chromium.org/2580493002/diff/100001/content/renderer/gpu/q...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:153:
promises_[0]->WillSwap(NULL);
On 2016/12/15 21:54:31, vmpstr wrote:
> DidSwap as well?

Done.

https://codereview.chromium.org/2580493002/diff/100001/content/renderer/gpu/q...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:257:
promises_[0]->WillSwap(NULL);
On 2016/12/15 21:54:31, vmpstr wrote:
> DidSwap as well?

Done.

[vmpstr, 2016-12-15 22:17:20]

cc lgtm

[commit-bot: I haz the power, 2016-12-16 00:21:01]

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

[commit-bot: I haz the power, 2016-12-16 00:21:03]

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

[jam, 2016-12-16 01:03:36]

lgtm

[Saman Sami, 2016-12-16 01:04:09]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2016-12-16 01:04:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2580493002/#ps120001
(title: "x")

[commit-bot: I haz the power, 2016-12-16 01:05:03]

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-12-16 02:51:20]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481850249070480,
"parent_rev": "bd4bc5e5d31855e1d3582746825b6716f9eea5df", "commit_rev":
"e0ba2981b51b119359437b3a399e6e58e9e30af2"}

[commit-bot: I haz the power, 2016-12-16 02:51:45]

Description was changed from

==========
Splitting DidSwap in cc::SwapPromise to WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that is run after it. DidSwap is also not the best name because
it's called before the swap is done, so we believe WillSwap will
be a better name for it. So DidSwap will be renamed to WillSwap
and we will create a new DidSwap method that is run after the swap.

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

to

==========
Splitting DidSwap in cc::SwapPromise to WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that is run after it. DidSwap is also not the best name because
it's called before the swap is done, so we believe WillSwap will
be a better name for it. So DidSwap will be renamed to WillSwap
and we will create a new DidSwap method that is run after the swap.

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

Review-Url: https://codereview.chromium.org/2580493002
==========

[commit-bot: I haz the power, 2016-12-16 02:51:46]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-12-16 02:54:21]

Description was changed from

==========
Splitting DidSwap in cc::SwapPromise to WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that is run after it. DidSwap is also not the best name because
it's called before the swap is done, so we believe WillSwap will
be a better name for it. So DidSwap will be renamed to WillSwap
and we will create a new DidSwap method that is run after the swap.

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

Review-Url: https://codereview.chromium.org/2580493002
==========

to

==========
Splitting DidSwap in cc::SwapPromise to WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that is run after it. DidSwap is also not the best name because
it's called before the swap is done, so we believe WillSwap will
be a better name for it. So DidSwap will be renamed to WillSwap
and we will create a new DidSwap method that is run after the swap.

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

Committed: https://crrev.com/e7345c8c5ec3831fb46788c903ea62b67a7a4174
Cr-Commit-Position: refs/heads/master@{#438993}
==========

[commit-bot: I haz the power, 2016-12-16 02:54:23]

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

[Saman Sami, 2016-12-16 21:48:47]

Description was changed from

==========
Splitting DidSwap in cc::SwapPromise to WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that is run after it. DidSwap is also not the best name because
it's called before the swap is done, so we believe WillSwap will
be a better name for it. So DidSwap will be renamed to WillSwap
and we will create a new DidSwap method that is run after the swap.

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

Committed: https://crrev.com/e7345c8c5ec3831fb46788c903ea62b67a7a4174
Cr-Commit-Position: refs/heads/master@{#438993}
==========

to

==========
Splitting DidSwap in cc::SwapPromise into WillSwap and DidSwap

Currently DidSwap runs before the swap, but we need a method
that runs after it. DidSwap is also not the best name because
it implies it'll run after the swap, so we believe the best course of
action is to rename DidSwap to WillSwap and create a separate DidSwap
method that is run after the swap.

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

Committed: https://crrev.com/e7345c8c5ec3831fb46788c903ea62b67a7a4174
Cr-Commit-Position: refs/heads/master@{#438993}
==========