TYPE_APP_POPUP uses 'legacy app' features not TYPE_POPUP features

With my recent crrev.com/c/2189893 to create popups from apps as
TYPE_APP_POPUP, rather than TYPE_APP, we are now showing the location
bar on these popups which is not expected.

TYPE_APP_POPUP should match the set of features from legacy apps, rather
than those of popups.  The legacy apps are already 'inheriting' from
popups, with 2 differences:

FEATURE_TITLEBAR:
Popups will show this for non-trusted sites which are not in fullscreen.
Legacy apps show this for     all     sites which are not in fullscreen.

FEATURE_LOCATIONBAR:
Popups will show this for non-trusted sites which are not in fullscreen.
Legacy apps never show this. However they will display the
CustomTabBarView when the URL navigates out of scope of the app.

This CL renames 'LegacyApp...' function to 'AppPopup...', and modifies
the TODO suggestion of having TYPE_WEB_APP and TYPE_LEGACY_APP which
predates the creating of TYPE_APP_POPUP.  It seems that the solution now
would be to create legacy apps using TYPE_APP_POPUP rather than
TYPE_APP.

Bug: 1088300
Change-Id: I77044b62cc5ff88208d02df74af42b59783b78e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2224473Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773824}

chrome/browser/ui/browser.cc

@@ -2864,7 +2864,7 @@ bool Browser::PopupBrowserSupportsWindowFeature(WindowFeature feature,
   }
 }
 
