cc: Complete swap promise for aborted commit after pending activation.

cc: Complete swap promise for aborted commit after pending activation.

Swap promises are used by android webview (via the visual state message
in FrameSwapMessageQueue) to wait until all changes made on the main
thread are ready to be drawn. This happens in two ways either we go
through the commit and activate it or if there is aborted commit because
there are no updates in the commit. In either case we assume that the
compositor is ready to draw all changes.

With main frame before activation we have two frames in flight where one
frame is being activated and the next one is sent to the main thread. In
case of aborted commits the swap promise for a frame might be completed
before the previous frame is activated. This means that the assumption
above re: being ready to draw is invalid.

This CL allows passing swap promises from the main thread to the impl
thread on aborted commits. If activation is pending on the impl thread
the swap promises are queued up to be completed upon activation,
otherwise the promises are completed immediately. For SingleThreadProxy
and RemoteChannel we don't have multiple frames in flight so we complete
the promises on the main thread as before.

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

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

[sunnyps, 2016-07-28 01:50:47]

Description was changed from

==========
cc: Complete swap promise for aborted commit after pending activation.

Swap promises are used by android webview (via the visual state message
in FrameSwapMessageQueue) to wait until all changes made on the main
thread are ready to be drawn. This happens in two ways either we go
through the commit and activate it or if there is aborted commit because
there are no updates in the commit. In either case we assume that the
compositor is ready to draw all changes.

With main frame before activation we have two frames in flight where one
frame is being activated and the next one is sent to the main thread. In
case of aborted commits the swap promise for a frame might be completed
before the previous frame is activated. This means that the assumption
above re: being ready to draw is invalid.

This CL allows passing swap promises from the main thread to the impl
thread on aborted commits. If activation is pending on the impl thread
the swap promises are queued up to be completed upon activation,
otherwise the promises are completed immediately. For SingleThreadProxy
and RemoteChannel we don't have multiple frames in flight so we complete
the promises on the main thread as before.

BUG=612596
==========

to

==========
cc: Complete swap promise for aborted commit after pending activation.

Swap promises are used by android webview (via the visual state message
in FrameSwapMessageQueue) to wait until all changes made on the main
thread are ready to be drawn. This happens in two ways either we go
through the commit and activate it or if there is aborted commit because
there are no updates in the commit. In either case we assume that the
compositor is ready to draw all changes.

With main frame before activation we have two frames in flight where one
frame is being activated and the next one is sent to the main thread. In
case of aborted commits the swap promise for a frame might be completed
before the previous frame is activated. This means that the assumption
above re: being ready to draw is invalid.

This CL allows passing swap promises from the main thread to the impl
thread on aborted commits. If activation is pending on the impl thread
the swap promises are queued up to be completed upon activation,
otherwise the promises are completed immediately. For SingleThreadProxy
and RemoteChannel we don't have multiple frames in flight so we complete
the promises on the main thread as before.

BUG=612596
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[sunnyps, 2016-07-28 01:50:54]

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

[commit-bot: I haz the power, 2016-07-28 01:51:05]

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-07-28 01:51:06]

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

[commit-bot: I haz the power, 2016-07-28 01:51:07]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[sunnyps, 2016-07-28 02:01:49]

Description was changed from

==========
cc: Complete swap promise for aborted commit after pending activation.

Swap promises are used by android webview (via the visual state message
in FrameSwapMessageQueue) to wait until all changes made on the main
thread are ready to be drawn. This happens in two ways either we go
through the commit and activate it or if there is aborted commit because
there are no updates in the commit. In either case we assume that the
compositor is ready to draw all changes.

With main frame before activation we have two frames in flight where one
frame is being activated and the next one is sent to the main thread. In
case of aborted commits the swap promise for a frame might be completed
before the previous frame is activated. This means that the assumption
above re: being ready to draw is invalid.

This CL allows passing swap promises from the main thread to the impl
thread on aborted commits. If activation is pending on the impl thread
the swap promises are queued up to be completed upon activation,
otherwise the promises are completed immediately. For SingleThreadProxy
and RemoteChannel we don't have multiple frames in flight so we complete
the promises on the main thread as before.

BUG=612596
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
cc: Complete swap promise for aborted commit after pending activation.

Swap promises are used by android webview (via the visual state message
in FrameSwapMessageQueue) to wait until all changes made on the main
thread are ready to be drawn. This happens in two ways either we go
through the commit and activate it or if there is aborted commit because
there are no updates in the commit. In either case we assume that the
compositor is ready to draw all changes.

