Converts ash to use aura-mus

Converts ash to use aura-mus

There is still a bunch of cleanup and removal to do, but this patch is
already so big I'm hoping to keep more cleanup to future patches. The
biggest remaining issue after this patch is z-order. By that I mean
the non-client area is at a different z-order than the frame, which
leads to some interesting artifacts. That will be addressed soon, and
I'm hoping won't block other testing.

I've landed as many other patches as I could, so this is mostly just
changes to aura, but there are some properties that needed to be
changed. I could separate that out if you prefer.

BUG=669964
TEST=covered by tests
R=jamescook@chromium.org
TBR=rockot@chromium.org (for changes to testing/buildbot)

Committed: https://crrev.com/fcf18b528bc1908b9e50af6ff914b0049e693fbf
Cr-Commit-Position: refs/heads/master@{#436515}

[sky, 2016-12-03 00:19:49]

Sorry for the ginormo review. The big parts are in ash/mus, and the rest mostly
property tweaks/includes.
How properties work in the aura-mus world is *some* aura window properties can
make their way to the server. The set of properties that make it to the server
are defined in aura/mus/PropertyConverter. All primitives are mapped to int64
(PropertyConverter::PrimitiveType). This resulted in having to change some of
the property setters to cast to PrimitiveType. You may also notice some
primitive types don't use int64, this is for properties that are not persisted,
only used in creation of the window. For example, kWindowType_Property never
changes and is only used in the initial set of supplied properties. So, it's
transported as int64.

[sky, 2016-12-03 00:19:53]

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

[commit-bot: I haz the power, 2016-12-03 00:20:31]

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-12-03 01:20:59]

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

[commit-bot: I haz the power, 2016-12-03 01:21:00]

Dry run: This issue passed the CQ dry run.

[sky, 2016-12-03 17:12:15]

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

[commit-bot: I haz the power, 2016-12-03 17:12:27]

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-12-03 19:02:07]

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

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

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

[James Cook, 2016-12-05 17:21:49]

A couple early questions... I'm still looking through the bulk of it.

https://codereview.chromium.org/2539363005/diff/40001/ash/aura/wm_window_aura.h
File ash/aura/wm_window_aura.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/aura/wm_window_aura...
ash/aura/wm_window_aura.h:18: class ASH_EXPORT WmWindowAura : public WmWindow,
Please add a comment here that these are only allowed in "classic ash" /
"non-aura-mus". Things are going to be temporarily confusing while there are two
aura impls.

