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

chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.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.
 
 #import "chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h"
 
 #include <stdint.h>
 
 #include <utility>
 
@@ skipping 53 matching lines @@
   }
 
  private:
   TabContentsController* const controller_;
 
   DISALLOW_COPY_AND_ASSIGN(FullscreenObserver);
 };
 
 @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;
 
 // Returns YES if the content view is inside a popup.
 - (BOOL)isPopup;
 
 @end
 
 // An NSView with special-case handling for when the contents view does not
@@ skipping 36 matching lines @@
 - (void)resizeSubviewsWithOldSize:(NSSize)oldBoundsSize {
   NSView* const contentsView =
       [[self subviews] count] > 0 ? [[self subviews] objectAtIndex:0] : nil;
   if (!contentsView || [contentsView autoresizingMask] == NSViewNotSizable ||
       !delegate_ ||
       (![delegate_ shouldResizeContentView] && [delegate_ isPopup])) {
     return;
   }
 
   ScopedCAActionDisabler disabler;
-  [contentsView setFrame:[delegate_ frameForContentsView]];
+  [contentsView setFrame:[delegate_ frameForContentsViewIn:self]];
 }
 
 // Update the background layer's color whenever the view needs to repaint.
 - (void)setNeedsDisplayInRect:(NSRect)rect {
   [super setNeedsDisplayInRect:rect];
   [self updateBackgroundColor];
 }
 
 - (void)updateBackgroundColor {
   // This view is sometimes flashed into visibility (e.g, when closing
@@ skipping 66 matching lines @@
   [super dealloc];
 }
 
 - (void)loadView {
   base::scoped_nsobject<NSView> view(
       [[TabContentsContainerView alloc] initWithDelegate:self]);
   [view setAutoresizingMask:NSViewHeightSizable|NSViewWidthSizable];
   [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 =
       isEmbeddingFullscreenWidget_ ?
       contents_->GetFullscreenRenderWidgetHostView() : NULL;
   if (fullscreenView) {
     contentsNativeView = fullscreenView->GetNativeView();
   } else {
     isEmbeddingFullscreenWidget_ = NO;
     contentsNativeView = contents_->GetNativeView();
   }
 
   if ([self shouldResizeContentView])
-    [contentsNativeView setFrame:[self frameForContentsView]];
+    [contentsNativeView setFrame:[self frameForContentsViewIn:superview]];
 
   if ([subviews count] == 0) {
     [contentsContainer addSubview:contentsNativeView];
   } else if ([subviews objectAtIndex:0] != contentsNativeView) {
     [contentsContainer replaceSubview:[subviews objectAtIndex:0]
                                  with:contentsNativeView];
   }
+
+  [contentsNativeView setAutoresizingMask:NSViewNotSizable];
+  [contentsContainer setFrame:[superview bounds]];
+  [superview addSubview:contentsContainer];
   [contentsNativeView setAutoresizingMask:NSViewWidthSizable|
                                           NSViewHeightSizable];
 
   [contentsContainer setNeedsDisplay:YES];
 
   // Push the background color down to the RenderWidgetHostView, so that if
   // there is a flash between contents appearing, it will be the theme's color,
   // not white.
   SkColor skBackgroundColor = SK_ColorWHITE;
   const ThemeProvider* theme = [[[self view] window] themeProvider];
   if (theme)
     skBackgroundColor = theme->GetColor(ThemeProperties::COLOR_NTP_BACKGROUND);
   content::RenderWidgetHostView* rwhv = contents_->GetRenderWidgetHostView();
   if (rwhv)
     rwhv->SetBackgroundColor(skBackgroundColor);
 }
 
 - (void)updateFullscreenWidgetFrame {
   // This should only apply if a fullscreen widget is embedded.
   if (!isEmbeddingFullscreenWidget_ || blockFullscreenResize_)
     return;
 
   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 {
   contents_ = newContents;
   fullscreenObserver_->Observe(contents_);
   isEmbeddingFullscreenWidget_ =
       contents_ && contents_->GetFullscreenRenderWidgetHostView();
 }
 
 // Returns YES if the tab represented by this controller is the front-most.
@@ skipping 31 matching lines @@
   // focus (for example, if the omnibox has focus instead).  The WebContents
   // logic will restore focus to the appropriate view.
 }
 
 - (void)tabDidChange:(WebContents*)updatedContents {
   // Calling setContentView: here removes any first responder status
   // the view may have, so avoid changing the view hierarchy unless
   // 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 {
   // Note: Grab a known-valid WebContents pointer from |fullscreenObserver_|.
   content::WebContents* const wc = fullscreenObserver_->web_contents();
   if (!wc ||
       wc->GetCapturerCount() == 0 ||
       wc->GetPreferredSize().IsEmpty() ||
       !(isEmbeddingFullscreenWidget_ ||
         (wc->GetDelegate() &&
          wc->GetDelegate()->IsFullscreenForTabOrPending(wc)))) {
     return NO;
   }
   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
   // screen-captured will engage the special layout/sizing behavior.
   if (![self contentsInFullscreenCaptureMode])
     return NSRectFromCGRect(rect.ToCGRect());
 
   // Size the contents view to the capture video resolution and center it. If
   // the container view is not large enough to fit it at the preferred size,
   // scale down to fit (preserving aspect ratio).
@@ skipping 24 matching lines @@
 
 - (BOOL)shouldResizeContentView {
   return !isEmbeddingFullscreenWidget_ || !blockFullscreenResize_;
 }
 
 - (BOOL)isPopup {
   return isPopup_;
 }
 
 @end