With main frame before activation we have two frames in flight where one
frame is being activated and the next one is sent to the main thread. In
case of aborted commits the swap promise for a frame might be completed
before the previous frame is activated. This means that the assumption
above re: being ready to draw is invalid.

This CL allows passing swap promises from the main thread to the impl
thread on aborted commits. If activation is pending on the impl thread
the swap promises are queued up to be completed upon activation,
otherwise the promises are completed immediately. For SingleThreadProxy
and RemoteChannel we don't have multiple frames in flight so we complete
the promises on the main thread as before.

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

[sunnyps, 2016-07-28 02:02:20]

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

[commit-bot: I haz the power, 2016-07-28 02:02:41]

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

[sunnyps, 2016-07-28 02:06:06]

sunnyps@chromium.org changed reviewers:
+ boliu@chromium.org, brianderson@chromium.org, enne@chromium.org

[sunnyps, 2016-07-28 02:06:07]

PTAL

This allows the android webview visual state tests to pass - running for a few
hours on Bo's special nexus 5 with retries disabled.

This CL enabled MFBA by default so see the try jobs for android webview there:
https://codereview.chromium.org/2184243003/

[commit-bot: I haz the power, 2016-07-28 02:09:42]

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

[commit-bot: I haz the power, 2016-07-28 02:09:43]

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/...)

[sunnyps, 2016-07-28 02:22:43]

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

[commit-bot: I haz the power, 2016-07-28 02:23:07]

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-07-28 03:30:22]

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

[commit-bot: I haz the power, 2016-07-28 03:30:23]

Dry run: This issue passed the CQ dry run.

[brianderson, 2016-07-28 18:10:53]

lgtm w/ nits.

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1498: new_swap_promises.clear();
Is this call to clear needed?

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:361: // Take the |new_swap_promise| and append it to
|swap_promise_list_|.
Can you update this comment or remove it and  rename PassSwapPromises to
something like PassSwapPromisesAndFulfillPrevious?

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:745: // synchronous commit.
Should this go inside the dcheck and be indented?

[enne (OOO), 2016-07-28 18:18:03]

Can you add unit tests? We have plenty of other swap promise-related unit tests,
so it'd be good to make sure this edge case is captured there.

[boliu, 2016-07-28 18:38:42]

Two high level things to point out (assuming I read the code correctly..)

DidNotSwap(SwapPromise::COMMIT_NO_UPDATE) moved from main thread to impl thread.
Visual state implementation handles that correctly, but I don't know all other
uses cases of swap promises. Plus this effectively delays COMMIT_NO_UPDATE a bit
as well, which may matter for things like latency tracking (assuming it actually
tracks COMMIT_NO_UPDATE)

If a swap promise does get appended to the pending tree, then I don't think it
ever sees the "OnCommit" callback, it will only get DidActivate and DidSwap?
That's also inconsistent.

[sunnyps, 2016-07-28 21:25:02]

On 2016/07/28 18:38:42, boliu wrote:
> Two high level things to point out (assuming I read the code correctly..)
> 
> DidNotSwap(SwapPromise::COMMIT_NO_UPDATE) moved from main thread to impl
thread.
> Visual state implementation handles that correctly, but I don't know all other
> uses cases of swap promises. Plus this effectively delays COMMIT_NO_UPDATE a
bit
> as well, which may matter for things like latency tracking (assuming it
actually
> tracks COMMIT_NO_UPDATE)

Yeah, it moved to the impl thread but it becomes DidActivate.

> 
> If a swap promise does get appended to the pending tree, then I don't think it
> ever sees the "OnCommit" callback, it will only get DidActivate and DidSwap?
> That's also inconsistent.

Almost all notification sequences in SwapPromise are inconsistent unfortunately.
I've been thinking of simplifying SwapPromise (removing OnCommit, DidActivate)
and making a new VisualStatePromise that only has a single notification and
behaves like SwapPromise::DidActivate (i.e. it passes from main thread to impl
thread). WDYT?

[boliu, 2016-07-28 21:29:06]

