MacViews: Hide windows before closing.

ui/views/cocoa/bridged_native_widget.mm

Index: ui/views/cocoa/bridged_native_widget.mm
diff --git a/ui/views/cocoa/bridged_native_widget.mm b/ui/views/cocoa/bridged_native_widget.mm
index ce3731d026165eb42ef39e0b00bafe6953e2fcf9..7fdc5f342792acb8c9b9e3749c32f2965210b79d 100644
--- a/ui/views/cocoa/bridged_native_widget.mm
+++ b/ui/views/cocoa/bridged_native_widget.mm
@@ -363,6 +363,8 @@ NSUInteger CountBridgedWindows(NSArray* child_windows) {
   DCHECK(child_windows_.empty());
   SetFocusManager(nullptr);
   SetRootView(nullptr);
+
+  DCHECK(![window_ isVisible]);
   DestroyCompositor();
 }
 
@@ -681,6 +683,11 @@ NSUInteger CountBridgedWindows(NSArray* child_windows) {
 }
 
 void BridgedNativeWidget::OnWindowWillClose() {
+  // Hide the window before closing. This ensures that destroying the compositor
+  // during the close does not cause a flash, which would be the case if the
+  // window was still visible.
+  SetVisibilityState(HIDE_WINDOW);

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

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?

[tapted, 2016/10/28 02:35:39]

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()
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; ?
I like this better.
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:51:09]

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.
Don't think it'll help. I think it only disables animations etc. when in scope.

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

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.
Yeah - it's probably worth trying if we can find a measurable performance
benefit.

+
   native_widget_mac_->GetWidget()->OnNativeWidgetDestroying();
 
   // Ensure BridgedNativeWidget does not have capture, otherwise