[Mac] Change to detach the web contents view when the panel is not taller than the titlebar

chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.h"
 
 #import <Cocoa/Cocoa.h>
 
 #include "base/auto_reset.h"
-#include "base/command_line.h"
 #include "base/logging.h"
 #include "base/mac/bundle_locations.h"
 #include "base/mac/mac_util.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
 #include "base/strings/sys_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"  // IDC_*
 #include "chrome/browser/chrome_browser_application_mac.h"
 #include "chrome/browser/profiles/profile.h"
 #import "chrome/browser/ui/cocoa/browser_command_executor.h"
 #import "chrome/browser/ui/cocoa/browser_window_utils.h"
 #import "chrome/browser/ui/cocoa/panels/mouse_drag_controller.h"
 #import "chrome/browser/ui/cocoa/panels/panel_cocoa.h"
 #import "chrome/browser/ui/cocoa/panels/panel_titlebar_view_cocoa.h"
 #import "chrome/browser/ui/cocoa/panels/panel_utils_cocoa.h"
 #import "chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.h"
 #import "chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h"
 #import "chrome/browser/ui/cocoa/tabs/throbber_view.h"
 #include "chrome/browser/ui/panels/panel_bounds_animation.h"
 #include "chrome/browser/ui/panels/panel_collection.h"
 #include "chrome/browser/ui/panels/panel_constants.h"
 #include "chrome/browser/ui/panels/panel_manager.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/toolbar/encoding_menu_controller.h"
-#include "chrome/common/chrome_version_info.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
 #include "grit/ui_resources.h"
 #include "third_party/WebKit/public/web/WebCursorInfo.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image.h"
 #include "webkit/common/cursors/webcursor.h"
 
 using content::WebContents;
@@ skipping 757 matching lines @@
   // message, since it just means that a menu extra (on the "system status bar")
   // was activated; we'll get another |-windowDidResignKey| if we ever really
   // lose key window status.
   if ([NSApp isActive] && ([NSApp keyWindow] == [self window]))
     return;
 
   [self onWindowDidResignKey];
 }
 
 - (void)windowDidResize:(NSNotification*)notification {
-  // This is a temporary check to track down a crash that occurs occasionally
-  // (http://crbug.com/265932).
-  chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
-  if (channel == chrome::VersionInfo::CHANNEL_CANARY &&
-      CommandLine::ForCurrentProcess()->HasSwitch("enable-panel-experiment"))
+  // Remove the web contents view from the view hierarchy when the panel is not
+  // taller than the titlebar. Put it back when the panel grows taller than
+  // the titlebar. Note that RenderWidgetHostViewMac works for the case that
+  // the web contents view does not exist in the view hierarchy (i.e. the tab
+  // is not the main one), but it does not work well, like occasional crashes,
+  // if the web contents view is made hidden.

[Dmitry Titov, 2013/08/12 23:04:45]

lets put a bug number here, for reference to those crashes.

[jianli, 2013/08/12 23:43:14]

Done.

+  //
+  // The reason for doing this is to ensure that our titlebar view, that is
+  // somewhat taller than the standard titlebar, does not overlap with the web
+  // contents view because the the web contents view assumes that its view will
+  // never overlap with another view in order to perform optimization. If we do

[Dmitry Titov, 2013/08/12 23:04:45]

"in order to perform optimization" is unclear. Do you mean avoiding painting
transparent rect for 'hole punching' by [RenderVidgetHostViewMac drawRect]?

[jianli, 2013/08/12 23:43:14]

Updated comment.

+  // not do this, some part of the web contents view will become visible and
+  // overlapp the bottom area of the titlebar.
+  content::WebContents* webContents = windowShim_->panel()->GetWebContents();
+  if (!webContents)
     return;
-  // Hide the web contents view when the panel is not taller than the titlebar.
-  // This is to ensure that the titlebar view is not overlapped with the web
-  // contents view because the the web contents view assumes that its
-  // view will never be overlapped by another view in order to perform
-  // optimization. If we do not do this, some part of the web contents view
-  // will become visible and overlapp the bottom area of the titlebar.
-  if (WebContents* contents = windowShim_->panel()->GetWebContents()) {
-    BOOL hideContents =
-        NSHeight([self contentRectForFrameRect:[[self window] frame]]) <=
-        panel::kTitlebarHeight;
-    [contents->GetView()->GetNativeView() setHidden:hideContents];
+  NSView* contentView = webContents->GetView()->GetNativeView();
+  if (NSHeight([self contentRectForFrameRect:[[self window] frame]]) <=
+      panel::kTitlebarHeight) {

[Dmitry Titov, 2013/08/12 23:04:45]

This condition happens when titlebar is all we can see, so the webcontent view
height is 0. Should we do it before this happens?  Will the crash happen
anyways?

[jianli, 2013/08/12 23:43:14]

When we minimize a panel, the window is being animated from full bounds to the
titlebar bounds. Before the animation starts, we disable autosizing (calling
disableWebContentsViewAutosizing in setPanelFrame). So the web contents view
will indeed remain unchanged. Previously we tried to hide the web contents view
here but RenderWidgetHostViewMac still thinks the view is there and asks the
compositor to do the work.

+    // No need to retain the view before it is removed from its superview
+    // because WebContentsView keeps a reference to this view.
+    if ([contentView superview])
+      [contentView removeFromSuperview];
+  } else {
+    if (![contentView superview])
+      [[[self window] contentView] addSubview:contentView];

[Dmitry Titov, 2013/08/12 23:04:45]

Perhaps enableWebContentsViewAutosizing should be called here after attaching
the view back to view tree, to set up its size correctly.

[jianli, 2013/08/12 23:43:14]

We can't enable the autosizing for the web contents view now since the animation
has not ended. Otherwise, we could some sort of flickering due to contents view
relayout. The animation cleanup code will reenable the autosizing.

   }
 }
 
 - (void)activate {
   // Activate the window. -|windowDidBecomeKey:| will be called when
   // window becomes active.
   base::AutoReset<BOOL> pin(&activationRequestedByPanel_, true);
   [BrowserWindowUtils activateWindowForController:self];
 }
 
@@ skipping 136 matching lines @@
 - (NSRect)contentRectForFrameRect:(NSRect)frameRect {
   NSRect contentRect = [[[self window] contentView] convertRect:frameRect
                                                        fromView:nil];
   contentRect.size.height -= panel::kTitlebarHeight;
   if (contentRect.size.height < 0)
     contentRect.size.height = 0;
   return contentRect;
 }
 
 @end