MacViews: Initial skeleton for implementing NativeWidgetMac

MacViews: Initial skeleton for implementing NativeWidgetMac

This adds two classes under ui/views/cocoa: BridgedNativeWidget (a C++
class which bridges an NSWindow) and BridgedContentView (an Objective-C 
"root" NSView which bridges a views::View).

With respect to NativeWidgetMac, BridgedNativeWidget performs a role
similar to aura::Window on Aura platforms. Among other things, it will
(soon) be a ui::LayerOwner. It serves for implementation-hiding: it owns
Objective-C objects for NativeWidgetMac so that native_widget_mac.h does
not force .cc files to become .mm. It also allows implementation-sharing
between NativeWidgetMac and, later, DesktopNativeWidgetMac.

BridgedContentView allows messages to feed through from Cocoa into
toolkit-views, by implementing the required Objective-C interfaces and
bridging to C++ code.

Adds 3 tests, and gets 17 others passing. Total 202/468 passing.

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

[tapted, 2014-06-11 08:46:19]

andre: for a general Mac review
sky: owners/design/review

This is a polished version of crrev/309483009 where I got some early feedback.

I've been trying out this design for a while (and I built lots more on it in
local branches) to convince me it should stick. Everything has been falling into
place nicely, and I hit a chicken-and-egg problem in getting widget_unittests.cc
to compile which this resolves. This way we can ensure that new stuff being
added to NativeWidgetMac either has test coverage, or gets a new test.

[sky, 2014-06-11 16:15:53]

