Only send the FrameSinkId to client when it is necessary

Only send the FrameSinkId to client when it is necessary

The ui::ClientCompositorFrameSink doesn't really need the FameSinkId
for submitting frames, so remove it.

And for TopLevel Window, Embeddee and DisplayRoot, we don't need the
FrameSinkId to create the ui::ClientCompositorFrameSink, so remove
the FrameSinkId arg from related mojo IPC.

This CL also passes enable_surface_synchronization flag to
ui::ClientCompositorFrameSink via ctor.

BUG=720600
Review-Url: https://codereview.chromium.org/2886873002
Cr-Commit-Position: refs/heads/master@{#472841}
Committed: https://chromium.googlesource.com/chromium/src/+/77a8c758cc4731844a0308f23ccd2fd0e8fe14d9

[Peng, 2017-05-16 17:55:54]

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

[Peng, 2017-05-16 17:55:54]

Hi Fady, PTAL. Thanks.

[Peng, 2017-05-16 18:01:58]

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

[commit-bot: I haz the power, 2017-05-16 18:04:42]

Dry run: CQ is trying da patch.

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

[Fady Samuel, 2017-05-16 18:13:47]

https://codereview.chromium.org/2886873002/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/client_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2886873002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/client_compositor_frame_sink.cc:87: if
(!enable_surface_synchronization_)
Why did you make this change?

https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.cc:271: frame_sink_id_ = frame_sink_id;
DCHECK(window_mus_type() == WindowMusType::TOP_LEVEL_IN_WM ||
       window_mus_type() == WinodwMusType::EMBED_IN_OWNER);

[Peng, 2017-05-16 18:44:55]

https://codereview.chromium.org/2886873002/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/client_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2886873002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/client_compositor_frame_sink.cc:87: if
(!enable_surface_synchronization_)
On 2017/05/16 18:13:47, Fady Samuel wrote:
> Why did you make this change?

The change is copied from CL 2875753002. It is useful for that CL. It only
updates last_submitted_frame_size_ when necessary.

It is better to have but it is not necessary in this CL.

https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.cc:271: frame_sink_id_ = frame_sink_id;
On 2017/05/16 18:13:47, Fady Samuel wrote:
> DCHECK(window_mus_type() == WindowMusType::TOP_LEVEL_IN_WM ||
>        window_mus_type() == WinodwMusType::EMBED_IN_OWNER);

This function could be called for embeddee and display root as well. So we can
not DCHECK it.

[Fady Samuel, 2017-05-16 20:20:24]

https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.cc:271: frame_sink_id_ = frame_sink_id;
On 2017/05/16 18:44:55, Peng wrote:
> On 2017/05/16 18:13:47, Fady Samuel wrote:
> > DCHECK(window_mus_type() == WindowMusType::TOP_LEVEL_IN_WM ||
> >        window_mus_type() == WinodwMusType::EMBED_IN_OWNER);
> 
> This function could be called for embeddee and display root as well. So we can
> not DCHECK it. 

Why would the embeddee do that?

[Peng, 2017-05-16 21:26:35]

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

[Peng, 2017-05-16 21:26:37]

https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.cc:271: frame_sink_id_ = frame_sink_id;
On 2017/05/16 20:20:23, Fady Samuel wrote:
> On 2017/05/16 18:44:55, Peng wrote:
> > On 2017/05/16 18:13:47, Fady Samuel wrote:
> > > DCHECK(window_mus_type() == WindowMusType::TOP_LEVEL_IN_WM ||
> > >        window_mus_type() == WinodwMusType::EMBED_IN_OWNER);
> > 
> > This function could be called for embeddee and display root as well. So we
can
> > not DCHECK it. 
> 
> Why would the embeddee do that?

Done.

[commit-bot: I haz the power, 2017-05-16 21:27:55]

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, 2017-05-16 21:58:35]

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

[commit-bot: I haz the power, 2017-05-16 21:58:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peng, 2017-05-17 02:04:22]

Description was changed from

==========
Remove frame sink id from ui::ClientCompositorFrameSink.

The ui::ClientCompositorFrameSink doesn't really need the FameSinkId
for submitting frames, so remove it.

This CL also passes enable_surface_synchronization flag to
ui::ClientCompositorFrameSink via ctor.

BUG=720600
==========

to

==========
Only send the FrameSinkId to client when it is necessary

The ui::ClientCompositorFrameSink doesn't really need the FameSinkId
for submitting frames, so remove it.

And for TopLevel Window, Embeddee and DisplayRoot, we don't need the
FrameSinkId to create the ui::ClientCompositorFrameSink, so remove
the FrameSinkId arg from related mojo IPC.

This CL also passes enable_surface_synchronization flag to
ui::ClientCompositorFrameSink via ctor.

BUG=720600
==========

[Peng, 2017-05-17 14:34:09]

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

[commit-bot: I haz the power, 2017-05-17 14:34:35]

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, 2017-05-17 14:50:50]

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

[commit-bot: I haz the power, 2017-05-17 14:50:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peng, 2017-05-17 15:15:54]

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

[commit-bot: I haz the power, 2017-05-17 15:16:33]

Dry run: CQ is trying da patch.

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

[Fady Samuel, 2017-05-17 17:44:21]

Thanks for cleaning this up. LGTM

