MacViews: Bubbles and dialogs behave more like sheets wrt main status

MacViews: Bubbles and dialogs behave more like sheets wrt main status

Currently dialog boxes do not take main status from the parent window.
Change this so that any child window does not become a main window.

Also, use the same trick that Cocoa bubbles use to avoid the traffic
lights dimming, but only do this for child windows that don't have their
own traffic lights.

BUG=543689
Committed: https://crrev.com/5243ef97ef67f4525987e40d28e6150f0d852f39
Cr-Commit-Position: refs/heads/master@{#372036}

[tapted, 2016-01-21 07:46:10]

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

[tapted, 2016-01-21 07:46:10]

Hiya Robert - could you please take a look?

[tapted, 2016-01-21 22:32:25]

Patchset #5 (id:80001) has been deleted

[tapted, 2016-01-21 23:02:27]

rsesek: PTAL (whoopsies)

https://codereview.chromium.org/1614663002/diff/60001/ui/views/cocoa/native_w...
File ui/views/cocoa/native_widget_mac_nswindow.mm (right):

https://codereview.chromium.org/1614663002/diff/60001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:90:
!views::NativeWidgetMac::GetBridgeForNativeWindow(self)->parent();
oops - this was crashing in tests that (a) have parent/child relationships and
(b) use Widget::CloseNow instead of Widget::Close. This is because
~NativeWidgetMac first sets the bridge to be null before starting teardown, but
~BridgedNativeWidget doesn't (and can't) set the NSWidnowDelegate to be null
before starting *its* teardown.

I realised NSWindow actually has all the info we need, and I can just call
[NSWindow parentWindow] to avoid the C++ object dependency - much nicer :)

https://codereview.chromium.org/1614663002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_interactive_uitest.mm (right):

https://codereview.chromium.org/1614663002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac_interactive_uitest.mm:198:
EXPECT_TRUE([active_button_image isEqualToData:button_image_with_child]);
oops#2 - I meant to comment on this too that this all seems a bit nuts, but I
couldn't figure out any NSButton[Cell] property I could query that gave the same
information -- it all seems to be internal to the particular NSButtonCell used
for the traffic lights. But maybe you have an idea?

I think test coverage of the private API usage is important, and this isn't any
scarier that what we do for checking the `showsWindowTitle` private API
overrides we use elsewhere.

[Robert Sesek, 2016-01-21 23:41:35]

LGTM w/ a nit

https://codereview.chromium.org/1614663002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_interactive_uitest.mm (right):

https://codereview.chromium.org/1614663002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac_interactive_uitest.mm:157: }
"  // namespace" and a line before

[tapted, 2016-01-22 00:28:06]

bleh - jumped the gun trying to sneak in while there's timezone overlap.
Although this seems to work fine in practice, I'm wary of an trap lying in wait
(details below). So, I'm going to have to think on this a bit more..

https://codereview.chromium.org/1614663002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_interactive_uitest.mm (right):

https://codereview.chromium.org/1614663002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac_interactive_uitest.mm:157: }
On 2016/01/21 23:41:35, Robert Sesek wrote:
> "  // namespace" and a line before

Done.

https://codereview.chromium.org/1614663002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac_interactive_uitest.mm:192: EXPECT_TRUE([parent
isMainWindow]);
yegads AppKit. So with the bridge()->parent() => [self parentWindow] change,
this line now fails. But if you spin a run loop just after this, `parent`
clearly is the main window.

So, while the parentWindow approach works fine in practice, there may be a trap
lurking. And I'm still scarred by the hangouts multiple-key-status bug that this
makes me worry a little.

I think the problem is that AppKit's parent/child relationships can only be
relied upon while the window is visible (calling addChildWindow on a hidden
window has always led to strife). So, the first time a window becomes visible,
it temporarily reports `yes` in canBecomeMain, then when the window server gets
around to actually activating it, it reports `no`, and doesn't actually become
main. Yet, this isn't properly reflected `isMainWindow`.

