views/mus: Have explicit ownership of views::WindowManagerConnection.

views/mus: Have explicit ownership of views::WindowManagerConnection.

TBR=ben@chromium.org for API call update in //services/navigation
BUG=none

Committed: https://crrev.com/4b04c1d22f4188d242dc39b982b091329102f636
Cr-Commit-Position: refs/heads/master@{#398343}

[sadrul, 2016-05-30 23:12:36]

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

[sadrul, 2016-05-30 23:12:37]

[includes https://codereview.chromium.org/2023933002/ and
https://codereview.chromium.org/2023863003/ too, but I would land this
separately]

[sky, 2016-05-31 18:57:25]

LGTM with views_mus_->window_manager_connection_

https://codereview.chromium.org/2026623002/diff/1/ash/sysui/sysui_application.cc
File ash/sysui/sysui_application.cc (right):

https://codereview.chromium.org/2026623002/diff/1/ash/sysui/sysui_application...
ash/sysui/sysui_application.cc:306:
std::unique_ptr<views::WindowManagerConnection> views_mus_;
views_mus_->window_manager_connection_

https://codereview.chromium.org/2026623002/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2026623002/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.h:289:
std::unique_ptr<views::WindowManagerConnection> views_mus_;
window_manager_connection_

https://codereview.chromium.org/2026623002/diff/1/mash/browser/browser.h
File mash/browser/browser.h (right):

https://codereview.chromium.org/2026623002/diff/1/mash/browser/browser.h#newc...
mash/browser/browser.h:58: std::unique_ptr<views::WindowManagerConnection>
views_mus_;
ditto

https://codereview.chromium.org/2026623002/diff/1/mash/catalog_viewer/catalog...
File mash/catalog_viewer/catalog_viewer.h (right):

https://codereview.chromium.org/2026623002/diff/1/mash/catalog_viewer/catalog...
mash/catalog_viewer/catalog_viewer.h:57:
std::unique_ptr<views::WindowManagerConnection> views_mus_;
ditto

https://codereview.chromium.org/2026623002/diff/1/mash/example/views_examples...
File mash/example/views_examples/views_examples_application_delegate.h (right):

https://codereview.chromium.org/2026623002/diff/1/mash/example/views_examples...
mash/example/views_examples/views_examples_application_delegate.h:47:
std::unique_ptr<views::WindowManagerConnection> views_mus_;
again

https://codereview.chromium.org/2026623002/diff/1/mash/example/window_type_la...
File mash/example/window_type_launcher/window_type_launcher.h (right):

https://codereview.chromium.org/2026623002/diff/1/mash/example/window_type_la...
mash/example/window_type_launcher/window_type_launcher.h:50:
std::unique_ptr<views::WindowManagerConnection> views_mus_;
moar

https://codereview.chromium.org/2026623002/diff/1/mash/login/login.cc
File mash/login/login.cc (right):

https://codereview.chromium.org/2026623002/diff/1/mash/login/login.cc#newcode129
mash/login/login.cc:129: std::unique_ptr<views::WindowManagerConnection>
views_mus_;
mooorrrr

https://codereview.chromium.org/2026623002/diff/1/mash/quick_launch/quick_lau...
File mash/quick_launch/quick_launch_application.h (right):

https://codereview.chromium.org/2026623002/diff/1/mash/quick_launch/quick_lau...
mash/quick_launch/quick_launch_application.h:55:
std::unique_ptr<views::WindowManagerConnection> views_mus_;
x

[sadrul, 2016-05-31 20:02:44]

Renamed all |views_mus_| to |window_manager_connection_|

https://codereview.chromium.org/2026623002/diff/1/ash/sysui/sysui_application.cc
File ash/sysui/sysui_application.cc (right):

https://codereview.chromium.org/2026623002/diff/1/ash/sysui/sysui_application...
ash/sysui/sysui_application.cc:306:
std::unique_ptr<views::WindowManagerConnection> views_mus_;
On 2016/05/31 18:57:24, sky wrote:
> views_mus_->window_manager_connection_

Done.

[sadrul, 2016-05-31 23:18:59]

sadrul@chromium.org changed reviewers:
+ ben@chromium.org

[sadrul, 2016-05-31 23:19:00]

+ben@ for content/browser/ change

[Ben Goodger (Google), 2016-05-31 23:23:39]

https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.h:67: #if defined(MOJO_SHELL_CLIENT) &&
defined(USE_AURA)
Are you sure MOJO_SHELL_CLIENT is defined in all the places that #include this
header?

[sadrul, 2016-05-31 23:43:51]

https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.h:67: #if defined(MOJO_SHELL_CLIENT) &&
defined(USE_AURA)
On 2016/05/31 23:23:39, Ben Goodger (Google) wrote:
> Are you sure MOJO_SHELL_CLIENT is defined in all the places that #include this
> header?

Oh, nice catch! I hadn't thought about that.

Looks like browser_main_loop.h is included from code only in //content/browser,
and one file outside of that: //content/public/browser/media_device_id.cc. For
code in //content/browser/, MOJO_SHELL_CLIENT is defined. But it wasn't defined
for media_device_id.cc. I have added that define (through gn config
//content/public/common:mojo_shell_client). That should take care of that. PTAL

[Ben Goodger (Google), 2016-06-01 01:52:14]

On 2016/05/31 23:43:51, sadrul wrote:
>
https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
> File content/browser/browser_main_loop.h (right):
> 
>
https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
> content/browser/browser_main_loop.h:67: #if defined(MOJO_SHELL_CLIENT) &&
> defined(USE_AURA)
> On 2016/05/31 23:23:39, Ben Goodger (Google) wrote:
> > Are you sure MOJO_SHELL_CLIENT is defined in all the places that #include
this
> > header?
> 
> Oh, nice catch! I hadn't thought about that.
> 
> Looks like browser_main_loop.h is included from code only in
//content/browser,
> and one file outside of that: //content/public/browser/media_device_id.cc. For
> code in //content/browser/, MOJO_SHELL_CLIENT is defined. But it wasn't
defined
> for media_device_id.cc. I have added that define (through gn config
> //content/public/common:mojo_shell_client). That should take care of that.
PTAL

The problem with this is that if someone else ever #includes
browser_main_loop.h, they're in for some really interesting crashes that will be
nigh on inexplicable.

Can't you just forward declare this class & add this member for everyone? That's
what I've done elsewhere in similar circumstances.

[sadrul, 2016-06-01 01:55:20]

On 2016/06/01 01:52:14, Ben Goodger (Google) wrote:
> On 2016/05/31 23:43:51, sadrul wrote:
> >
>
https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
> > File content/browser/browser_main_loop.h (right):
> > 
> >
>
https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
> > content/browser/browser_main_loop.h:67: #if defined(MOJO_SHELL_CLIENT) &&
> > defined(USE_AURA)
> > On 2016/05/31 23:23:39, Ben Goodger (Google) wrote:
> > > Are you sure MOJO_SHELL_CLIENT is defined in all the places that #include
> this
> > > header?
> > 
> > Oh, nice catch! I hadn't thought about that.
> > 
> > Looks like browser_main_loop.h is included from code only in
> //content/browser,
> > and one file outside of that: //content/public/browser/media_device_id.cc.
For
> > code in //content/browser/, MOJO_SHELL_CLIENT is defined. But it wasn't
> defined
> > for media_device_id.cc. I have added that define (through gn config
> > //content/public/common:mojo_shell_client). That should take care of that.
> PTAL
> 
> The problem with this is that if someone else ever #includes
> browser_main_loop.h, they're in for some really interesting crashes that will
be
> nigh on inexplicable.
> 
> Can't you just forward declare this class & add this member for everyone?
That's
> what I've done elsewhere in similar circumstances.

OK. Let me try that (although I think that will require to link against
~WindowManagerConnection() on all platforms as well?)

[Ben Goodger (Google), 2016-06-01 02:05:53]

On 2016/06/01 01:55:20, sadrul wrote:
> On 2016/06/01 01:52:14, Ben Goodger (Google) wrote:
> > On 2016/05/31 23:43:51, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
> > > File content/browser/browser_main_loop.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
> > > content/browser/browser_main_loop.h:67: #if defined(MOJO_SHELL_CLIENT) &&
> > > defined(USE_AURA)
> > > On 2016/05/31 23:23:39, Ben Goodger (Google) wrote:
> > > > Are you sure MOJO_SHELL_CLIENT is defined in all the places that
#include
> > this
> > > > header?
> > > 
> > > Oh, nice catch! I hadn't thought about that.
> > > 
> > > Looks like browser_main_loop.h is included from code only in
> > //content/browser,
> > > and one file outside of that: //content/public/browser/media_device_id.cc.
> For
> > > code in //content/browser/, MOJO_SHELL_CLIENT is defined. But it wasn't
> > defined
> > > for media_device_id.cc. I have added that define (through gn config
> > > //content/public/common:mojo_shell_client). That should take care of that.
> > PTAL
> > 
> > The problem with this is that if someone else ever #includes
> > browser_main_loop.h, they're in for some really interesting crashes that
will
> be
> > nigh on inexplicable.
> > 
> > Can't you just forward declare this class & add this member for everyone?
> That's
> > what I've done elsewhere in similar circumstances.
> 
> OK. Let me try that (although I think that will require to link against
> ~WindowManagerConnection() on all platforms as well?)

Oh annoying. Can you define it in the .cc file if !defined(MOJO_SHELL_CLIENT)?

[sadrul, 2016-06-06 20:23:38]

On 2016/06/01 02:05:53, Ben Goodger (Google) wrote:
> On 2016/06/01 01:55:20, sadrul wrote:
> > On 2016/06/01 01:52:14, Ben Goodger (Google) wrote:
> > > On 2016/05/31 23:43:51, sadrul wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
> > > > File content/browser/browser_main_loop.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2026623002/diff/80001/content/browser/browser...
> > > > content/browser/browser_main_loop.h:67: #if defined(MOJO_SHELL_CLIENT)
&&
> > > > defined(USE_AURA)
> > > > On 2016/05/31 23:23:39, Ben Goodger (Google) wrote:
> > > > > Are you sure MOJO_SHELL_CLIENT is defined in all the places that
> #include
> > > this
> > > > > header?
> > > > 
> > > > Oh, nice catch! I hadn't thought about that.
> > > > 
> > > > Looks like browser_main_loop.h is included from code only in
> > > //content/browser,
> > > > and one file outside of that:
//content/public/browser/media_device_id.cc.
> > For
> > > > code in //content/browser/, MOJO_SHELL_CLIENT is defined. But it wasn't
> > > defined
> > > > for media_device_id.cc. I have added that define (through gn config
> > > > //content/public/common:mojo_shell_client). That should take care of
that.
> > > PTAL
> > > 
> > > The problem with this is that if someone else ever #includes
> > > browser_main_loop.h, they're in for some really interesting crashes that
> will
> > be
> > > nigh on inexplicable.
> > > 
> > > Can't you just forward declare this class & add this member for everyone?
> > That's
> > > what I've done elsewhere in similar circumstances.
> > 
> > OK. Let me try that (although I think that will require to link against
> > ~WindowManagerConnection() on all platforms as well?)
> 
> Oh annoying. Can you define it in the .cc file if !defined(MOJO_SHELL_CLIENT)?

Updated this with the change in https://codereview.chromium.org/2027093002/ .
PTAL

[sky, 2016-06-06 23:15:26]

LGTM

[sadrul, 2016-06-07 16:46:47]

Description was changed from

==========
views/mus: Have explicit ownership of views::WindowManagerConnection.

BUG=none
==========

to

==========
views/mus: Have explicit ownership of views::WindowManagerConnection.

TBR=ben@chromium.org for API call update in //services/navigation
BUG=none
==========

[sadrul, 2016-06-07 17:32:37]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-06-07 17:32:38]

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/2026623002/#ps180001
(title: "tot-merge")

[commit-bot: I haz the power, 2016-06-07 17:33:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2026623002/180001

[commit-bot: I haz the power, 2016-06-07 18:15:11]

Description was changed from

==========
views/mus: Have explicit ownership of views::WindowManagerConnection.

TBR=ben@chromium.org for API call update in //services/navigation
BUG=none
==========

to

==========
views/mus: Have explicit ownership of views::WindowManagerConnection.

TBR=ben@chromium.org for API call update in //services/navigation
BUG=none
==========

[commit-bot: I haz the power, 2016-06-07 18:15:12]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-07 18:17:07]

Description was changed from

==========
views/mus: Have explicit ownership of views::WindowManagerConnection.

TBR=ben@chromium.org for API call update in //services/navigation
BUG=none
==========

to

==========
views/mus: Have explicit ownership of views::WindowManagerConnection.

TBR=ben@chromium.org for API call update in //services/navigation
BUG=none

Committed: https://crrev.com/4b04c1d22f4188d242dc39b982b091329102f636
Cr-Commit-Position: refs/heads/master@{#398343}
==========

[commit-bot: I haz the power, 2016-06-07 18:17:09]

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