Propagates window type properties during aura-mus window creation

Propagates window type properties during aura-mus window creation

There are two types used to identify windows:
. aura::Window::type(), which is ui::wm::WindowType.
. ui::mojom::WindowType. This corresponds to the types defined in
  views::Widget::InitParams::Type. This is a superset of
  ui::wm::WindowType.

As ui::mojom::WindowType is a superset it is really the only one
needed. This patch makes it so during window creation a property is
passed to the server giving the type. The windowmanager can then use
this type when creating the corresponding window so that it knows the
type. As this property can't change after creation it doesn't go
through PropertyConverter.

Really the two types should be combined, but to keep this patch to a
minimum I'm not doing that here.

BUG=664625
TEST=covered by tests
R=msw@chromium.org, tsepez@chromium.org

Committed: https://crrev.com/91b6ee4b99b61dd10d741328a1a548a745cf4ffe
Cr-Commit-Position: refs/heads/master@{#433756}

[sky, 2016-11-20 17:24:52]

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

[commit-bot: I haz the power, 2016-11-20 17:24:56]

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-11-20 17:46:24]

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

[commit-bot: I haz the power, 2016-11-20 17:46:25]

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

[sky, 2016-11-21 00:21:51]

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

[commit-bot: I haz the power, 2016-11-21 00:22:03]

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-11-21 02:42:51]

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

[commit-bot: I haz the power, 2016-11-21 02:42:51]

Dry run: This issue passed the CQ dry run.

[msw, 2016-11-21 19:26:47]

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/client/aura_con...
File ui/aura/client/aura_constants.h (right):

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/client/aura_con...
ui/aura/client/aura_constants.h:111: AURA_EXPORT extern const
aura::WindowProperty<WindowTypeProperty*>* const
Can we avoid the struct via aura::WindowProperty<ui::mojom::WindowType>?

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/mus/property_ut...
File ui/aura/mus/property_utils.cc (right):

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/mus/property_ut...
ui/aura/mus/property_utils.cc:33: return ui::wm::WINDOW_TYPE_POPUP;
Should this return WINDOW_TYPE_UNKNOWN?

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

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1390: ui::mojom::WindowType window_type;
nit: assign an explicit default value?

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1395: // TODO: validation!
nit: cite http://crbug.com/654924 like above?

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_base.cc (right):

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_base.cc:172: if (window_type)
This seems a bit strange, should we allow a window without a type here? Should
the enums include unknown/invalid to avoid passing ptrs for null?
(ui::wm::WindowType has WINDOW_TYPE_UNKNOWN, but ui::mojom::WindowType doesn't
have one)

https://codereview.chromium.org/2514243002/diff/40001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2514243002/diff/40001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:70: static_cast<int32_t>(init_params.type));
Hmm, this sets kWindowType_Property to a Widget::InitParams::Type value, but
window_tree_client.cc at lines 134 and 1396 static-casts it to
ui::mojom::WindowType; I realize they're parallel, but this seems a bit strange
without some explicit type conversion or at least a comment.

https://codereview.chromium.org/2514243002/diff/40001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:88: std::map<std::string, std::vector<uint8_t>>
mus_properties =
Should this use the helper above?

[sky, 2016-11-21 22:10:50]

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

[sky, 2016-11-21 22:10:51]

Description was changed from

==========
Propagates window type properties during aura-mus window creation

There are two types used to identify windows:
. aura::Window::type(), which is ui::wm::WindowType.
. ui::mojom::WindowType. This corresponds to the types defined in
  views::Widget::InitParams::Type. This is a superset of
  ui::wm::WindowType.

As ui::mojom::WindowType is a superset it is really the only one
needed. This patch makes it so during window creation a property is
passed to the server giving the type. The windowmanager can then use
this type when creating the corresponding window so that it knows the
type. As this property can't change after creation it doesn't go
through PropertyConverter.

Really the two types should be combined, but to keep this patch to a
minimum I'm not doing that here.

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

to

==========
Propagates window type properties during aura-mus window creation

There are two types used to identify windows:
. aura::Window::type(), which is ui::wm::WindowType.
. ui::mojom::WindowType. This corresponds to the types defined in
  views::Widget::InitParams::Type. This is a superset of
  ui::wm::WindowType.

