Lots of non-functional cleanup to ExtensionView{Host,Views} and related.

* DISALLOW_COPY_AND_ASSIGN() -> explicit deletions
* Remove using declarations
* Use =default
* Use in-declaration initializers
* NULL -> nullptr
* Combine ExtensionViewHost::CreateView() with constructor
* Use const and auto more
* Use make_unique instead of bare new
* Shorten code
* Move most of ExtensionDialog::Show() into the constructor
* Combine ExtensionDialog::InitWindow() with constructor
* Replace GetExtensionView() functions with |extension_view_| members,
  since the return value is constant (and its source will change in an
  upcoming CL)
* Remove always-true condition from ExtensionDialog::CanResize()
  (size was always set)
* Consistently use |host_|, not host(), in ExtensionPopup
* Have ExtensionViewViews obtain the relevant profile from the |host|
  instead of having it explicitly passed in
* For clarity, make ExtensionViewViews::minimum_size_ an Optional
  instead of treating Size() as a magic value that means "not set"
* Eliminate unnecessary friend declaration

Bug: none
Change-Id: I2733f7460b09c07ada3fcae819d71bee960886ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209593
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770603}

chrome/browser/extensions/extension_view_host.h

@@ -16,7 +16,6 @@
 #include "extensions/browser/extension_host.h"
 
 class Browser;
-class Profile;
 
 namespace content {
 class SiteInstance;
@@ -36,21 +35,17 @@ class ExtensionViewHost
       public web_modal::WebContentsModalDialogHost,
       public content::NotificationObserver {
  public:
+  // |browser| may be null, since extension views may be bound to TabContents
+  // hosted in ExternalTabContainer objects, which do not instantiate Browsers.
   ExtensionViewHost(const Extension* extension,
                     content::SiteInstance* site_instance,
                     const GURL& url,
-                    ViewType host_type);
+                    ViewType host_type,
+                    Browser* browser);
   ~ExtensionViewHost() override;
 
   Browser* browser() { return browser_; }
   ExtensionView* view() { return view_.get(); }
-  const ExtensionView* view() const { return view_.get(); }
-
-  // Create an ExtensionView and tie it to this host and |browser|.  Note NULL
-  // is a valid argument for |browser|.  Extension views may be bound to
-  // tab-contents hosted in ExternalTabContainer objects, which do not
-  // instantiate Browser objects.
-  void CreateView(Browser* browser);
 
   void SetAssociatedWebContents(content::WebContents* web_contents);
 
@@ -118,13 +113,16 @@ class ExtensionViewHost
  private:
   // Implemented per-platform. Create the platform-specific ExtensionView.
   static std::unique_ptr<ExtensionView> CreateExtensionView(
-      ExtensionViewHost* host,
-      Profile* profile);
+      ExtensionViewHost* host);
+
+  // Returns whether the provided event is a raw escape keypress in a
+  // VIEW_TYPE_EXTENSION_POPUP.
+  bool IsEscapeInPopup(const content::NativeWebKeyboardEvent& event) const;
 
   // The browser associated with the ExtensionView, if any.
-  Browser* browser_ = nullptr;
+  Browser* browser_;
 
-  // Optional view that shows the rendered content in the UI.
+  // View that shows the rendered content in the UI.
   std::unique_ptr<ExtensionView> view_;
 
   // The relevant WebContents associated with this ExtensionViewHost, if any.

chrome/browser/extensions/extension_view_host_factory.cc

@@ -34,10 +34,8 @@ std::unique_ptr<ExtensionViewHost> CreateViewHostForExtension(
   DCHECK(browser || view_type == VIEW_TYPE_EXTENSION_DIALOG);
   scoped_refptr<content::SiteInstance> site_instance =
       ProcessManager::Get(profile)->GetSiteInstanceForURL(url);
-  auto host = std::make_unique<ExtensionViewHost>(
-      extension, site_instance.get(), url, view_type);
-  host->CreateView(browser);
-  return host;
+  return std::make_unique<ExtensionViewHost>(extension, site_instance.get(),
+                                             url, view_type, browser);
 }
 
 // Creates a view host for an extension in an incognito window. Returns NULL

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

@@ -34,9 +34,6 @@
 #include "ui/aura/window.h"
 #endif
 
-using content::BrowserContext;
-using content::WebContents;
-
 ExtensionDialog::InitParams::InitParams(gfx::Size size)
     : size(std::move(size)) {}
 ExtensionDialog::InitParams::InitParams(const InitParams& other) = default;
