mash: Relay some Chrome aura window properties to Ash.

mash: Relay some Chrome aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create MusPropertyMirror to copy properties to root windows (mash frames).
Make DesktopWindowTreeHostMus surface content window prop changes.
Implement an ash mirror with a best-guess set of properties to copy at first.

BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org

Review-Url: https://codereview.chromium.org/2655513002
Cr-Commit-Position: refs/heads/master@{#446862}
Committed: https://chromium.googlesource.com/chromium/src/+/dc1ee54c6abd46f02adb02fcb119a553ee661a84

[msw, 2017-01-27 00:15:14]

Description was changed from

==========
Register ash window properties with Chrome's PropertyConverter.

Debugging shelf item type for panels.

BUG=
==========

to

==========
Relay Chrome's aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create WindowPropertyRelay to copy properties to grandparents.
Make DesktopWindowTreeHostMus surface content window prop changes.
(DesktopWindowTreeHostMus embeds windows in mash frame windows)
(select a best-guess set of properties to copy at first)


BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org
==========

[msw, 2017-01-27 00:15:14]

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

[msw, 2017-01-27 00:16:03]

Description was changed from

==========
Relay Chrome's aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create WindowPropertyRelay to copy properties to grandparents.
Make DesktopWindowTreeHostMus surface content window prop changes.
(DesktopWindowTreeHostMus embeds windows in mash frame windows)
(select a best-guess set of properties to copy at first)


BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org
==========

to

==========
Relay Chrome's aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create WindowPropertyRelay to copy properties to grandparents.
Make DesktopWindowTreeHostMus surface content window prop changes.
(DesktopWindowTreeHostMus embeds windows in mash frame windows)
(select a best-guess set of properties to copy at first)

BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org
==========

[msw, 2017-01-27 00:16:20]

Description was changed from

==========
Relay Chrome's aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create WindowPropertyRelay to copy properties to grandparents.
Make DesktopWindowTreeHostMus surface content window prop changes.
(DesktopWindowTreeHostMus embeds windows in mash frame windows)
(select a best-guess set of properties to copy at first)

BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org
==========

to

==========
mash: Relay some Chrome aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create WindowPropertyRelay to copy properties to grandparents.
Make DesktopWindowTreeHostMus surface content window prop changes.
(DesktopWindowTreeHostMus embeds windows in mash frame windows)
(select a best-guess set of properties to copy at first)

BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org
==========

[msw, 2017-01-27 00:16:36]

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

[msw, 2017-01-27 00:17:52]

Hey Scott, please take a look; thanks!
(help refining comments and cl desc is also appreciated)

[commit-bot: I haz the power, 2017-01-27 00:19:39]

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-01-27 00:44:05]

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

[commit-bot: I haz the power, 2017-01-27 00:44:06]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-01-27 16:20:20]

https://codereview.chromium.org/2655513002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/2655513002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:56: int32_t
value = window->GetProperty(ash::kShelfIDKey);
This sure is ICK, isn't it!

https://codereview.chromium.org/2655513002/diff/80001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2655513002/diff/80001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:185:
desktop_native_widget_aura->content_window()->AddObserver(this);
You never remove this observer.

https://codereview.chromium.org/2655513002/diff/80001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:700:
MusClient::Get()->OnWindowPropertyChanged(window, key);
I recommend something like this:

MusClient::Get()->MirrorPropertyFromWidgetWindowToRootWindow(window, key,
window());

Hopefully that better conveys what is expected to happen when this occurs. Does
it make it any clearer to you?

I suggested lumping this on MusClientObserver, but I think it would be clearer
to have an explicit interface for this function, perhaps MusPropertyMirror.
WDYT?

[msw, 2017-01-27 19:10:17]

Patchset #8 (id:140001) has been deleted

[msw, 2017-01-27 19:11:03]

Description was changed from

==========
mash: Relay some Chrome aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create WindowPropertyRelay to copy properties to grandparents.
Make DesktopWindowTreeHostMus surface content window prop changes.
(DesktopWindowTreeHostMus embeds windows in mash frame windows)
(select a best-guess set of properties to copy at first)

BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org
==========

to

==========
mash: Relay some Chrome aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create MusPropertyMirror to copy properties to root windows (mash frames).
Make DesktopWindowTreeHostMus surface content window prop changes.
Implement an ash mirror with a best-guess set of properties to copy at first.

BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org
==========

[msw, 2017-01-27 19:11:34]

Comments addressed. Please take another look; thanks!

https://codereview.chromium.org/2655513002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/2655513002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:56: int32_t
value = window->GetProperty(ash::kShelfIDKey);
On 2017/01/27 16:20:20, sky wrote:
> This sure is ICK, isn't it!

Yup, let me know if you can envision a better approach.

https://codereview.chromium.org/2655513002/diff/80001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2655513002/diff/80001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:185:
desktop_native_widget_aura->content_window()->AddObserver(this);
On 2017/01/27 16:20:20, sky wrote:
> You never remove this observer.

Now done in the destructor.

https://codereview.chromium.org/2655513002/diff/80001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:700:
MusClient::Get()->OnWindowPropertyChanged(window, key);
On 2017/01/27 16:20:20, sky wrote:
> I recommend something like this:
> 
> MusClient::Get()->MirrorPropertyFromWidgetWindowToRootWindow(window, key,
> window());
> 
> Hopefully that better conveys what is expected to happen when this occurs.
Does
> it make it any clearer to you?
> 
> I suggested lumping this on MusClientObserver, but I think it would be clearer
> to have an explicit interface for this function, perhaps MusPropertyMirror.
> WDYT?

I tried to adopt your suggestions, let me know what you think.

[msw, 2017-01-27 19:11:41]

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

[commit-bot: I haz the power, 2017-01-27 19:12:28]

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

[sky, 2017-01-27 20:10:45]