https://codereview.chromium.org/2539363005/diff/40001/ash/common/DEPS
File ash/common/DEPS (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/common/DEPS#newcode1
ash/common/DEPS:1: include_rules = [
Since we don't need the DEPS separation of ash/common any more I think we should
eventually flatten the directory structure to eliminate /common/. Can you file a
tracking bug for that?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (left):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:49: // This class is used so that the
WindowState constructor can be made protected.
Nice to see funky little things like this going away.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.h:31: class WmWindowMus : public WmWindowAura {
Q: What's the plan for cleanup here? It's super-confusing that there's now
WmWindow, aura::Window, WmWindowAura, and WmWindowMus which is-a WmWindowAura.
Do WmWindow and WmWindowAura get collapsed together? Do we eventually get back
to a state where everything is aura::Window?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.h:63: static aura::Window* GetAuraWindow(WmWindow*
wm_window) {
Do you still need these given that WmWindowAura::GetAuraWindow exists? (C++
function lookup rules sometimes make my head hurt.)

ditto 2 functions below

https://codereview.chromium.org/2539363005/diff/40001/services/ui/ws/focus_co...
File services/ui/ws/focus_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/services/ui/ws/focus_co...
services/ui/ws/focus_controller.cc:188: // The type must match that of
PropertyConverter::PrimitiveType.
services/ui/DEPS allows +ui. Should this include
ui/aura/mus/property_converter.h and use the actual type? Or does that create
circular dependencies?

(Related: Should that type be in some shared header in something like
services/ui/public ?)

https://codereview.chromium.org/2539363005/diff/40001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:1416:
window_->SetSharedProperty<aura::PropertyConverter::PrimitiveType>(
nit: These statements are really long and ugly. Would "using
aura::PropertyConverter" make them wrap better?

[sky, 2016-12-05 17:56:55]

https://codereview.chromium.org/2539363005/diff/40001/ash/aura/wm_window_aura.h
File ash/aura/wm_window_aura.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/aura/wm_window_aura...
ash/aura/wm_window_aura.h:18: class ASH_EXPORT WmWindowAura : public WmWindow,
On 2016/12/05 17:21:48, James Cook wrote:
> Please add a comment here that these are only allowed in "classic ash" /
> "non-aura-mus". Things are going to be temporarily confusing while there are
two
> aura impls.

The truth is you can actually use this class in aura-mus. WmWindowAura::Get()
returns an WmWindowMus when in aura-mus. So, it's ok to call
WmWindowAura::Get(). I made the constructor protected to ensure no one is
creating directly.

https://codereview.chromium.org/2539363005/diff/40001/ash/common/DEPS
File ash/common/DEPS (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/common/DEPS#newcode1
ash/common/DEPS:1: include_rules = [
On 2016/12/05 17:21:48, James Cook wrote:
> Since we don't need the DEPS separation of ash/common any more I think we
should
> eventually flatten the directory structure to eliminate /common/. Can you file
a
> tracking bug for that?

I filed 671246.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.h:31: class WmWindowMus : public WmWindowAura {
On 2016/12/05 17:21:49, James Cook wrote:
> Q: What's the plan for cleanup here? It's super-confusing that there's now
> WmWindow, aura::Window, WmWindowAura, and WmWindowMus which is-a WmWindowAura.
> Do WmWindow and WmWindowAura get collapsed together? Do we eventually get back
> to a state where everything is aura::Window?

Unfortunately I don't have a good answer for that now. This patch is about
moving away from ui::Window, without much thought into the right long term
structure. After this I plan to better understand exactly what needs to differ
between classic aura and aura-mus. For example, widget lookup will always be
different in the two environments, as will be a number of other things. So,
there still needs to be a way to isolate these differences, but not to the same
degree that we have now.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.h:63: static aura::Window* GetAuraWindow(WmWindow*
wm_window) {
On 2016/12/05 17:21:49, James Cook wrote:
> Do you still need these given that WmWindowAura::GetAuraWindow exists? (C++
> function lookup rules sometimes make my head hurt.)
> 
> ditto 2 functions below

You are right, nuked.

https://codereview.chromium.org/2539363005/diff/40001/services/ui/ws/focus_co...
File services/ui/ws/focus_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/services/ui/ws/focus_co...
services/ui/ws/focus_controller.cc:188: // The type must match that of
PropertyConverter::PrimitiveType.
On 2016/12/05 17:21:49, James Cook wrote:
> services/ui/DEPS allows +ui. Should this include
> ui/aura/mus/property_converter.h and use the actual type? Or does that create
> circular dependencies?
> 
> (Related: Should that type be in some shared header in something like
> services/ui/public ?)

The window-server doesn't link with aura, and I would like to keep it that way.
Probably the best place for this is services/ui/common, but that's still a
little weird. Are you ok with the comment for now?

https://codereview.chromium.org/2539363005/diff/40001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:1416:
window_->SetSharedProperty<aura::PropertyConverter::PrimitiveType>(
On 2016/12/05 17:21:49, James Cook wrote:
> nit: These statements are really long and ugly. Would "using
> aura::PropertyConverter" make them wrap better?

Done.

[James Cook, 2016-12-05 19:21:45]

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_root_...
File ash/mus/bridge/wm_root_window_controller_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_root_...
ash/mus/bridge/wm_root_window_controller_mus.cc:98: void
WmRootWindowControllerMus::ConfigureWidgetInitParamsForContainer(
Q: Will this eventually go away, since ash as the window manager knows about its
aura window hierarchy and in theory could parent a new window to an existing
container?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:289: if (!root_window_controller_mus)
Q: How could this be null when root_window_controller wasn't null in the old
code? How does a window not have a RWC?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (left):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:176: void WmWindowMus::Destroy() {
Q: Does getting rid of this mean we can get rid of WmWindowOwner in tests?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:120: gfx::Size WmWindowMus::GetMinimumSize()
const {
Q: Will methods like this eventually be replaced with the WmWindowAura version?

If not, it might be worth a comment at class level about why some of these
methods have to be different.

If so, maybe a TODO somewhere?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:175: // TODO: investigate if needed.
super nit: TODO(mash) or TODO(sky)

https://google.github.io/styleguide/cppguide.html#TODO_Comments

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:285: }
Woo hoo! So nice to see so much code going away.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus_test_api.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus_test_api.h:15: // Used by tests to set the default
value of\
nit: eliminate \

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus_test_api.h:17: class GlobalMinimumSizeLock {
nit: document why this must exist. Do some tests call into shared code that
changes empty-min-size? Do windows have to be constructed with the param
initially set?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/workspac...
File ash/mus/bridge/workspace_event_handler_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/workspac...
ash/mus/bridge/workspace_event_handler_mus.h:21:
WorkspaceEventHandlerMus(aura::Window* workspace_window);
nit: explicit

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/disconnected_ap...
File ash/mus/disconnected_app_handler.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/disconnected_ap...
ash/mus/disconnected_app_handler.h:14: // Tracks ui::Windows for when they get
disconnected from the embedded app.
ui::Windows?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:185: : views::NativeWidgetAura(delegate,
true),
The "true" here for is_parallel_widget_in_window_manager is pretty significant.
Comment at function level or inline?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:295: params.context = context;
nit: If either |parent| or |context| must be non-null, DCHECK them above.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.h:48: // frame decorations. This eletes
itself when |window| is destroyed.
eletes -> deletes

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.h:49: // |parent| is the parent to place the
newly created window in.
Comment whether parent, context or both can be null.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/property_util.h
File ash/mus/property_util.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/property_util.h...
ash/mus/property_util.h:20: namespace mus {
How about you copy/paste the explanation about window properties you sent me in
the review request into the code somewhere. Maybe here?

(I assume the functions here are related to the ones that go to the server.)

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:81: wm_shelf_.reset();
I presume there's an ordering dependency in destruction here. If so, comment
what it is.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:115: context = window_tree_host_->window();
Using root() here might be a little clearer.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:125:
(*properties)[ui::mojom::kWaitForUnderlay_Property].clear();
I know you didn't change this, but what's it for / how does this work?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:149: WmWindowMus* root =
WmWindowMus::Get(window_tree_host_->window());
ditto, might be clearer to call root()

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:205: ui::SHOW_STATE_FULLSCREEN) {
nit: This if() is ugly. Would it help to cache property_converter() in a local,
or use "using ui::mojom::WindowManager"?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:208: if (!container_window) {
Why is fullscreen not using the root window bounds like it was before? Is this a
screen vs. root window coords thing? Or was the code wrong before?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
File ash/mus/root_window_controller.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.h:41: // RootWindowController takes ownership of
the Window that it passed to it.
nit: update comment

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
File ash/mus/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller_unittest.cc:43: std::unique_ptr<aura::Window>
window_primary_display(
Hooray for the return to unique_ptr<aura::Window> in tests!

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h
File ash/mus/screen_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h#ne...
ash/mus/screen_mus.h:13: class ScreenMus : public display::ScreenBase {
nit: class comment please

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h#ne...
ash/mus/screen_mus.h:15: explicit ScreenMus();
nit: no explicit

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/shadow.h
File ash/mus/shadow.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/shadow.h#newcode23
ash/mus/shadow.h:23: // http://crbug.com/670840.
nit: What is the bug saying to do? Eliminate this class? Investigate something?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/test/wm_test_ba...
File ash/mus/test/wm_test_base.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/test/wm_test_ba...
ash/mus/test/wm_test_base.cc:106: const ui::mojom::WindowType mus_window_type =
nit: move down closer to where it's used

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:77: // TODO: need to register
OSExchangeDataProviderMus. http://crbug.com/665077.
Thanks for filing bugs for these TODO items.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:99: DCHECK(!window_tree_client_);
nit: Can you also DCHECK(window_manager_client_)? That will make it more obvious
that SetWindowManagerClient() has to be called before Init() without reading
through this entire function.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:167:
properties->erase(ui::mojom::WindowManager::kWindowIgnoredByShelf_Property);
I think there should be a central place where we document which properties are
vs. aren't persisted.  WDYT of documenting this in the window_manager.mojom
file?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:413: // CreateRootWindowController() means a new
display is being added, so the
nit: CreateRootWindowController -> "A call to this function" or name it

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:505: // This ensures WmWindowAura won't be called
before WmWindowMus.
because WmWindowMus::Get() has side effects that have to happen before
WmWindowAura::Get()? Might be nice to say why this is important.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager.h
File ash/mus/window_manager.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.h:36: class PointerWatcherEventRouter2;
Ugh. Is there a bug open to track getting rid of PWER in favor of PWER2 and
renaming it?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_properti...
File ash/mus/window_properties.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_properti...
ash/mus/window_properties.h:15: AURA_EXPORT extern const
aura::WindowProperty<bool>* const
ASH_EXPORT?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_properti...
ash/mus/window_properties.h:18: AURA_EXPORT extern const
aura::WindowProperty<bool>* const kWindowIsJanky;
nit: Document or point to mojo property it maps to.

[sky, 2016-12-05 21:39:20]

Thanks for the great feedback. I think I got them all. Take another look?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_root_...
File ash/mus/bridge/wm_root_window_controller_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_root_...
ash/mus/bridge/wm_root_window_controller_mus.cc:98: void
WmRootWindowControllerMus::ConfigureWidgetInitParamsForContainer(
On 2016/12/05 19:21:43, James Cook wrote:
> Q: Will this eventually go away, since ash as the window manager knows about
its
> aura window hierarchy and in theory could parent a new window to an existing
> container?

The important thing is associating the widget with the WmWindow. That may be
possible by way of a native widget factory.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:289: if (!root_window_controller_mus)
On 2016/12/05 19:21:44, James Cook wrote:
> Q: How could this be null when root_window_controller wasn't null in the old
> code? How does a window not have a RWC?

You are right. Fixed.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:120: gfx::Size WmWindowMus::GetMinimumSize()
const {
On 2016/12/05 19:21:44, James Cook wrote:
> Q: Will methods like this eventually be replaced with the WmWindowAura
version?
> 
> If not, it might be worth a comment at class level about why some of these
> methods have to be different.
> 
> If so, maybe a TODO somewhere?

I would lump this with the bug I filed about figuring out the right way to
structure the porting layer. Really I want a TODO at the directory level, which
we don't really have. I'm not going to put a TODO as I think the TODO would be
needed in all these classes, so for now just the bug. Does that seem reasonable?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:175: // TODO: investigate if needed.
On 2016/12/05 19:21:44, James Cook wrote:
> super nit: TODO(mash) or TODO(sky)
> 
> https://google.github.io/styleguide/cppguide.html#TODO_Comments

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus_test_api.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus_test_api.h:15: // Used by tests to set the default
value of\
On 2016/12/05 19:21:44, James Cook wrote:
> nit: eliminate \

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus_test_api.h:17: class GlobalMinimumSizeLock {
On 2016/12/05 19:21:44, James Cook wrote:
> nit: document why this must exist. Do some tests call into shared code that
> changes empty-min-size? Do windows have to be constructed with the param
> initially set?

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/workspac...
File ash/mus/bridge/workspace_event_handler_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/workspac...
ash/mus/bridge/workspace_event_handler_mus.h:21:
WorkspaceEventHandlerMus(aura::Window* workspace_window);
On 2016/12/05 19:21:44, James Cook wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/disconnected_ap...
File ash/mus/disconnected_app_handler.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/disconnected_ap...
ash/mus/disconnected_app_handler.h:14: // Tracks ui::Windows for when they get
disconnected from the embedded app.
On 2016/12/05 19:21:44, James Cook wrote:
> ui::Windows?

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:185: : views::NativeWidgetAura(delegate,
true),
On 2016/12/05 19:21:44, James Cook wrote:
> The "true" here for is_parallel_widget_in_window_manager is pretty
significant.
> Comment at function level or inline?

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:295: params.context = context;
On 2016/12/05 19:21:44, James Cook wrote:
> nit: If either |parent| or |context| must be non-null, DCHECK them above.

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.h:48: // frame decorations. This eletes
itself when |window| is destroyed.
On 2016/12/05 19:21:44, James Cook wrote:
> eletes -> deletes

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.h:49: // |parent| is the parent to place the
newly created window in.
On 2016/12/05 19:21:44, James Cook wrote:
> Comment whether parent, context or both can be null.

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/property_util.h
File ash/mus/property_util.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/property_util.h...
ash/mus/property_util.h:20: namespace mus {
On 2016/12/05 19:21:44, James Cook wrote:
> How about you copy/paste the explanation about window properties you sent me
in
> the review request into the code somewhere. Maybe here?
> 
> (I assume the functions here are related to the ones that go to the server.)

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:81: wm_shelf_.reset();
On 2016/12/05 19:21:44, James Cook wrote:
> I presume there's an ordering dependency in destruction here. If so, comment
> what it is.

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:115: context = window_tree_host_->window();
On 2016/12/05 19:21:44, James Cook wrote:
> Using root() here might be a little clearer.

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:125:
(*properties)[ui::mojom::kWaitForUnderlay_Property].clear();
On 2016/12/05 19:21:44, James Cook wrote:
> I know you didn't change this, but what's it for / how does this work?

It's for mus to make sure mus has both parts of the window before drawing it.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:149: WmWindowMus* root =
WmWindowMus::Get(window_tree_host_->window());
On 2016/12/05 19:21:44, James Cook wrote:
> ditto, might be clearer to call root()

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:205: ui::SHOW_STATE_FULLSCREEN) {
On 2016/12/05 19:21:44, James Cook wrote:
> nit: This if() is ugly. Would it help to cache property_converter() in a
local,
> or use "using ui::mojom::WindowManager"?

Well, I made this function easier to parse, but I'm not sure if I just pushed
the unreadability around. Any better?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:208: if (!container_window) {
On 2016/12/05 19:21:44, James Cook wrote:
> Why is fullscreen not using the root window bounds like it was before? Is this
a
> screen vs. root window coords thing? Or was the code wrong before?

It's because of differences in how NativeWidgetAura parents and treats
coordinates (the new code) vs how NativeWidgetMus was doing it.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
File ash/mus/root_window_controller.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.h:41: // RootWindowController takes ownership of
the Window that it passed to it.
On 2016/12/05 19:21:44, James Cook wrote:
> nit: update comment

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h
File ash/mus/screen_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h#ne...
ash/mus/screen_mus.h:13: class ScreenMus : public display::ScreenBase {
On 2016/12/05 19:21:45, James Cook wrote:
> nit: class comment please

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h#ne...
ash/mus/screen_mus.h:15: explicit ScreenMus();
On 2016/12/05 19:21:45, James Cook wrote:
> nit: no explicit

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/shadow.h
File ash/mus/shadow.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/shadow.h#newcode23
ash/mus/shadow.h:23: // http://crbug.com/670840.
On 2016/12/05 19:21:45, James Cook wrote:
> nit: What is the bug saying to do? Eliminate this class? Investigate
something?

Yes. This class is mostly a copy of the class in ui/wm/core. Now that ash is
using aura it may be possible to get rid of this class entirely.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/test/wm_test_ba...
File ash/mus/test/wm_test_base.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/test/wm_test_ba...
ash/mus/test/wm_test_base.cc:106: const ui::mojom::WindowType mus_window_type =
On 2016/12/05 19:21:45, James Cook wrote:
> nit: move down closer to where it's used

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:99: DCHECK(!window_tree_client_);
On 2016/12/05 19:21:45, James Cook wrote:
> nit: Can you also DCHECK(window_manager_client_)? That will make it more
obvious
> that SetWindowManagerClient() has to be called before Init() without reading
> through this entire function.

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:167:
properties->erase(ui::mojom::WindowManager::kWindowIgnoredByShelf_Property);
On 2016/12/05 19:21:45, James Cook wrote:
> I think there should be a central place where we document which properties are
> vs. aren't persisted.  WDYT of documenting this in the window_manager.mojom
> file?

Definitely. In fact Mike asked for the same thing. In hopes of not growing this
patch I was planning on updating window_manager.mojom after this. Sound good?

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:413: // CreateRootWindowController() means a new
display is being added, so the
On 2016/12/05 19:21:45, James Cook wrote:
> nit: CreateRootWindowController -> "A call to this function" or name it

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:505: // This ensures WmWindowAura won't be called
before WmWindowMus.
On 2016/12/05 19:21:45, James Cook wrote:
> because WmWindowMus::Get() has side effects that have to happen before
> WmWindowAura::Get()? Might be nice to say why this is important.

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager.h
File ash/mus/window_manager.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.h:36: class PointerWatcherEventRouter2;
On 2016/12/05 19:21:45, James Cook wrote:
> Ugh. Is there a bug open to track getting rid of PWER in favor of PWER2 and
> renaming it?

671363 (just filed)

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_properti...
File ash/mus/window_properties.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_properti...
ash/mus/window_properties.h:15: AURA_EXPORT extern const
aura::WindowProperty<bool>* const
On 2016/12/05 19:21:45, James Cook wrote:
> ASH_EXPORT?

Actually, no export is needed. Removed.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_properti...
ash/mus/window_properties.h:18: AURA_EXPORT extern const
aura::WindowProperty<bool>* const kWindowIsJanky;
On 2016/12/05 19:21:45, James Cook wrote:
> nit: Document or point to mojo property it maps to.

Done.

[sky, 2016-12-05 21:39:32]

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

[commit-bot: I haz the power, 2016-12-05 21:40:10]

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-12-05 22:35:37]

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

[commit-bot: I haz the power, 2016-12-05 22:35:38]

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

[James Cook, 2016-12-05 22:56:34]

LGTM with nits. Epic patch! :-)

Not for this CL, but I think you should also drop a README.md file in
ui/aura/mus that briefly explains the aura-mus architecture.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:120: gfx::Size WmWindowMus::GetMinimumSize()
const {
On 2016/12/05 21:39:19, sky wrote:
> On 2016/12/05 19:21:44, James Cook wrote:
> > Q: Will methods like this eventually be replaced with the WmWindowAura
> version?
> > 
> > If not, it might be worth a comment at class level about why some of these
> > methods have to be different.
> > 
> > If so, maybe a TODO somewhere?
> 
> I would lump this with the bug I filed about figuring out the right way to
> structure the porting layer. Really I want a TODO at the directory level,
which
> we don't really have. I'm not going to put a TODO as I think the TODO would be
> needed in all these classes, so for now just the bug. Does that seem
reasonable?

I suggest sticking a README file in this directory, then. It could be one
sentence about what the directory contains now, a note that you're refactoring
it, and a link to the bug.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.h:31: class WmWindowMus : public WmWindowAura {
On 2016/12/05 17:56:55, sky wrote:
> On 2016/12/05 17:21:49, James Cook wrote:
> > Q: What's the plan for cleanup here? It's super-confusing that there's now
> > WmWindow, aura::Window, WmWindowAura, and WmWindowMus which is-a
WmWindowAura.
> > Do WmWindow and WmWindowAura get collapsed together? Do we eventually get
back
> > to a state where everything is aura::Window?
> 
> Unfortunately I don't have a good answer for that now. This patch is about
> moving away from ui::Window, without much thought into the right long term
> structure. After this I plan to better understand exactly what needs to differ
> between classic aura and aura-mus. For example, widget lookup will always be
> different in the two environments, as will be a number of other things. So,
> there still needs to be a way to isolate these differences, but not to the
same
> degree that we have now.

OK. I strongly think we need a cleanup plan for our multiple *Window types and
the split between ash/common and ash/non-common. Otherwise we're leaving ash/ in
a pretty confusing state, especially for the people working in classic ash
during our transition.

This feels to me like it could use a 1-2 page design doc once the aura-mus stuff
is a little further along. Once we have a plan I can help convert things.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:205: ui::SHOW_STATE_FULLSCREEN) {
On 2016/12/05 21:39:19, sky wrote:
> On 2016/12/05 19:21:44, James Cook wrote:
> > nit: This if() is ugly. Would it help to cache property_converter() in a
> local,
> > or use "using ui::mojom::WindowManager"?
> 
> Well, I made this function easier to parse, but I'm not sure if I just pushed
> the unreadability around. Any better?

Better, one other idea above.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h
File ash/mus/screen_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h#ne...
ash/mus/screen_mus.h:13: class ScreenMus : public display::ScreenBase {
On 2016/12/05 21:39:20, sky wrote:
> On 2016/12/05 19:21:45, James Cook wrote:
> > nit: class comment please
> 
> Done.

Not to bikeshed on docs, but the comment would be better if it said briefly why
mus needs its own impl. I presume the answer is that display management is
different.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:167:
properties->erase(ui::mojom::WindowManager::kWindowIgnoredByShelf_Property);
On 2016/12/05 21:39:20, sky wrote:
> On 2016/12/05 19:21:45, James Cook wrote:
> > I think there should be a central place where we document which properties
are
> > vs. aren't persisted.  WDYT of documenting this in the window_manager.mojom
> > file?
> 
> Definitely. In fact Mike asked for the same thing. In hopes of not growing
this
> patch I was planning on updating window_manager.mojom after this. Sound good?

Updating window_manager.mojom separately SGTM

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:299: DCHECK((parent && !context) ||
(!parent && context));
Can both parent and context be non-null at the same time?

I vaguely recall that being valid in the aura::Window / Widget code, but I'm not
sure.

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.h (right):

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.h:49: // is the parent to place the newly
created window in, and my be null. If
nit: my -> may

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/property_util.h
File ash/mus/property_util.h (right):

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/property_util.h...
ash/mus/property_util.h:28: // Long lived properties are converted and stored as
properties on the
super-nit: empty line above or re-wrap

Nice docs, btw.

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:55: return
property_converter->GetPropertyValueFromTransportValue(
if (!property_converter->....)
  return false;
return static_cast<> == ui::SHOW_STATE_FULLSCREEN;

might be even a bit nicer

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/window_properti...
File ash/mus/window_properties.cc (right):

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/window_properti...
ash/mus/window_properties.cc:15: //
WindowManagerDelegate::OnWmClientJankinessChanged()).
Either this should go in the header, or the other one should go in the .cc file.

[sky, 2016-12-06 00:31:08]

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

[commit-bot: I haz the power, 2016-12-06 00:31:47]

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

[sky, 2016-12-06 00:55:04]

Description was changed from

==========
Converts ash to use aura-mus

There is still a bunch of cleanup and removal to do, but this patch is
already so big I'm hoping to keep more cleanup to future patches. The
biggest remaining issue after this patch is z-order. By that I mean
the non-client area is at a different z-order than the frame, which
leads to some interesting artifacts. That will be addressed soon, and
I'm hoping won't block other testing.

I've landed as many other patches as I could, so this is mostly just
changes to aura, but there are some properties that needed to be
changed. I could separate that out if you prefer.

BUG=669964
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Converts ash to use aura-mus

There is still a bunch of cleanup and removal to do, but this patch is
already so big I'm hoping to keep more cleanup to future patches. The
biggest remaining issue after this patch is z-order. By that I mean
the non-client area is at a different z-order than the frame, which
leads to some interesting artifacts. That will be addressed soon, and
I'm hoping won't block other testing.

I've landed as many other patches as I could, so this is mostly just
changes to aura, but there are some properties that needed to be
changed. I could separate that out if you prefer.

BUG=669964
TEST=covered by tests
R=jamescook@chromium.org
==========

[sky, 2016-12-06 00:55:04]

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

[sky, 2016-12-06 00:56:16]

My patch started tripping over a bug in how overlays are setup. I added a flag
to the browser_tests as suggested by Ken. I'm TBRing Ken for that change (to
testing/buildbot)

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:120: gfx::Size WmWindowMus::GetMinimumSize()
const {
On 2016/12/05 22:56:34, James Cook wrote:
> On 2016/12/05 21:39:19, sky wrote:
> > On 2016/12/05 19:21:44, James Cook wrote:
> > > Q: Will methods like this eventually be replaced with the WmWindowAura
> > version?
> > > 
> > > If not, it might be worth a comment at class level about why some of these
> > > methods have to be different.
> > > 
> > > If so, maybe a TODO somewhere?
> > 
> > I would lump this with the bug I filed about figuring out the right way to
> > structure the porting layer. Really I want a TODO at the directory level,
> which
> > we don't really have. I'm not going to put a TODO as I think the TODO would
be
> > needed in all these classes, so for now just the bug. Does that seem
> reasonable?
> 
> I suggest sticking a README file in this directory, then. It could be one
> sentence about what the directory contains now, a note that you're refactoring
> it, and a link to the bug.

Done.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.h:31: class WmWindowMus : public WmWindowAura {
On 2016/12/05 22:56:34, James Cook wrote:
> On 2016/12/05 17:56:55, sky wrote:
> > On 2016/12/05 17:21:49, James Cook wrote:
> > > Q: What's the plan for cleanup here? It's super-confusing that there's now
> > > WmWindow, aura::Window, WmWindowAura, and WmWindowMus which is-a
> WmWindowAura.
> > > Do WmWindow and WmWindowAura get collapsed together? Do we eventually get
> back
> > > to a state where everything is aura::Window?
> > 
> > Unfortunately I don't have a good answer for that now. This patch is about
> > moving away from ui::Window, without much thought into the right long term
> > structure. After this I plan to better understand exactly what needs to
differ
> > between classic aura and aura-mus. For example, widget lookup will always be
> > different in the two environments, as will be a number of other things. So,
> > there still needs to be a way to isolate these differences, but not to the
> same
> > degree that we have now.
> 
> OK. I strongly think we need a cleanup plan for our multiple *Window types and
> the split between ash/common and ash/non-common. Otherwise we're leaving ash/
in
> a pretty confusing state, especially for the people working in classic ash
> during our transition.
> 
> This feels to me like it could use a 1-2 page design doc once the aura-mus
stuff
> is a little further along. Once we have a plan I can help convert things.

Agreed on the confusion and thanks for the offer of help. Straightening all this
out is my top priority.

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h
File ash/mus/screen_mus.h (right):

https://codereview.chromium.org/2539363005/diff/40001/ash/mus/screen_mus.h#ne...
ash/mus/screen_mus.h:13: class ScreenMus : public display::ScreenBase {
On 2016/12/05 22:56:34, James Cook wrote:
> On 2016/12/05 21:39:20, sky wrote:
> > On 2016/12/05 19:21:45, James Cook wrote:
> > > nit: class comment please
> > 
> > Done.
> 
> Not to bikeshed on docs, but the comment would be better if it said briefly
why
> mus needs its own impl. I presume the answer is that display management is
> different.

Done.

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:299: DCHECK((parent && !context) ||
(!parent && context));
On 2016/12/05 22:56:34, James Cook wrote:
> Can both parent and context be non-null at the same time?
> 
> I vaguely recall that being valid in the aura::Window / Widget code, but I'm
not
> sure.

Not for NativeWidgetAura.

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.h (right):

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.h:49: // is the parent to place the newly
created window in, and my be null. If
On 2016/12/05 22:56:34, James Cook wrote:
> nit: my -> may

Done.

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:55: return
property_converter->GetPropertyValueFromTransportValue(
On 2016/12/05 22:56:34, James Cook wrote:
> if (!property_converter->....)
>   return false;
> return static_cast<> == ui::SHOW_STATE_FULLSCREEN;
> 
> might be even a bit nicer

I thought if that as well, but I think which you like is more a personal
preference. So, I'm leaving this.

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/window_properti...
File ash/mus/window_properties.cc (right):

https://codereview.chromium.org/2539363005/diff/80001/ash/mus/window_properti...
ash/mus/window_properties.cc:15: //
WindowManagerDelegate::OnWmClientJankinessChanged()).
On 2016/12/05 22:56:34, James Cook wrote:
> Either this should go in the header, or the other one should go in the .cc
file.

Done.

[sky, 2016-12-06 00:56:34]

Description was changed from

==========
Converts ash to use aura-mus

There is still a bunch of cleanup and removal to do, but this patch is
already so big I'm hoping to keep more cleanup to future patches. The
biggest remaining issue after this patch is z-order. By that I mean
the non-client area is at a different z-order than the frame, which
leads to some interesting artifacts. That will be addressed soon, and
I'm hoping won't block other testing.

I've landed as many other patches as I could, so this is mostly just
changes to aura, but there are some properties that needed to be
changed. I could separate that out if you prefer.

BUG=669964
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Converts ash to use aura-mus

There is still a bunch of cleanup and removal to do, but this patch is
already so big I'm hoping to keep more cleanup to future patches. The
biggest remaining issue after this patch is z-order. By that I mean
the non-client area is at a different z-order than the frame, which
leads to some interesting artifacts. That will be addressed soon, and
I'm hoping won't block other testing.

I've landed as many other patches as I could, so this is mostly just
changes to aura, but there are some properties that needed to be
changed. I could separate that out if you prefer.

BUG=669964
TEST=covered by tests
R=jamescook@chromium.org
TBR=rockot@chromium.org (for changes to testing/buildbot)
==========

[sky, 2016-12-06 00:56:41]

The CQ bit was checked by sky@chromium.org

[sky, 2016-12-06 00:56:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2539363005/#ps140001
(title: "add overrides")

[commit-bot: I haz the power, 2016-12-06 00:57:07]

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-06 01:43:11]

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

[commit-bot: I haz the power, 2016-12-06 01:43:13]

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

[sky, 2016-12-06 02:39:12]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-12-06 02:39:50]

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-06 03:53:57]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480991952220870,
"parent_rev": "56f14f8942fc57532fed4697dfbc9bc7c0401fdc", "commit_rev":
"61da4472fede382d846c250648b3b18152eaa121"}

[commit-bot: I haz the power, 2016-12-06 03:54:29]

Description was changed from

==========
Converts ash to use aura-mus

There is still a bunch of cleanup and removal to do, but this patch is
already so big I'm hoping to keep more cleanup to future patches. The
biggest remaining issue after this patch is z-order. By that I mean
the non-client area is at a different z-order than the frame, which
leads to some interesting artifacts. That will be addressed soon, and
I'm hoping won't block other testing.

I've landed as many other patches as I could, so this is mostly just
changes to aura, but there are some properties that needed to be
changed. I could separate that out if you prefer.

BUG=669964
TEST=covered by tests
R=jamescook@chromium.org
TBR=rockot@chromium.org (for changes to testing/buildbot)
==========

to

==========
Converts ash to use aura-mus

There is still a bunch of cleanup and removal to do, but this patch is
already so big I'm hoping to keep more cleanup to future patches. The
biggest remaining issue after this patch is z-order. By that I mean
the non-client area is at a different z-order than the frame, which
leads to some interesting artifacts. That will be addressed soon, and
I'm hoping won't block other testing.

I've landed as many other patches as I could, so this is mostly just
changes to aura, but there are some properties that needed to be
changed. I could separate that out if you prefer.

BUG=669964
TEST=covered by tests
R=jamescook@chromium.org
TBR=rockot@chromium.org (for changes to testing/buildbot)
==========

[commit-bot: I haz the power, 2016-12-06 03:54:31]

Committed patchset #8 (id:140001)

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

Description was changed from

==========
Converts ash to use aura-mus

There is still a bunch of cleanup and removal to do, but this patch is
already so big I'm hoping to keep more cleanup to future patches. The
biggest remaining issue after this patch is z-order. By that I mean
the non-client area is at a different z-order than the frame, which
leads to some interesting artifacts. That will be addressed soon, and
I'm hoping won't block other testing.

I've landed as many other patches as I could, so this is mostly just
changes to aura, but there are some properties that needed to be
changed. I could separate that out if you prefer.

BUG=669964
TEST=covered by tests
R=jamescook@chromium.org
TBR=rockot@chromium.org (for changes to testing/buildbot)
==========

to

==========
Converts ash to use aura-mus

There is still a bunch of cleanup and removal to do, but this patch is
already so big I'm hoping to keep more cleanup to future patches. The
biggest remaining issue after this patch is z-order. By that I mean
the non-client area is at a different z-order than the frame, which
leads to some interesting artifacts. That will be addressed soon, and
I'm hoping won't block other testing.

I've landed as many other patches as I could, so this is mostly just
changes to aura, but there are some properties that needed to be
changed. I could separate that out if you prefer.

BUG=669964
TEST=covered by tests
R=jamescook@chromium.org
TBR=rockot@chromium.org (for changes to testing/buildbot)

Committed: https://crrev.com/fcf18b528bc1908b9e50af6ff914b0049e693fbf
Cr-Commit-Position: refs/heads/master@{#436515}
==========

[commit-bot: I haz the power, 2016-12-06 03:56:57]

Patchset 8 (id:??) landed as
https://crrev.com/fcf18b528bc1908b9e50af6ff914b0049e693fbf
Cr-Commit-Position: refs/heads/master@{#436515}