Move VISUAL_STATE promise to activation

Move VISUAL_STATE promise to activation

Motivation is for android webview where compositor swaps
is not controlled by chromium code, but activations are.

Add a DidActivate to SwapPromise. This is an additional
call before DidSwap. It is not meant to be the end of a
SwapPromise as either DidSwap or DidNotSwap can still be
called afterwards.

Then modify the message queue to mark VISUAL_STATE messages
as ready to deliver in DidActivate. On chrome, the messages
are still delivered in swap, so there is no additional IPCs.
In webview, they can be delivered in the activation
callback.

BUG=431166
Committed: https://crrev.com/93d4648413ae2c80388e667dc37257308fa9e889
Cr-Commit-Position: refs/heads/master@{#330128}

[boliu, 2015-05-08 04:27:22]

boliu@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2015-05-08 04:27:24]

How about just Promises and don't distinguish between swap vs activation. So
each promise will go through both DidActivate and DidSwap, and DidNotActivate
implies DidNotSwap, but DidNotSwap can come after DidActivate? Then don't need
separate things like BreakActivationPromises + BreakSwapPromises. Probably won't
need PromiseList either?

Similarly, don't need separate QueueMessageActivationPromise and
QueueMessageSwapPromise implementations. The policy can stay in
FrameUpdateMessageQueue, and the QueueSwapPromise can deliver both activation
and swap signals.

Don't need MESSAGE_DELIVERY_POLICY_WITH_ACTIVATION I think? VISUAL_STATE should
cover it.

Overall the size of the patch is due to renaming classes. Otherwise this would
not actually be a huge change

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

https://codereview.chromium.org/1126963006/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1865: CompositorFrameMetadata metadata =
MakeCompositorFrameMetadata();
There's no frame for activation. DidActivate should not need frame metadata.

[boliu, 2015-05-13 16:07:47]

Need *new* cc_unittests, check after activate but before swap that DidActivate
is called but DidSwap is not.

https://codereview.chromium.org/1126963006/diff/260001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/260001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:4956:
EXPECT_FALSE(result_->did_not_swap_called);
EXPECT_FALSE(did_activate_called) as well

https://codereview.chromium.org/1126963006/diff/260001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:4972: EXPECT_FALSE(reason ==
DidNotSwapReason::ACTIVATION_FAILS &&
There is DCHECK_IMPLIES, could use that to make this clearer?

[boliu, 2015-05-13 21:26:00]

boliu@chromium.org changed reviewers:
+ piman@chromium.org

[boliu, 2015-05-13 21:26:01]

piman: ptal

Still need to add new cc tests, but the rest of the change is good to review

[piman, 2015-05-13 22:47:15]

A few things but otherwise LGTM once Bo is happy.

https://codereview.chromium.org/1126963006/diff/280001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1126963006/diff/280001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1080:
swap_promise_list_.insert_and_take(swap_promise_list_.end(),
nit: can this be a swap now? That's likely more efficient.

https://codereview.chromium.org/1126963006/diff/280001/content/renderer/gpu/f...
File content/renderer/gpu/frame_swap_message_queue.cc (right):

https://codereview.chromium.org/1126963006/diff/280001/content/renderer/gpu/f...
content/renderer/gpu/frame_swap_message_queue.cc:173: // If COMMIT_FAILS the
renderer is shutting down, which will
Is the comment still true for ACTIVATION_FAILS?
If yes, can you fix the comment?
If no, is there something else we need to do?

https://codereview.chromium.org/1126963006/diff/280001/content/renderer/gpu/f...
File content/renderer/gpu/frame_swap_message_queue_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/280001/content/renderer/gpu/f...
content/renderer/gpu/frame_swap_message_queue_unittest.cc:290: }
Can you add more tests for the new conditions?

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

https://codereview.chromium.org/1126963006/diff/280001/content/renderer/gpu/q...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:284: // COMMIT_FAILS
and ACTIVATE_FAILS are treated differently:
nit: this reads wrong - it sounds like COMMIT_FAILS and ACTIVATE_FAILS are
treated differently from each other.

[Tobias Sargeant, 2015-05-14 11:31:59]

https://codereview.chromium.org/1126963006/diff/260001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/260001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:4956:
EXPECT_FALSE(result_->did_not_swap_called);
On 2015/05/13 16:07:46, boliu wrote:
> EXPECT_FALSE(did_activate_called) as well

Done.

