Restrict use of two app-launching command line flags

apps/load_and_launch_browsertest.cc

Index: apps/load_and_launch_browsertest.cc
diff --git a/apps/load_and_launch_browsertest.cc b/apps/load_and_launch_browsertest.cc
index fc95daec61bc75a6fdd0be387e65942f9d00ab93..cf52a368266da52f4a486e06cf8f942c41eefdbd 100644
--- a/apps/load_and_launch_browsertest.cc
+++ b/apps/load_and_launch_browsertest.cc
@@ -8,14 +8,20 @@
 
 #include "apps/switches.h"
 #include "base/process/launch.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/test/test_timeouts.h"
 #include "chrome/browser/apps/app_browsertest_util.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/simple_message_box_internal.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/test_launcher.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/test/extension_test_message_listener.h"
+#include "testing/gmock/include/gmock/gmock.h"

[msw, 2016/07/25 23:44:46]

nit: remove

[proberge, 2016/07/26 15:09:58]

Done.

+#include "testing/gtest/include/gtest/gtest.h"

[msw, 2016/07/25 23:44:46]

optional nit: remove; included via extension_browsertest.h, etc.

[proberge, 2016/07/26 15:09:57]

Done.

 
 using extensions::PlatformAppBrowserTest;
 
@@ -28,6 +34,8 @@ const char* kSwitchesToCopy[] = {
     switches::kNoSandbox,
 };
 
+static const std::string kTestExtensionId_("behllobkkfkfnphdnhnkndlbkcpglgmj");

[msw, 2016/07/25 23:44:46]

nit: no trailing underscore.

[proberge, 2016/07/26 15:09:57]

Done.

+
 }  // namespace
 
 // TODO(jackhou): Enable this test once it works on OSX. It currently does not
@@ -112,18 +120,17 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
 
 namespace {
 
-// TestFixture that appends --load-and-launch-app before calling BrowserMain.
-class PlatformAppLoadAndLaunchBrowserTest : public PlatformAppBrowserTest {
+// TestFixture that appends --load-and-launch-app with an app before calling
+// BrowserMain.
+class LoadAndLaunchPlatformAppBrowserTest : public PlatformAppBrowserTest {
  protected:
-  PlatformAppLoadAndLaunchBrowserTest() {}
+  LoadAndLaunchPlatformAppBrowserTest() {}
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     PlatformAppBrowserTest::SetUpCommandLine(command_line);
-    app_path_ = test_data_dir_
-        .AppendASCII("platform_apps")
-        .AppendASCII("minimal");
-    command_line->AppendSwitchNative(apps::kLoadAndLaunchApp,
-                                     app_path_.value());
+    base::FilePath app_path =
+        test_data_dir_.AppendASCII("platform_apps").AppendASCII("minimal");
+    command_line->AppendSwitchNative(apps::kLoadAndLaunchApp, app_path.value());
   }
 
   void LoadAndLaunchApp() {
@@ -136,9 +143,32 @@ class PlatformAppLoadAndLaunchBrowserTest : public PlatformAppBrowserTest {
   }
 
  private:
-  base::FilePath app_path_;
+  DISALLOW_COPY_AND_ASSIGN(LoadAndLaunchPlatformAppBrowserTest);
+};
+
+// TestFixture that appends --load-and-launch-app with an extension before
+// calling BrowserMain.
+class LoadAndLaunchExtensionBrowserTest : public PlatformAppBrowserTest {
+ protected:
+  LoadAndLaunchExtensionBrowserTest() {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    PlatformAppBrowserTest::SetUpCommandLine(command_line);
+    base::FilePath app_path = test_data_dir_.AppendASCII("good")
+                                  .AppendASCII("Extensions")
+                                  .AppendASCII(kTestExtensionId_)
+                                  .AppendASCII("1.0.0.0");
+    command_line->AppendSwitchNative(apps::kLoadAndLaunchApp, app_path.value());
+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    PlatformAppBrowserTest::SetUpInProcessBrowserTestFixture();
 
-  DISALLOW_COPY_AND_ASSIGN(PlatformAppLoadAndLaunchBrowserTest);
+    // Skip showing the error message box to avoid freezing the main thread.
+    chrome::internal::g_should_skip_message_box_for_test = true;
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(LoadAndLaunchExtensionBrowserTest);
 };
 
 }  // namespace
@@ -155,9 +185,32 @@ class PlatformAppLoadAndLaunchBrowserTest : public PlatformAppBrowserTest {
 #endif
 
 // Case where Chrome is not running.
-IN_PROC_BROWSER_TEST_F(PlatformAppLoadAndLaunchBrowserTest,
+IN_PROC_BROWSER_TEST_F(LoadAndLaunchPlatformAppBrowserTest,
                        MAYBE_LoadAndLaunchAppChromeNotRunning) {
   LoadAndLaunchApp();
 }
 
+IN_PROC_BROWSER_TEST_F(LoadAndLaunchExtensionBrowserTest,
+                       LoadAndLaunchExtension) {
+  const std::vector<base::string16>* errors =
+      ExtensionErrorReporter::GetInstance()->GetErrors();
+
+#if defined(GOOGLE_CHROME_BUILD)
+  // The error is skipped on official builds.
+  EXPECT_EQ(0u, errors->size());

[msw, 2016/07/25 23:44:46]

optional nit: EXPECT_TRUE(errors->empty());

[proberge, 2016/07/26 15:09:57]

Done.

+#else
+  // Expect kUnpackedExtensionInsteadOfAppError.
+  EXPECT_EQ(1u, errors->size());
+  EXPECT_THAT(base::UTF16ToUTF8(errors->at(0)),

[msw, 2016/07/25 23:44:46]

nit: consider doing this instead:
  EXPECT_NE(base::string16::npos, errors[0].find(base::ASCIIToUTF16("...")));

[proberge, 2016/07/26 15:09:57]

Done without the ASCIIToUTF16 and with ->at(0) instead of [0].

qq: Why is EXPECT_NE(npos, ...) preferred over testing::HasSubstr?

+              testing::HasSubstr(
+                  "App loading flags cannot be used to load extensions"));
+#endif
+
+  extensions::ExtensionRegistry* registry =
+      extensions::ExtensionRegistry::Get(profile());
+  EXPECT_EQ(nullptr,
+            registry->GetExtensionById(
+                kTestExtensionId_, extensions::ExtensionRegistry::EVERYTHING));
+}
+
 }  // namespace apps