LGTM

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
ui/views/cocoa/bridged_content_view.mm:19: if ((self = [super
initWithFrame:initialFrame])) {
nit: no {}

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
ui/views/cocoa/bridged_content_view.mm:19: if ((self = [super
initWithFrame:initialFrame])) {
Why the self = check here? Can initWithFrame return NULL? Shouldn't we crash if
that happens?

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

https://codereview.chromium.org/329743002/diff/70001/ui/views/widget/native_w...
ui/views/widget/native_widget_mac.mm:317: [bridge_->ns_view()
setNeedsDisplay:YES];
setNeedsDisplayInRect?

[Andre, 2014-06-11 18:02:52]

lgtm

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_n...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_n...
ui/views/cocoa/bridged_native_widget.mm:7: #include "base/logging.h"
Is this for DCHECK?
Seems inconsistent with BridgedContentView which also uses DCHECK but does not
include the header.

[Andre, 2014-06-11 18:02:52]

lgtm

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_n...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_n...
ui/views/cocoa/bridged_native_widget.mm:7: #include "base/logging.h"
Is this for DCHECK?
Seems inconsistent with BridgedContentView which also uses DCHECK but does not
include the header.

[tapted, 2014-06-12 09:13:12]

+rsesek since he left some feedback in crrev/309483009

otherwise comments addressed, and rebased on ToT to resolve a merge conflict
amongst #includes

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
ui/views/cocoa/bridged_content_view.mm:19: if ((self = [super
initWithFrame:initialFrame])) {
On 2014/06/11 16:15:53, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
ui/views/cocoa/bridged_content_view.mm:19: if ((self = [super
initWithFrame:initialFrame])) {
On 2014/06/11 16:15:53, sky wrote:
> Why the self = check here? Can initWithFrame return NULL? Shouldn't we crash
if
> that happens?

Yeah - this is Objective C weirdness ;). Constructors follow this pattern partly
by convention (see eg `grep 'if ((self =' -- '*.mm'`). Superclass constructors
are allowed to return nil and, if they do, child classes should skip their own
initialization and also return nil.

And, since method calls on `nil` objects in Objective C are allowed and
well-defined (and checks for nil discouraged - http://go/objcguide#nil_Checks),
often things just chug along nicely even if the constructor returns nil (while
often making things a real pain to debug :/).

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_n...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_n...
ui/views/cocoa/bridged_native_widget.mm:7: #include "base/logging.h"
On 2014/06/11 18:02:52, Andre wrote:
> Is this for DCHECK?
> Seems inconsistent with BridgedContentView which also uses DCHECK but does not
> include the header.

Yep it's for the DCHECK. logging.h slips in easily from other headers, but for
iwyu it should be in  bridged_content_view.mm as well. Added.

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

https://codereview.chromium.org/329743002/diff/70001/ui/views/widget/native_w...
ui/views/widget/native_widget_mac.mm:317: [bridge_->ns_view()
setNeedsDisplay:YES];
On 2014/06/11 16:15:53, sky wrote:
> setNeedsDisplayInRect?

Yep - well spotted :) We will definitely want this, but it needs a coordinate
transform which I want to make sure I get right. Added a TODO, since a
NOTIMPLEMENTED() here is a bit awkward for getting tests off the ground.
Something similar just came up in crrev/309483009 too

[Robert Sesek, 2014-06-12 13:42:28]

Nice, LGTM

https://codereview.chromium.org/329743002/diff/110001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/329743002/diff/110001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:26: @synthesize hostedView = hostedView_;
@synthesize comes after @implementation with a blank line between.

https://codereview.chromium.org/329743002/diff/110001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/329743002/diff/110001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:36: Init();
Do you manually need to call Init() here?

[sky, 2014-06-12 16:51:47]

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
ui/views/cocoa/bridged_content_view.mm:19: if ((self = [super
initWithFrame:initialFrame])) {
On 2014/06/12 09:13:12, tapted wrote:
> On 2014/06/11 16:15:53, sky wrote:
> > Why the self = check here? Can initWithFrame return NULL? Shouldn't we crash
> if
> > that happens?
> 
> Yeah - this is Objective C weirdness ;). Constructors follow this pattern
partly
> by convention (see eg `grep 'if ((self =' -- '*.mm'`). Superclass constructors
> are allowed to return nil and, if they do, child classes should skip their own
> initialization and also return nil.
> 
> And, since method calls on `nil` objects in Objective C are allowed and
> well-defined (and checks for nil discouraged -
http://go/objcguide#nil_Checks),
> often things just chug along nicely even if the constructor returns nil (while
> often making things a real pain to debug :/).

I realize this is all allowed, but shouldn't we treat it similar to a malloc
failing and crash? Say for example you're creating a tabbed browser and the
creation of the underlying window fails, shouldn't we crash?

[Robert Sesek, 2014-06-12 17:31:37]

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/329743002/diff/70001/ui/views/cocoa/bridged_c...
ui/views/cocoa/bridged_content_view.mm:19: if ((self = [super
initWithFrame:initialFrame])) {
On 2014/06/12 16:51:47, sky wrote:
> On 2014/06/12 09:13:12, tapted wrote:
> > On 2014/06/11 16:15:53, sky wrote:
> > > Why the self = check here? Can initWithFrame return NULL? Shouldn't we
crash
> > if
> > > that happens?
> > 
> > Yeah - this is Objective C weirdness ;). Constructors follow this pattern
> partly
> > by convention (see eg `grep 'if ((self =' -- '*.mm'`). Superclass
constructors
> > are allowed to return nil and, if they do, child classes should skip their
own
> > initialization and also return nil.
> > 
> > And, since method calls on `nil` objects in Objective C are allowed and
> > well-defined (and checks for nil discouraged -
> http://go/objcguide#nil_Checks),
> > often things just chug along nicely even if the constructor returns nil
(while
> > often making things a real pain to debug :/).
> 
> I realize this is all allowed, but shouldn't we treat it similar to a malloc
> failing and crash? Say for example you're creating a tabbed browser and the
> creation of the underlying window fails, shouldn't we crash?

If allocation fails, that will be caught by
base::EnableTerminationOnOutOfMemory. This is idiomatic for Cocoa (though I'd
not have removed the {}).

[tapted, 2014-06-13 00:14:07]

On 2014/06/12 16:51:47, sky wrote:
> I realize this is all allowed, but shouldn't we treat it similar to a malloc
> failing and crash? Say for example you're creating a tabbed browser and the
> creation of the underlying window fails, shouldn't we crash?

I think I see your point - the base class is allowed to return nil for errors
other than allocation. If that happens BridgedNativeWidget would then treat it
as if the view had just been cleared. I've added a CHECK in
BridgedNativeWidget::SetRootView to ensure this is caught early.

https://codereview.chromium.org/329743002/diff/110001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/329743002/diff/110001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:26: @synthesize hostedView = hostedView_;
On 2014/06/12 13:42:27, rsesek wrote:
> @synthesize comes after @implementation with a blank line between.

Done.

https://codereview.chromium.org/329743002/diff/110001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/329743002/diff/110001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:36: Init();
On 2014/06/12 13:42:27, rsesek wrote:
> Do you manually need to call Init() here?

Wow - nope. Removed. (and mind:blown)

Lots of ui::CocoaTest subclasses do this (e.g. [non-ui] ::CocoaTest in
c/b/ui/cocoa/. I think because there's an odd comment on ui::CocoaTest::Init()
"Allows subclasses to do initialization before calling through to the base
class's initialization" but of course that's backwards.

ui::CocoaTest::CocoaTest() is the only base constructor and it calls Init().
Init is not static, so I think the only way to call Init before the base class
constructor does is something awful involving multiple inheritance and passing
`this` in an initializer list so a different base class can call it.

(I'll put up a CL to make ui::CocoaTest::Init() private -- I think that, or make
it static, would be sensible).

[tapted, 2014-06-13 02:59:17]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-06-13 03:00:58]

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

[commit-bot: I haz the power, 2014-06-13 07:24:05]

Change committed as 276957