Makes WindowTreeHostMus supply properties directly to window creation

Makes WindowTreeHostMus supply properties directly to window creation

This way properties that don't make there way to Window properties can
be passed to the wm. For example,
ui::mojom::WindowManager::kInitialBounds_Property is only needed at
creation time and need not be stored in the aura::Window.

BUG=664625
669964
TEST=covered by test
R=msw@chromium.org

Committed: https://crrev.com/3663991e5d08bfa67d6c673bb46fb4e4f2875aac
Cr-Commit-Position: refs/heads/master@{#435845}

[sky, 2016-12-01 23:37:09]

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

[commit-bot: I haz the power, 2016-12-01 23:38:07]

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

[msw, 2016-12-02 00:31:09]

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1538: std::unordered_map<std::string,
std::vector<uint8_t>> transport_properties;
Why do we have to make a copy? map vs unordered_map? (if so, why?)

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client_unittest.cc:149:
static_cast<int32_t>(ui::mojom::WindowType::BUBBLE));
Should this use ConvertToPropertyTransportValue?

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client_unittest.cc:1059:
mojo::ConvertTo<std::vector<uint8_t>>(kUnknownPropertyValue);
I guess PropertyConverter *never* converts this value, so it's okay to use a
smaller primitive type? (PropertyConverter ensures primitive property arrays are
8u for int64_t, which is a decent guard against users setting incompatible array
values) I wonder if some other ConvertToPropertyTransportValue calls in this
file should also use direct conversion instead? I wonder how else we can
guarantee that this property is never converted to an aura::WindowProperty, and
if we should comment here or somewhere about the difference between
PropertyConverter primitive transport values and other (not sure what to call
them? mus-only?) primitive transport values?

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.cc:95: properties) {}
The other constructor re-applies these properties; is that right?
Sorry, I'm a bit confused... not sure if this makes sense.

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.h (right):

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.h:43: // the server, e.g. no conversion is
done, and then applied to the window
Isn't it possible that some of these *are* converted to aura::WindowProperty
values if the key is registered and we create a corresponding aura::Window?

[sky, 2016-12-02 01:08:43]

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1538: std::unordered_map<std::string,
std::vector<uint8_t>> transport_properties;
On 2016/12/02 00:31:08, msw wrote:
> Why do we have to make a copy? map vs unordered_map? (if so, why?)

I added a couple of TODOs and filed a bug. Maps in mojom correspond to
unordered_map in c++, hence the conversion here. We should use unordered_map in
a bunch of these places to avoid needless conversion.

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client_unittest.cc:149:
static_cast<int32_t>(ui::mojom::WindowType::BUBBLE));
On 2016/12/02 00:31:08, msw wrote:
> Should this use ConvertToPropertyTransportValue?

As the window type can't change after the window is created it is supplied in
the initial set of properties and not mapped to a property on the aura::Window.
Because of this the type doesn't need to be int64.

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client_unittest.cc:1059:
mojo::ConvertTo<std::vector<uint8_t>>(kUnknownPropertyValue);
On 2016/12/02 00:31:08, msw wrote:
> I guess PropertyConverter *never* converts this value, so it's okay to use a
> smaller primitive type?

That's right.

> (PropertyConverter ensures primitive property arrays are
> 8u for int64_t, which is a decent guard against users setting incompatible
array
> values) I wonder if some other ConvertToPropertyTransportValue calls in this
> file should also use direct conversion instead? I wonder how else we can
> guarantee that this property is never converted to an aura::WindowProperty,
and
> if we should comment here or somewhere about the difference between
> PropertyConverter primitive transport values and other (not sure what to call
> them? mus-only?) primitive transport values?

I think window_manager.mojom is the right place to document what the transport
types are. We are a bit inconsistent right now, I'll follow up on this in a
future patch when I make it consistent.

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.cc:95: properties) {}
On 2016/12/02 00:31:08, msw wrote:
> The other constructor re-applies these properties; is that right?
> Sorry, I'm a bit confused... not sure if this makes sense.

Ya, sorry, I agree this is weird. WindowPort is the porting layer introduced for
mus. Each Window has an associated WindowPort. CreateWindowPortForTopLevel()
creates the WindowPort (not the Window) and asks the server to create the
representation of the window on its side. |properties| is supplied to
CreateWindowPortForTopLevel so properties can be passed to the server. The other
WindowTreeHostMus constructor (actually, the superclass) creates the Window with
the WindowPort created from WindowTreeClient. Then the properties are passed
through PropertyConverter and applied to the window.

The old code created the Window first, applied the properties to it, then when
the call to create the window on the server side was called the properties were
extracted from the Window and passed to the server. This meant any properties
not known to PropertyConverter were dropped on the floor. This is bad as some
properties are only useful during initial construction.

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.h (right):

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.h:43: // the server, e.g. no conversion is
done, and then applied to the window
On 2016/12/02 00:31:08, msw wrote:
> Isn't it possible that some of these *are* converted to aura::WindowProperty
> values if the key is registered and we create a corresponding aura::Window?

