Switching to base::Optional<CompositorFrame> in Android's synchronous compositor IPC messages

Switching to base::Optional<CompositorFrame> in Android's synchronous compositor IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
an empty base::Optional<CompositorFrame> when a swap doesn't happen.

BUG=653741
Committed: https://crrev.com/9641873cb887a3e1179162eef6463f000355f9b1
Cr-Commit-Position: refs/heads/master@{#428142}

[Saman Sami, 2016-10-27 18:03:21]

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

[Saman Sami, 2016-10-27 18:03:22]

I pulled out the synchronous compositor changes for easier reviewing.

[Saman Sami, 2016-10-27 18:03:29]

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

[commit-bot: I haz the power, 2016-10-27 18:04:14]

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-27 18:06:00]

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

[commit-bot: I haz the power, 2016-10-27 18:06:01]

Dry run: Try jobs failed on following builders:
  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...)
  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...)

[Saman Sami, 2016-10-27 18:10:16]

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

[commit-bot: I haz the power, 2016-10-27 18:11:09]

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

[boliu, 2016-10-27 18:11:47]

I think the Optional suggestion makes the other comments moot

https://codereview.chromium.org/2458743002/diff/1/content/browser/android/syn...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/2458743002/diff/1/content/browser/android/syn...
content/browser/android/synchronous_compositor_host.cc:126: bool has_swapped;
initialize to false

https://codereview.chromium.org/2458743002/diff/1/content/browser/android/syn...
content/browser/android/synchronous_compositor_host.cc:136: return
ProcessHardwareFrame(compositor_frame_sink_id,
just return empty frame if false, rather than passing it into another function

https://codereview.chromium.org/2458743002/diff/1/content/common/android/sync...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/2458743002/diff/1/content/common/android/sync...
content/common/android/sync_compositor_messages.h:134: cc::CompositorFrame,
might be easier to replace this with base::Optional<cc::CompositorFrame> ?

Looks like there's a ParamTraits for base::Optional already, so it should just
work?

[Saman Sami, 2016-10-27 18:44:01]

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

[commit-bot: I haz the power, 2016-10-27 18:44:38]

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-27 19:19:30]

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

[commit-bot: I haz the power, 2016-10-27 19:19:32]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-10-27 19:22:15]

PTAL
I'm now using base::Optional instead of bool. The code is much cleaner now. I
wasn't aware that it can be passed though IPC.

[Saman Sami, 2016-10-27 19:48:08]

Description was changed from

==========
Added has_swapped to Android's synchronous compositor IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. There's now a boolean
value passed alongside the compositor frame to indicate this.


BUG=653741
==========

to

==========
Added has_swapped to Android's synchronous compositor IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass a
base::Optional<CompositorFrame> to take care of the null situation.


BUG=653741
==========

[Saman Sami, 2016-10-27 19:48:20]

Description was changed from

==========
Added has_swapped to Android's synchronous compositor IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass a
base::Optional<CompositorFrame> to take care of the null situation.


BUG=653741
==========

to

==========
Added has_swapped to Android's synchronous compositor IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
a base::Optional<CompositorFrame> to take care of the null situation.


BUG=653741
==========

[Saman Sami, 2016-10-27 19:49:29]

Description was changed from

==========
Added has_swapped to Android's synchronous compositor IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
a base::Optional<CompositorFrame> to take care of the null situation.


BUG=653741
==========

to

==========
Switching to base::Optional<CompositorFrame> in Android's synchronous compositor
IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
a base::Optional<CompositorFrame> to take care of the null situation.


BUG=653741
==========

[Saman Sami, 2016-10-27 19:50:33]

Description was changed from

==========
Switching to base::Optional<CompositorFrame> in Android's synchronous compositor
IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
a base::Optional<CompositorFrame> to take care of the null situation.


BUG=653741
==========

to

==========
Switching to base::Optional<CompositorFrame> in Android's synchronous compositor
IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
an empty base::Optional<CompositorFrame> when a swap didn't happen.


BUG=653741
==========

[Saman Sami, 2016-10-27 19:51:17]

Description was changed from

==========
Switching to base::Optional<CompositorFrame> in Android's synchronous compositor
IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
an empty base::Optional<CompositorFrame> when a swap didn't happen.


BUG=653741
==========

to

==========
Switching to base::Optional<CompositorFrame> in Android's synchronous compositor
IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
an empty base::Optional<CompositorFrame> when a swap doesn't happen.


BUG=653741
==========

[boliu, 2016-10-27 19:52:16]

https://codereview.chromium.org/2458743002/diff/20001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/2458743002/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:130: !compositor_frame) {
not quite the same. ProcessHardwareFrame can be skipped, but ProcessCommonParams
shouldn't be dropped if the frame is empty

https://codereview.chromium.org/2458743002/diff/20001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/2458743002/diff/20001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:191:
SendDemandDrawHwReplyAsync(base::Optional<cc::CompositorFrame>(), 0u);
would nullptr work here?

[Saman Sami, 2016-10-27 20:12:49]

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

[commit-bot: I haz the power, 2016-10-27 20:13:10]

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-10-27 20:19:00]

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

[commit-bot: I haz the power, 2016-10-27 20:19:41]

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-10-27 20:20:15]

PTAL
I applied your suggestions.

[boliu, 2016-10-27 20:23:07]

lgtm % style

I recommend dcheng for ipc review

https://codereview.chromium.org/2458743002/diff/60001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/2458743002/diff/60001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:135: if
(compositor_frame)
style: need braces on both blocks if and else block

(only exception is if the conditional and body are single line, which is not the
case here)

[Saman Sami, 2016-10-27 20:29:11]

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

[Saman Sami, 2016-10-27 20:43:56]

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

[commit-bot: I haz the power, 2016-10-27 20:44:51]

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

[dcheng, 2016-10-27 21:12:43]

ipc lgtm

[Saman Sami, 2016-10-27 21:16:12]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2016-10-27 21:16:12]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2458743002/#ps80001
(title: "Style")

[commit-bot: I haz the power, 2016-10-27 21:17:27]

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

[Saman Sami, 2016-10-27 21:17:59]

The CQ bit was checked by samans@chromium.org

[commit-bot: I haz the power, 2016-10-27 21:19:18]

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-27 21:25:09]

Description was changed from

==========
Switching to base::Optional<CompositorFrame> in Android's synchronous compositor
IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
an empty base::Optional<CompositorFrame> when a swap doesn't happen.


BUG=653741
==========

to

==========
Switching to base::Optional<CompositorFrame> in Android's synchronous compositor
IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
an empty base::Optional<CompositorFrame> when a swap doesn't happen.


BUG=653741
==========

[commit-bot: I haz the power, 2016-10-27 21:25:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-27 21:28:33]

Description was changed from

==========
Switching to base::Optional<CompositorFrame> in Android's synchronous compositor
IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
an empty base::Optional<CompositorFrame> when a swap doesn't happen.


BUG=653741
==========

to

==========
Switching to base::Optional<CompositorFrame> in Android's synchronous compositor
IPC messages

In an effort to eliminate DelegatedFrameData, we need to get rid of the
dependecy on CompositorFrame::delegated_frame_data for figuring out
whether a frame swap has been performed or not. We now pass
an empty base::Optional<CompositorFrame> when a swap doesn't happen.

BUG=653741

Committed: https://crrev.com/9641873cb887a3e1179162eef6463f000355f9b1
Cr-Commit-Position: refs/heads/master@{#428142}
==========

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

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