Revert 75625 - Refactor app launching code for clarity. Add unit tests.

Was failing on check_perms. Check http://build.chromium.org/p/chromium/builders/Linux/builds/3520 for more info.

BUG=None
TEST=BrowserInitTest.*

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

TBR=skerner@google.com
Review URL: http://codereview.chromium.org/6469100

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

9 files changed, 58 insertions, 277 deletions

diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index c61ec2d..17d54be 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -490,6 +490,26 @@ 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,
@@ -529,7 +549,7 @@ TabContents* Browser::OpenApplicationWindow(
       DCHECK(extension->web_extent().ContainsURL(url_input));
     url = url_input;
   } else {
-    DCHECK(extension);  // Empty url and no extension.  Nothing to open.
+    DCHECK(extension);
     url = extension->GetFullLaunchURL();
   }
 
diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
index b03b39b..ba4a6cd 100644
--- a/chrome/browser/ui/browser.h
+++ b/chrome/browser/ui/browser.h
@@ -224,6 +224,14 @@ 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.
@@ -724,10 +732,6 @@ 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 7631f69..56d0def 100644
--- a/chrome/browser/ui/browser_init.cc
+++ b/chrome/browser/ui/browser_init.cc
@@ -447,34 +447,6 @@ 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,
@@ -703,21 +675,23 @@ 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 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 {
+  // Open the required browser windows and tabs.
+  // First, see if we're being run as an application window.
+  if (!OpenApplicationWindow(profile)) {
     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.
-    OpenApplicationTab(profile);
+    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);
+    }
 
     if (process_startup) {
       if (browser_defaults::kOSSupportsOtherBrowsers &&
@@ -731,6 +705,8 @@ bool BrowserInit::LaunchWithProfile::Launch(
       KeystoneInfoBar::PromotionInfoBar(profile);
 #endif
     }
+  } else {
+    RecordLaunchModeHistogram(LM_AS_WEBAPP);
   }
 
 #if defined(OS_WIN)
@@ -780,30 +756,6 @@ 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))
@@ -814,22 +766,24 @@ 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()) {
-    extension_misc::LaunchContainer launch_container;
-    const Extension* extension;
-    if (!GetAppLaunchContainer(profile, app_id, &extension, &launch_container))
-      return false;
-
-    // TODO(skerner): Could pass in |extension| and |launch_container|,
-    // and avoid calling GetAppLaunchContainer() both here and in
-    // OpenApplicationTab().
+    ExtensionService* extensions_service = profile->GetExtensionService();
+    const Extension* extension =
+        extensions_service->GetExtensionById(app_id, false);
 
-    if (launch_container == extension_misc::LAUNCH_TAB)
+    // 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);
+
     RecordCmdLineAppHistogram();
-    TabContents* tab_in_app_window = Browser::OpenApplication(
+    TabContents* app_window = Browser::OpenApplication(
         profile, extension, launch_container, NULL);
-    return (tab_in_app_window != NULL);
+    return (app_window != NULL);
   }
 
   if (url_string.empty())
diff --git a/chrome/browser/ui/browser_init.h b/chrome/browser/ui/browser_init.h
index 323b0626..17e91e7 100644
--- a/chrome/browser/ui/browser_init.h
+++ b/chrome/browser/ui/browser_init.h
@@ -140,10 +140,6 @@ 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 672d01c..4f1e044 100644
--- a/chrome/browser/ui/browser_init_browsertest.cc
+++ b/chrome/browser/ui/browser_init_browsertest.cc
@@ -7,55 +7,14 @@
 #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"
 
-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())));
+namespace {
 
-    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 BrowserInitTest : public InProcessBrowserTest {
 };
 
 class OpenURLsPopupObserver : public BrowserList::Observer {
@@ -100,120 +59,4 @@ IN_PROC_BROWSER_TEST_F(BrowserInitTest, OpenURLsPopup) {
   BrowserList::RemoveObserver(&observer);
 }
 
-// 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)
+}  // namespace
diff --git a/chrome/test/data/extensions/app_with_panel_container/empty.html b/chrome/test/data/extensions/app_with_panel_container/empty.html
deleted file mode 100755
index 8ec9ed5..0000000
--- a/chrome/test/data/extensions/app_with_panel_container/empty.html
+++ /dev/null
@@ -1,5 +0,0 @@
-<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
deleted file mode 100755
index 0065261..0000000
--- a/chrome/test/data/extensions/app_with_panel_container/manifest.json
+++ /dev/null
@@ -1,13 +0,0 @@
-{
-  "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
deleted file mode 100755
index 8ec9ed5..0000000
--- a/chrome/test/data/extensions/app_with_tab_container/empty.html
+++ /dev/null
@@ -1,5 +0,0 @@
-<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
deleted file mode 100755
index c0e39b3..0000000
--- a/chrome/test/data/extensions/app_with_tab_container/manifest.json
+++ /dev/null
@@ -1,13 +0,0 @@
-{
-  "name": "App Test: tab container",
-  "version": "1",
-  "permissions": [
-    "notifications"
-  ],
-  "app": {
-    "launch": {
-      "local_path": "empty.html",
-      "container": "tab"
-    }
-  }
-}