MacViews: Hide windows before closing.

MacViews: Hide windows before closing.

This CL modifies BridgedNativeWidget::OnWindowWillClose to hide the window
before closing. This fixes various bugs related to closing of windows, and
ensures destroying the compositor during [window_ close] does not cause a flash.

BUG=649354, 645343, 660257

[karandeepb, 2016-10-28 00:59:58]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 01:00:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[karandeepb, 2016-10-28 01:52:43]

Description was changed from

==========
MacViews: Hide windows before closing.

This CL modifies BridgedNativeWidget::OnWindowWillClose to hide the window
before closing. This ensures that destroying the compositor while destroying the
BridgedNativeWidget during [window_ close] does not cause a flash, since the
window is already hidden.

BUG=649354
==========

to

==========
MacViews: Hide windows before closing.

This CL modifies BridgedNativeWidget::OnWindowWillClose to hide the window
before closing. This fixes various bugs related to closing of windows, and
ensures destroying the compositor during [window_ close] does not cause a flash.

BUG=649354, 645343, 660257
==========

[karandeepb, 2016-10-28 02:12:08]

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

[karandeepb, 2016-10-28 02:12:09]

PTAL Trent.

https://codereview.chromium.org/2454283002/diff/1/ui/views/cocoa/bridged_nati...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/2454283002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:689: SetVisibilityState(HIDE_WINDOW);
Is it possible that some animations may be in progress which can be affected by
this? There is layer()->CompleteAllAnimations call in DestroyCompositor.

Also, I haven't been able to figure out how RenderWidgetHostViewMac handles
this. It also resets its browser compositor instance while being destroyed, but
closes w/o a flash.

On http://crbug.com/649354, I had also suggested overriding the close method to
first call [super close] and then destroying everything, as one of the possible
solutions. Let me know if that should be explored.

Also, there was this - https://codereview.chromium.org/2061693003, but the
associated bug seems to be fixed. Are visibility changes during the closing of a
widget an issue?

[commit-bot: I haz the power, 2016-10-28 02:23:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 02:23:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[karandeepb, 2016-10-28 02:30:17]

Trent: So two of the tests are failing. You can hold off the review, till I fix
those.

[tapted, 2016-10-28 02:35:40]

https://codereview.chromium.org/2454283002/diff/1/ui/views/cocoa/bridged_nati...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/2454283002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:689: SetVisibilityState(HIDE_WINDOW);
On 2016/10/28 02:12:09, karandeepb wrote:
> Is it possible that some animations may be in progress which can be affected
by
> this? There is layer()->CompleteAllAnimations call in DestroyCompositor.

CompleteAllAnimations probably just sets the end state so that destruction is
cleaner. I think we can require that any widgets wanting close animations should
call the asynchronous Close()

> Also, I haven't been able to figure out how RenderWidgetHostViewMac handles
> this. It also resets its browser compositor instance while being destroyed,
but
> closes w/o a flash.

BrowserCompositorMac is currently funkier than the one we use for MacViews. It
calls BrowserCompositorMac::TransitionToState(HasNoCompositor). This
disassociates the NSView, but doesn't destroy the compositor -- it gets
recycled. Buuut - I don't see how that would affect things really. 

Did you try playing with ScopedCAActionDisabler disabler; ?

> On http://crbug.com/649354, I had also suggested overriding the close method
to
> first call [super close] and then destroying everything, as one of the
possible
> solutions. Let me know if that should be explored.

I like this better.

> Also, there was this - https://codereview.chromium.org/2061693003, but the
> associated bug seems to be fixed. Are visibility changes during the closing of
a
> widget an issue?

They were for that bug, but that was coincidence. We could still explore
suppressing changes, but it would be a performance optimization.

[karandeepb, 2016-10-28 04:43:07]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 04:43:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[karandeepb, 2016-10-28 04:51:09]

PTAL Trent.

https://codereview.chromium.org/2454283002/diff/1/ui/views/cocoa/bridged_nati...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/2454283002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:689: SetVisibilityState(HIDE_WINDOW);
>>BrowserCompositorMac is currently funkier than the one we use for MacViews. It
>>calls BrowserCompositorMac::TransitionToState(HasNoCompositor). This
>>disassociates the NSView, but doesn't destroy the compositor -- it gets
>>recycled. Buuut - I don't see how that would affect things really. 

Yeah so I debugged a bit more. The reason the webcontents don't flash is that
BrowserCompositorMac doesn't destroy the compositor instance, but keeps it alive
by recycling. Destroying the compositor instance destroys the compositor data
here -
https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t....
Don't really understand the graphic stack, but I would have imagined, just
setting the content view of the window to nil would have caused the contents to
erase, but that does not seem to be the case. 

So destroying the compositor data, destroys the CALayer contents of the
compositor_superview_ in BNW right? And its parent view i.e. the BCV is already
disconnected from the window, but this still causes flashing, weird.

We can also recycle the compositor for BNW, but this seems more intuitive and
clearer.

>> Did you try playing with ScopedCAActionDisabler disabler; ?
Don't think it'll help. I think it only disables animations etc. when in scope.

[tapted, 2016-10-28 05:13:17]

lgtm

https://codereview.chromium.org/2454283002/diff/1/ui/views/cocoa/bridged_nati...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/2454283002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:689: SetVisibilityState(HIDE_WINDOW);
On 2016/10/28 04:51:09, karandeepb wrote:
> >>BrowserCompositorMac is currently funkier than the one we use for MacViews.
It
> >>calls BrowserCompositorMac::TransitionToState(HasNoCompositor). This
> >>disassociates the NSView, but doesn't destroy the compositor -- it gets
> >>recycled. Buuut - I don't see how that would affect things really. 
> 
> Yeah so I debugged a bit more. The reason the webcontents don't flash is that
> BrowserCompositorMac doesn't destroy the compositor instance, but keeps it
alive
> by recycling. Destroying the compositor instance destroys the compositor data
> here -
>
https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t....
> Don't really understand the graphic stack, but I would have imagined, just
> setting the content view of the window to nil would have caused the contents
to
> erase, but that does not seem to be the case. 
> 
> So destroying the compositor data, destroys the CALayer contents of the
> compositor_superview_ in BNW right? And its parent view i.e. the BCV is
already
> disconnected from the window, but this still causes flashing, weird.

ccameron might have some further insight. I think this is reasonable. The
plumbing that allows the GPU process and the browser process to share the
resources that back the CALayer is some pretty crazy voodoo.


> We can also recycle the compositor for BNW, but this seems more intuitive and
> clearer.

Yeah - it's probably worth trying if we can find a measurable performance
benefit.

> >> Did you try playing with ScopedCAActionDisabler disabler; ?
> Don't think it'll help. I think it only disables animations etc. when in
scope.

https://codereview.chromium.org/2454283002/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/2454283002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_unittest.mm:1165: }
huh - there's not much difference between these now. I think that's OK. I think
the most important difference is that

[child->GetNativeWindow() delegate] should be non-nil in OnWidgetDestroying, but
nil in OnWidgetDestroyed. But there's coverage elsewhere. Up to you if you want
to add it

[commit-bot: I haz the power, 2016-10-28 05:30:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 05:31:00]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2016-10-28 10:23:58]

So turns out this is not a reliable solution. orderOut: is not
synchronous/blocking.