On 2016/07/28 21:25:02, sunnyps wrote:
> On 2016/07/28 18:38:42, boliu wrote:
> > Two high level things to point out (assuming I read the code correctly..)
> > 
> > DidNotSwap(SwapPromise::COMMIT_NO_UPDATE) moved from main thread to impl
> thread.
> > Visual state implementation handles that correctly, but I don't know all
other
> > uses cases of swap promises. Plus this effectively delays COMMIT_NO_UPDATE a
> bit
> > as well, which may matter for things like latency tracking (assuming it
> actually
> > tracks COMMIT_NO_UPDATE)
> 
> Yeah, it moved to the impl thread but it becomes DidActivate.
> 
> > 
> > If a swap promise does get appended to the pending tree, then I don't think
it
> > ever sees the "OnCommit" callback, it will only get DidActivate and DidSwap?
> > That's also inconsistent.
> 
> Almost all notification sequences in SwapPromise are inconsistent
unfortunately.
> I've been thinking of simplifying SwapPromise (removing OnCommit, DidActivate)
> and making a new VisualStatePromise that only has a single notification and
> behaves like SwapPromise::DidActivate (i.e. it passes from main thread to impl
> thread). WDYT?

I guess this depends on what else uses swap promises today and what they need.
Of course that's totally fine for visual state callback.

My original point was that each swap promise used to go through commit,
activate, swap in that sequence (or be interrupted by DidNotSwap). And that's
not the case anymore.

[sunnyps, 2016-08-02 02:55:09]

Added a test. PTAL

boliu@ I believe I addressed your concerns when we discussed this.

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1498: new_swap_promises.clear();
On 2016/07/28 18:10:53, brianderson wrote:
> Is this call to clear needed?

I believe so because we're moving the individual unique_ptrs and the vector
contains empty unique_ptrs after that's done.

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:361: // Take the |new_swap_promise| and append it to
|swap_promise_list_|.
On 2016/07/28 18:10:53, brianderson wrote:
> Can you update this comment or remove it and  rename PassSwapPromises to
> something like PassSwapPromisesAndFulfillPrevious?

Done. Updated comment.

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2188093002/diff/40001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:745: // synchronous commit.
On 2016/07/28 18:10:53, brianderson wrote:
> Should this go inside the dcheck and be indented?

Done.

[enne (OOO), 2016-08-04 17:41:10]

lgtm

[sunnyps, 2016-08-05 20:54:18]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2016-08-05 20:54:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2188093002/#ps100001
(title: "brianderson's review")

[commit-bot: I haz the power, 2016-08-05 20:54:50]

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-08-05 20:57:01]

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

[commit-bot: I haz the power, 2016-08-05 20:57:02]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sunnyps, 2016-08-09 01:58:27]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2016-08-09 01:58:28]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2188093002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-09 01:58:47]

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-08-09 02:09:20]

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

[commit-bot: I haz the power, 2016-08-09 02:09:22]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunnyps, 2016-08-09 03:20:13]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2016-08-09 03:20:14]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2188093002/#ps140001
(title: "post rebase fix")

[commit-bot: I haz the power, 2016-08-09 03:20:39]

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-08-09 04:58:09]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-09 05:00:04]

Description was changed from

==========
cc: Complete swap promise for aborted commit after pending activation.

Swap promises are used by android webview (via the visual state message
in FrameSwapMessageQueue) to wait until all changes made on the main
thread are ready to be drawn. This happens in two ways either we go
through the commit and activate it or if there is aborted commit because
there are no updates in the commit. In either case we assume that the
compositor is ready to draw all changes.

With main frame before activation we have two frames in flight where one
frame is being activated and the next one is sent to the main thread. In
case of aborted commits the swap promise for a frame might be completed
before the previous frame is activated. This means that the assumption
above re: being ready to draw is invalid.

This CL allows passing swap promises from the main thread to the impl
thread on aborted commits. If activation is pending on the impl thread
the swap promises are queued up to be completed upon activation,
otherwise the promises are completed immediately. For SingleThreadProxy
and RemoteChannel we don't have multiple frames in flight so we complete
the promises on the main thread as before.

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

to

==========
cc: Complete swap promise for aborted commit after pending activation.

Swap promises are used by android webview (via the visual state message
in FrameSwapMessageQueue) to wait until all changes made on the main
thread are ready to be drawn. This happens in two ways either we go
through the commit and activate it or if there is aborted commit because
there are no updates in the commit. In either case we assume that the
compositor is ready to draw all changes.

With main frame before activation we have two frames in flight where one
frame is being activated and the next one is sent to the main thread. In
case of aborted commits the swap promise for a frame might be completed
before the previous frame is activated. This means that the assumption
above re: being ready to draw is invalid.

This CL allows passing swap promises from the main thread to the impl
thread on aborted commits. If activation is pending on the impl thread
the swap promises are queued up to be completed upon activation,
otherwise the promises are completed immediately. For SingleThreadProxy
and RemoteChannel we don't have multiple frames in flight so we complete
the promises on the main thread as before.

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

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

[commit-bot: I haz the power, 2016-08-09 05:00:06]

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