@@ -46,43 +43,28 @@ ExtensionDialog::InitParams::~InitParams() = default;
 ExtensionDialog* ExtensionDialog::Show(const GURL& url,
                                        gfx::NativeWindow parent_window,
                                        Profile* profile,
-                                       WebContents* web_contents,
+                                       content::WebContents* web_contents,
                                        ExtensionDialogObserver* observer,
                                        const InitParams& init_params) {
+  DCHECK(parent_window);
+
   std::unique_ptr<extensions::ExtensionViewHost> host =
       extensions::ExtensionViewHostFactory::CreateDialogHost(url, profile);
   if (!host)
     return nullptr;
-  // Preferred size must be set before views::Widget::CreateWindowWithParent()
-  // is called because CreateWindowWithParent() references CanResize().
-  ExtensionViewViews* view = GetExtensionView(host.get());
-  view->SetPreferredSize(init_params.size);
-  view->set_minimum_size(init_params.min_size);
   host->SetAssociatedWebContents(web_contents);
 
-  DCHECK(parent_window);
-  extensions::ExtensionViewHost* host_ptr = host.get();
-  ExtensionDialog* dialog = new ExtensionDialog(std::move(host), observer);
-  dialog->set_title(init_params.title);
-  dialog->InitWindow(parent_window, init_params);
-
-  // Show a white background while the extension loads.  This is prettier than
-  // flashing a black unfilled window frame.
-  view->SetBackground(views::CreateSolidBackground(SK_ColorWHITE));
-  view->SetVisible(true);
-
-  // Ensure the DOM JavaScript can respond immediately to keyboard shortcuts.
-  host_ptr->host_contents()->Focus();
-  return dialog;
+  return new ExtensionDialog(std::move(host), observer, parent_window,
+                             init_params);
 }
 
 void ExtensionDialog::ObserverDestroyed() {
-  observer_ = NULL;
+  observer_ = nullptr;
 }
 
 void ExtensionDialog::MaybeFocusRenderView() {
   views::FocusManager* focus_manager = GetWidget()->GetFocusManager();
-  DCHECK(focus_manager != NULL);
+  DCHECK(focus_manager);
 
   // Already there's a focused view, so no need to switch the focus.
   if (focus_manager->GetFocusedView())
@@ -97,7 +79,7 @@ void ExtensionDialog::MaybeFocusRenderView() {
 }
 
 void ExtensionDialog::SetMinimumContentsSize(int width, int height) {
-  GetExtensionView()->SetPreferredSize(gfx::Size(width, height));
+  extension_view_->SetPreferredSize(gfx::Size(width, height));
 }
 
 bool ExtensionDialog::CanResize() const {
@@ -106,8 +88,7 @@ bool ExtensionDialog::CanResize() const {
   if (ash::TabletMode::Get() && ash::TabletMode::Get()->InTabletMode())
     return false;
 #endif
-  // Can resize only if minimum contents size set.
-  return GetExtensionView()->GetPreferredSize() != gfx::Size();
+  return true;
 }
 
 ui::ModalType ExtensionDialog::GetModalType() const {
@@ -133,15 +114,23 @@ void ExtensionDialog::DeleteDelegate() {
 }
 
 views::Widget* ExtensionDialog::GetWidget() {
-  return GetExtensionView()->GetWidget();
+  return extension_view_->GetWidget();
 }
 
 const views::Widget* ExtensionDialog::GetWidget() const {
-  return GetExtensionView()->GetWidget();
+  return extension_view_->GetWidget();
 }
 
 views::View* ExtensionDialog::GetContentsView() {
-  return GetExtensionView();
+  if (!extension_view_) {
+    extension_view_ = static_cast<ExtensionViewViews*>(host_->view());
+
+    // Show a white background while the extension loads.  This is prettier than
+    // flashing a black unfilled window frame.
+    extension_view_->SetBackground(views::CreateSolidBackground(SK_ColorWHITE));
+  }
+
+  return extension_view_;
 }
 
 void ExtensionDialog::Observe(int type,
@@ -151,7 +140,7 @@ void ExtensionDialog::Observe(int type,
     case extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_FIRST_LOAD:
       // Avoid potential overdraw by removing the temporary background after
       // the extension finishes loading.
-      GetExtensionView()->SetBackground(nullptr);
+      extension_view_->SetBackground(nullptr);
       // The render view is created during the LoadURL(), so we should
       // set the focus to the view if nobody else takes the focus.
       if (content::Details<extensions::ExtensionHost>(host()) == details)
@@ -179,40 +168,44 @@ ExtensionDialog::~ExtensionDialog() = default;
 
 ExtensionDialog::ExtensionDialog(
     std::unique_ptr<extensions::ExtensionViewHost> host,
-    ExtensionDialogObserver* observer)
+    ExtensionDialogObserver* observer,
+    gfx::NativeWindow parent_window,
+    const InitParams& init_params)
     : host_(std::move(host)), observer_(observer) {
   SetButtons(ui::DIALOG_BUTTON_NONE);
   set_use_custom_frame(false);
 
   AddRef();  // Balanced in DeleteDelegate();
 
+  const content::Source<content::BrowserContext> source =
+      host_->browser_context();
   registrar_.Add(this,
                  extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_FIRST_LOAD,
-                 content::Source<BrowserContext>(host_->browser_context()));
+                 source);
   // Listen for the containing view calling window.close();
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE,
-                 content::Source<BrowserContext>(host_->browser_context()));
+  registrar_.Add(
+      this, extensions::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE, source);
   // Listen for a crash or other termination of the extension process.
   registrar_.Add(this, extensions::NOTIFICATION_EXTENSION_PROCESS_TERMINATED,
-                 content::Source<BrowserContext>(host_->browser_context()));
+                 source);
   chrome::RecordDialogCreation(chrome::DialogIdentifier::EXTENSION);
-}
 
-void ExtensionDialog::InitWindow(gfx::NativeWindow parent,
-                                 const InitParams& init_params) {
+  set_title(init_params.title);
+
   views::Widget* window =
       init_params.is_modal
-          ? constrained_window::CreateBrowserModalDialogViews(this, parent)
-          : views::DialogDelegate::CreateDialogWidget(
-                this, nullptr /* context */, nullptr /* parent */);
+          ? constrained_window::CreateBrowserModalDialogViews(this,
+                                                              parent_window)
+          : views::DialogDelegate::CreateDialogWidget(this, nullptr, nullptr);
 
   // Center the window over the parent browser window or the screen.
-  gfx::Rect screen_rect =
-      display::Screen::GetScreen()->GetDisplayNearestWindow(parent).work_area();
-  gfx::Rect bounds = parent ? views::Widget::GetWidgetForNativeWindow(parent)
-                                  ->GetWindowBoundsInScreen()
-                            : screen_rect;
+  gfx::Rect screen_rect = display::Screen::GetScreen()
+                              ->GetDisplayNearestWindow(parent_window)
+                              .work_area();
+  gfx::Rect bounds =
+      parent_window ? views::Widget::GetWidgetForNativeWindow(parent_window)
+                          ->GetWindowBoundsInScreen()
+                    : screen_rect;
   bounds.ClampToCenteredSize(init_params.size);
 
   // Make sure bounds is larger than {min_size}.
@@ -248,13 +241,14 @@ void ExtensionDialog::InitWindow(gfx::NativeWindow parent,
   window->Show();
   // TODO(jamescook): Remove redundant call to Activate()?
   window->Activate();
-}
 
-ExtensionViewViews* ExtensionDialog::GetExtensionView() const {
-  return GetExtensionView(host_.get());
-}
+  // Creating the Widget should have called GetContentsView() and created
+  // |extension_view_|.
+  DCHECK(extension_view_);
+  extension_view_->SetPreferredSize(init_params.size);
+  extension_view_->set_minimum_size(init_params.min_size);
+  extension_view_->SetVisible(true);
 
-ExtensionViewViews* ExtensionDialog::GetExtensionView(
-    extensions::ExtensionViewHost* host) {
-  return static_cast<ExtensionViewViews*>(host->view());
+  // Ensure the DOM JavaScript can respond immediately to keyboard shortcuts.
+  host_->host_contents()->Focus();
 }

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

@@ -114,21 +114,18 @@ class ExtensionDialog : public views::DialogDelegate,
 
   // Use Show() to create instances.
   ExtensionDialog(std::unique_ptr<extensions::ExtensionViewHost> host,
-                  ExtensionDialogObserver* observer);
-
-  void InitWindow(gfx::NativeWindow parent_window,
+                  ExtensionDialogObserver* observer,
+                  gfx::NativeWindow parent_window,
                   const InitParams& init_params);
 
-  ExtensionViewViews* GetExtensionView() const;
-  static ExtensionViewViews* GetExtensionView(
-      extensions::ExtensionViewHost* host);
-
   // Window Title
   base::string16 window_title_;
 
   // The contained host for the view.
   std::unique_ptr<extensions::ExtensionViewHost> host_;
 
+  ExtensionViewViews* extension_view_ = nullptr;
+
   content::NotificationRegistrar registrar_;
 
   // The observer of this popup.

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

@@ -69,7 +69,7 @@ void ExtensionPopup::AddedToWidget() {
   BubbleDialogDelegateView::AddedToWidget();
   const int radius = GetBubbleFrameView()->corner_radius();
   const bool contents_has_rounded_corners =
-      GetExtensionView()->holder()->SetCornerRadius(radius);
+      extension_view_->holder()->SetCornerRadius(radius);
   SetBorder(views::CreateEmptyBorder(
       gfx::Insets(contents_has_rounded_corners ? 0 : radius, 0)));
 }
@@ -128,7 +128,7 @@ void ExtensionPopup::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const extensions::Extension* extension,
     extensions::UnloadedExtensionReason reason) {
-  if (extension->id() == host()->extension_id()) {
+  if (extension->id() == host_->extension_id()) {
     host_.reset();
     GetWidget()->Close();
   }
@@ -138,7 +138,7 @@ void ExtensionPopup::Observe(int type,
                              const content::NotificationSource& source,
                              const content::NotificationDetails& details) {
   if (type == content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME) {
-    DCHECK_EQ(host()->host_contents(),
+    DCHECK_EQ(host_->host_contents(),
               content::Source<content::WebContents>(source).ptr());
     // Show when the content finishes loading and its width is computed.
     ShowBubble();
@@ -147,7 +147,7 @@ void ExtensionPopup::Observe(int type,
 
   DCHECK_EQ(extensions::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE, type);
   // If we aren't the host of the popup, then disregard the notification.
-  if (content::Details<extensions::ExtensionHost>(host()) == details)
+  if (content::Details<extensions::ExtensionHost>(host_.get()) == details)
     GetWidget()->Close();
 }
 
@@ -161,7 +161,7 @@ void ExtensionPopup::OnTabStripModelChanged(
 
 void ExtensionPopup::DevToolsAgentHostAttached(
     content::DevToolsAgentHost* agent_host) {
-  if (host()->host_contents() == agent_host->GetWebContents())
+  if (host_->host_contents() == agent_host->GetWebContents())
     show_action_ = SHOW_AND_INSPECT;
 }
 
@@ -171,9 +171,9 @@ void ExtensionPopup::DevToolsAgentHostDetached(
   // is uninstalled, and if DevTools are attached, we will be notified here.
   // But because OnExtensionUnloaded was already called, |host_| is
   // no longer valid.
-  if (!host())
+  if (!host_)
     return;
-  if (host()->host_contents() == agent_host->GetWebContents())
+  if (host_->host_contents() == agent_host->GetWebContents())
     show_action_ = SHOW;
 }
 
@@ -193,8 +193,10 @@ ExtensionPopup::ExtensionPopup(
 
   set_margins(gfx::Insets());
   SetLayoutManager(std::make_unique<views::FillLayout>());
-  AddChildView(GetExtensionView());
-  GetExtensionView()->set_container(this);
+
+  extension_view_ =
+      AddChildView(static_cast<ExtensionViewViews*>(host_.get()->view()));
+  extension_view_->set_container(this);
 
   // See comments in OnWidgetActivationChanged().
   set_close_on_deactivate(false);
@@ -225,11 +227,11 @@ void ExtensionPopup::ShowBubble() {
   GetWidget()->Show();
 
   // Focus on the host contents when the bubble is first shown.
-  host()->host_contents()->Focus();
+  host_->host_contents()->Focus();
 
   if (show_action_ == SHOW_AND_INSPECT) {
     DevToolsWindow::OpenDevToolsWindow(
-        host()->host_contents(), DevToolsToggleAction::ShowConsolePanel());
+        host_->host_contents(), DevToolsToggleAction::ShowConsolePanel());
   }
 }
 
@@ -237,7 +239,3 @@ void ExtensionPopup::CloseUnlessUnderInspection() {
   if (show_action_ != SHOW_AND_INSPECT)
     GetWidget()->CloseWithReason(views::Widget::ClosedReason::kLostFocus);
 }
-
-ExtensionViewViews* ExtensionPopup::GetExtensionView() {
-  return static_cast<ExtensionViewViews*>(host_.get()->view());
-}

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

@@ -128,11 +128,11 @@ class ExtensionPopup : public views::BubbleDialogDelegateView,
   // Closes the bubble if the devtools window is not attached.
   void CloseUnlessUnderInspection();
 
-  ExtensionViewViews* GetExtensionView();
-
   // The contained host for the view.
   std::unique_ptr<extensions::ExtensionViewHost> host_;
 
+  ExtensionViewViews* extension_view_;
+
   ScopedObserver<extensions::ExtensionRegistry,
                  extensions::ExtensionRegistryObserver>
       extension_registry_observer_;

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

@@ -16,7 +16,6 @@
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
-#include "extensions/browser/extension_host.h"
 #include "extensions/common/view_type.h"
 #include "ui/events/event.h"
 #include "ui/views/controls/native/native_view_host.h"
@@ -26,9 +25,9 @@
 #include "ui/base/cursor/cursor.h"
 #endif
 
-ExtensionViewViews::ExtensionViewViews(extensions::ExtensionHost* host,
-                                       Profile* profile)
-    : views::WebView(profile), host_(host), container_(nullptr) {
+ExtensionViewViews::ExtensionViewViews(extensions::ExtensionViewHost* host)
+    : views::WebView(host->browser() ? host->browser()->profile() : nullptr),
+      host_(host) {
   SetWebContents(host_->web_contents());
   if (host->extension_host_type() == extensions::VIEW_TYPE_EXTENSION_POPUP) {
     EnableSizingFromWebContents(
@@ -107,9 +106,7 @@ gfx::NativeCursor ExtensionViewViews::GetCursor(const ui::MouseEvent& event) {
 }
 
 gfx::Size ExtensionViewViews::GetMinimumSize() const {
-  // If the minimum size has never been set, returns the preferred size (same
-  // behavior as views::View).
-  return (minimum_size_ == gfx::Size()) ? GetPreferredSize() : minimum_size_;
+  return minimum_size_.value_or(GetPreferredSize());
 }
 
 void ExtensionViewViews::PreferredSizeChanged() {
@@ -127,9 +124,8 @@ namespace extensions {
 
 // static
 std::unique_ptr<ExtensionView> ExtensionViewHost::CreateExtensionView(
-    ExtensionViewHost* host,
-    Profile* profile) {
-  auto view = std::make_unique<ExtensionViewViews>(host, profile);
+    ExtensionViewHost* host) {
+  auto view = std::make_unique<ExtensionViewViews>(host);
   // We own |view_|, so don't auto delete when it's removed from the view
   // hierarchy.
   view->set_owned_by_client();

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

@@ -6,20 +6,18 @@
 #define CHROME_BROWSER_UI_VIEWS_EXTENSIONS_EXTENSION_VIEW_VIEWS_H_
 
 #include "base/compiler_specific.h"
-#include "base/macros.h"
+#include "base/optional.h"
 #include "chrome/browser/extensions/extension_view.h"
 #include "content/public/browser/native_web_keyboard_event.h"
 #include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
 #include "ui/views/controls/webview/webview.h"
 
-class Profile;
-
 namespace content {
 class RenderViewHost;
 }
 
 namespace extensions {
-class ExtensionHost;
+class ExtensionViewHost;
 }
 
 // This handles the display portion of an ExtensionHost.
@@ -30,12 +28,14 @@ class ExtensionViewViews : public views::WebView,
   // (bottom shelf, side bar, etc.)
   class Container {
    public:
-    virtual ~Container() {}
+    virtual ~Container() = default;
 
     virtual void OnExtensionSizeChanged(ExtensionViewViews* view) {}
   };
 
-  ExtensionViewViews(extensions::ExtensionHost* host, Profile* profile);
+  explicit ExtensionViewViews(extensions::ExtensionViewHost* host);
+  ExtensionViewViews(const ExtensionViewViews&) = delete;
+  ExtensionViewViews& operator=(const ExtensionViewViews&) = delete;
   ~ExtensionViewViews() override;
 
   // views::WebView:
@@ -47,8 +47,6 @@ class ExtensionViewViews : public views::WebView,
   void set_container(Container* container) { container_ = container; }
 
  private:
-  friend class extensions::ExtensionHost;
-
   // extensions::ExtensionView:
   gfx::NativeView GetNativeView() override;
   void ResizeDueToAutoResize(content::WebContents* web_contents,
@@ -66,22 +64,21 @@ class ExtensionViewViews : public views::WebView,
   void OnWebContentsAttached() override;
 
   // Note that host_ owns view
-  extensions::ExtensionHost* host_;
+  extensions::ExtensionViewHost* host_;
 
   // What we should set the preferred width to once the ExtensionViewViews has
   // loaded.
   gfx::Size pending_preferred_size_;
-  gfx::Size minimum_size_;
+
+  base::Optional<gfx::Size> minimum_size_;
 
   // The container this view is in (not necessarily its direct superview).
   // Note: the view does not own its container.
-  Container* container_;
+  Container* container_ = nullptr;
 
   // A handler to handle unhandled keyboard messages coming back from the
   // renderer process.
   views::UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_;
-
-  DISALLOW_COPY_AND_ASSIGN(ExtensionViewViews);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_EXTENSIONS_EXTENSION_VIEW_VIEWS_H_