MacViews: Adds NativeWidgetMac and gets views target compiling on Mac.

MacViews: Adds NativeWidgetMac and gets views target compiling on Mac.

This is a minimal amount to get the `views` target compiling on Mac. It
adds an implementation of NativeWidgetMac that spews NOTIMPLEMENTED().
So, nothing works yet, but this gives an FYI builder something to chew
on to catch regressions in things ported so far.

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

[tapted, 2014-05-21 09:40:28]

Hi Andre - could you do an initial pass? It's so close now!

Let me know if you have something locally for the ui::GestureRecognizer::Get()
missing symbol. After poking it for a while, I'm not seeing an elegant fix.

It's used in a few places under content and chrome/browser/ui, so the right
thing to do might just be to put together a dummy implementation on mac for
non-aura. I think I'll need to investigate a bit more and get that landed before
sending this to ben..

https://codereview.chromium.org/249113002/diff/90001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/249113002/diff/90001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:9: #include
"ui/events/gestures/gesture_recognizer.h"
would need to guard this too (but that breaks apply_patch right now)

https://codereview.chromium.org/249113002/diff/90001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:77: #endif
This is to resolve

Undefined symbols for architecture i386:
  "ui::GestureRecognizer::Get()", referenced from:
      views::MenuHost::ShowMenuHost(bool) in views.menu_host.o

I wonder if we can just move this into NativeWidgetAura::SetCapture(), or give
that an argument..

[Andre, 2014-05-21 17:13:30]

https://codereview.chromium.org/249113002/diff/90001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/249113002/diff/90001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:77: #endif
That symbol was defined the last time I tried to build views.
Looks like gesture_recognizer_impl.cc was changed to be excluded for !aura a few
days ago,
https://codereview.chromium.org/282593002/diff/120001/ui/events/events.gyp

I agree, NativeWidgetAura::SetCapture seems like a better home for this code.

https://codereview.chromium.org/249113002/diff/90001/ui/views/widget/native_w...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/249113002/diff/90001/ui/views/widget/native_w...
ui/views/widget/native_widget_mac.mm:9: #include "ui/compositor/layer_owner.h"
Appears to be unused, do we need this include?

