Out-of-process import on Windows.

chrome/browser/first_run/first_run_browsertest.cc

Index: chrome/browser/first_run/first_run_browsertest.cc
diff --git a/chrome/browser/first_run/first_run_browsertest.cc b/chrome/browser/first_run/first_run_browsertest.cc
index 07015a1e30389f708204d81106a22f65c87b65a5..62ace91df6beceedd0132bf56ec6cb660cc6529a 100644
--- a/chrome/browser/first_run/first_run_browsertest.cc
+++ b/chrome/browser/first_run/first_run_browsertest.cc
@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 #include "base/command_line.h"
+#include "base/files/file_path.h"
 #include "base/prefs/pref_service.h"
+#include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/component_loader.h"
@@ -64,89 +66,151 @@ IN_PROC_BROWSER_TEST_F(FirstRunBrowserTest, SetShouldShowWelcomePage) {
 
 #if !defined(OS_CHROMEOS)
 namespace {
-class FirstRunIntegrationBrowserTest : public InProcessBrowserTest {
+
+// A generic test class to be subclassed by test classes testing specific
+// master_preferences. All subclasses must call SetMasterPreferencesForTest()
+// from their SetUp() method before deferring the remainder of Setup() to this
+// class.
+class FirstRunMasterPrefsBrowserTestBase : public InProcessBrowserTest {
  public:
-  FirstRunIntegrationBrowserTest() {}
+  FirstRunMasterPrefsBrowserTestBase() {}
+
  protected:
+  virtual void SetUp() OVERRIDE {
+    // All users of this test class need to call SetMasterPreferencesForTest()
+    // before this class' SetUp() is invoked.
+    ASSERT_TRUE(text_.get());
+
+    ASSERT_TRUE(file_util::CreateTemporaryFile(&prefs_file_));
+    EXPECT_TRUE(file_util::WriteFile(prefs_file_, text_->c_str(),
+                                     text_->size()));
+    first_run::SetMasterPrefsPathForTesting(prefs_file_);
+
+    // This invokes BrowserMain, and does the import, so must be done last.
+    InProcessBrowserTest::SetUp();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    EXPECT_TRUE(file_util::Delete(prefs_file_, false));
+    InProcessBrowserTest::TearDown();
+  }
+
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
     InProcessBrowserTest::SetUpCommandLine(command_line);
     command_line->AppendSwitch(switches::kForceFirstRun);
-    EXPECT_FALSE(first_run::DidPerformProfileImport(NULL));
+    EXPECT_EQ(first_run::AUTO_IMPORT_NONE, first_run::GetAutoImportState());
 
     extensions::ComponentLoader::EnableBackgroundExtensionsForTesting();
+  }
 
-    // The forked import process should run BrowserMain.
-    CommandLine import_arguments((CommandLine::NoProgram()));
-    import_arguments.AppendSwitch(content::kLaunchAsBrowser);
-    first_run::SetExtraArgumentsForImportProcess(import_arguments);
+  void SetMasterPreferencesForTest(const char text[]) {
+      text_.reset(new std::string(text));

[tapted, 2013/04/24 08:08:35]

nit: indenting

[gab, 2013/04/24 20:24:59]

Done.

   }
+
  private:
-  DISALLOW_COPY_AND_ASSIGN(FirstRunIntegrationBrowserTest);
+  base::FilePath prefs_file_;
+  scoped_ptr<std::string> text_;
+
+  DISALLOW_COPY_AND_ASSIGN(FirstRunMasterPrefsBrowserTestBase);
 };
 
-class FirstRunMasterPrefsBrowserTest : public FirstRunIntegrationBrowserTest {
+template<const char Text[]>
+class FirstRunMasterPrefsBrowserTestT
+    : public FirstRunMasterPrefsBrowserTestBase {
  public:
-  FirstRunMasterPrefsBrowserTest() {}
+  FirstRunMasterPrefsBrowserTestT() {}
 
  protected:
   virtual void SetUp() OVERRIDE {
-    ASSERT_TRUE(file_util::CreateTemporaryFile(&prefs_file_));
-    // TODO(tapted): Make this reusable.

[gab, 2013/04/18 21:51:18]

Done via templates, see below :)!

[tapted, 2013/04/24 08:08:35]

nice!

-    const char text[] =
-        "{\n"
-        "  \"distribution\": {\n"
-        "    \"import_bookmarks\": false,\n"
-        "    \"import_history\": false,\n"
-        "    \"import_home_page\": false,\n"
-        "    \"import_search_engine\": false\n"
-        "  }\n"
-        "}\n";
-    EXPECT_TRUE(file_util::WriteFile(prefs_file_, text, strlen(text)));
-    first_run::SetMasterPrefsPathForTesting(prefs_file_);
-
-    // This invokes BrowserMain, and does the import, so must be done last.
-    FirstRunIntegrationBrowserTest::SetUp();
-  }
-
-  virtual void TearDown() OVERRIDE {
-    EXPECT_TRUE(file_util::Delete(prefs_file_, false));
-    FirstRunIntegrationBrowserTest::TearDown();
+    SetMasterPreferencesForTest(Text);
+    FirstRunMasterPrefsBrowserTestBase::SetUp();
   }
 
  private:
-  base::FilePath prefs_file_;
-
-  DISALLOW_COPY_AND_ASSIGN(FirstRunMasterPrefsBrowserTest);
+  DISALLOW_COPY_AND_ASSIGN(FirstRunMasterPrefsBrowserTestT);
 };
+
+}

[tapted, 2013/04/25 00:02:42]

}  // namespace

