blimp: Use SwapPromises for readback API.

blimp: Use SwapPromises for readback API.

The tests try to grab a snapshot of the compositor output right after
receiving an update from the engine, and so want to make sure that the
CompositorFrame on which the request is made is from the source frame
that came from the engine.

This is done by reading the commit messages before giving them to
the compositor, and keeping a track of frames drawn for commits made
to ensure that a request is made after the drawn frame reaches the
main thread.

The tracking logic is messy, and requires unnecessary commit tracking,
which especially gets confusing with the mode that uses a threaded
compositor on the client, since each commit/draw no longer maps to
an update from the engine.

This patch replaces that logic with SwapPromises, which is the API
meant to be used for tracking the progress of a frame through the
cc pipeline. The existing logic is still retained for the old code
path, since using SwapPromises requires local commits, which can't
be done in that mode.

BUG=648442
Committed: https://crrev.com/2be75f5c7053765d71b23424d94501194c9e6f81
Cr-Commit-Position: refs/heads/master@{#429230}

[Khushal, 2016-10-28 07:42:48]

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

[Khushal, 2016-10-28 07:48:28]

On 2016/10/28 07:42:49, Khushal wrote:

Thanks for taking a look at this! :)

[Khushal, 2016-10-31 21:51:02]

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

[Khushal, 2016-10-31 21:51:02]

+dtrainor also.

[Khushal, 2016-10-31 23:21:34]

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

[commit-bot: I haz the power, 2016-10-31 23:22:03]

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-10-31 23:53:17]

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

[commit-bot: I haz the power, 2016-10-31 23:53:18]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-11-02 07:02:10]

lgtm modulo nits

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.cc:231: if
(use_threaded_layer_tree_host_)
For my edification, what is !use_threaded_layer_tree_host_ mode used for?

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.h:115: class
FrameTrackingSwapPromise;
There's no need to put this in the header.  Instead I suggest using namespace {
class FrameTrackingSwapPromise {...}; }

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.h:240: // When
NotifyWhenDonePendingCommitsis called |outstanding_commits_| is copied
This comment needs to be updated.

[Khushal, 2016-11-02 07:24:21]

Thanks a lot Alex.

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.cc:231: if
(use_threaded_layer_tree_host_)
On 2016/11/02 07:02:10, aelias wrote:
> For my edification, what is !use_threaded_layer_tree_host_ mode used for?

Right now our split point in the pipeline is right in the middle of the thread
boundary. cc::RemoteChannelImpl has the details on how it works but basically we
throw in a network pipe to ship messages across the network instead of posting
tasks across threads. The !use_threaded_layer_tree_host_ is that mode.

The use_threaded_layer_tree_host_ mode instead makes blimp an embedder of the
threaded compositor on the client, so the split is exactly between blink and cc.
This is all just staging code for the transition while we remove the former code
path.

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.h:115: class
FrameTrackingSwapPromise;
On 2016/11/02 07:02:10, aelias wrote:
> There's no need to put this in the header.  Instead I suggest using namespace
{
> class FrameTrackingSwapPromise {...}; }

I'm actually calling a private method from that class,
BlimpCompositor::MakeCopyRequestOnNextSwap, which is why declared it as a nested
class. And I had a slight preference to keep it private because it really is an
internal detail.

[Khushal, 2016-11-02 07:31:32]

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2459823003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.h:240: // When
NotifyWhenDonePendingCommitsis called |outstanding_commits_| is copied
On 2016/11/02 07:02:10, aelias wrote:
> This comment needs to be updated.

Done.

[Khushal, 2016-11-02 07:32:34]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-11-02 07:32:34]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2459823003/#ps20001
(title: "comment updated")

[commit-bot: I haz the power, 2016-11-02 07:32:51]

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-11-02 07:45:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-02 07:47:24]

Description was changed from

==========
blimp: Use SwapPromises for readback API.

The tests try to grab a snapshot of the compositor output right after
receiving an update from the engine, and so want to make sure that the
CompositorFrame on which the request is made is from the source frame
that came from the engine.

This is done by reading the commit messages before giving them to
the compositor, and keeping a track of frames drawn for commits made
to ensure that a request is made after the drawn frame reaches the
main thread.

The tracking logic is messy, and requires unnecessary commit tracking,
which especially gets confusing with the mode that uses a threaded
compositor on the client, since each commit/draw no longer maps to
an update from the engine.

This patch replaces that logic with SwapPromises, which is the API
meant to be used for tracking the progress of a frame through the
cc pipeline. The existing logic is still retained for the old code
path, since using SwapPromises requires local commits, which can't
be done in that mode.

BUG=648442
==========

to

==========
blimp: Use SwapPromises for readback API.

The tests try to grab a snapshot of the compositor output right after
receiving an update from the engine, and so want to make sure that the
CompositorFrame on which the request is made is from the source frame
that came from the engine.

This is done by reading the commit messages before giving them to
the compositor, and keeping a track of frames drawn for commits made
to ensure that a request is made after the drawn frame reaches the
main thread.

The tracking logic is messy, and requires unnecessary commit tracking,
which especially gets confusing with the mode that uses a threaded
compositor on the client, since each commit/draw no longer maps to
an update from the engine.

This patch replaces that logic with SwapPromises, which is the API
meant to be used for tracking the progress of a frame through the
cc pipeline. The existing logic is still retained for the old code
path, since using SwapPromises requires local commits, which can't
be done in that mode.

BUG=648442

Committed: https://crrev.com/2be75f5c7053765d71b23424d94501194c9e6f81
Cr-Commit-Position: refs/heads/master@{#429230}
==========

[commit-bot: I haz the power, 2016-11-02 07:47:25]

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