https://codereview.chromium.org/2655513002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/2655513002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:36: // Relays
aura content window properties to its root window (the mash frame).
Explain why this is done.

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_client.h
File ui/views/mus/mus_client.h (right):

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_clien...
ui/views/mus/mus_client.h:18: #include "ui/views/mus/mus_property_mirror.h"
Forward declare?

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_clien...
ui/views/mus/mus_client.h:110: void
set_mus_property_mirror(std::unique_ptr<MusPropertyMirror> mirror) {
As this runs a destructor I think it should be SetMusPropertyMirror and not
inlined.

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_prope...
File ui/views/mus/mus_property_mirror.h (right):

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_prope...
ui/views/mus/mus_property_mirror.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
2017

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_prope...
ui/views/mus/mus_property_mirror.h:18: virtual void
MirrorPropertyFromWidgetWindowToRootWindow(aura::Window* window,
I'm surprised you don't need to forward declare window.

You should also mention window is the window returned from
widget->GetNativeWindow(). And to make it super class what the expectation is, I
would have this take the rootwindow and only call this function if there is a
root window.

[commit-bot: I haz the power, 2017-01-27 20:13:48]

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

[commit-bot: I haz the power, 2017-01-27 20:13:49]

Dry run: This issue passed the CQ dry run.

[msw, 2017-01-27 21:19:25]

Comments addressed. Please take another look; thanks!

https://codereview.chromium.org/2655513002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/2655513002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:36: // Relays
aura content window properties to its root window (the mash frame).
On 2017/01/27 20:10:45, sky wrote:
> Explain why this is done.

Done. (I attempted to give a clear explanation, suggestions welcome)

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_client.h
File ui/views/mus/mus_client.h (right):

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_clien...
ui/views/mus/mus_client.h:18: #include "ui/views/mus/mus_property_mirror.h"
On 2017/01/27 20:10:45, sky wrote:
> Forward declare?

Done.

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_clien...
ui/views/mus/mus_client.h:110: void
set_mus_property_mirror(std::unique_ptr<MusPropertyMirror> mirror) {
On 2017/01/27 20:10:45, sky wrote:
> As this runs a destructor I think it should be SetMusPropertyMirror and not
> inlined.

Done.

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_prope...
File ui/views/mus/mus_property_mirror.h (right):

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_prope...
ui/views/mus/mus_property_mirror.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
On 2017/01/27 20:10:45, sky wrote:
> 2017

Done.

https://codereview.chromium.org/2655513002/diff/160001/ui/views/mus/mus_prope...
ui/views/mus/mus_property_mirror.h:18: virtual void
MirrorPropertyFromWidgetWindowToRootWindow(aura::Window* window,
On 2017/01/27 20:10:45, sky wrote:
> I'm surprised you don't need to forward declare window.
> 
> You should also mention window is the window returned from
> widget->GetNativeWindow(). And to make it super class what the expectation is,
I
> would have this take the rootwindow and only call this function if there is a
> root window.

Done (and added aura::Window fwd decl).

[msw, 2017-01-27 21:19:37]

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

[commit-bot: I haz the power, 2017-01-27 21:20:07]

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-01-27 22:15:08]

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

[commit-bot: I haz the power, 2017-01-27 22:15:09]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-27 23:46:22]

LGTM

https://codereview.chromium.org/2655513002/diff/180001/ui/views/mus/desktop_w...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2655513002/diff/180001/ui/views/mus/desktop_w...
ui/views/mus/desktop_window_tree_host_mus.cc:703: aura::Window* root_window =
window->GetRootWindow();
root_window is the same as window() in this file. So, no local and use window().

https://codereview.chromium.org/2655513002/diff/180001/ui/views/mus/mus_prope...
File ui/views/mus/mus_property_mirror.h (right):

https://codereview.chromium.org/2655513002/diff/180001/ui/views/mus/mus_prope...
ui/views/mus/mus_property_mirror.h:23: // |root_window| is the frame that mash
constructs in order to render the
The description of root_window is mildly confusing. Maybe something like:

|root_window| is the top-level window representing the widget that is owned by
the window manager and that the window manager observes changes on.

Is that any better?

[msw, 2017-01-28 00:09:16]

Landing; thanks for your help and review!

https://codereview.chromium.org/2655513002/diff/180001/ui/views/mus/desktop_w...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2655513002/diff/180001/ui/views/mus/desktop_w...
ui/views/mus/desktop_window_tree_host_mus.cc:703: aura::Window* root_window =
window->GetRootWindow();
On 2017/01/27 23:46:22, sky wrote:
> root_window is the same as window() in this file. So, no local and use
window().

Done.

https://codereview.chromium.org/2655513002/diff/180001/ui/views/mus/mus_prope...
File ui/views/mus/mus_property_mirror.h (right):

https://codereview.chromium.org/2655513002/diff/180001/ui/views/mus/mus_prope...
ui/views/mus/mus_property_mirror.h:23: // |root_window| is the frame that mash
constructs in order to render the
On 2017/01/27 23:46:22, sky wrote:
> The description of root_window is mildly confusing. Maybe something like:
> 
> |root_window| is the top-level window representing the widget that is owned by
> the window manager and that the window manager observes changes on.
> 
> Is that any better?

Done.

[msw, 2017-01-28 00:09:21]

The CQ bit was checked by msw@chromium.org

[msw, 2017-01-28 00:09:22]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2655513002/#ps200001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-01-28 00:10:09]

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-01-28 00:56:29]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1485562161928210,
"parent_rev": "4bb102b532614f3a973fa22a15cd90e84282f338", "commit_rev":
"dc1ee54c6abd46f02adb02fcb119a553ee661a84"}

[commit-bot: I haz the power, 2017-01-28 00:57:01]

Description was changed from

==========
mash: Relay some Chrome aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create MusPropertyMirror to copy properties to root windows (mash frames).
Make DesktopWindowTreeHostMus surface content window prop changes.
Implement an ash mirror with a best-guess set of properties to copy at first.

BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org
==========

to

==========
mash: Relay some Chrome aura window properties to Ash.

Register ash window properties with Chrome's PropertyConverter.
(register kPanelAttachedKey and kShelfItemTypeKey)

Create MusPropertyMirror to copy properties to root windows (mash frames).
Make DesktopWindowTreeHostMus surface content window prop changes.
Implement an ash mirror with a best-guess set of properties to copy at first.

BUG=681233
TEST='chrome --mash' settings window title and shelf item are right.
R=sky@chromium.org

Review-Url: https://codereview.chromium.org/2655513002
Cr-Commit-Position: refs/heads/master@{#446862}
Committed:
https://chromium.googlesource.com/chromium/src/+/dc1ee54c6abd46f02adb02fcb119...
==========

[commit-bot: I haz the power, 2017-01-28 00:57:02]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/dc1ee54c6abd46f02adb02fcb119...