Split cc::mojom::DisplayCompositor::CreateCompositorFrameSink() into CreateDisplayCompositorFrameSi…

Split cc::mojom::DisplayCompositor::CreateCompositorFrameSink() into CreateDisplayCompositorFrameSink() and CreateOffscreenCompositorFrameSink()

This is step one towards having a cc::Display mojo interface.

After this, a mojo interface for cc::Display will be added and
CreateDisplayCompositorFrameSink will take the interface as a parameter.

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

Committed: https://crrev.com/2bed325a1f295c76f23182a8824847dd8fd60c4b
Cr-Commit-Position: refs/heads/master@{#438839}

[Alex Z., 2016-12-14 16:58:42]

Description was changed from

==========
Split cc::mojom::DisplayCompositor::CreateCompositorFrameSink() into
CreateDisplayCompositorFrameSink() and CreateOffscreenCompositorFrameSink()

This is step one towards having a cc::Display mojo interface.

After this, a mojo interface for cc::Display will be added and
CreateDisplayCompositorFrameSink will take the interface as a parameter.

BUG=670364
==========

to

==========
Split cc::mojom::DisplayCompositor::CreateCompositorFrameSink() into
CreateDisplayCompositorFrameSink() and CreateOffscreenCompositorFrameSink()

This is step one towards having a cc::Display mojo interface.

After this, a mojo interface for cc::Display will be added and
CreateDisplayCompositorFrameSink will take the interface as a parameter.

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

[Alex Z., 2016-12-14 16:58:49]

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

[commit-bot: I haz the power, 2016-12-14 16:59:25]

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

[Alex Z., 2016-12-14 17:02:28]

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

[Alex Z., 2016-12-14 17:02:29]

Please review my changes.

[Fady Samuel, 2016-12-14 17:19:02]

This is pretty good. Just a few comments.

https://codereview.chromium.org/2576723002/diff/20001/services/ui/surfaces/di...
File services/ui/surfaces/display_compositor.cc (left):

https://codereview.chromium.org/2576723002/diff/20001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:101:
DCHECK(thread_checker_.CalledOnValidThread());
I would leave this here too because we're doing work prior to going to
CreateCompositorFrameSinkInternal.

https://codereview.chromium.org/2576723002/diff/20001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right):

https://codereview.chromium.org/2576723002/diff/20001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink_manager.cc:39:
mojom::CompositorFrameSinkType compositor_frame_sink_type,
This will always be DEFAULT. I'd drop this parameter.

https://codereview.chromium.org/2576723002/diff/20001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink_manager.cc:150: // or
fix alignment.

[Alex Z., 2016-12-14 18:44:12]

https://codereview.chromium.org/2576723002/diff/20001/services/ui/surfaces/di...
File services/ui/surfaces/display_compositor.cc (left):

https://codereview.chromium.org/2576723002/diff/20001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:101:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/12/14 17:19:01, Fady Samuel wrote:
> I would leave this here too because we're doing work prior to going to
> CreateCompositorFrameSinkInternal.

Done.

https://codereview.chromium.org/2576723002/diff/20001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right):

https://codereview.chromium.org/2576723002/diff/20001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink_manager.cc:39:
mojom::CompositorFrameSinkType compositor_frame_sink_type,
On 2016/12/14 17:19:01, Fady Samuel wrote:
> This will always be DEFAULT. I'd drop this parameter.

Done.

https://codereview.chromium.org/2576723002/diff/20001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink_manager.cc:150: // or
On 2016/12/14 17:19:01, Fady Samuel wrote:
> fix alignment.

Done.

[Fady Samuel, 2016-12-14 18:48:43]

lgtm

[Alex Z., 2016-12-14 18:55:38]

staraz@chromium.org changed reviewers:
+ msw@chromium.org, tsepez@chromium.org

[Alex Z., 2016-12-14 18:55:39]

msw@chromium.org: Please review changes in frame_generator.cc, server_window*
and window_tree.cc

tsepez@: Please review changes in ipc files.

[Alex Z., 2016-12-14 18:56:28]

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

[commit-bot: I haz the power, 2016-12-14 18:57:15]

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

[Tom Sepez, 2016-12-14 19:25:08]

mojom LGTM

[msw, 2016-12-14 20:07:44]

I don't know this code well, but I can rubber stamp, given Fady's review. I just
have one question.

https://codereview.chromium.org/2576723002/diff/60001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right):

https://codereview.chromium.org/2576723002/diff/60001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink_manager.cc:148: if (widget !=
gfx::kNullAcceleratedWidget) {
Why does this conditional help? It looks like it could just always supply
|widget| regardless of whether the value is gfx::kNullAcceleratedWidget or not,
right?

[Fady Samuel, 2016-12-14 20:17:33]

https://codereview.chromium.org/2576723002/diff/60001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right):

https://codereview.chromium.org/2576723002/diff/60001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink_manager.cc:148: if (widget !=
gfx::kNullAcceleratedWidget) {
On 2016/12/14 20:07:44, msw wrote:
> Why does this conditional help? It looks like it could just always supply
> |widget| regardless of whether the value is gfx::kNullAcceleratedWidget or
not,
> right?

Ohh I missed this. Thanks Mike We should be calling
CreateOffscreenCompositorFrameSink if widget is gfx::kNullAcceleratedWidget

[Alex Z., 2016-12-14 20:20:02]

https://codereview.chromium.org/2576723002/diff/60001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right):

https://codereview.chromium.org/2576723002/diff/60001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink_manager.cc:148: if (widget !=
gfx::kNullAcceleratedWidget) {
On 2016/12/14 20:07:44, msw wrote:
> Why does this conditional help? It looks like it could just always supply
> |widget| regardless of whether the value is gfx::kNullAcceleratedWidget or
not,
> right?

The widget vs gfx::kNullAcceleratedWidget is the only difference, for now.

In the short term future, CreateDisplayCompositorFrameSink() would also take a
DisplayPrivate mojo interface pointer while CreateOffscreenCompositorFrameSink()
remains the same.

[Alex Z., 2016-12-14 20:26:21]

https://codereview.chromium.org/2576723002/diff/60001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right):

https://codereview.chromium.org/2576723002/diff/60001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink_manager.cc:148: if (widget !=
gfx::kNullAcceleratedWidget) {
On 2016/12/14 20:07:44, msw wrote:
> Why does this conditional help? It looks like it could just always supply
> |widget| regardless of whether the value is gfx::kNullAcceleratedWidget or
not,
> right?

Done.

[msw, 2016-12-14 20:28:20]

rubber stamp lgtm

[Alex Z., 2016-12-14 20:28:55]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2016-12-14 20:28:56]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2576723002/#ps80001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-12-14 20:30:06]

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:09:05]

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