(probably my bad from an earlier CL).

[gab, 2013/04/25 18:50:00]

Done.

+
+// TODO(tapted): Investigate why this fails on Linux bots but does not
+// reproduce locally. See http://crbug.com/178062 .
+// TODO(tapted): Investigate why this fails on mac_asan flakily
+// http://crbug.com/181499 .
+#if defined(OS_LINUX) || (defined(OS_MACOSX) && defined(ADDRESS_SANITIZER))
+#define MAYBE_ImportDefault DISABLED_ImportDefault
+#else
+#define MAYBE_ImportDefault ImportDefault
+#endif
+
+extern const char kImportDefault[] =

[tapted, 2013/04/24 08:08:35]

shouldn't be extern -- compiler will ignore an extern storage class specifier if
you also initialize it. Maybe static? or move into an anonymous namespace. (more
below).

gcc actually emits a warning about this, but only when compiling for C (e.g. the
file is ".c" not ".cc") - weird.

[gab, 2013/04/24 20:24:59]

If I don't make it extern, I get:
error C2970: '`anonymous-namespace'::FirstRunMasterPrefsBrowserTestT' : template
parameter 'Text' : 'kImportDefault' : an expression involving objects with
internal linkage cannot be used as a non-type argument

This is already in an anonymous namespace.

[tapted, 2013/04/25 00:02:42]

Ah, drat. Where did I read that ~"static and anonymous namespaces are the same
except you can use things in anonymous namespaces for template arguments". This
seems to destroy that - neither gcc nor visual studio let this happen for me.

extern seems to be the workaround - good work on figuring that out! And it looks
like compilers in C++ mode are happy with that being on the definition.
Otherwise you could separate an extern declaration with a non-extern definition.

But I think the template type is in an anonymous namespace but not the string
constants? For me, it still works if the definition is in an anonymous
namespace, with the extern specifier. That might be an improvement (but probably
more of a nit ;).

[gab, 2013/04/25 18:50:00]

Feel free to try different things after this lands as part of the work to
re-enable for Linux/MacASAN, for now I think, as you seem to agree, that this is
fine.

+    "{\n"
+    "}\n";
+typedef FirstRunMasterPrefsBrowserTestT<kImportDefault>
+    FirstRunMasterPrefsImportDefault;
+IN_PROC_BROWSER_TEST_F(FirstRunMasterPrefsImportDefault, MAYBE_ImportDefault) {
+  int auto_import_state = first_run::GetAutoImportState();
+  // Aura builds skip over the import process.
+#if defined(USE_AURA)
+  EXPECT_EQ(first_run::AUTO_IMPORT_CALLED, auto_import_state);
+#else
+  EXPECT_EQ(first_run::AUTO_IMPORT_CALLED |
+                first_run::AUTO_IMPORT_PROFILE_IMPORTED,
+            auto_import_state);
+#endif
 }
 