-bool Browser::LegacyAppBrowserSupportsWindowFeature(
+bool Browser::AppPopupBrowserSupportsWindowFeature(
     WindowFeature feature,
     bool check_can_support) const {
   bool fullscreen = ShouldHideUIForFullscreen();
@@ -2878,8 +2878,8 @@ bool Browser::LegacyAppBrowserSupportsWindowFeature(
   }
 }
 
-bool Browser::WebAppBrowserSupportsWindowFeature(WindowFeature feature,
-                                                 bool check_can_support) const {
+bool Browser::AppBrowserSupportsWindowFeature(WindowFeature feature,
+                                              bool check_can_support) const {
   DCHECK(app_controller_);
   bool fullscreen = ShouldHideUIForFullscreen();
   switch (feature) {
@@ -2931,16 +2931,15 @@ bool Browser::SupportsWindowFeatureImpl(WindowFeature feature,
     case TYPE_NORMAL:
       return NormalBrowserSupportsWindowFeature(feature, check_can_support);
     case TYPE_POPUP:
-    case TYPE_APP_POPUP:
       return PopupBrowserSupportsWindowFeature(feature, check_can_support);
     case TYPE_APP:
-      // TODO(crbug.com/992834): Change to TYPE_WEB_APP.
       if (app_controller_)
-        return WebAppBrowserSupportsWindowFeature(feature, check_can_support);
-      // TODO(crbug.com/992834): Change to TYPE_LEGACY_APP.
-      return LegacyAppBrowserSupportsWindowFeature(feature, check_can_support);
+        return AppBrowserSupportsWindowFeature(feature, check_can_support);
+      // TODO(crbug.com/992834): Change legacy apps to TYPE_APP_POPUP.
+      return AppPopupBrowserSupportsWindowFeature(feature, check_can_support);
     case TYPE_DEVTOOLS:
-      return LegacyAppBrowserSupportsWindowFeature(feature, check_can_support);
+    case TYPE_APP_POPUP:
+      return AppPopupBrowserSupportsWindowFeature(feature, check_can_support);
 #if defined(OS_CHROMEOS)
     case TYPE_CUSTOM_TAB:
       return CustomTabBrowserSupportsWindowFeature(feature);

chrome/browser/ui/browser.h

@@ -1008,11 +1008,11 @@ class Browser : public TabStripModelObserver,
   bool PopupBrowserSupportsWindowFeature(WindowFeature feature,
                                          bool check_can_support) const;
 
-  bool LegacyAppBrowserSupportsWindowFeature(WindowFeature feature,
-                                             bool check_can_support) const;
+  bool AppPopupBrowserSupportsWindowFeature(WindowFeature feature,
+                                            bool check_can_support) const;
 
-  bool WebAppBrowserSupportsWindowFeature(WindowFeature feature,
-                                          bool check_can_support) const;
+  bool AppBrowserSupportsWindowFeature(WindowFeature feature,
+                                       bool check_can_support) const;
 
 #if defined(OS_CHROMEOS)
   bool CustomTabBrowserSupportsWindowFeature(WindowFeature feature) const;

chrome/browser/ui/browser_navigator_browsertest.cc

@@ -442,6 +442,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
   EXPECT_EQ(1, popup->tab_strip_model()->count());
   EXPECT_EQ(1, params.browser->tab_strip_model()->count());
   EXPECT_TRUE(params.browser->is_type_normal());
+  EXPECT_TRUE(params.browser->window()->IsToolbarVisible());
 }
 
 // This test verifies that navigating with WindowOpenDisposition = NEW_POPUP
@@ -453,13 +454,14 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_NewPopup) {
   // Wait for new popup to to load and gain focus.
   ui_test_utils::NavigateToURL(&params);
 
-  // Navigate() should have opened a new, focused popup window.
+  // Navigate() should have opened a new, focused popup window, with a toolbar.
   EXPECT_NE(browser(), params.browser);
 #if 0
   // TODO(stevenjb): Enable this test. See: crbug.com/79493
   EXPECT_TRUE(browser->window()->IsActive());
 #endif
   EXPECT_TRUE(params.browser->is_type_popup());
+  EXPECT_TRUE(params.browser->window()->IsToolbarVisible());
 
   // We should have two windows, the browser() provided by the framework and the
   // new popup window.
@@ -469,7 +471,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_NewPopup) {
 }
 
 // This test verifies that navigating with WindowOpenDisposition = NEW_POPUP
-// from a normal Browser results in a new Browser with TYPE_APP_POPUP.
+// from a (kind of app) Browser results in a new Browser with TYPE_APP_POPUP.
 IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_NewPopup_ExtensionId) {
   NavigateParams params(MakeNavigateParams());
   params.disposition = WindowOpenDisposition::NEW_POPUP;
@@ -478,9 +480,11 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_NewPopup_ExtensionId) {
   // Wait for new popup to to load and gain focus.
   ui_test_utils::NavigateToURL(&params);
 
-  // Navigate() should have opened a new, focused popup window.
+  // Navigate() should have opened a new, focused TYPE_APP_POPUP window with no
+  // toolbar.
   EXPECT_NE(browser(), params.browser);
   EXPECT_TRUE(params.browser->is_type_app_popup());
+  EXPECT_FALSE(params.browser->window()->IsToolbarVisible());
 
   // We should have two windows, the browser() provided by the framework and the
   // new popup window.
@@ -506,6 +510,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_NewPopupFromPopup) {
   // Navigate() should have opened a new normal popup window.
   EXPECT_NE(params1.browser, params2.browser);
   EXPECT_TRUE(params2.browser->is_type_popup());
+  EXPECT_TRUE(params2.browser->window()->IsToolbarVisible());
 
   // We should have three windows, the browser() provided by the framework,
   // the first popup window, and the second popup window.
@@ -525,10 +530,11 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
   params.window_bounds = gfx::Rect(0, 0, 200, 200);
   Navigate(&params);
 
-  // Navigate() should have opened a new popup app window.
+  // Navigate() should have opened a new TYPE_APP_POPUP window with no toolbar.
   EXPECT_NE(app_browser, params.browser);
   EXPECT_NE(browser(), params.browser);
   EXPECT_TRUE(params.browser->is_type_app_popup());
+  EXPECT_FALSE(params.browser->window()->IsToolbarVisible());
 
   // We should now have three windows, the app window, the app popup it created,
   // and the original browser() provided by the framework.
@@ -557,6 +563,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_NewPopupFromAppPopup) {
   EXPECT_NE(browser(), params1.browser);
   EXPECT_NE(params1.browser, params2.browser);
   EXPECT_TRUE(params2.browser->is_type_app_popup());
+  EXPECT_FALSE(params2.browser->window()->IsToolbarVisible());
 
   // We should now have four windows, the app window, the first app popup,
   // the second app popup, and the original browser() provided by the framework.
@@ -587,6 +594,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_NewPopupUnfocused) {
   // Navigate() should have opened a new, unfocused, popup window.
   EXPECT_NE(browser(), params.browser);
   EXPECT_TRUE(params.browser->is_type_popup());
+  EXPECT_TRUE(params.browser->window()->IsToolbarVisible());
 #if 0
 // TODO(stevenjb): Enable this test. See: crbug.com/79493
   EXPECT_FALSE(p.browser->window()->IsActive());
@@ -604,10 +612,12 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_NewPopupTrusted) {
   // Wait for new popup to to load and gain focus.
   ui_test_utils::NavigateToURL(&params);
 
-  // Navigate() should have opened a new popup window of TYPE_TRUSTED_POPUP.
+  // Navigate() should have opened a new popup window of TYPE_POPUP with no
+  // toolbar.
   EXPECT_NE(browser(), params.browser);
   EXPECT_TRUE(params.browser->is_type_popup());
   EXPECT_TRUE(params.browser->is_trusted_source());
+  EXPECT_FALSE(params.browser->window()->IsToolbarVisible());
 }
 
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
@@ -623,9 +633,11 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
   // Wait for new popup to to load and gain focus.
   ui_test_utils::NavigateToURL(&params);
 
-  // Navigate() should have opened a new popup window of TYPE_TRUSTED_POPUP.
+  // Navigate() should have opened a new popup window of TYPE_POPUP with a
+  // toolbar.
   EXPECT_NE(browser(), params.browser);
   EXPECT_TRUE(params.browser->is_type_popup());
+  EXPECT_TRUE(params.browser->window()->IsToolbarVisible());
   EXPECT_TRUE(captive_portal::CaptivePortalTabHelper::FromWebContents(
                   params.navigated_or_inserted_contents)
                   ->is_captive_portal_window());
@@ -642,6 +654,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_NewWindow) {
   // Navigate() should have opened a new toplevel window.
   EXPECT_NE(browser(), params.browser);
   EXPECT_TRUE(params.browser->is_type_normal());
+  EXPECT_TRUE(params.browser->window()->IsToolbarVisible());
 
   // We should now have two windows, the browser() provided by the framework and
   // the new normal window.
@@ -1019,6 +1032,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, DISABLED_TargetContents_Popup) {
   // Navigate() should have opened a new popup window.
   EXPECT_NE(browser(), params.browser);
   EXPECT_TRUE(params.browser->is_type_popup());
+  EXPECT_TRUE(params.browser->window()->IsToolbarVisible());
 
   // The web platform is weird. The window bounds specified in
   // |params.window_bounds| are used as follows:

chrome/browser/ui/views/location_bar/custom_tab_bar_view_browsertest.cc

@@ -245,6 +245,7 @@ class CustomTabBarViewBrowserTest
 // Check the custom tab bar is not instantiated for a tabbed browser window.
 IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
                        IsNotCreatedInTabbedBrowser) {
+  EXPECT_TRUE(browser()->is_type_normal());
   EXPECT_TRUE(browser_view_->IsBrowserTypeNormal());
   EXPECT_FALSE(custom_tab_bar_);
 }
@@ -262,6 +263,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest, IsNotCreatedInPopup) {
 
   // Popups are not the normal browser view.
   EXPECT_FALSE(popup_view->IsBrowserTypeNormal());
+  EXPECT_TRUE(popup->is_type_popup());
   // Popups should not have a custom tab bar view.
   EXPECT_FALSE(popup_view->toolbar()->custom_tab_bar());
 }
