Includes toolbar height in PWA popup window size calculation.

When window.open is called, the created app window doesn't know
what url it will be opening, so we can't determine if the toolbar
will be shown. Thus, once the web contents starts a navigation,
the window bounds must be checked for correctness.

This CL adds a property 'initial_url' function to
AppBrowserController. This will be used in a future CL to determine
whether or not to show the 'X' button on the custom tab bar (popups
opened out of scope should not show it).

Before and after screenshots for an in and out of scope popup.
The popups should both have their web contents sized at 480x300

Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=394814&signed_aid=zqCH2pYzkhISnPQRiPjzuQ==&inline=1
After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=394815&signed_aid=owOrcmaGQWpKcub8E8orNw==&inline=1

Bug: 777854
Change-Id: I893c21d0c13a12b079eb578e68f1167bf335be8e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1626058
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664129}

chrome/browser/ui/browser_window.h

@@ -241,6 +241,10 @@ class BrowserWindow : public ui::BaseWindow {
   // the TabStripModel has an active tab.
   virtual gfx::Size GetContentsSize() const = 0;
 
+  // Resizes the window to fit a WebContents of a certain size. This should only
+  // be called after the TabStripModel has an active tab.
+  virtual void SetContentsSize(const gfx::Size& size) = 0;
+
   // Returns the container of page action icons.
   virtual PageActionIconContainer* GetOmniboxPageActionIconContainer() = 0;
 

chrome/browser/ui/extensions/hosted_app_browser_controller.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/browser_window_state.h"
 #include "chrome/browser/ui/location_bar/location_bar.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
@@ -151,8 +152,7 @@ bool HostedAppBrowserController::ShouldShowToolbar() const {
   content::WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
 
-  // Don't show a toolbar until a navigation has occurred.
-  if (!web_contents || web_contents->GetLastCommittedURL().is_empty())
+  if (!web_contents)
     return false;
 
   GURL launch_url = AppLaunchInfo::GetLaunchWebURL(extension);
@@ -161,13 +161,6 @@ bool HostedAppBrowserController::ShouldShowToolbar() const {
   bool is_internal_launch_scheme = launch_scheme == kExtensionScheme ||
                                    launch_scheme == content::kChromeUIScheme;
 
-  GURL last_committed_url = web_contents->GetLastCommittedURL();
-
-  // We check the visible URL to indicate to the user that they are navigating
-  // to a different origin than that of the app as soon as the navigation
-  // starts, even if the navigation hasn't committed yet.
-  GURL visible_url = web_contents->GetVisibleURL();
-
   // The current page must be secure for us to hide the toolbar. However,
   // the chrome-extension:// and chrome:// launch URL apps can hide the toolbar,
   // if the current WebContents URLs are the same as the launch scheme.
@@ -177,28 +170,44 @@ bool HostedAppBrowserController::ShouldShowToolbar() const {
   base::StringPiece secure_page_scheme =
       is_internal_launch_scheme ? launch_scheme : url::kHttpsScheme;
 
-  // Insecure page schemes show the toolbar.
-  if (last_committed_url.scheme_piece() != secure_page_scheme ||
-      visible_url.scheme_piece() != secure_page_scheme) {
-    return true;
-  }
+  auto should_show_toolbar_for_url = [&](const GURL& url) -> bool {
+    // If the url is unset, it doesn't give a signal as to whether the toolbar
+    // should be shown or not. In lieu of more information, do not show the
+    // toolbar.
+    if (url.is_empty())
+      return false;
 
-  if (IsForSystemWebApp()) {
-    DCHECK_EQ(last_committed_url.scheme_piece(), content::kChromeUIScheme);
-    return false;
-  }
+    if (url.scheme_piece() != secure_page_scheme)
+      return true;
+
+    if (IsForSystemWebApp()) {
+      DCHECK_EQ(url.scheme_piece(), content::kChromeUIScheme);
+      return false;
+    }
 
-  // Page URLs that are not within scope
-  // (https://www.w3.org/TR/appmanifest/#dfn-within-scope) of the app
-  // corresponding to |launch_url| show the toolbar.
-  if (!IsSameScope(launch_url, last_committed_url,
-                   web_contents->GetBrowserContext()) ||
-      !IsSameScope(launch_url, visible_url, web_contents->GetBrowserContext()))
+    // Page URLs that are not within scope
+    // (https://www.w3.org/TR/appmanifest/#dfn-within-scope) of the app
+    // corresponding to |launch_url| show the toolbar.
+    return !IsSameScope(launch_url, url, web_contents->GetBrowserContext());
+  };
+
+  GURL visible_url = web_contents->GetVisibleURL();
+  GURL last_committed_url = web_contents->GetLastCommittedURL();
+
+  if (last_committed_url.is_empty() && visible_url.is_empty())
+    return should_show_toolbar_for_url(initial_url());
+
+  if (should_show_toolbar_for_url(visible_url) ||
+      should_show_toolbar_for_url(last_committed_url)) {
     return true;
+  }
 
   // Insecure external web sites show the toolbar.
-  if (!is_internal_launch_scheme && !IsSiteSecure(web_contents))
+  // Note: IsSiteSecure is false until a url is committed.
+  if (!last_committed_url.is_empty() && !is_internal_launch_scheme &&
+      !IsSiteSecure(web_contents)) {
     return true;
+  }
 
   return false;
 }
@@ -308,6 +317,21 @@ bool HostedAppBrowserController::IsInstalled() const {
   return GetExtension();
 }
 
+void HostedAppBrowserController::OnReceivedInitialURL() {
+  UpdateToolbarVisibility(false);
+
+  // If the window bounds have not been overridden, there is no need to resize
+  // the window.
+  if (!browser()->bounds_overridden())
+    return;
+
+  DCHECK(chrome::SavedBoundsAreContentBounds(browser()));
+  // TODO(crbug.com/964825): Correctly set the window size at creation time.
+  // This is currently not possible because the current url is not easily known
+  // at popup construction time.
+  browser()->window()->SetContentsSize(browser()->override_bounds().size());
+}
+
 void HostedAppBrowserController::OnTabInserted(content::WebContents* contents) {
   AppBrowserController::OnTabInserted(contents);
   extensions::HostedAppBrowserController::SetAppPrefsForWebContents(this,

chrome/browser/ui/extensions/hosted_app_browser_controller.h

@@ -98,6 +98,7 @@ class HostedAppBrowserController : public ExtensionUninstallDialog::Delegate,
   bool IsHostedApp() const override;
 
  protected:
+  void OnReceivedInitialURL() override;
   void OnTabInserted(content::WebContents* contents) override;
   void OnTabRemoved(content::WebContents* contents) override;
 

chrome/browser/ui/extensions/hosted_app_browsertest.cc

@@ -1150,6 +1150,36 @@ IN_PROC_BROWSER_TEST_P(HostedAppPWAOnlyTest, OffScopePWAPopupsHaveCorrectSize) {
   EXPECT_EQ(size, popup_browser->window()->GetContentsSize());
 }
 
+// Tests that using window.open to create a popup window in scope results in
+// a correctly sized window.
+IN_PROC_BROWSER_TEST_P(HostedAppPWAOnlyTest, InScopePWAPopupsHaveCorrectSize) {
+  ASSERT_TRUE(https_server()->Start());
+
+  InstallSecurePWA();
+
+  LaunchApp();
+  EXPECT_TRUE(web_app::AppBrowserController::IsForWebAppBrowser(app_browser_));
+
+  gfx::Size size(500, 500);
+  Browser* popup_browser =
+      OpenPopupAndWait(app_browser_, GetSecureAppURL(), size);
+
+  // The navigation should have happened in a new window.
+  EXPECT_NE(popup_browser, app_browser_);
+
+  // The popup browser should be a PWA.
+  EXPECT_TRUE(web_app::AppBrowserController::IsForWebAppBrowser(popup_browser));
+
+  // Toolbar should not be shown, as the popup is in scope.
+  EXPECT_FALSE(popup_browser->app_controller()->ShouldShowToolbar());
+
+  // Skip animating the toolbar visibility.
+  popup_browser->app_controller()->UpdateToolbarVisibility(false);
+
+  // The popup window should be the size we specified.
+  EXPECT_EQ(size, popup_browser->window()->GetContentsSize());
+}
+
 // Tests that desktop PWAs open links in the browser.
 IN_PROC_BROWSER_TEST_P(HostedAppPWAOnlyTest,
                        DesktopPWAsOpenLinksInBrowserWhenFeatureDisabled) {

chrome/browser/ui/views/frame/browser_view.cc

@@ -967,6 +967,25 @@ gfx::Size BrowserView::GetContentsSize() const {
   return contents_web_view_->size();
 }
 
+void BrowserView::SetContentsSize(const gfx::Size& size) {
+  DCHECK(!GetContentsSize().IsEmpty());
+
+  const int width_diff = size.width() - GetContentsSize().width();
+  const int height_diff = size.height() - GetContentsSize().height();
+
+  // Resizing the window may be expensive, so only do it if the size is wrong.
+  if (width_diff == 0 && height_diff == 0)
+    return;
+
+  gfx::Rect bounds = GetBounds();
+  bounds.set_width(bounds.width() + width_diff);
+  bounds.set_height(bounds.height() + height_diff);
+  SetBounds(bounds);
+
+  DCHECK_EQ(GetContentsSize().width(), size.width());
+  DCHECK_EQ(GetContentsSize().height(), size.height());
+}
+
 bool BrowserView::IsMaximized() const {
   return frame_->IsMaximized();
 }

chrome/browser/ui/views/frame/browser_view.h

@@ -322,6 +322,7 @@ class BrowserView : public BrowserWindow,
   ui::WindowShowState GetRestoredState() const override;
   gfx::Rect GetBounds() const override;
   gfx::Size GetContentsSize() const override;
+  void SetContentsSize(const gfx::Size& size) override;
   bool IsMaximized() const override;
   bool IsMinimized() const override;
   void Maximize() override;

chrome/browser/ui/web_applications/app_browser_controller.cc

@@ -16,6 +16,7 @@
 #include "components/url_formatter/url_formatter.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/escape.h"
 #include "url/gurl.h"
@@ -105,6 +106,17 @@ bool AppBrowserController::IsForSystemWebApp() const {
       .IsSystemWebApp(*GetAppId());
 }
 
+void AppBrowserController::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!initial_url().is_empty())
+    return;
+  if (!navigation_handle->IsInMainFrame())
+    return;
+  if (navigation_handle->GetURL().is_empty())
+    return;
+  SetInitialURL(navigation_handle->GetURL());
+}
+
 void AppBrowserController::DidChangeThemeColor(
     base::Optional<SkColor> theme_color) {
   browser_->window()->UpdateFrameColor();
@@ -157,6 +169,9 @@ void AppBrowserController::OnTabInserted(content::WebContents* contents) {
   DCHECK(!web_contents()) << " App windows are single tabbed only";
   content::WebContentsObserver::Observe(contents);
   DidChangeThemeColor(GetThemeColor());
+
+  if (!contents->GetVisibleURL().is_empty())
+    SetInitialURL(contents->GetVisibleURL());
 }
 
 void AppBrowserController::OnTabRemoved(content::WebContents* contents) {
@@ -164,4 +179,11 @@ void AppBrowserController::OnTabRemoved(content::WebContents* contents) {
   content::WebContentsObserver::Observe(nullptr);
 }
 
+void AppBrowserController::SetInitialURL(const GURL& initial_url) {
+  DCHECK(initial_url_.is_empty());
+  initial_url_ = initial_url;
+
+  OnReceivedInitialURL();
+}
+
 }  // namespace web_app

chrome/browser/ui/web_applications/app_browser_controller.h

@@ -95,7 +95,12 @@ class AppBrowserController : public TabStripModelObserver,
 
   Browser* browser() const { return browser_; }
 
+  // Gets the url that the app browser controller was created with. Note: This
+  // may be empty until the web contents begins navigating.
+  const GURL& initial_url() const { return initial_url_; }
+
   // content::WebContentsObserver:
+  void DidStartNavigation(content::NavigationHandle* handle) override;
   void DidChangeThemeColor(base::Optional<SkColor> theme_color) override;
 
   // TabStripModelObserver:
@@ -106,12 +111,20 @@ class AppBrowserController : public TabStripModelObserver,
 
  protected:
   explicit AppBrowserController(Browser* browser);
+
+  // Called once the app browser controller has determined its initial url.
+  virtual void OnReceivedInitialURL() {}
+
   // Called by OnTabstripModelChanged().
   virtual void OnTabInserted(content::WebContents* contents);
   virtual void OnTabRemoved(content::WebContents* contents);
 
  private:
+  // Sets the url that the app browser controller was created with.
+  void SetInitialURL(const GURL& initial_url);
+
   Browser* const browser_;
+  GURL initial_url_;
 
   DISALLOW_COPY_AND_ASSIGN(AppBrowserController);
 };

chrome/test/base/test_browser_window.cc

@@ -108,6 +108,8 @@ gfx::Size TestBrowserWindow::GetContentsSize() const {
   return gfx::Size();
 }
 
+void TestBrowserWindow::SetContentsSize(const gfx::Size& size) {}
+
 bool TestBrowserWindow::IsMaximized() const {
   return false;
 }

chrome/test/base/test_browser_window.h

@@ -84,6 +84,7 @@ class TestBrowserWindow : public BrowserWindow {
   ui::WindowShowState GetRestoredState() const override;
   gfx::Rect GetBounds() const override;
   gfx::Size GetContentsSize() const override;
+  void SetContentsSize(const gfx::Size& size) override;
   bool IsMaximized() const override;
   bool IsMinimized() const override;
   void Maximize() override {}