Implement settings in a widnow

chrome/browser/ui/browser.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/browser.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shellapi.h>
 #endif  // defined(OS_WIN)
@@ skipping 223 matching lines @@
 BrowserWindow* CreateBrowserWindow(Browser* browser) {
   return BrowserWindow::CreateBrowserWindow(browser);
 }
 
 // Is the fast tab unload experiment enabled?
 bool IsFastTabUnloadEnabled() {
   return CommandLine::ForCurrentProcess()->HasSwitch(
         switches::kEnableFastUnload);
 }
 
+const char kChromeUIScheme[] = "chrome";
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 // Browser, CreateParams:
 
 Browser::CreateParams::CreateParams(Profile* profile,
                                     chrome::HostDesktopType host_desktop_type)
     : type(TYPE_TABBED),
       profile(profile),
       host_desktop_type(host_desktop_type),
@@ skipping 1903 matching lines @@
   for (size_t i = 0; i < interstitial_observers_.size(); i++) {
     if (interstitial_observers_[i]->web_contents() != contents)
       continue;
 
     delete interstitial_observers_[i];
     interstitial_observers_.erase(interstitial_observers_.begin() + i);
     return;
   }
 }
 
+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();
 
   unsigned int features = FEATURE_INFOBAR | FEATURE_DOWNLOADSHELF;
 
   if (is_type_tabbed())
     features |= FEATURE_BOOKMARKBAR;
 
   if (!hide_ui_for_fullscreen) {
     if (!is_type_tabbed())
       features |= FEATURE_TITLEBAR;
 
     if (is_type_tabbed())
       features |= FEATURE_TABSTRIP;
 
     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);
 }
 
 void Browser::UpdateBookmarkBarState(BookmarkBarStateChangeReason reason) {
   BookmarkBar::State state;
   // The bookmark bar is hidden in fullscreen mode, unless on the new tab page.
   if (browser_defaults::bookmarks_enabled &&
       profile_->GetPrefs()->GetBoolean(prefs::kShowBookmarkBar) &&
       !ShouldHideUIForFullscreen()) {
@@ skipping 112 matching lines @@
   if (contents && !allow_js_access) {
     contents->web_contents()->GetController().LoadURL(
         target_url,
         content::Referrer(),
         content::PAGE_TRANSITION_LINK,
         std::string());  // No extra headers.
   }
 
   return contents != NULL;
 }