MacViews: Fix BrowserClient for toolkit-views on Mac

MacViews: Fix BrowserClient for toolkit-views on Mac

wm::WMState isn't available or required on Mac.

This guards the wm::WMState in ChromeBrowserMainExtraPartsViews with an
#if defined(USE_AURA).

BUG=390755
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284325

[tapted, 2014-07-17 13:55:05]

Hi sky, please take a look

https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:741: #endif
Moved this up because Ash does stuff like `OpenAsh` which I think needs the
WMState. This also seems nicer from a dependency perspective (i.e. do Aura
first, since views and ash depend on it).

[sky, 2014-07-17 16:12:13]

https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:741: #endif
On 2014/07/17 13:55:04, tapted wrote:
> Moved this up because Ash does stuff like `OpenAsh` which I think needs the
> WMState. This also seems nicer from a dependency perspective (i.e. do Aura
> first, since views and ash depend on it).

I this code would be cleaner if we keep ChromeBrowserMainExtraPartsAura as
*just* aura, eg no views. ChromeBrowserMainExtraPartsViews inits views and any
aura-views dependencies. This does imply a bit of shuffling of this code. WDYT?

[tapted, 2014-07-17 23:08:17]

On 2014/07/17 16:12:13, sky wrote:
>
https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
> chrome/browser/chrome_content_browser_client.cc:741: #endif
> On 2014/07/17 13:55:04, tapted wrote:
> > Moved this up because Ash does stuff like `OpenAsh` which I think needs the
> > WMState. This also seems nicer from a dependency perspective (i.e. do Aura
> > first, since views and ash depend on it).
> 
> I this code would be cleaner if we keep ChromeBrowserMainExtraPartsAura as
> *just* aura, eg no views. ChromeBrowserMainExtraPartsViews inits views and any
> aura-views dependencies. This does imply a bit of shuffling of this code.
WDYT?

[tapted, 2014-07-17 23:12:50]

On 2014/07/17 16:12:13, sky wrote:
>
https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
> chrome/browser/chrome_content_browser_client.cc:741: #endif
> On 2014/07/17 13:55:04, tapted wrote:
> > Moved this up because Ash does stuff like `OpenAsh` which I think needs the
> > WMState. This also seems nicer from a dependency perspective (i.e. do Aura
> > first, since views and ash depend on it).
> 
> I this code would be cleaner if we keep ChromeBrowserMainExtraPartsAura as
> *just* aura, eg no views. ChromeBrowserMainExtraPartsViews inits views and any
> aura-views dependencies. This does imply a bit of shuffling of this code.
WDYT?

(oops - mystery Rietveld click submitted an empty reply - sorry)

I've been thinking of of ui/wm as part of Aura.. (But I think aura grew from
initially just a window manager to much more). 

There's a different approach in the CL I'm splitting these off from -
https://codereview.chromium.org/375843002/ . Basically make a
chrome_browser_main_extra_parts_views_aura and a _views_mac - was that what you
had in mind?

Of course chrome_browser_main_extra_parts_views is pretty simple right now -- An
#ifdef to avoid the WMState in the header and .cc on Mac might be OK at this
stage. (either USE_AURA or !OS_MACOSX). Let me know if that's acceptable and
i'll update this CL here.

[sky, 2014-07-17 23:22:54]

On Thu, Jul 17, 2014 at 4:12 PM,  <tapted@chromium.org> wrote:
> On 2014/07/17 16:12:13, sky wrote:
>
>
https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
>>
>> File chrome/browser/chrome_content_browser_client.cc (right):
>
>
>
>
https://codereview.chromium.org/400543002/diff/20001/chrome/browser/chrome_co...
>>
>> chrome/browser/chrome_content_browser_client.cc:741: #endif
>> On 2014/07/17 13:55:04, tapted wrote:
>> > Moved this up because Ash does stuff like `OpenAsh` which I think needs
>> > the
>> > WMState. This also seems nicer from a dependency perspective (i.e. do
>> > Aura
>> > first, since views and ash depend on it).
>
>
>> I this code would be cleaner if we keep ChromeBrowserMainExtraPartsAura as
>> *just* aura, eg no views. ChromeBrowserMainExtraPartsViews inits views and
>> any
>> aura-views dependencies. This does imply a bit of shuffling of this code.
>
> WDYT?
>
> (oops - mystery Rietveld click submitted an empty reply - sorry)
>
> I've been thinking of of ui/wm as part of Aura.. (But I think aura grew from
> initially just a window manager to much more).
>
> There's a different approach in the CL I'm splitting these off from -
> https://codereview.chromium.org/375843002/ . Basically make a
> chrome_browser_main_extra_parts_views_aura and a _views_mac - was that what
> you
> had in mind?

I hadn't considered a views_mac yet, but if the views only results in
too many ifdefs, go for it.

> Of course chrome_browser_main_extra_parts_views is pretty simple right now
> -- An
> #ifdef to avoid the WMState in the header and .cc on Mac might be OK at this
> stage. (either USE_AURA or !OS_MACOSX). Let me know if that's acceptable and
> i'll update this CL here.

I'm ok with a minimal set of ifdefs. If it becomes too much, we can
split the files.

  -Scott

>
>
>
> https://codereview.chromium.org/400543002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tapted, 2014-07-18 00:08:40]

On 2014/07/17 23:22:54, sky wrote:
> On Thu, Jul 17, 2014 at 4:12 PM,  <mailto:tapted@chromium.org> wrote:
> > Of course chrome_browser_main_extra_parts_views is pretty simple right now
> > -- An
> > #ifdef to avoid the WMState in the header and .cc on Mac might be OK at this
> > stage. (either USE_AURA or !OS_MACOSX). Let me know if that's acceptable and
> > i'll update this CL here.
> 
> I'm ok with a minimal set of ifdefs.

Done.

[sky, 2014-07-18 15:34:09]

LGTM

[tapted, 2014-07-19 05:04:07]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-07-19 05:05:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/400543002/80001

[commit-bot: I haz the power, 2014-07-19 06:47:55]

Change committed as 284325