Ya, sorry, maybe my comment isn't clear. Is the new one any better?

[sky, 2016-12-02 01:08:55]

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

[commit-bot: I haz the power, 2016-12-02 01:09:12]

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

[msw, 2016-12-02 01:21:40]

lgtm

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.cc:95: properties) {}
On 2016/12/02 01:08:43, sky wrote:
> On 2016/12/02 00:31:08, msw wrote:
> > The other constructor re-applies these properties; is that right?
> > Sorry, I'm a bit confused... not sure if this makes sense.
> 
> Ya, sorry, I agree this is weird. WindowPort is the porting layer introduced
for
> mus. Each Window has an associated WindowPort. CreateWindowPortForTopLevel()
> creates the WindowPort (not the Window) and asks the server to create the
> representation of the window on its side. |properties| is supplied to
> CreateWindowPortForTopLevel so properties can be passed to the server. The
other
> WindowTreeHostMus constructor (actually, the superclass) creates the Window
with
> the WindowPort created from WindowTreeClient. Then the properties are passed
> through PropertyConverter and applied to the window.
> 
> The old code created the Window first, applied the properties to it, then when
> the call to create the window on the server side was called the properties
were
> extracted from the Window and passed to the server. This meant any properties
> not known to PropertyConverter were dropped on the floor. This is bad as some
> properties are only useful during initial construction.

Okay, thanks for the explanation, would a comment here be worthwhile?
(or is there somewhere else that documents this pattern?)

[sky, 2016-12-02 02:41:56]

On 2016/12/02 01:21:40, msw wrote:
> lgtm
> 
>
https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
> File ui/aura/mus/window_tree_host_mus.cc (right):
> 
>
https://codereview.chromium.org/2539263005/diff/20001/ui/aura/mus/window_tree...
> ui/aura/mus/window_tree_host_mus.cc:95: properties) {}
> On 2016/12/02 01:08:43, sky wrote:
> > On 2016/12/02 00:31:08, msw wrote:
> > > The other constructor re-applies these properties; is that right?
> > > Sorry, I'm a bit confused... not sure if this makes sense.
> > 
> > Ya, sorry, I agree this is weird. WindowPort is the porting layer introduced
> for
> > mus. Each Window has an associated WindowPort. CreateWindowPortForTopLevel()
> > creates the WindowPort (not the Window) and asks the server to create the
> > representation of the window on its side. |properties| is supplied to
> > CreateWindowPortForTopLevel so properties can be passed to the server. The
> other
> > WindowTreeHostMus constructor (actually, the superclass) creates the Window
> with
> > the WindowPort created from WindowTreeClient. Then the properties are passed
> > through PropertyConverter and applied to the window.
> > 
> > The old code created the Window first, applied the properties to it, then
when
> > the call to create the window on the server side was called the properties
> were
> > extracted from the Window and passed to the server. This meant any
properties
> > not known to PropertyConverter were dropped on the floor. This is bad as
some
> > properties are only useful during initial construction.
> 
> Okay, thanks for the explanation, would a comment here be worthwhile?
> (or is there somewhere else that documents this pattern?)

I'll add a comment to WindowTreeHostMus.

[sky, 2016-12-02 02:42:16]

The CQ bit was checked by sky@chromium.org

[sky, 2016-12-02 02:42:16]

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

[commit-bot: I haz the power, 2016-12-02 02:42: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-02 03:34:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480646536275640,
"parent_rev": "cbd138c8854454cfb3f5b2d2690978ebae400b1a", "commit_rev":
"0a40bacd5312333731bdd099178ebae7db5e8857"}

[commit-bot: I haz the power, 2016-12-02 03:35:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-02 03:38:06]

Description was changed from

==========
Makes WindowTreeHostMus supply properties directly to window creation

This way properties that don't make there way to Window properties can
be passed to the wm. For example,
ui::mojom::WindowManager::kInitialBounds_Property is only needed at
creation time and need not be stored in the aura::Window.

BUG=664625 669964
TEST=covered by test
R=msw@chromium.org
==========

to

==========
Makes WindowTreeHostMus supply properties directly to window creation

This way properties that don't make there way to Window properties can
be passed to the wm. For example,
ui::mojom::WindowManager::kInitialBounds_Property is only needed at
creation time and need not be stored in the aura::Window.

BUG=664625 669964
TEST=covered by test
R=msw@chromium.org

Committed: https://crrev.com/3663991e5d08bfa67d6c673bb46fb4e4f2875aac
Cr-Commit-Position: refs/heads/master@{#435845}
==========

[commit-bot: I haz the power, 2016-12-02 03:38:07]

Patchset 4 (id:??) landed as
https://crrev.com/3663991e5d08bfa67d6c673bb46fb4e4f2875aac
Cr-Commit-Position: refs/heads/master@{#435845}