Pass root ServerWindow id to FrameGenerator.

Pass root ServerWindow id to FrameGenerator.

The FrameSinkId for surfaces corresponding to a ServerWindow encodes the
ServerWindow WindowId as the client_id. This doesn't happen for
FrameGenerator because the display root ServerWindow was created after
the display FrameGenerator. Change the order of creation and pass the
WindowId to FrameGenerator.

BUG=659227
Committed: https://crrev.com/09c16e13a45528abf1994e036f488a09dc97d76b
Cr-Commit-Position: refs/heads/master@{#427699}

[Fady Samuel, 2016-10-24 21:53:21]

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

[Fady Samuel, 2016-10-24 21:53:22]

Generally looks good once my comments are addressed. Thanks!

https://codereview.chromium.org/2445873003/diff/1/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2445873003/diff/1/services/ui/ws/display.cc#n...
services/ui/ws/display.cc:36: PlatformDisplayInitParams init_params)
This is more or less the same in terms of cost, but I generally prefer passing
by const ref, copying locally and updating.

https://codereview.chromium.org/2445873003/diff/1/services/ui/ws/frame_genera...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2445873003/diff/1/services/ui/ws/frame_genera...
services/ui/ws/frame_generator.cc:33:
static_cast<uint32_t>(mojom::CompositorFrameSinkType::DEFAULT)),
https://cs.chromium.org/chromium/src/base/numerics/safe_conversions.h?q=base:...

https://cs.chromium.org/chromium/src/content/browser/frame_host/render_widget...

[kylechar, 2016-10-25 13:35:15]

Description was changed from

==========
Pass root ServerWindow id to FrameGenerator.

The FrameSinkId for surfaces corresponding to a ServerWindow encodes the
ServerWindow WindowId as the client_id. This doesn't happen for
FrameGenerator because the display root ServerWindow was created after
the display FrameGenerator. Change the order of creation and pass the
WindowId to FrameGenerator.

Also move the hash function definitions for WindowId from base_hash:: to
std:: so they work with std::unordered_set/map.

BUG=TBD
==========

to

==========

==========

[kylechar, 2016-10-25 18:08:17]

Description was changed from

==========

==========

to

==========
Pass root ServerWindow id to FrameGenerator.

The FrameSinkId for surfaces corresponding to a ServerWindow encodes the
ServerWindow WindowId as the client_id. This doesn't happen for
FrameGenerator because the display root ServerWindow was created after
the display FrameGenerator. Change the order of creation and pass the
WindowId to FrameGenerator.

BUG=659227
==========

[kylechar, 2016-10-25 18:15:01]

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

[kylechar, 2016-10-25 18:15:02]

+sky for OWNERS review.

https://codereview.chromium.org/2445873003/diff/1/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2445873003/diff/1/services/ui/ws/display.cc#n...
services/ui/ws/display.cc:36: PlatformDisplayInitParams init_params)
On 2016/10/24 21:53:22, Fady Samuel wrote:
> This is more or less the same in terms of cost, but I generally prefer passing
> by const ref, copying locally and updating.

Done.

https://codereview.chromium.org/2445873003/diff/1/services/ui/ws/frame_genera...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2445873003/diff/1/services/ui/ws/frame_genera...
services/ui/ws/frame_generator.cc:33:
static_cast<uint32_t>(mojom::CompositorFrameSinkType::DEFAULT)),
On 2016/10/24 21:53:22, Fady Samuel wrote:
>
https://cs.chromium.org/chromium/src/base/numerics/safe_conversions.h?q=base:...
> 
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/render_widget...

The error is actually with enum inputs to base::checked_cast. Looks like
enum/enum class returns false for std::is_arithmetic<> and a static_assert fails
when using base::checked_cast<>. It's basically this that fails:

static_assert(std::is_arithmetic<mojom::CompositorFrameSinkType>::value, "...");

[Fady Samuel, 2016-10-25 18:18:05]

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:27: WindowId root_window_id,
per offline discussion, we just pass in the root window.

[sky, 2016-10-25 19:23:31]

LGTM

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/display.h
File services/ui/ws/display.h (right):

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/display....
services/ui/ws/display.h:167: // |size| in DIP.
I thought we were keeping mus itself pixel?

