Implement a MusBrowserFrame.

Implement a MusBrowserFrame.

Connects to the window manager during startup, and adds a MusBrowserFrame impl to the NativeBrowserFrame factories.

R=sky@chromium.org
http://crbug.com/548694

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

[Ben Goodger (Google), 2015-11-05 21:57:04]

Description was changed from

==========
Implement a MusBrowserFrame.

Connects to the window manager during startup, and adds a MusBrowserFrame impl
to the NativeBrowserFrame factories.

R=sky@chromium.org
http://crbug.com/548694
==========

to

==========
Implement a MusBrowserFrame.

Connects to the window manager during startup, and adds a MusBrowserFrame impl
to the NativeBrowserFrame factories.

R=sky@chromium.org
http://crbug.com/548694
==========

[sky, 2015-11-06 17:07:46]

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:580: "//components/mus/public/interfaces",
Seems like this will effect the size of the official builds. Should we only
include this if a compile time switch is set?

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/mojo_run...
File chrome/browser/mojo_runner_state.cc (right):

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/mojo_run...
chrome/browser/mojo_runner_state.cc:21: mus::mojom::WindowManagerPtr
window_manager;
This looks weird since you drop the WindowManagerPtr on the floor. Add a comment
as to why it's necessary.

Also, you'll need a data dep on the example for this, which feels equally weird.
We should really move the code to ash/mus.

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/mus_browser_frame.cc (right):

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/mus_browser_frame.cc:30: bool
MusBrowserFrame::UseCustomFrame() const {
nit: more space!

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/mus_browser_frame.h (right):

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/mus_browser_frame.h:12: class MusBrowserFrame :
public NativeBrowserFrame,
Should this be BrowserFrameMus? That seems more consistent with other names
we've gone with.

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

https://codereview.chromium.org/1416373010/diff/40001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:72: class NativeWidgetMusWindowTreeClient :
public aura::client::WindowTreeClient {
Wow, I hadn't realized we have aura::client::WindowTreeClient and
mus::mojom::WindowTreeClient...

https://codereview.chromium.org/1416373010/diff/40001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:79:
aura::client::SetWindowTreeClient(root_window_, NULL);
nit: nullptr.

[Ben Goodger (Google), 2015-11-09 20:23:14]

working on fixes, but here are my interim comments...

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/mojo_run...
File chrome/browser/mojo_runner_state.cc (right):

https://codereview.chromium.org/1416373010/diff/40001/chrome/browser/mojo_run...
chrome/browser/mojo_runner_state.cc:21: mus::mojom::WindowManagerPtr
window_manager;
On 2015/11/06 17:07:46, sky wrote:
> This looks weird since you drop the WindowManagerPtr on the floor. Add a
comment
> as to why it's necessary.

I don't drop it on the floor, I .Pass() it to WindowManagerConnection::Create().

> Also, you'll need a data dep on the example for this, which feels equally
weird.
> We should really move the code to ash/mus.

I agree the dep is weird. I wonder if an application should obtain the window
manager interface from the WindowTree interface. It would be up to the init app
to register a window manager(s).

[sky, 2015-11-09 21:43:49]

LGTM

[Ben Goodger (Google), 2015-11-09 22:20:27]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-11-09 22:20:27]

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/1416373010/#ps80001
(title: ".")

[commit-bot: I haz the power, 2015-11-09 22:22:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416373010/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416373010/80001

[commit-bot: I haz the power, 2015-11-09 23:11:26]

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

[commit-bot: I haz the power, 2015-11-09 23:11:27]

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

[Ben Goodger (Google), 2015-11-10 05:49:21]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-11-10 05:49: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/1416373010/#ps120001
(title: ".")

[commit-bot: I haz the power, 2015-11-10 05:49:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416373010/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416373010/120001

[commit-bot: I haz the power, 2015-11-10 06:13:50]

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

[commit-bot: I haz the power, 2015-11-10 06:13:51]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Ben Goodger (Google), 2015-11-10 06:25:58]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-11-10 06:25:58]

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/1416373010/#ps160001
(title: ".")

[commit-bot: I haz the power, 2015-11-10 06:26:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416373010/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416373010/160001

[commit-bot: I haz the power, 2015-11-10 07:56:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-11-10 07:57:04]

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