Refactor app launching code for clarity. Add unit tests.

BUG=None
TEST=BrowserInitTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75625

Review URL: http://codereview.chromium.org/6543006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@75743 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 277 insertions, 58 deletions

diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 0b3820c..b80ef61 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -490,26 +490,6 @@ void Browser::OpenURLOffTheRecord(Profile* profile, const GURL& url) {
 }
 
 // static
-// TODO(erikkay): There are multiple reasons why this could fail.  Should
-// this function return an error reason as well so that callers can show
-// reasonable errors?
-TabContents* Browser::OpenApplication(Profile* profile,
-                                      const std::string& app_id,
-                                      TabContents* existing_tab) {
-  ExtensionService* extensions_service = profile->GetExtensionService();
-
-  // If the extension with |app_id| could't be found, most likely because it
-  // was uninstalled.
-  const Extension* extension =
-      extensions_service->GetExtensionById(app_id, false);
-  if (!extension)
-    return NULL;
-
-  return OpenApplication(profile, extension, extension->launch_container(),
-                         existing_tab);
-}
-
-// static
 TabContents* Browser::OpenApplication(
     Profile* profile,
     const Extension* extension,
@@ -549,7 +529,7 @@ TabContents* Browser::OpenApplicationWindow(
       DCHECK(extension->web_extent().ContainsURL(url_input));
     url = url_input;
   } else {
-    DCHECK(extension);
+    DCHECK(extension);  // Empty url and no extension.  Nothing to open.
     url = extension->GetFullLaunchURL();
   }
 
diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
index ba4a6cd..b03b39b 100644
--- a/chrome/browser/ui/browser.h
+++ b/chrome/browser/ui/browser.h
@@ -224,14 +224,6 @@ class Browser : public TabHandlerDelegate,
   // |profile|, that session is re-used.
   static void OpenURLOffTheRecord(Profile* profile, const GURL& url);
 
-  // Open an application specified by |app_id| in the appropriate launch
-  // container. |existing_tab| is reused if it is not NULL and the launch
-  // container is a tab. Returns NULL if the app_id is invalid or if
-  // ExtensionService isn't ready/available.
-  static TabContents* OpenApplication(Profile* profile,
-                                      const std::string& app_id,
-                                      TabContents* existing_tab);
-
   // Open |extension| in |container|, using |existing_tab| if not NULL and if
   // the correct container type.  Returns the TabContents* that was created or
   // NULL.
@@ -732,6 +724,10 @@ class Browser : public TabHandlerDelegate,
   FRIEND_TEST_ALL_PREFIXES(BrowserTest, ConvertTabToAppShortcut);
   FRIEND_TEST_ALL_PREFIXES(BrowserTest, OpenAppWindowLikeNtp);
   FRIEND_TEST_ALL_PREFIXES(BrowserTest, AppIdSwitch);
+  FRIEND_TEST_ALL_PREFIXES(BrowserInitTest, OpenAppShortcutNoPref);
+  FRIEND_TEST_ALL_PREFIXES(BrowserInitTest, OpenAppShortcutWindowPref);
+  FRIEND_TEST_ALL_PREFIXES(BrowserInitTest, OpenAppShortcutTabPref);
+  FRIEND_TEST_ALL_PREFIXES(BrowserInitTest, OpenAppShortcutPanel);
 
   // Used to describe why a tab is being detached. This is used by
   // TabDetachedAtImpl.
diff --git a/chrome/browser/ui/browser_init.cc b/chrome/browser/ui/browser_init.cc
index 7ddc43c..d8cf4fc 100644
--- a/chrome/browser/ui/browser_init.cc
+++ b/chrome/browser/ui/browser_init.cc
@@ -447,6 +447,34 @@ void UrlsToTabs(const std::vector<GURL>& urls,
   }
 }
 