As ui::mojom::WindowType is a superset it is really the only one
needed. This patch makes it so during window creation a property is
passed to the server giving the type. The windowmanager can then use
this type when creating the corresponding window so that it knows the
type. As this property can't change after creation it doesn't go
through PropertyConverter.

Really the two types should be combined, but to keep this patch to a
minimum I'm not doing that here.

BUG=664625
TEST=covered by tests
R=msw@chromium.org, tsepez@chromium.org
==========

[sky, 2016-11-21 22:10:51]

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

[commit-bot: I haz the power, 2016-11-21 22:11:29]

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

[sky, 2016-11-21 22:13:56]

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/client/aura_con...
File ui/aura/client/aura_constants.h (right):

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/client/aura_con...
ui/aura/client/aura_constants.h:111: AURA_EXPORT extern const
aura::WindowProperty<WindowTypeProperty*>* const
On 2016/11/21 19:26:46, msw wrote:
> Can we avoid the struct via aura::WindowProperty<ui::mojom::WindowType>?

I went with the struct so that I can tell whether the value was set of not. But
adding UNKNOWN means I can use it as the default and nuke the struct.

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/mus/property_ut...
File ui/aura/mus/property_utils.cc (right):

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/mus/property_ut...
ui/aura/mus/property_utils.cc:33: return ui::wm::WINDOW_TYPE_POPUP;
On 2016/11/21 19:26:46, msw wrote:
> Should this return WINDOW_TYPE_UNKNOWN?

This case should never be hit, but you're right, UNKNOWN is better.

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

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1390: ui::mojom::WindowType window_type;
On 2016/11/21 19:26:47, msw wrote:
> nit: assign an explicit default value?

Done.

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1395: // TODO: validation!
On 2016/11/21 19:26:47, msw wrote:
> nit: cite http://crbug.com/654924 like above?

Done.

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_base.cc (right):

https://codereview.chromium.org/2514243002/diff/40001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_base.cc:172: if (window_type)
On 2016/11/21 19:26:47, msw wrote:
> This seems a bit strange, should we allow a window without a type here? Should
> the enums include unknown/invalid to avoid passing ptrs for null?
> (ui::wm::WindowType has WINDOW_TYPE_UNKNOWN, but ui::mojom::WindowType doesn't
> have one)

Added UNKNOWN.

https://codereview.chromium.org/2514243002/diff/40001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2514243002/diff/40001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:70: static_cast<int32_t>(init_params.type));
On 2016/11/21 19:26:47, msw wrote:
> Hmm, this sets kWindowType_Property to a Widget::InitParams::Type value, but
> window_tree_client.cc at lines 134 and 1396 static-casts it to
> ui::mojom::WindowType; I realize they're parallel, but this seems a bit
strange
> without some explicit type conversion or at least a comment.

Done.

https://codereview.chromium.org/2514243002/diff/40001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:88: std::map<std::string, std::vector<uint8_t>>
mus_properties =
On 2016/11/21 19:26:47, msw wrote:
> Should this use the helper above?

Done.

[sky, 2016-11-21 22:14:10]

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

[commit-bot: I haz the power, 2016-11-21 22:14:21]

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-11-21 22:18:17]

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

[commit-bot: I haz the power, 2016-11-21 22:18:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Tom Sepez, 2016-11-21 22:22:00]

RS LGTM on mojom adding constant.

[msw, 2016-11-21 22:40:45]

lgtm with a nit

https://codereview.chromium.org/2514243002/diff/80001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2514243002/diff/80001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:47: WINDOW_TYPES_MATCH(DRAG);
nit: add UNKNOWN too

[sky, 2016-11-21 22:42:09]

https://codereview.chromium.org/2514243002/diff/80001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2514243002/diff/80001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:47: WINDOW_TYPES_MATCH(DRAG);
On 2016/11/21 22:40:45, msw wrote:
> nit: add UNKNOWN too

I can't. Widget::InitParams::Type doesn't have UNKNOWN.

[sky, 2016-11-21 22:43:08]

On 2016/11/21 22:42:09, sky wrote:
>
https://codereview.chromium.org/2514243002/diff/80001/ui/views/mus/mus_client.cc
> File ui/views/mus/mus_client.cc (right):
> 
>
https://codereview.chromium.org/2514243002/diff/80001/ui/views/mus/mus_client...
> ui/views/mus/mus_client.cc:47: WINDOW_TYPES_MATCH(DRAG);
> On 2016/11/21 22:40:45, msw wrote:
> > nit: add UNKNOWN too
> 
> I can't. Widget::InitParams::Type doesn't have UNKNOWN.