https://codereview.chromium.org/1126963006/diff/260001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:4972: EXPECT_FALSE(reason ==
DidNotSwapReason::ACTIVATION_FAILS &&
On 2015/05/13 16:07:46, boliu wrote:
> There is DCHECK_IMPLIES, could use that to make this clearer?

I think that it's a good idea to stick with gtest-based macros here. To that end
I did something a bit different here to make it more readable. Tell me what you
think.

[boliu, 2015-05-14 14:40:22]

I can think of two more cc_unittests to add. I think they aren't there

* SWAP_FAILED after DidActivate
* ACTIVATION_FAILED

https://codereview.chromium.org/1126963006/diff/300001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/300001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:4974: EXPECT_TRUE(implies(reason ==
DidNotSwapReason::ACTIVATION_FAILS,
That's overkill. Can you just write it as EXPECT_TRUE(!foo || bar) then

https://codereview.chromium.org/1126963006/diff/300001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:5010: if (host_impl->pending_tree()) {
Is this check the same as "host_impl->settings().impl_side_painting". Prefer to
use that, and assert the pending tree is there if impl side painting is on.

https://codereview.chromium.org/1126963006/diff/300001/content/renderer/gpu/f...
File content/renderer/gpu/frame_swap_message_queue_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/300001/content/renderer/gpu/f...
content/renderer/gpu/frame_swap_message_queue_unittest.cc:218:
ASSERT_TRUE(msg_delivered == HasMessageForId(messages,
first_message_.routing_id()));
80 char / line

https://codereview.chromium.org/1126963006/diff/300001/content/renderer/gpu/f...
content/renderer/gpu/frame_swap_message_queue_unittest.cc:242:
TestDidNotSwap(cc::SwapPromise::SWAP_FAILS);
In theory, SWAP_FAILS has to come after DidActivate. It's just we are not
DCHECKing that right now.

[boliu, 2015-05-14 16:30:17]

Oh, one more test. Check DidActivate already happened in
tree_activation_callback_, since that's webview is delivering messages.

[Tobias Sargeant, 2015-05-14 21:40:28]

On 2015/05/14 16:30:17, boliu wrote:
> Oh, one more test. Check DidActivate already happened in
> tree_activation_callback_, since that's webview is delivering messages.

Done.

[Tobias Sargeant, 2015-05-14 21:40:48]

https://codereview.chromium.org/1126963006/diff/300001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/300001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:4974: EXPECT_TRUE(implies(reason ==
DidNotSwapReason::ACTIVATION_FAILS,
On 2015/05/14 14:40:21, boliu wrote:
> That's overkill. Can you just write it as EXPECT_TRUE(!foo || bar) then

Yes, but then we're almost back to where we started.

IMO, the following is the most legible:

EXPECT_FALSE(result_->did_activate_called && reason ==
DidNotSwapReason::ACTIVATION_FAILS);

https://codereview.chromium.org/1126963006/diff/300001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:5010: if (host_impl->pending_tree()) {
On 2015/05/14 14:40:21, boliu wrote:
> Is this check the same as "host_impl->settings().impl_side_painting". Prefer
to
> use that, and assert the pending tree is there if impl side painting is on.

No, it's not the same.

If you assert that pending_tree is not null, the following tests fail:

8 tests failed:
   
LayerTreeHostTestBreakSwapPromise.RunMultiThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestBreakSwapPromise.RunMultiThread_DirectRenderer_MainThreadPaint
   
LayerTreeHostTestKeepSwapPromise.RunMultiThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestKeepSwapPromise.RunMultiThread_DirectRenderer_MainThreadPaint
   
LayerTreeHostTestKeepSwapPromise.RunSingleThread_DelegatingRenderer_ImplSidePaint
   
LayerTreeHostTestKeepSwapPromise.RunSingleThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestKeepSwapPromise.RunSingleThread_DirectRenderer_ImplSidePaint
   
LayerTreeHostTestKeepSwapPromise.RunSingleThread_DirectRenderer_MainThreadPaint

https://codereview.chromium.org/1126963006/diff/300001/content/renderer/gpu/f...
File content/renderer/gpu/frame_swap_message_queue_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/300001/content/renderer/gpu/f...
content/renderer/gpu/frame_swap_message_queue_unittest.cc:218:
ASSERT_TRUE(msg_delivered == HasMessageForId(messages,
first_message_.routing_id()));
On 2015/05/14 14:40:21, boliu wrote:
> 80 char / line

Done.