@@ -272,11 +274,13 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
   const GURL& out_of_scope_url = GURL("https://example.com");
 
   InstallBookmark(app_url);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
 
   Browser* popup_browser =
       OpenPopup(app_view->GetActiveWebContents(), out_of_scope_url);
+  EXPECT_TRUE(popup_browser->is_type_app_popup());
 
   // Out of scope, so custom tab bar should be shown.
   EXPECT_TRUE(popup_browser->app_controller()->ShouldShowCustomTabBar());
@@ -302,6 +306,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest, IsUsedForDesktopPWA) {
   EXPECT_NE(app_view, browser_view_);
 
   EXPECT_FALSE(app_view->IsBrowserTypeNormal());
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   // Custom tab bar should be created.
   EXPECT_TRUE(app_view->toolbar()->custom_tab_bar());
@@ -316,6 +321,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest, ShowsWithMixedContent) {
   InstallPWA(url);
 
   ASSERT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   CustomTabBarView* bar = BrowserView::GetBrowserViewForBrowser(app_browser_)
                               ->toolbar()
@@ -347,6 +353,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest, TitleAndLocationUpdate) {
   InstallPWA(app_url);
 
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   EXPECT_NE(app_view, browser_view_);
@@ -369,6 +376,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
   InstallPWA(app_url);
 
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   EXPECT_NE(app_view, browser_view_);
@@ -389,6 +397,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
   InstallPWA(app_url);
 
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   auto* web_contents = app_view->GetActiveWebContents();
@@ -424,6 +433,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
                        MAYBE_RightClickMenuShowsCopyUrl) {
   const GURL& app_url = https_server()->GetURL("app.com", "/ssl/google.html");
   InstallPWA(app_url);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   auto* web_contents = app_view->GetActiveWebContents();
@@ -457,6 +467,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
   InstallPWA(app_url);
 
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   auto* web_contents = app_view->GetActiveWebContents();
@@ -492,6 +503,7 @@ IN_PROC_BROWSER_TEST_P(
   InstallPWA(app_url);
 
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   auto* web_contents = app_view->GetActiveWebContents();
@@ -524,6 +536,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
   InstallPWA(app_url);
 
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   EXPECT_NE(app_view, browser_view_);
@@ -545,6 +558,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
   InstallPWA(app_url);
 
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   EXPECT_NE(app_view, browser_view_);
@@ -572,6 +586,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
   InstallPWA(app_url);
 
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   EXPECT_NE(app_view, browser_view_);
@@ -618,6 +633,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest,
   const GURL& out_of_scope_url = GURL("https://example.com");
 
   InstallBookmark(app_url);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   EXPECT_NE(app_view, browser_view_);
@@ -647,6 +663,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest, InterstitialCanHideOrigin) {
 
   InstallPWA(https_server()->GetURL("app.com", "/ssl/google.html"));
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
 
   BrowserView* app_view = BrowserView::GetBrowserViewForBrowser(app_browser_);
   EXPECT_NE(app_view, browser_view_);
@@ -678,6 +695,7 @@ IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest, InterstitialCanHideOrigin) {
 IN_PROC_BROWSER_TEST_P(CustomTabBarViewBrowserTest, BlobUrlLocation) {
   InstallPWA(https_server()->GetURL("/simple.html"));
   EXPECT_TRUE(app_browser_);
+  EXPECT_TRUE(app_browser_->is_type_app());
   BrowserView* app_browser_view =
       BrowserView::GetBrowserViewForBrowser(app_browser_);
   EXPECT_NE(app_browser_view, browser_view_);