Mac: Do layout before adding the WebContents to the view hierarchy when switching tabs

chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm

Index: chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm
diff --git a/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm b/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm
index 706f3709916b2682b5b212f43e863c7d9a69f320..b683def8af89c8cac8e4493e4d0a8563007f3ede 100644
--- a/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm
+++ b/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm
@@ -71,9 +71,9 @@ class FullscreenObserver : public WebContentsObserver {
 
 @interface TabContentsController (TabContentsContainerViewDelegate)
 - (BOOL)contentsInFullscreenCaptureMode;
-// Computes and returns the frame to use for the contents view within the
-// container view.
-- (NSRect)frameForContentsView;
+// Computes and returns the frame to use for the contents view using the size of
+// |container| as the target size.
+- (NSRect)frameForContentsViewIn:(NSView*)container;
 
 // Returns YES if the content view should be resized.
 - (BOOL)shouldResizeContentView;
@@ -130,7 +130,7 @@ class FullscreenObserver : public WebContentsObserver {
   }
 
   ScopedCAActionDisabler disabler;
-  [contentsView setFrame:[delegate_ frameForContentsView]];
+  [contentsView setFrame:[delegate_ frameForContentsViewIn:self]];
 }
 
 // Update the background layer's color whenever the view needs to repaint.
@@ -217,20 +217,21 @@ class FullscreenObserver : public WebContentsObserver {
   [self setView:view];
 }
 
-- (void)ensureContentsSizeDoesNotChange {
-  NSView* contentsContainer = [self view];
-  NSArray* subviews = [contentsContainer subviews];
-  if ([subviews count] > 0) {
-    NSView* currentSubview = [subviews objectAtIndex:0];
-    [currentSubview setAutoresizingMask:NSViewNotSizable];
-  }
-}
-
-- (void)ensureContentsVisible {
+- (void)addContentsToView:(NSView*)superview {

[ccameron, 2016/07/12 18:56:58]

I'd prefer to avoid having the "nil defaults to something" behavior. Maybe have
this function be

- (void)ensureContentsVisibleInsuperview:(NSView*)superview

And require that callers that would specify nil specify [contentsContainer
superview].

[tapted, 2016/07/13 03:33:17]

hehe - I actually started with this, but it made it harder to write the
`defensive` DCHECK in below. Instead of

  DCHECK(!superview || ![contentsContainer superview]);

I'd need to write something like

  DCHECK(![contentsContainer superview] || superview == [contentsContainer
superview]);

but that's not interesting because if a caller were to "forget" to remove from
the view hierarchy in the first place, it's going to be [contentsContainer
superview] anyway.

So, I've just removed the DCHECK - it's less confusing. If I get inspired I'll
look into a way to write tests for these, but with the pause in setFrameSize
potentially timing out, it's going to require some finesse to ensure they don't
flake.

   if (!contents_)
     return;
+
   ScopedCAActionDisabler disabler;

[ccameron, 2016/07/12 18:56:58]

Would it be useful to have a "ScopedRWHViewActionDisabler" structure, which
would 
- have a scoped CAAction disabler
- delay any RWHView resizes until its dtor is called
- delay any RWHView show/hide calls until its dtor is called
- would do any required "wait for resized frame from GPU process" in the dtor

Food for thought.

[tapted, 2016/07/13 03:33:18]

I'll keep it at the back of my mind - it does sound useful. I tried thinking of
use-cases it couldn't support, and couldn't think of much. Just some uncertainty
about how it would interact with with AppKit codepaths, like those to/from
setFrameSize

E.g. the call to [contentsNativeView setFrame:] below may pause inside
setFrameSize. We'd want to move that pause to a ScopedRWHViewActionDisabler dtor
in a higher stack (if there is one), but AppKit may have other expectations when
setFrameSize returns, like for generating window shadows or thumbnails (in
reality that specific case probably happens later anyway, but it's hard to know)

   NSView* contentsContainer = [self view];
+
+  // |superview| will be nil when toggling fullscreen or changing tabs via a
+  // notification, which preserves the current superview. Otherwise, [self view]
+  // should have been removed from the view hierarchy to suppress resizes.
+  DCHECK(!superview || ![contentsContainer superview]);
+  if (!superview)
+    superview = [contentsContainer superview];
+  DCHECK(superview);
+
   NSArray* subviews = [contentsContainer subviews];
   NSView* contentsNativeView;
   content::RenderWidgetHostView* const fullscreenView =
@@ -244,7 +245,7 @@ class FullscreenObserver : public WebContentsObserver {
   }
 
   if ([self shouldResizeContentView])
-    [contentsNativeView setFrame:[self frameForContentsView]];
+    [contentsNativeView setFrame:[self frameForContentsViewIn:superview]];
 
   if ([subviews count] == 0) {
     [contentsContainer addSubview:contentsNativeView];
@@ -252,6 +253,10 @@ class FullscreenObserver : public WebContentsObserver {
     [contentsContainer replaceSubview:[subviews objectAtIndex:0]
                                  with:contentsNativeView];
   }
+
+  [contentsNativeView setAutoresizingMask:NSViewNotSizable];
+  [contentsContainer setFrame:[superview bounds]];
+  [superview addSubview:contentsContainer];
   [contentsNativeView setAutoresizingMask:NSViewWidthSizable|
                                           NSViewHeightSizable];
 
@@ -276,8 +281,10 @@ class FullscreenObserver : public WebContentsObserver {
 
   content::RenderWidgetHostView* const fullscreenView =
       contents_->GetFullscreenRenderWidgetHostView();
-  if (fullscreenView)
-    [fullscreenView->GetNativeView() setFrame:[self frameForContentsView]];
+  if (fullscreenView) {
+    [fullscreenView->GetNativeView()
+        setFrame:[self frameForContentsViewIn:[self view]]];
+  }
 }
 
 - (void)changeWebContents:(WebContents*)newContents {
@@ -329,14 +336,14 @@ class FullscreenObserver : public WebContentsObserver {
   // the view is different.
   if ([self webContents] != updatedContents) {
     [self changeWebContents:updatedContents];
-    [self ensureContentsVisible];
+    [self addContentsToView:nil];

[ccameron, 2016/07/12 18:56:58]

[self ensureContentsVisibleWithSuperview:[[self view] superview]]

[tapted, 2016/07/13 03:33:18]

Done.

   }
 }
 
 - (void)toggleFullscreenWidget:(BOOL)enterFullscreen {
   isEmbeddingFullscreenWidget_ = enterFullscreen &&
       contents_ && contents_->GetFullscreenRenderWidgetHostView();
-  [self ensureContentsVisible];
+  [self addContentsToView:nil];

[ccameron, 2016/07/12 18:56:58]

ditto.

[tapted, 2016/07/13 03:33:18]

Done.

 }
 
 - (BOOL)contentsInFullscreenCaptureMode {
@@ -353,11 +360,8 @@ class FullscreenObserver : public WebContentsObserver {
   return YES;
 }
 
-- (NSRect)frameForContentsView {
-  const NSSize containerSize = [[self view] frame].size;
-  gfx::Rect rect;
-  rect.set_width(containerSize.width);
-  rect.set_height(containerSize.height);
+- (NSRect)frameForContentsViewIn:(NSView*)container {
+  gfx::Rect rect([container bounds]);
 
   // In most cases, the contents view is simply sized to fill the container
   // view's bounds. Only WebContentses that are in fullscreen mode and being