+// Return true if the command line option --app-id is used.  Set
+// |out_extension| to the app to open, and |out_launch_container|
+// to the type of window into which the app should be open.
+bool GetAppLaunchContainer(
+    Profile* profile,
+    const std::string& app_id,
+    const Extension** out_extension,
+    extension_misc::LaunchContainer* out_launch_container) {
+
+  ExtensionService* extensions_service = profile->GetExtensionService();
+  const Extension* extension =
+      extensions_service->GetExtensionById(app_id, false);
+
+  // The extension with id |app_id| may have been uninstalled.
+  if (!extension)
+    return false;
+
+  // Look at preferences to find the right launch container.  If no
+  // preference is set, launch as a window.
+  extension_misc::LaunchContainer launch_container =
+      extensions_service->extension_prefs()->GetLaunchContainer(
+          extension, ExtensionPrefs::LAUNCH_WINDOW);
+
+  *out_extension = extension;
+  *out_launch_container = launch_container;
+  return true;
+}
+
 void RecordCmdLineAppHistogram() {
   UMA_HISTOGRAM_ENUMERATION(extension_misc::kAppLaunchHistogram,
                             extension_misc::APP_LAUNCH_CMD_LINE_APP,
@@ -679,23 +707,21 @@ bool BrowserInit::LaunchWithProfile::Launch(
         switches::kUserAgent));
   }
 