https://codereview.chromium.org/1126963006/diff/300001/content/renderer/gpu/f...
content/renderer/gpu/frame_swap_message_queue_unittest.cc:242:
TestDidNotSwap(cc::SwapPromise::SWAP_FAILS);
On 2015/05/14 14:40:22, boliu wrote:
> In theory, SWAP_FAILS has to come after DidActivate. It's just we are not
> DCHECKing that right now.

Acknowledged.

[Tobias Sargeant, 2015-05-14 21:43:33]

On 2015/05/14 14:40:22, boliu wrote:
> I can think of two more cc_unittests to add. I think they aren't there
> 
> * SWAP_FAILED after DidActivate

LayerTreeHostTestBreakSwapPromise has a code path that asserts that DidActivate
occurs in the case where SWAP_FAILS occurs due to no damage.

[boliu, 2015-05-15 03:50:50]

https://codereview.chromium.org/1126963006/diff/300001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/300001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:5010: if (host_impl->pending_tree()) {
On 2015/05/14 21:40:48, Tobias Sargeant wrote:
> On 2015/05/14 14:40:21, boliu wrote:
> > Is this check the same as "host_impl->settings().impl_side_painting". Prefer
> to
> > use that, and assert the pending tree is there if impl side painting is on.
> 
> No, it's not the same.
> 
> If you assert that pending_tree is not null, the following tests fail:
> 
> 8 tests failed:
>    

You should DCHECK pending_tree() only if impl_side_painting is true. So only the
*_ImplSidePaint failures are relevant. I'm not sure why those ones are failing
though. Can you look into it?

I guess LayerTreeHostImpl::ActivateSyncTree only checks for pending_tree()
instead of impl_side_painting, so maybe it's good enough.

https://codereview.chromium.org/1126963006/diff/320001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/320001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:4974: reason ==
DidNotSwapReason::ACTIVATION_FAILS);
Actually on second thought, this is not strict enough. The only reason allowed
after DidActivate is SWAP_FAIL, this would not check for COMMIT_FAIL/NO_UPDATE.

https://codereview.chromium.org/1126963006/diff/320001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:5137:
host_impl->active_tree()->source_frame_number() == 0);
SwapPromise is only queued in frame 1, but this is inserted in frame 0 then
removed in frame 1? Sketchy even if it actually works. Do this insert in
WillActivateTreeOnThread instead?

[Tobias Sargeant, 2015-05-15 16:30:49]

https://codereview.chromium.org/1126963006/diff/320001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1126963006/diff/320001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:4974: reason ==
DidNotSwapReason::ACTIVATION_FAILS);
On 2015/05/15 03:50:50, boliu wrote:
> Actually on second thought, this is not strict enough. The only reason allowed
> after DidActivate is SWAP_FAIL, this would not check for
COMMIT_FAIL/NO_UPDATE.

Done.

https://codereview.chromium.org/1126963006/diff/320001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:5137:
host_impl->active_tree()->source_frame_number() == 0);
On 2015/05/15 03:50:50, boliu wrote:
> SwapPromise is only queued in frame 1, but this is inserted in frame 0 then
> removed in frame 1? Sketchy even if it actually works. Do this insert in
> WillActivateTreeOnThread instead?

Done.

[boliu, 2015-05-15 16:43:07]

lgtm % last comment

https://codereview.chromium.org/1126963006/diff/340001/content/renderer/gpu/f...
File content/renderer/gpu/frame_swap_message_queue.h (right):

https://codereview.chromium.org/1126963006/diff/340001/content/renderer/gpu/f...
content/renderer/gpu/frame_swap_message_queue.h:61: // activation can be
obtained by calling DrainMessages.
s/activation/swap/

[Tobias Sargeant, 2015-05-15 17:05:08]

https://codereview.chromium.org/1126963006/diff/340001/content/renderer/gpu/f...
File content/renderer/gpu/frame_swap_message_queue.h (right):

https://codereview.chromium.org/1126963006/diff/340001/content/renderer/gpu/f...
content/renderer/gpu/frame_swap_message_queue.h:61: // activation can be
obtained by calling DrainMessages.
On 2015/05/15 16:43:07, boliu wrote:
> s/activation/swap/

Done.

[Tobias Sargeant, 2015-05-15 17:05:15]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2015-05-15 17:05:16]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1126963006/#ps360001
(title: "amend comments")

[commit-bot: I haz the power, 2015-05-15 17:05:30]

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

[commit-bot: I haz the power, 2015-05-15 17:53:02]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-05-15 17:54:29]

Patchset 19 (id:??) landed as
https://crrev.com/93d4648413ae2c80388e667dc37257308fa9e889
Cr-Commit-Position: refs/heads/master@{#330128}