Mac: Attach Extension NSViews to the view hierarchy before creating renderers

Extensions with background pages (e.g. Google Cast) currently have a
shorter code path when showing an extension popup that can cause the
renderer to ask for screen metrics before the hosting NSView is added to
the view hierarchy. Since the NSWindow is nil in this case, screen
metrics for the primary screen are used, and they may be the incorrect
screen for the popup.

On non-Mac, toolkit-views platforms get around this problem by deferring
creation until View::ViewHierarchyChanged is triggered. However, there
is no NSView apart from the render view on the extension-side on Mac, so
to do the same we'd either need to add one (and keep it sized
appropriately), or modify the WebContents view itself to override
[NSView viewDid/WillMoveToSuperview] and feed it through to embedders.

Instead, this CL tweaks the ExtensionPopupController initialization to
always create the popup NSWindow (initially hidden), before creating the
ExtensionViewHost.

BUG=324748, 305620
TEST=On a retina mac, plug in a (non-retina) external monitor, move
Chrome there, and open the Chromecast dialog. It should look "nice" (not
as nice as the retina screen, but consistent with other text in Chrome).
See http://crbug.com/324748#c41

Review URL: https://codereview.chromium.org/653843002

Cr-Commit-Position: refs/heads/master@{#317011}

chrome/browser/extensions/extension_view.h

@@ -25,9 +25,6 @@ class ExtensionView {
  public:
   virtual ~ExtensionView() {}
 
-  // Initialize the view, once a newly created view has been set in the host.
-  virtual void Init() = 0;
-
   // If attached to a Browser (e.g. popups), the Browser it is attached to.
   virtual Browser* GetBrowser() = 0;
 

chrome/browser/extensions/extension_view_host.cc

@@ -77,7 +77,6 @@ ExtensionViewHost::~ExtensionViewHost() {
 
 void ExtensionViewHost::CreateView(Browser* browser) {
   view_ = CreateExtensionView(this, browser);
-  view_->Init();
 }
 
 void ExtensionViewHost::SetAssociatedWebContents(WebContents* web_contents) {

chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm

@@ -46,12 +46,13 @@ CGFloat Clamp(CGFloat value, CGFloat min, CGFloat max) {
 
 @interface ExtensionPopupController (Private)
 // Callers should be using the public static method for initialization.
-// NOTE: This takes ownership of |host|.
-- (id)initWithHost:(extensions::ExtensionViewHost*)host
-      parentWindow:(NSWindow*)parentWindow
-        anchoredAt:(NSPoint)anchoredAt
-     arrowLocation:(info_bubble::BubbleArrowLocation)arrowLocation
-           devMode:(BOOL)devMode;
+- (id)initWithParentWindow:(NSWindow*)parentWindow
+                anchoredAt:(NSPoint)anchoredAt
+             arrowLocation:(info_bubble::BubbleArrowLocation)arrowLocation
+                   devMode:(BOOL)devMode;
+
+// Set the ExtensionViewHost, taking ownership.
+- (void)setExtensionViewHost:(scoped_ptr<extensions::ExtensionViewHost>)host;
 
 // Called when the extension's hosted NSView has been resized.
 - (void)extensionViewFrameChanged;
@@ -150,11 +151,10 @@ class DevtoolsNotificationBridge : public content::NotificationObserver {
 
 @synthesize extensionId = extensionId_;
 
-- (id)initWithHost:(extensions::ExtensionViewHost*)host
-      parentWindow:(NSWindow*)parentWindow
-        anchoredAt:(NSPoint)anchoredAt
-     arrowLocation:(info_bubble::BubbleArrowLocation)arrowLocation
-           devMode:(BOOL)devMode {
+- (id)initWithParentWindow:(NSWindow*)parentWindow
+                anchoredAt:(NSPoint)anchoredAt
+             arrowLocation:(info_bubble::BubbleArrowLocation)arrowLocation
+                   devMode:(BOOL)devMode {
   base::scoped_nsobject<InfoBubbleWindow> window([[InfoBubbleWindow alloc]
       initWithContentRect:ui::kWindowSizeDeterminedLater
                 styleMask:NSBorderlessWindowMask
@@ -167,36 +167,9 @@ class DevtoolsNotificationBridge : public content::NotificationObserver {
   if ((self = [super initWithWindow:window
                        parentWindow:parentWindow
                          anchoredAt:anchoredAt])) {
-    host_.reset(host);
-    extensionId_ = host_->extension_id();
     beingInspected_ = devMode;
     ignoreWindowDidResignKey_ = NO;
-
-    InfoBubbleView* view = self.bubble;
-    [view setArrowLocation:arrowLocation];
-
-    extensionView_ = host->view()->GetNativeView();
-    container_.reset(new ExtensionPopupContainer(self));
-    static_cast<ExtensionViewMac*>(host->view())
-        ->set_container(container_.get());
-
-    NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
-    [center addObserver:self
-               selector:@selector(extensionViewFrameChanged)
-                   name:NSViewFrameDidChangeNotification
-                 object:extensionView_];
-
-    [view addSubview:extensionView_];
-
-    notificationBridge_.reset(new DevtoolsNotificationBridge(self));
-    registrar_.reset(new content::NotificationRegistrar);
-    if (beingInspected_) {
-      // Listen for the extension to finish loading so the dev tools can be
-      // opened.
-      registrar_->Add(notificationBridge_.get(),
-                      extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
-                      content::Source<BrowserContext>(host->browser_context()));
-    }
+    [[self bubble] setArrowLocation:arrowLocation];
   }
   return self;
 }
@@ -285,29 +258,25 @@ class DevtoolsNotificationBridge : public content::NotificationObserver {
   // Make Mac behavior the same with Windows and others.
   if (gPopup) {
     std::string extension_id = url.host();
-    extensions::ExtensionViewHost* host = [gPopup extensionViewHost];
-    if (extension_id == host->extension_id()) {
-      [gPopup close];
+    std::string old_extension_id = [gPopup extensionViewHost]->extension_id();
+    [gPopup close];  // Starts the animation to fade out the popup.
+    if (extension_id == old_extension_id)
       return nil;
-    }
   }
 
-  extensions::ExtensionViewHost* host =
-      extensions::ExtensionViewHostFactory::CreatePopupHost(url, browser);
-  DCHECK(host);
-  if (!host)
-    return nil;
-
-  [gPopup close];
-
-  // Takes ownership of |host|. Also will autorelease itself when the popup is
-  // closed, so no need to do that here.
+  // Create the popup first. This establishes an initially hidden NSWindow so
+  // that the renderer is able to gather correct screen metrics for the initial
+  // paint.
   gPopup = [[ExtensionPopupController alloc]
-      initWithHost:host
-      parentWindow:browser->window()->GetNativeWindow()
-        anchoredAt:anchoredAt
-     arrowLocation:arrowLocation
-           devMode:devMode];
+      initWithParentWindow:browser->window()->GetNativeWindow()
+                anchoredAt:anchoredAt
+             arrowLocation:arrowLocation
+                   devMode:devMode];
+
+  scoped_ptr<extensions::ExtensionViewHost> host(
+      extensions::ExtensionViewHostFactory::CreatePopupHost(url, browser));
+  DCHECK(host);
+  [gPopup setExtensionViewHost:host.Pass()];
   return gPopup;
 }
 
@@ -315,6 +284,36 @@ class DevtoolsNotificationBridge : public content::NotificationObserver {
   return gPopup;
 }
 
+- (void)setExtensionViewHost:(scoped_ptr<extensions::ExtensionViewHost>)host {
+  DCHECK(!host_);
+  DCHECK(host);
+  host_.swap(host);
+
+  extensionId_ = host_->extension_id();
+  container_.reset(new ExtensionPopupContainer(self));
+  ExtensionViewMac* hostView = static_cast<ExtensionViewMac*>(host_->view());
+  hostView->set_container(container_.get());
+  hostView->CreateWidgetHostViewIn([self bubble]);
+
+  extensionView_ = hostView->GetNativeView();
+
+  NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
+  [center addObserver:self
+             selector:@selector(extensionViewFrameChanged)
+                 name:NSViewFrameDidChangeNotification
+               object:extensionView_];
+
+  notificationBridge_.reset(new DevtoolsNotificationBridge(self));
+  registrar_.reset(new content::NotificationRegistrar);
+  if (beingInspected_) {
+    // Listen for the extension to finish loading so the dev tools can be
+    // opened.
+    registrar_->Add(notificationBridge_.get(),
+                    extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
+                    content::Source<BrowserContext>(host_->browser_context()));
+  }
+}
+
 - (void)extensionViewFrameChanged {
   // If there are no changes in the width or height of the frame, then ignore.
   if (NSEqualSizes([extensionView_ frame].size, extensionFrame_.size))
@@ -382,7 +381,7 @@ class DevtoolsNotificationBridge : public content::NotificationObserver {
   // When we update the size, the window will become visible. Stay hidden until
   // the host is loaded.
   pendingSize_ = newSize;
-  if (!host_->did_stop_loading())
+  if (!host_ || !host_->did_stop_loading())
     return;
 
   // No need to use CA here, our caller calls us repeatedly to animate the

chrome/browser/ui/cocoa/extensions/extension_view_mac.h

@@ -53,8 +53,10 @@ class ExtensionViewMac : public extensions::ExtensionView {
   // Informs the view that its containing window's frame changed.
   void WindowFrameChanged();
 
+  // Create the host view, adding it as a subview of |superview|.
+  void CreateWidgetHostViewIn(gfx::NativeView superview);
+
   // extensions::ExtensionView:
-  void Init() override;
   Browser* GetBrowser() override;
   gfx::NativeView GetNativeView() override;
   void ResizeDueToAutoResize(const gfx::Size& new_size) override;
@@ -67,8 +69,6 @@ class ExtensionViewMac : public extensions::ExtensionView {
  private:
   content::RenderViewHost* render_view_host() const;
 
-  void CreateWidgetHostView();
-
   // We wait to show the ExtensionView until several things have loaded.
   void ShowIfCompletelyLoaded();
 

chrome/browser/ui/cocoa/extensions/extension_view_mac.mm

@@ -38,8 +38,9 @@ void ExtensionViewMac::WindowFrameChanged() {
     render_view_host()->GetView()->WindowFrameChanged();
 }
 
-void ExtensionViewMac::Init() {
-  CreateWidgetHostView();
+void ExtensionViewMac::CreateWidgetHostViewIn(gfx::NativeView superview) {
+  [superview addSubview:GetNativeView()];
+  extension_host_->CreateRenderViewSoon();
 }
 
 Browser* ExtensionViewMac::GetBrowser() {
@@ -89,10 +90,6 @@ content::RenderViewHost* ExtensionViewMac::render_view_host() const {
   return extension_host_->render_view_host();
 }
 
-void ExtensionViewMac::CreateWidgetHostView() {
-  extension_host_->CreateRenderViewSoon();
-}
-
 void ExtensionViewMac::ShowIfCompletelyLoaded() {
   // We wait to show the ExtensionView until it has loaded, and the view has
   // actually been created. These can happen in different orders.

chrome/browser/ui/views/extensions/extension_view_views.cc

@@ -81,10 +81,6 @@ void ExtensionViewViews::SetIsClipped(bool is_clipped) {
   }
 }
 
-void ExtensionViewViews::Init() {
-  // Initialization continues in ViewHierarchyChanged().
-}
-
 Browser* ExtensionViewViews::GetBrowser() {
   return browser_;
 }

chrome/browser/ui/views/extensions/extension_view_views.h

@@ -56,7 +56,6 @@ class ExtensionViewViews : public views::NativeViewHost,
   void SetIsClipped(bool is_clipped);
 
   // extensions::ExtensionView:
-  void Init() override;
   Browser* GetBrowser() override;
   gfx::NativeView GetNativeView() override;
   void ResizeDueToAutoResize(const gfx::Size& new_size) override;