Implement settings in a widnow

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 0bb562ca1fa9394f5f1b2d02ecf10bdd29cf15ce..c28a092cf7ee56814e2a21aa05a6f79cb7295540 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -241,6 +241,8 @@ bool IsFastTabUnloadEnabled() {
         switches::kEnableFastUnload);
 }
 
+const char kChromeUIScheme[] = "chrome";
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -2164,6 +2166,44 @@ void Browser::TabDetachedAtImpl(content::WebContents* contents,
   }
 }
 
+bool Browser::ShowLocationBar() const {

[Ben Goodger (Google), 2014/03/03 18:55:16]

Here's a question... can settings shown in a popup use APP_TYPE_HOST  for its
Browser? APP_TYPE_HOST windows shouldn't get a location bar, so you wouldn't
need to special case the settings page in this function.

[stevenjb, 2014/03/03 23:45:24]

I am reluctant to use APP_TYPE_HOST for something that isn't actually an app,
however I believe this may be an opportunity to eliminate AppType (which I think
is only used for window decoration) and replace it with a more descriptive enum
(or maybe a WindowFeature mask)?

I will put that up as a separate CL for review/discussion.

[stevenjb, 2014/03/06 23:51:56]

OK, so I looked through the code and did some experiments. 
APP_TYPE_HOST is the *default* setting. In order to make that meaningful for
non-app windows, we would have to change that i.e. currently if a non app
Browser tab opens a popup window, AppType is APP_TYPE_HOST, which is fine since
it is ignored for non apps). Fixing this turns out to be a fairly invasive and
not entirely straightforward change. 

Instead, I moved the logic below into a helper function in chrome_pages, with
TODO comments to change/simplify it if/when we clean up AppType.

+  if (!is_app()) {
+    // Hide the URL for singleton settings windows
+    if (!is_type_tabbed() &&
+        CommandLine::ForCurrentProcess()->HasSwitch(
+            ::switches::kEnableSettingsWindow)) {
+      WebContents* web_contents = tab_strip_model_->GetWebContentsAt(0);
+      if (web_contents) {
+        GURL url(web_contents->GetURL());
+        if (url.SchemeIs(kChromeUIScheme) &&
+            url.spec().find(chrome::kChromeUISettingsURL) == 0) {
+          return false;
+        }
+      }
+    }
+    // Otherwise always show the location bar for non app browsers.
+    return true;
+  }
+
+  // Normally apps do not show a location bar.
+  if (app_type() != APP_TYPE_HOST ||
+      app_name() == DevToolsWindow::kDevToolsApp ||
+      !CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableStreamlinedHostedApps))
+    return false;
+
+  // If kEnableStreamlinedHostedApps is true, hide the locaiton bar for non
+  // legacy packaged apps.

[stevenjb, 2014/03/06 23:51:56]

Actually, this comment is wrong. We want to -show- the location bar for
non-legacy packaged apps. Fixed.

+  ExtensionService* service =
+      extensions::ExtensionSystem::Get(profile_)->extension_service();
+  if (!service)
+    return false;  // Assume app is non-legacy.

[calamity, 2014/03/05 02:38:11]

Non-legacy apps would get a location bar. I don't mind whether you change the
value or comment.

[stevenjb, 2014/03/06 23:51:56]

Restored the existing logic.

+
+  const extensions::Extension* extension = service->GetInstalledExtension(
+      web_app::GetExtensionIdFromApplicationName(app_name()));
+  return (!extension || !extension->is_legacy_packaged_app());
+}
+
 bool Browser::SupportsWindowFeatureImpl(WindowFeature feature,
                                         bool check_fullscreen) const {
   bool hide_ui_for_fullscreen = check_fullscreen && ShouldHideUIForFullscreen();
@@ -2183,20 +2223,8 @@ bool Browser::SupportsWindowFeatureImpl(WindowFeature feature,
     if (is_type_tabbed())
       features |= FEATURE_TOOLBAR;
 
-    ExtensionService* service =
-        extensions::ExtensionSystem::Get(profile_)->extension_service();
-    const extensions::Extension* extension =
-        service ? service->GetInstalledExtension(
-                      web_app::GetExtensionIdFromApplicationName(app_name()))
-                : NULL;
-
-    if (!is_app() || (app_type() == APP_TYPE_HOST &&
-                      app_name() != DevToolsWindow::kDevToolsApp &&
-                      (!extension || !extension->is_legacy_packaged_app()) &&
-                      CommandLine::ForCurrentProcess()->HasSwitch(
-                          switches::kEnableStreamlinedHostedApps))) {
+    if (ShowLocationBar())
       features |= FEATURE_LOCATIONBAR;
-    }
   }
   return !!(features & feature);
 }