[kylechar, 2016-10-26 14:13:16]

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/display.h
File services/ui/ws/display.h (right):

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/display....
services/ui/ws/display.h:167: // |size| in DIP.
On 2016/10/25 19:23:31, sky wrote:
> I thought we were keeping mus itself pixel?

Mus has knowledge of DIP and DPP. The ServerWindow sizes are in DIP though to
match the client window coordinates.

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:27: WindowId root_window_id,
On 2016/10/25 18:18:04, Fady Samuel wrote:
> per offline discussion, we just pass in the root window.

Done.

[kylechar, 2016-10-26 14:14:52]

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

[commit-bot: I haz the power, 2016-10-26 14:15:08]

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

[Fady Samuel, 2016-10-26 14:33:45]

lgtm + nits

https://codereview.chromium.org/2445873003/diff/60001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2445873003/diff/60001/services/ui/ws/display....
services/ui/ws/display.cc:40: // Pass the display root ServerWindow id to
PlatformDisplay.
nit: drop id.

https://codereview.chromium.org/2445873003/diff/60001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator_unittest.cc (right):

https://codereview.chromium.org/2445873003/diff/60001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator_unittest.cc:46: root_window_(new
ServerWindow(&window_delegate_, kRootDisplayId)) {}
nit: base::MakeUnique<ServerWindow>(&window_delegate_, kRootDisplayId) is the
preferred style.

[kylechar, 2016-10-26 14:55:23]

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/display.h
File services/ui/ws/display.h (right):

https://codereview.chromium.org/2445873003/diff/40001/services/ui/ws/display....
services/ui/ws/display.h:167: // |size| in DIP.
On 2016/10/26 14:13:16, kylechar wrote:
> On 2016/10/25 19:23:31, sky wrote:
> > I thought we were keeping mus itself pixel?
> 
> Mus has knowledge of DIP and DPP. The ServerWindow sizes are in DIP though to
> match the client window coordinates.

Hmm, it seems like there has been a bit of confusion here. Since I added the
support for device scale factor I've always been creating a ServerWindow with
DIP size. Talking to sadrul@ that doesn't match what is expected. I'll update
this in a separate CL.

[kylechar, 2016-10-26 14:58:59]

https://codereview.chromium.org/2445873003/diff/60001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2445873003/diff/60001/services/ui/ws/display....
services/ui/ws/display.cc:40: // Pass the display root ServerWindow id to
PlatformDisplay.
On 2016/10/26 14:33:45, Fady Samuel wrote:
> nit: drop id.

Done.

https://codereview.chromium.org/2445873003/diff/60001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator_unittest.cc (right):

https://codereview.chromium.org/2445873003/diff/60001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator_unittest.cc:46: root_window_(new
ServerWindow(&window_delegate_, kRootDisplayId)) {}
On 2016/10/26 14:33:45, Fady Samuel wrote:
> nit: base::MakeUnique<ServerWindow>(&window_delegate_, kRootDisplayId) is the
> preferred style.

Done.

[kylechar, 2016-10-26 14:59:07]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-10-26 14:59:08]

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

[commit-bot: I haz the power, 2016-10-26 14:59:19]

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-26 15:49:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-26 16:15:26]

Description was changed from

==========
Pass root ServerWindow id to FrameGenerator.

The FrameSinkId for surfaces corresponding to a ServerWindow encodes the
ServerWindow WindowId as the client_id. This doesn't happen for
FrameGenerator because the display root ServerWindow was created after
the display FrameGenerator. Change the order of creation and pass the
WindowId to FrameGenerator.

BUG=659227
==========

to

==========
Pass root ServerWindow id to FrameGenerator.

The FrameSinkId for surfaces corresponding to a ServerWindow encodes the
ServerWindow WindowId as the client_id. This doesn't happen for
FrameGenerator because the display root ServerWindow was created after
the display FrameGenerator. Change the order of creation and pass the
WindowId to FrameGenerator.

BUG=659227

Committed: https://crrev.com/09c16e13a45528abf1994e036f488a09dc97d76b
Cr-Commit-Position: refs/heads/master@{#427699}
==========

[commit-bot: I haz the power, 2016-10-26 16:15:27]

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