-  // Open the required browser windows and tabs.
-  // First, see if we're being run as an application window.
-  if (!OpenApplicationWindow(profile)) {
+  // Open the required browser windows and tabs. First, see if
+  // we're being run as an application window. If so, the user
+  // opened an app shortcut.  Don't restore tabs or open initial
+  // URLs in that case. The user should see the window as an app,
+  // not as chrome.
+  if (OpenApplicationWindow(profile)) {
+    RecordLaunchModeHistogram(LM_AS_WEBAPP);
+  } else {
     RecordLaunchModeHistogram(urls_to_open.empty()?
                               LM_TO_BE_DECIDED : LM_WITH_URLS);
     ProcessLaunchURLs(process_startup, urls_to_open);
 
     // If this is an app launch, but we didn't open an app window, it may
     // be an app tab.
-    std::string app_id;
-    if (IsAppLaunch(NULL, &app_id) && !app_id.empty()) {
-      // TODO(erikkay): This could fail if |app_id| is invalid (the app was
-      // uninstalled).  We may want to show some reasonable error here.
-
-      RecordCmdLineAppHistogram();
-      Browser::OpenApplication(profile, app_id, NULL);
-    }
+    OpenApplicationTab(profile);
 
     if (process_startup) {
       if (browser_defaults::kOSSupportsOtherBrowsers &&
@@ -709,8 +735,6 @@ bool BrowserInit::LaunchWithProfile::Launch(
       KeystoneInfoBar::PromotionInfoBar(profile);
 #endif
     }
-  } else {
-    RecordLaunchModeHistogram(LM_AS_WEBAPP);
   }
 
 #if defined(OS_WIN)
@@ -760,6 +784,30 @@ bool BrowserInit::LaunchWithProfile::IsAppLaunch(std::string* app_url,
   return false;
 }
 
+bool BrowserInit::LaunchWithProfile::OpenApplicationTab(Profile* profile) {
+  std::string app_id;
+  // App shortcuts to URLs always open in an app window.  Because this
+  // function will open an app that should be in a tab, there is no need
+  // to look at the app URL.  OpenApplicationWindow() will open app url
+  // shortcuts.
+  if (!IsAppLaunch(NULL, &app_id) || app_id.empty())
+    return false;
+
+  extension_misc::LaunchContainer launch_container;
+  const Extension* extension;
+  if (!GetAppLaunchContainer(profile, app_id, &extension, &launch_container))
+    return false;
+
+  // If the user doesn't want to open a tab, fail.
+  if (launch_container != extension_misc::LAUNCH_TAB)
+    return false;
+
+  RecordCmdLineAppHistogram();
+
+  TabContents* app_tab = Browser::OpenApplicationTab(profile, extension, NULL);
+  return (app_tab != NULL);
+}
+
 bool BrowserInit::LaunchWithProfile::OpenApplicationWindow(Profile* profile) {
   std::string url_string, app_id;
   if (!IsAppLaunch(&url_string, &app_id))
@@ -770,24 +818,22 @@ bool BrowserInit::LaunchWithProfile::OpenApplicationWindow(Profile* profile) {
   // TODO(skerner): Do something reasonable here. Pop up a warning panel?
   // Open an URL to the gallery page of the extension id?
   if (!app_id.empty()) {
-    ExtensionService* extensions_service = profile->GetExtensionService();
-    const Extension* extension =
-        extensions_service->GetExtensionById(app_id, false);
-
-    // The extension with id |app_id| may have been uninstalled.
-    if (!extension)
+    extension_misc::LaunchContainer launch_container;
+    const Extension* extension;
+    if (!GetAppLaunchContainer(profile, app_id, &extension, &launch_container))
       return false;
 
-    // Look at preferences to find the right launch container.  If no
-    // preference is set, launch as a window.
-    extension_misc::LaunchContainer launch_container =
-        extensions_service->extension_prefs()->GetLaunchContainer(
-            extension, ExtensionPrefs::LAUNCH_WINDOW);
+    // TODO(skerner): Could pass in |extension| and |launch_container|,
+    // and avoid calling GetAppLaunchContainer() both here and in
+    // OpenApplicationTab().
+
+    if (launch_container == extension_misc::LAUNCH_TAB)
+      return false;
 
     RecordCmdLineAppHistogram();
-    TabContents* app_window = Browser::OpenApplication(
+    TabContents* tab_in_app_window = Browser::OpenApplication(
         profile, extension, launch_container, NULL);
-    return (app_window != NULL);
+    return (tab_in_app_window != NULL);
   }
 
   if (url_string.empty())
diff --git a/chrome/browser/ui/browser_init.h b/chrome/browser/ui/browser_init.h
index 17e91e7..323b0626 100644
--- a/chrome/browser/ui/browser_init.h
+++ b/chrome/browser/ui/browser_init.h
@@ -140,6 +140,10 @@ class BrowserInit {
     // returns false to specify default processing.
     bool OpenApplicationWindow(Profile* profile);
 
+    // If IsAppLaunch is true and the user set a pref indicating that the app
+    // should open in a tab, do so.
+    bool OpenApplicationTab(Profile* profile);
+
     // Invoked from OpenURLsInBrowser to handle processing of urls. This may
     // do any of the following:
     // . Invoke ProcessStartupURLs if |process_startup| is true.
diff --git a/chrome/browser/ui/browser_init_browsertest.cc b/chrome/browser/ui/browser_init_browsertest.cc
index 4f1e044..672d01c 100644
--- a/chrome/browser/ui/browser_init_browsertest.cc
+++ b/chrome/browser/ui/browser_init_browsertest.cc
@@ -7,14 +7,55 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_list.h"
 #include "chrome/browser/browser_window.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_init.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/test/in_process_browser_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-namespace {
+class BrowserInitTest : public ExtensionBrowserTest {
+ protected:
+  // Helper functions return void so that we can ASSERT*().
+  // Use ASSERT_FALSE(HasFatalFailure()) after calling these functions
+  // to stop the test if an assert fails.
+  void LoadApp(const std::string& app_name,
+               const Extension** out_app_extension) {
+    ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII(app_name.c_str())));
 
-class BrowserInitTest : public InProcessBrowserTest {
+    ExtensionService* service = browser()->profile()->GetExtensionService();
+    *out_app_extension = service->GetExtensionById(
+        last_loaded_extension_id_, false);
+    ASSERT_TRUE(*out_app_extension);
+
+    // Code that opens a new browser assumes we start with exactly one.
+    ASSERT_EQ(1u, BrowserList::GetBrowserCount(browser()->profile()));
+  }
+
+  void SetAppLaunchPref(const std::string& app_id,
+                        ExtensionPrefs::LaunchType launch_type) {
+    ExtensionService* service = browser()->profile()->GetExtensionService();
+    service->extension_prefs()->SetLaunchType(app_id, launch_type);
+  }
+
+  // Check that there are two browsers.  Find the one that is not |browser()|.
+  void FindOneOtherBrowser(Browser** out_other_browser) {
+    // There should only be one other browser.
+    ASSERT_EQ(2u, BrowserList::GetBrowserCount(browser()->profile()));
+
+    // Find the new browser.
+    Browser* other_browser = NULL;
+    for (BrowserList::const_iterator i = BrowserList::begin();
+         i != BrowserList::end() && !other_browser; ++i) {
+      if (*i != browser())
+        other_browser = *i;
+    }
+    ASSERT_TRUE(other_browser);
+    ASSERT_TRUE(other_browser != browser());
+    *out_other_browser = other_browser;
+  }
 };
 
 class OpenURLsPopupObserver : public BrowserList::Observer {
@@ -59,4 +100,120 @@ IN_PROC_BROWSER_TEST_F(BrowserInitTest, OpenURLsPopup) {
   BrowserList::RemoveObserver(&observer);
 }
 
-}  // namespace
+// App shortcuts are not implemented on mac os.
+#if !defined(OS_MACOSX)
+IN_PROC_BROWSER_TEST_F(BrowserInitTest, OpenAppShortcutNoPref) {
+  // Load an app with launch.container = 'tab'.
+  const Extension* extension_app = NULL;
+  LoadApp("app_with_tab_container", &extension_app);
+  ASSERT_FALSE(HasFatalFailure());  // Check for ASSERT failures in LoadApp().
+
+  // Add --app-id=<extension->id()> to the command line.
+  CommandLine command_line(CommandLine::NO_PROGRAM);
+  command_line.AppendSwitchASCII(switches::kAppId, extension_app->id());
+
+  BrowserInit::LaunchWithProfile launch(FilePath(), command_line);
+  ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), false));
+
+  // No pref was set, so the app should have opened in a window.
+  // The launch should have created a new browser.
+  Browser* new_browser = NULL;
+  FindOneOtherBrowser(&new_browser);
+  ASSERT_FALSE(HasFatalFailure());
+
+  // Expect an app window.
+  EXPECT_EQ(Browser::TYPE_APP, new_browser->type());
+
+  // The browser's app_name should include the app's ID.
+  EXPECT_NE(
+      new_browser->app_name_.find(extension_app->id()),
+      std::string::npos) << new_browser->app_name_;
+}
+
+IN_PROC_BROWSER_TEST_F(BrowserInitTest, OpenAppShortcutWindowPref) {
+  const Extension* extension_app = NULL;
+  LoadApp("app_with_tab_container", &extension_app);
+  ASSERT_FALSE(HasFatalFailure());  // Check for ASSERT failures in LoadApp().
+
+  // Set a pref indicating that the user wants to open this app in a window.
+  SetAppLaunchPref(extension_app->id(), ExtensionPrefs::LAUNCH_WINDOW);
+
+  CommandLine command_line(CommandLine::NO_PROGRAM);
+  command_line.AppendSwitchASCII(switches::kAppId, extension_app->id());
+  BrowserInit::LaunchWithProfile launch(FilePath(), command_line);
+  ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), false));
+
+  // Pref was set to open in a window, so the app should have opened in a
+  // window.  The launch should have created a new browser. Find the new
+  // browser.
+  Browser* new_browser = NULL;
+  FindOneOtherBrowser(&new_browser);
+  ASSERT_FALSE(HasFatalFailure());
+
+  // Expect an app window.
+  EXPECT_EQ(Browser::TYPE_APP, new_browser->type());
+
+  // The browser's app_name should include the app's ID.
+  EXPECT_NE(
+      new_browser->app_name_.find(extension_app->id()),
+      std::string::npos) << new_browser->app_name_;
+}
+
+IN_PROC_BROWSER_TEST_F(BrowserInitTest, OpenAppShortcutTabPref) {
+  // Load an app with launch.container = 'tab'.
+  const Extension* extension_app = NULL;
+  LoadApp("app_with_tab_container", &extension_app);
+  ASSERT_FALSE(HasFatalFailure());  // Check for ASSERT failures in LoadApp().
+
+  // Set a pref indicating that the user wants to open this app in a window.
+  SetAppLaunchPref(extension_app->id(), ExtensionPrefs::LAUNCH_REGULAR);
+
+  CommandLine command_line(CommandLine::NO_PROGRAM);
+  command_line.AppendSwitchASCII(switches::kAppId, extension_app->id());
+  BrowserInit::LaunchWithProfile launch(FilePath(), command_line);
+  ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), false));
+
+  // When an app shortcut is open and the pref indicates a tab should
+  // open, the tab is open in a new browser window.  Expect a new window.
+  ASSERT_EQ(2u, BrowserList::GetBrowserCount(browser()->profile()));
+
+  Browser* new_browser = NULL;
+  FindOneOtherBrowser(&new_browser);
+  ASSERT_FALSE(HasFatalFailure());
+
+  // The tab should be in a normal window.
+  EXPECT_EQ(Browser::TYPE_NORMAL, new_browser->type());
+
+  // The browser's app_name should not include the app's ID: It is in a
+  // normal browser.
+  EXPECT_EQ(
+      new_browser->app_name_.find(extension_app->id()),
+      std::string::npos) << new_browser->app_name_;
+}
+
+IN_PROC_BROWSER_TEST_F(BrowserInitTest, OpenAppShortcutPanel) {
+  // Load an app with launch.container = 'panel'.
+  const Extension* extension_app = NULL;
+  LoadApp("app_with_panel_container", &extension_app);
+  ASSERT_FALSE(HasFatalFailure());  // Check for ASSERT failures in LoadApp().
+
+  CommandLine command_line(CommandLine::NO_PROGRAM);
+  command_line.AppendSwitchASCII(switches::kAppId, extension_app->id());
+  BrowserInit::LaunchWithProfile launch(FilePath(), command_line);
+  ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), false));
+
+  // The launch should have created a new browser, with a panel type.
+  Browser* new_browser = NULL;
+  FindOneOtherBrowser(&new_browser);
+  ASSERT_FALSE(HasFatalFailure());
+
+  // Expect an app panel.
+  EXPECT_EQ(Browser::TYPE_APP_POPUP, new_browser->type());
+
+  // The new browser's app_name should include the app's ID.
+  EXPECT_NE(
+      new_browser->app_name_.find(extension_app->id()),
+      std::string::npos) << new_browser->app_name_;
+}
+
+#endif  // !defined(OS_MACOSX)
diff --git a/chrome/test/data/extensions/app_with_panel_container/empty.html b/chrome/test/data/extensions/app_with_panel_container/empty.html
new file mode 100755
index 0000000..8ec9ed5
--- /dev/null
+++ b/chrome/test/data/extensions/app_with_panel_container/empty.html
@@ -0,0 +1,5 @@
+<html>

