MacViews: Fix WidgetTest.DesktopNativeWidgetNoPaintAfterCloseTest

MacViews: Fix WidgetTest.DesktopNativeWidgetNoPaintAfterCloseTest

But.. first make some tests fail.

DesktopNativeWidgetNoPaintAfterCloseTest currently passes but it
shouldn't. WidgetTests DesktopNativeWidgetNoPaintAfterHideTest and
TestWindowVisibilityAfterHide were also passing, but
NativeWidgetMac::Hide() is a no-op, so that shouldn't be the case.

This CL (first) makes them all fail on Mac. To do this,
BridgedNativeWidget needs to call Widget::OnNativeWidgetPaint() rather
than painting the RootView directly. And TestWindowVisibilityAfterHide
must show the widget.

DesktopNativeWidgetNoPaintAfterCloseTest was then fixed by skipping a
paint when the window is not visible, as toolkit-views expects. However,
Cocoa paints the window before changing -[NSWindow isVisible]. To fix,
intercept -[NSWindow orderWindow:relativeTo:]. We need to do this anyway
to send Widget::OnNativeWidgetVisibilityChanged(), but that's left for a
follow-up.

BUG=378134
Committed: https://crrev.com/5ccddf94823808b37a2179e3b3e2fd6f8a0f5286
Cr-Commit-Position: refs/heads/master@{#300056}

[tapted, 2014-10-16 08:27:06]

tapted@chromium.org changed reviewers:
+ rsesek@chromium.org

[tapted, 2014-10-16 08:27:07]

HI Robert, could you please take a look at the Mac changes? (Andre feel free to
jump in too).

Overriding [NSWindow orderWindow:..] feels a bit dirty, but there doesn't seem
to be another way to intercept it (and alternatives to intercepting don't feel
as robust). So, any ideas welcome :)

https://codereview.chromium.org/654393002/diff/80001/ui/views/cocoa/bridged_n...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/654393002/diff/80001/ui/views/cocoa/bridged_n...
ui/views/cocoa/bridged_native_widget_unittest.mm:258: view_.reset(new
views::internal::RootView(widget_.get()));
This lets hostedView_->GetWidget() return non-NULL and is more "authentic"
anyway

https://codereview.chromium.org/654393002/diff/80001/ui/views/cocoa/views_nsw...
File ui/views/cocoa/views_nswindow_delegate.h (right):

https://codereview.chromium.org/654393002/diff/80001/ui/views/cocoa/views_nsw...
ui/views/cocoa/views_nswindow_delegate.h:38: 
For some more context around "why this way", see http://crrev.com/630363003
which is the change to feed through to OnNativeWidgetVisibilityChanged().

(But that's currently held up by refactoring child windows :/. Turns out if you
orderOut an [NSWindow childWindow] the parent gets hidden, so implementing
Hide() that way makes menus pretty broken, since the opener window is hidden
each time you select an item.)

https://codereview.chromium.org/654393002/diff/80001/ui/views/focus/focus_man...
File ui/views/focus/focus_manager.h (left):

https://codereview.chromium.org/654393002/diff/80001/ui/views/focus/focus_man...
ui/views/focus/focus_manager.h:85: class RootView;
(this is just in the wrong namespace)

https://codereview.chromium.org/654393002/diff/80001/ui/views/widget/widget_u...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/654393002/diff/80001/ui/views/widget/widget_u...
ui/views/widget/widget_unittest.cc:1119: EXPECT_EQ(0, frame_view->width());
note this test above is also passing when it shouldn't... but putting
EXPECT_NE(0, frame_view->width()); before the call to Minimize() fails on all
platforms, so I'll deal with this separately.

[Robert Sesek, 2014-10-16 18:57:48]

This seems reasonable to me, but I'm not an expert in the views machinery. LGTM,
if all the variants for window ordering funnel through this (e.g.
-makeKeyAndOrderFront:, -orderBack:, etc.).

[Robert Sesek, 2014-10-16 18:58:53]

On 2014/10/16 18:57:48, Robert Sesek wrote:
> This seems reasonable to me, but I'm not an expert in the views machinery.
LGTM,
> if all the variants for window ordering funnel through this (e.g.
> -makeKeyAndOrderFront:, -orderBack:, etc.).

... you may want to even consider adding dedicated tests for these, in case
Cocoa sends a views-backed window one of those messages.

[tapted, 2014-10-16 21:59:39]

On 2014/10/16 18:58:53, Robert Sesek wrote:
> On 2014/10/16 18:57:48, Robert Sesek wrote:
> > This seems reasonable to me, but I'm not an expert in the views machinery.
> LGTM,
> > if all the variants for window ordering funnel through this (e.g.
> > -makeKeyAndOrderFront:, -orderBack:, etc.).
> 
> ... you may want to even consider adding dedicated tests for these, in case
> Cocoa sends a views-backed window one of those messages.

[tapted, 2014-10-16 22:02:31]

On 2014/10/16 18:58:53, Robert Sesek wrote:
> On 2014/10/16 18:57:48, Robert Sesek wrote:
> > This seems reasonable to me, but I'm not an expert in the views machinery.
> LGTM,
> > if all the variants for window ordering funnel through this (e.g.
> > -makeKeyAndOrderFront:, -orderBack:, etc.).
> 
> ... you may want to even consider adding dedicated tests for these, in case
> Cocoa sends a views-backed window one of those messages.

Yep - I have these mostly written in
https://codereview.chromium.org/630363003/diff/100001/ui/views/widget/native_...
which I'll land with NativeWidgetMac::Hide() . [NSApp hide:nil]; turned out to
be one of the annoying ones -- it doesn't send orderOut to the windows, but it's
not an issue for drawRect (since it is hide), and there's a notification to
detect the visibility change (which BrowserWindowCocoa uses as well). And the
[NSApp unhide*] methods to funnel through orderWindow, so it all works \o/

[tapted, 2014-10-16 22:08:39]

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

[tapted, 2014-10-16 22:08:40]

+sky - could you please take a look at the widget_unittest.cc changes and for
OWNERS.

[sky, 2014-10-16 22:39:25]

LGTM

https://codereview.chromium.org/654393002/diff/80001/ui/views/widget/widget_u...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/654393002/diff/80001/ui/views/widget/widget_u...
ui/views/widget/widget_unittest.cc:1159: bool ReadReceivedPaint() {
nit: make this name more explicit, maybe something like
ReadReceivedPaintAndReset

[tapted, 2014-10-17 02:50:34]

Thanks all!

https://codereview.chromium.org/654393002/diff/80001/ui/views/widget/widget_u...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/654393002/diff/80001/ui/views/widget/widget_u...
ui/views/widget/widget_unittest.cc:1159: bool ReadReceivedPaint() {
On 2014/10/16 22:39:25, sky wrote:
> nit: make this name more explicit, maybe something like
> ReadReceivedPaintAndReset

Done.

[tapted, 2014-10-17 02:50:40]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-10-17 02:52:14]

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

[commit-bot: I haz the power, 2014-10-17 03:46:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-17 03:47:35]

Patchset 6 (id:??) landed as
https://crrev.com/5ccddf94823808b37a2179e3b3e2fd6f8a0f5286
Cr-Commit-Position: refs/heads/master@{#300056}