[commit-bot: I haz the power, 2016-12-14 22:09:06]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Alex Z., 2016-12-14 23:09:59]

The CQ bit was checked by staraz@chromium.org

[commit-bot: I haz the power, 2016-12-14 23:10: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-12-15 00:04:58]

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

[commit-bot: I haz the power, 2016-12-15 00:04:59]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Fady Samuel, 2016-12-15 14:43:44]

https://codereview.chromium.org/2576723002/diff/80001/services/ui/surfaces/di...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2576723002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:108: CreateDisplay(frame_sink_id,
surface_handle, begin_frame_source.get()),
store this in a local variable above to make sure this param is evaluated first.
The problem is Win64 build is evaluating std::move(begin_frame_source) before it
evaluates this statement and so begin_frame_source.get() is nullptr.

[Alex Z., 2016-12-15 15:17:52]

https://codereview.chromium.org/2576723002/diff/80001/services/ui/surfaces/di...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2576723002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:108: CreateDisplay(frame_sink_id,
surface_handle, begin_frame_source.get()),
On 2016/12/15 14:43:44, Fady Samuel wrote:
> store this in a local variable above to make sure this param is evaluated
first.
> The problem is Win64 build is evaluating std::move(begin_frame_source) before
it
> evaluates this statement and so begin_frame_source.get() is nullptr.

Done.

[Alex Z., 2016-12-15 15:19:18]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2016-12-15 15:19:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
fsamuel@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2576723002/#ps100001
(title: "In DisplayCompositor::CreateDisplayCompositorFrameSink, let a local
variable hold the result of Cre…")

[commit-bot: I haz the power, 2016-12-15 15:19:30]

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-15 16:10:46]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481815158908230,
"parent_rev": "c71d7b550bcc04c178599394eca918a00379ffb5", "commit_rev":
"50b56164482f50fe49b5c7f6c6ebe5a4110201ee"}

[commit-bot: I haz the power, 2016-12-15 16:11:13]

Description was changed from

==========
Split cc::mojom::DisplayCompositor::CreateCompositorFrameSink() into
CreateDisplayCompositorFrameSink() and CreateOffscreenCompositorFrameSink()

This is step one towards having a cc::Display mojo interface.

After this, a mojo interface for cc::Display will be added and
CreateDisplayCompositorFrameSink will take the interface as a parameter.

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

to

==========
Split cc::mojom::DisplayCompositor::CreateCompositorFrameSink() into
CreateDisplayCompositorFrameSink() and CreateOffscreenCompositorFrameSink()

This is step one towards having a cc::Display mojo interface.

After this, a mojo interface for cc::Display will be added and
CreateDisplayCompositorFrameSink will take the interface as a parameter.

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

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

[commit-bot: I haz the power, 2016-12-15 16:11:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-15 16:13:15]

Description was changed from

==========
Split cc::mojom::DisplayCompositor::CreateCompositorFrameSink() into
CreateDisplayCompositorFrameSink() and CreateOffscreenCompositorFrameSink()

This is step one towards having a cc::Display mojo interface.

After this, a mojo interface for cc::Display will be added and
CreateDisplayCompositorFrameSink will take the interface as a parameter.

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

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

to

==========
Split cc::mojom::DisplayCompositor::CreateCompositorFrameSink() into
CreateDisplayCompositorFrameSink() and CreateOffscreenCompositorFrameSink()

This is step one towards having a cc::Display mojo interface.

After this, a mojo interface for cc::Display will be added and
CreateDisplayCompositorFrameSink will take the interface as a parameter.

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

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

[commit-bot: I haz the power, 2016-12-15 16:13:16]

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