+<body>

+Empty.html

+</body>

+</html>
\ No newline at end of file
diff --git a/chrome/test/data/extensions/app_with_panel_container/manifest.json b/chrome/test/data/extensions/app_with_panel_container/manifest.json
new file mode 100755
index 0000000..0065261
--- /dev/null
+++ b/chrome/test/data/extensions/app_with_panel_container/manifest.json
@@ -0,0 +1,13 @@
+{
+  "name": "App Test: panel container",
+  "version": "1",
+  "permissions": [
+    "notifications"
+  ],
+  "app": {
+    "launch": {
+      "local_path": "empty.html",
+      "container": "panel"
+    }
+  }
+}
diff --git a/chrome/test/data/extensions/app_with_tab_container/empty.html b/chrome/test/data/extensions/app_with_tab_container/empty.html
new file mode 100644
index 0000000..8ec9ed5
--- /dev/null
+++ b/chrome/test/data/extensions/app_with_tab_container/empty.html
@@ -0,0 +1,5 @@
+<html>

+<body>

+Empty.html

+</body>

+</html>
\ No newline at end of file
diff --git a/chrome/test/data/extensions/app_with_tab_container/manifest.json b/chrome/test/data/extensions/app_with_tab_container/manifest.json
new file mode 100644
index 0000000..c0e39b3
--- /dev/null
+++ b/chrome/test/data/extensions/app_with_tab_container/manifest.json
@@ -0,0 +1,13 @@
+{
+  "name": "App Test: tab container",
+  "version": "1",
+  "permissions": [
+    "notifications"
+  ],
+  "app": {
+    "launch": {
+      "local_path": "empty.html",
+      "container": "tab"
+    }
+  }
+}