Need to think some more about the right way to fix this.

[tapted, 2016-01-22 04:03:44]

Patchset #7 (id:140001) has been deleted

[tapted, 2016-01-22 04:16:08]

Patchset #7 (id:160001) has been deleted

[tapted, 2016-01-22 06:32:48]

PTAL at Patchset 7. canBecomeMainWindow returns back to how it was in patchset
4, but the CL includes a fix for BridgedNativeWidget::~BridgedNativeWidget() so
that it behaves "as expected". Basically, the teardown paths via
Widget::CloseNow or WIDGET_OWNS_NATIVE_WIDGET now behave the same as the
teardown path always did via [NSWindow close] (or Widget::Close(), which just
calls [NSWindow close] asynchronously). I think this is the best way to ensure
the implementation is robust to unknown AppKit "stuff" going forward.

Added a separate test for this so that it's not relying on subtle stuff in an
interactive UI test, or the particular implementation of canBecomeMainWindow.

https://codereview.chromium.org/1614663002/diff/180001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1614663002/diff/180001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:985: // Test the
Widget::CloseNow() flow.
One of the reasons that I like the bridged_native_widget.mm change is that,
prior to it, this test case would have passed the "normal" test, but failed on
line 955 for the CloseNow() and WIDGET_OWNS_NATIVE_WIDGET cases when they check
that the parent delegate is nil.

Now all the teardown flows are consistent and other code can safely rely on just
the presence of the delegate being a signal for a fully-formed bridge.

[tapted, 2016-01-27 10:36:37]

rsesek: ping? (PTAL at the changes after patchset 5)

[Robert Sesek, 2016-01-27 22:15:13]

LGTM

sorry, swamped this week

https://codereview.chromium.org/1614663002/diff/180001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1614663002/diff/180001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:946: DCHECK(child_closed_);  //
Otherwise the observer wasn't removed.
Should these be EXPECT_ instead of DCHECK?

[tapted, 2016-01-28 06:58:33]

Thanks robert!

https://codereview.chromium.org/1614663002/diff/180001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1614663002/diff/180001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:946: DCHECK(child_closed_);  //
Otherwise the observer wasn't removed.
On 2016/01/27 22:15:12, Robert Sesek wrote:
> Should these be EXPECT_ instead of DCHECK?

Done. I guess I was worried about things exploding with UAF and things if it
ever failed, and went for DCHECK, but I think it will just leak, and we get more
logging with EXPECT

[tapted, 2016-01-28 06:58:41]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-01-28 06:58:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/1614663002/#ps200001
(title: "DCHECK -> EXPECT")

[commit-bot: I haz the power, 2016-01-28 06:59:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1614663002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1614663002/200001

[commit-bot: I haz the power, 2016-01-28 07:04:50]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-01-28 07:05:52]

Description was changed from

==========
MacViews: Bubbles and dialogs behave more like sheets wrt main status

Currently dialog boxes do not take main status from the parent window.
Change this so that any child window does not become a main window.

Also, use the same trick that Cocoa bubbles use to avoid the traffic
lights dimming, but only do this for child windows that don't have their
own traffic lights.

BUG=543689
==========

to

==========
MacViews: Bubbles and dialogs behave more like sheets wrt main status

Currently dialog boxes do not take main status from the parent window.
Change this so that any child window does not become a main window.

Also, use the same trick that Cocoa bubbles use to avoid the traffic
lights dimming, but only do this for child windows that don't have their
own traffic lights.

BUG=543689

Committed: https://crrev.com/5243ef97ef67f4525987e40d28e6150f0d852f39
Cr-Commit-Position: refs/heads/master@{#372036}
==========

[commit-bot: I haz the power, 2016-01-28 07:05:53]

Patchset 8 (id:??) landed as
https://crrev.com/5243ef97ef67f4525987e40d28e6150f0d852f39
Cr-Commit-Position: refs/heads/master@{#372036}