I will add a comment though.

[sky, 2016-11-21 23:12:30]

The CQ bit was checked by sky@chromium.org

[sky, 2016-11-21 23:12:30]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2514243002/#ps100001
(title: "merge")

[commit-bot: I haz the power, 2016-11-21 23:13:03]

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-11-22 00:10:34]

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

[commit-bot: I haz the power, 2016-11-22 00:10:35]

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_...)

[sky, 2016-11-22 00:11:17]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-11-22 00:11:39]

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-11-22 02:53:57]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479773477285900,
"parent_rev": "c9a83f65d4c0383892a95ae31649842205fca704", "commit_rev":
"8b8655e1166f86ee28c3b69d68183900d0e9c5b9"}

[commit-bot: I haz the power, 2016-11-22 02:54:19]

Description was changed from

==========
Propagates window type properties during aura-mus window creation

There are two types used to identify windows:
. aura::Window::type(), which is ui::wm::WindowType.
. ui::mojom::WindowType. This corresponds to the types defined in
  views::Widget::InitParams::Type. This is a superset of
  ui::wm::WindowType.

As ui::mojom::WindowType is a superset it is really the only one
needed. This patch makes it so during window creation a property is
passed to the server giving the type. The windowmanager can then use
this type when creating the corresponding window so that it knows the
type. As this property can't change after creation it doesn't go
through PropertyConverter.

Really the two types should be combined, but to keep this patch to a
minimum I'm not doing that here.

BUG=664625
TEST=covered by tests
R=msw@chromium.org, tsepez@chromium.org
==========

to

==========
Propagates window type properties during aura-mus window creation

There are two types used to identify windows:
. aura::Window::type(), which is ui::wm::WindowType.
. ui::mojom::WindowType. This corresponds to the types defined in
  views::Widget::InitParams::Type. This is a superset of
  ui::wm::WindowType.

As ui::mojom::WindowType is a superset it is really the only one
needed. This patch makes it so during window creation a property is
passed to the server giving the type. The windowmanager can then use
this type when creating the corresponding window so that it knows the
type. As this property can't change after creation it doesn't go
through PropertyConverter.

Really the two types should be combined, but to keep this patch to a
minimum I'm not doing that here.

BUG=664625
TEST=covered by tests
R=msw@chromium.org, tsepez@chromium.org
==========

[commit-bot: I haz the power, 2016-11-22 02:54:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-22 02:56:36]

Description was changed from

==========
Propagates window type properties during aura-mus window creation

There are two types used to identify windows:
. aura::Window::type(), which is ui::wm::WindowType.
. ui::mojom::WindowType. This corresponds to the types defined in
  views::Widget::InitParams::Type. This is a superset of
  ui::wm::WindowType.

As ui::mojom::WindowType is a superset it is really the only one
needed. This patch makes it so during window creation a property is
passed to the server giving the type. The windowmanager can then use
this type when creating the corresponding window so that it knows the
type. As this property can't change after creation it doesn't go
through PropertyConverter.

Really the two types should be combined, but to keep this patch to a
minimum I'm not doing that here.

BUG=664625
TEST=covered by tests
R=msw@chromium.org, tsepez@chromium.org
==========

to

==========
Propagates window type properties during aura-mus window creation

There are two types used to identify windows:
. aura::Window::type(), which is ui::wm::WindowType.
. ui::mojom::WindowType. This corresponds to the types defined in
  views::Widget::InitParams::Type. This is a superset of
  ui::wm::WindowType.

As ui::mojom::WindowType is a superset it is really the only one
needed. This patch makes it so during window creation a property is
passed to the server giving the type. The windowmanager can then use
this type when creating the corresponding window so that it knows the
type. As this property can't change after creation it doesn't go
through PropertyConverter.

Really the two types should be combined, but to keep this patch to a
minimum I'm not doing that here.

BUG=664625
TEST=covered by tests
R=msw@chromium.org, tsepez@chromium.org

Committed: https://crrev.com/91b6ee4b99b61dd10d741328a1a548a745cf4ffe
Cr-Commit-Position: refs/heads/master@{#433756}
==========

[commit-bot: I haz the power, 2016-11-22 02:56:38]

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