[tapted, 2013/04/24 08:08:35]

nit: extra vertical whitespace

[gab, 2013/04/24 20:24:59]

Done.

+
 // TODO(tapted): Investigate why this fails on Linux bots but does not
 // reproduce locally. See http://crbug.com/178062 .
 // TODO(tapted): Investigate why this fails on mac_asan flakily
 // http://crbug.com/181499 .
 #if defined(OS_LINUX) || (defined(OS_MACOSX) && defined(ADDRESS_SANITIZER))
-#define MAYBE_WaitForImport DISABLED_WaitForImport
+#define MAYBE_ImportBookmarksFile DISABLED_ImportBookmarksFile
 #else
-#define MAYBE_WaitForImport WaitForImport
+#define MAYBE_ImportBookmarksFile ImportBookmarksFile
 #endif
 
-IN_PROC_BROWSER_TEST_F(FirstRunIntegrationBrowserTest, MAYBE_WaitForImport) {
-  bool success = false;
-  EXPECT_TRUE(first_run::DidPerformProfileImport(&success));
+extern const char kImportBookmarksFile[] =
+    "{\n"
+    "  \"distribution\": {\n"
+    "     \"import_bookmarks_from_file\": \"/foo/doesntexists.wtv\"\n"
+    "  }\n"
+    "}\n";
+typedef FirstRunMasterPrefsBrowserTestT<kImportBookmarksFile>
+    FirstRunMasterPrefsImportBookmarksFile;
+IN_PROC_BROWSER_TEST_F(FirstRunMasterPrefsImportBookmarksFile,
+                       MAYBE_ImportBookmarksFile) {
+  int auto_import_state = first_run::GetAutoImportState();
   // Aura builds skip over the import process.
 #if defined(USE_AURA)
-  EXPECT_FALSE(success);
+  EXPECT_EQ(first_run::AUTO_IMPORT_CALLED, auto_import_state);
 #else
-  EXPECT_TRUE(success);
+  EXPECT_EQ(first_run::AUTO_IMPORT_CALLED |
+                first_run::AUTO_IMPORT_PROFILE_IMPORTED |
+                first_run::AUTO_IMPORT_BOOKMARKS_FILE_IMPORTED,
+            auto_import_state);
 #endif
 }
 
 // Test an import with all import options disabled. This is a regression test
 // for http://crbug.com/169984 where this would cause the import process to
 // stay running, and the NTP to be loaded with no apps.
-IN_PROC_BROWSER_TEST_F(FirstRunMasterPrefsBrowserTest,
+// The file doesn't actually need to exist for this integration test to trigger

[tapted, 2013/04/24 08:08:35]

Could it be picking up '/Library/Google Chrome Master Preferences' in the corp
image instead when the file doesn't exist?

[gab, 2013/04/24 20:24:59]

This comment belonged with the ImportBookmarksFile test above, the "file"
mentioned is the bookmarks file (clarified comment), the master pref file itself
will exist.
This test is only testing the fact that the file import was launched, not
whether it succeeded, so that's ok. Makes sense?

[tapted, 2013/04/25 00:02:42]

Ah. Cool.

+// the interaction being tested.
+extern const char kImportNothing[] =
+    "{\n"
+    "  \"distribution\": {\n"
+    "    \"import_bookmarks\": false,\n"
+    "    \"import_history\": false,\n"
+    "    \"import_home_page\": false,\n"
+    "    \"import_search_engine\": false\n"
+    "  }\n"
+    "}\n";
+typedef FirstRunMasterPrefsBrowserTestT<kImportNothing>
+    FirstRunMasterPrefsImportNothing;
+IN_PROC_BROWSER_TEST_F(FirstRunMasterPrefsImportNothing,
                        ImportNothingAndShowNewTabPage) {
-  EXPECT_TRUE(first_run::DidPerformProfileImport(NULL));
+  EXPECT_EQ(first_run::AUTO_IMPORT_CALLED, first_run::GetAutoImportState());
   ui_test_utils::NavigateToURLWithDisposition(
       browser(), GURL(chrome::kChromeUINewTabURL), CURRENT_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);