https://codereview.chromium.org/249113002/diff/90001/ui/views/widget/native_w...
ui/views/widget/native_widget_mac.mm:105: void*
NativeWidgetMac::GetNativeWindowProperty(const char* name) const {
Why doesn't this one have NOTIMPLEMENTED() like the others?

[tapted, 2014-05-22 12:50:13]

+ben

Hi Ben. With this CL, target `views` compiles on Mac, and we can start running
an FYI bot to catch regressions. Please take a look.

https://codereview.chromium.org/249113002/diff/90001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/249113002/diff/90001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:77: #endif
On 2014/05/21 17:13:31, Andre wrote:
> That symbol was defined the last time I tried to build views.
> Looks like gesture_recognizer_impl.cc was changed to be excluded for !aura a
few
> days ago,
> https://codereview.chromium.org/282593002/diff/120001/ui/events/events.gyp
> 
> I agree, NativeWidgetAura::SetCapture seems like a better home for this code.

I poked at this a bit. Tried out just compiling the gesture stuff again the way
it was last week (crrev/291373002). But there are patches in flight that suggest
more aura-specific stuff on the way. Touch input on desktop OSX isn't currently
a priority AFAIK, so an empty stub felt right for now.

https://codereview.chromium.org/249113002/diff/90001/ui/views/widget/native_w...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/249113002/diff/90001/ui/views/widget/native_w...
ui/views/widget/native_widget_mac.mm:9: #include "ui/compositor/layer_owner.h"
On 2014/05/21 17:13:31, Andre wrote:
> Appears to be unused, do we need this include?

Not yet - it's a stray. Removed.

https://codereview.chromium.org/249113002/diff/90001/ui/views/widget/native_w...
ui/views/widget/native_widget_mac.mm:105: void*
NativeWidgetMac::GetNativeWindowProperty(const char* name) const {
On 2014/05/21 17:13:31, Andre wrote:
> Why doesn't this one have NOTIMPLEMENTED() like the others?

Fixed.

Good question. It was deliberate, the rationale went: if a set always "fails"
then it makes sense for a get to silently fail too. But on second thought that's
kinda silly :)

[tapted, 2014-05-22 12:56:31]

also +tdresser FYI or for comment (wrt gesture_recognizer_impl_mac.cc)

[tdresser, 2014-05-22 13:24:16]

On 2014/05/22 12:56:31, tapted wrote:
> also +tdresser FYI or for comment (wrt gesture_recognizer_impl_mac.cc)

gesture_recognizer_impl_mac.cc LGTM.

[Ben Goodger (Google), 2014-05-22 17:33:54]

lgtm

https://codereview.chromium.org/249113002/diff/200001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/249113002/diff/200001/ui/views/views.gyp#newc...
ui/views/views.gyp:473: 'accessibility/ax_aura_obj_cache.cc',
why here vs. use_aura==0?

i'm actually surprised this check exists here, since views shouldn't know about
ash.

[Andre, 2014-05-22 21:22:25]

https://codereview.chromium.org/249113002/diff/200001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/249113002/diff/200001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:36: defer:NO]);
  delegate_->OnNativeWidgetCreated(false);

Should add this, or is that for later?

[tapted, 2014-05-23 01:11:29]

https://codereview.chromium.org/249113002/diff/200001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/249113002/diff/200001/ui/views/views.gyp#newc...
ui/views/views.gyp:473: 'accessibility/ax_aura_obj_cache.cc',
On 2014/05/22 17:33:55, Ben Goodger (Google) wrote:
> why here vs. use_aura==0?
> 
> i'm actually surprised this check exists here, since views shouldn't know
about
> ash.

I think ash makes the most sense for this. It was tempting just to go for the
less-controversial use_aura, but `git grep views/accessibility/ax_` shows
clients only in chrome/browser/ui/ash.

These were actually moved fairly recently (r267487 ->
https://codereview.chromium.org/246433012/ ). Where it came with USE_ASH guard
in
https://codereview.chromium.org/246433012/diff/430001/chrome/browser/ui/views...


I think http://crbug.com/309681 is the relevant bug driving this, which is CrOS
only.

I agree it's a bit odd ash is here at all. Filed http://crbug.com/376603

https://codereview.chromium.org/249113002/diff/200001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/249113002/diff/200001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:36: defer:NO]);
On 2014/05/22 21:22:25, Andre wrote:
>   delegate_->OnNativeWidgetCreated(false);
> 
> Should add this, or is that for later?

I think for later - it will encourage us to ensure it has test coverage when it
goes in :)

[Andre, 2014-05-23 17:10:58]

On 2014/05/23 01:11:29, tapted wrote:
> https://codereview.chromium.org/249113002/diff/200001/ui/views/views.gyp
> File ui/views/views.gyp (right):
> 
>
https://codereview.chromium.org/249113002/diff/200001/ui/views/views.gyp#newc...
> ui/views/views.gyp:473: 'accessibility/ax_aura_obj_cache.cc',
> On 2014/05/22 17:33:55, Ben Goodger (Google) wrote:
> > why here vs. use_aura==0?
> > 
> > i'm actually surprised this check exists here, since views shouldn't know
> about
> > ash.
> 
> I think ash makes the most sense for this. It was tempting just to go for the
> less-controversial use_aura, but `git grep views/accessibility/ax_` shows
> clients only in chrome/browser/ui/ash.
> 
> These were actually moved fairly recently (r267487 ->
> https://codereview.chromium.org/246433012/ ). Where it came with USE_ASH guard
> in
>
https://codereview.chromium.org/246433012/diff/430001/chrome/browser/ui/views...
> 
> 
> I think http://crbug.com/309681 is the relevant bug driving this, which is
CrOS
> only.
> 
> I agree it's a bit odd ash is here at all. Filed http://crbug.com/376603
> 
>
https://codereview.chromium.org/249113002/diff/200001/ui/views/widget/native_...
> File ui/views/widget/native_widget_mac.mm (right):
> 
>
https://codereview.chromium.org/249113002/diff/200001/ui/views/widget/native_...
> ui/views/widget/native_widget_mac.mm:36: defer:NO]);
> On 2014/05/22 21:22:25, Andre wrote:
> >   delegate_->OnNativeWidgetCreated(false);
> > 
> > Should add this, or is that for later?
> 
> I think for later - it will encourage us to ensure it has test coverage when
it
> goes in :)

LGTM

[tapted, 2014-05-26 01:45:04]

https://codereview.chromium.org/249113002/diff/200001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/249113002/diff/200001/ui/views/views.gyp#newc...
ui/views/views.gyp:473: 'accessibility/ax_aura_obj_cache.cc',
On 2014/05/23 01:11:30, tapted wrote:
> On 2014/05/22 17:33:55, Ben Goodger (Google) wrote:
> > why here vs. use_aura==0?
> > 
> > i'm actually surprised this check exists here, since views shouldn't know
> about
> > ash.
> 
> I think ash makes the most sense for this. It was tempting just to go for the
> less-controversial use_aura, but `git grep views/accessibility/ax_` shows
> clients only in chrome/browser/ui/ash.
> 
> These were actually moved fairly recently (r267487 ->
> https://codereview.chromium.org/246433012/ ). Where it came with USE_ASH guard
> in
>
https://codereview.chromium.org/246433012/diff/430001/chrome/browser/ui/views...
> 
> 
> I think http://crbug.com/309681 is the relevant bug driving this, which is
CrOS
> only.
> 
> I agree it's a bit odd ash is here at all. Filed http://crbug.com/376603 

After some discussion in http://crbug.com/376603 moved these to use_aura. With
ash now on linux, there's no practical difference (i.e. I'm pretty sure, now,
aura && views \implies{} ash), and I think it will be less surprising. These may
be needed to an upcoming extension API, and are not intentionally restricted
only to ash.

[tapted, 2014-05-26 01:45:10]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-05-26 01:45:46]

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

[commit-bot: I haz the power, 2014-05-26 03:33:28]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-26 16:27:40]

Change committed as 272849