[commit-bot: I haz the power, 2017-05-17 18:05:17]

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

[commit-bot: I haz the power, 2017-05-17 18:05:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peng, 2017-05-17 20:22:31]

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

[Peng, 2017-05-17 20:23:23]

penghuang@chromium.org changed reviewers:
+ sky@chromium.org

[Peng, 2017-05-17 20:23:24]

Hi Scott, PTAL. Thanks.

[commit-bot: I haz the power, 2017-05-17 20:23:49]

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, 2017-05-17 22:09:14]

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

[commit-bot: I haz the power, 2017-05-17 22:09:15]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-18 03:18:09]

https://codereview.chromium.org/2886873002/diff/120001/services/ui/public/int...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2886873002/diff/120001/services/ui/public/int...
services/ui/public/interfaces/window_manager.mojom:275: // Returns true if all
accelerators were added successfully.
Returns true on success.

https://codereview.chromium.org/2886873002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_manager_client_unittest.cc (right):

https://codereview.chromium.org/2886873002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_manager_client_unittest.cc:384: #if 0
How come you are commenting out this code?

https://codereview.chromium.org/2886873002/diff/120001/ui/aura/mus/window_por...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2886873002/diff/120001/ui/aura/mus/window_por...
ui/aura/mus/window_port_mus.cc:272: window_mus_type() ==
WindowMusType::EMBED_IN_OWNER);
What about DISPLAY_MANUALLY_CREATED? Is this code path hit for
DISPLAY_MANUALLY_CREATED?

[Peng, 2017-05-18 13:34:56]

https://codereview.chromium.org/2886873002/diff/120001/services/ui/public/int...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2886873002/diff/120001/services/ui/public/int...
services/ui/public/interfaces/window_manager.mojom:275: // Returns true if all
accelerators were added successfully.
On 2017/05/18 03:18:08, sky wrote:
> Returns true on success.

Done.

https://codereview.chromium.org/2886873002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_manager_client_unittest.cc (right):

https://codereview.chromium.org/2886873002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_manager_client_unittest.cc:384: #if 0
On 2017/05/18 03:18:08, sky wrote:
> How come you are commenting out this code?

I added it for debugging and forgot to remove it. Done

https://codereview.chromium.org/2886873002/diff/120001/ui/aura/mus/window_por...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2886873002/diff/120001/ui/aura/mus/window_por...
ui/aura/mus/window_port_mus.cc:272: window_mus_type() ==
WindowMusType::EMBED_IN_OWNER);
On 2017/05/18 03:18:08, sky wrote:
> What about DISPLAY_MANUALLY_CREATED? Is this code path hit for
> DISPLAY_MANUALLY_CREATED?

I tested this CL with muschrome and mustash. I didn't find any issue.

[Peng, 2017-05-18 13:39:01]

penghuang@chromium.org changed reviewers:
+ tsepez@chromium.org

[Peng, 2017-05-18 13:39:02]

+tsepez@chromium.org for *.mojom

Hi Tom, PTAL. Thanks.

[Peng, 2017-05-18 13:39:16]

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

[commit-bot: I haz the power, 2017-05-18 13:39:40]

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, 2017-05-18 15:14:52]

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

[commit-bot: I haz the power, 2017-05-18 15:14:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-05-18 15:54:23]

LGTM

[Tom Sepez, 2017-05-18 15:56:10]

lgtm

RS on deleting an argument.

[Peng, 2017-05-18 16:00:05]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2017-05-18 16:00:05]

The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2886873002/#ps140001
(title: "Address review issues.")

[commit-bot: I haz the power, 2017-05-18 16:00: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, 2017-05-18 16:40:03]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495123205414430,
"parent_rev": "bdeccd4dffa9a6e62f588d93beeed4a324e47192", "commit_rev":
"77a8c758cc4731844a0308f23ccd2fd0e8fe14d9"}

[commit-bot: I haz the power, 2017-05-18 16:40:16]

Description was changed from

==========
Only send the FrameSinkId to client when it is necessary

The ui::ClientCompositorFrameSink doesn't really need the FameSinkId
for submitting frames, so remove it.

And for TopLevel Window, Embeddee and DisplayRoot, we don't need the
FrameSinkId to create the ui::ClientCompositorFrameSink, so remove
the FrameSinkId arg from related mojo IPC.

This CL also passes enable_surface_synchronization flag to
ui::ClientCompositorFrameSink via ctor.

BUG=720600
==========

to

==========
Only send the FrameSinkId to client when it is necessary

The ui::ClientCompositorFrameSink doesn't really need the FameSinkId
for submitting frames, so remove it.

And for TopLevel Window, Embeddee and DisplayRoot, we don't need the
FrameSinkId to create the ui::ClientCompositorFrameSink, so remove
the FrameSinkId arg from related mojo IPC.

This CL also passes enable_surface_synchronization flag to
ui::ClientCompositorFrameSink via ctor.

BUG=720600

Review-Url: https://codereview.chromium.org/2886873002
Cr-Commit-Position: refs/heads/master@{#472841}
Committed:
https://chromium.googlesource.com/chromium/src/+/77a8c758cc4731844a0308f23ccd...
==========

[commit-bot: I haz the power, 2017-05-18 16:40:17]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/77a8c758cc4731844a0308f23ccd...