Refactor language detection logic to allow non-static CLD data sources.

chrome/browser/translate/cld_data_harness.h

Index: chrome/browser/translate/cld_data_harness.h
diff --git a/chrome/browser/translate/cld_data_harness.h b/chrome/browser/translate/cld_data_harness.h
index e9b6f3210c3bdfa1ebc32ce8eb252f27c37f5b56..111e0cb11a518927128dc319204811f483498f45 100644
--- a/chrome/browser/translate/cld_data_harness.h
+++ b/chrome/browser/translate/cld_data_harness.h
@@ -12,16 +12,24 @@
 namespace test {
 
 // A utility class that sets up CLD dynamic data upon calling Init() and cleans
-// it up when destroyed.
+// it up when destroyed. Note that the Init() method will also configure the
+// CLD data source using translate::CldDataSource::Set(), overriding any
+// previously-set value unconditionally!
+//
 // Test data lives under: src/chrome/test/data/cld2_component
 //
 // This class is intended to be instantiated within IN_PROC_BROWSER_TEST_F
 // test fixtures; it uses ASSERT macros for correctness, so that tests will
-// fail gracefully in error conditions. Sample use:
+// fail gracefully in error conditions. Test code should generally use a
+// CldDataHarnessFactory to create CldDataHarness objects since this allows
+// the tests to run with whatever configuration is appropriate for the platform;
+// If that's not enough power, the testing code can set the factory itself.
+//
+// Sample usage:
 //
 //   IN_PROC_BROWSER_TEST_F(BrowserTest, PageLanguageDetection) {
 //     scoped_ptr<test::CldDataHarness> cld_data_scope =
-//       test::CreateCldDataHarness();
+//       test::CldDataHarnessFactory::Get()->CreateCldDataHarness();
 //     ASSERT_NO_FATAL_FAILURE(cld_data_scope->Init());
 //     // ... your code that depends on language detection goes here
 //   }
@@ -34,7 +42,9 @@ namespace test {
 //   class MyTestClass : public InProcessBrowserTest {
 //    public:
 //     MyTestClass() :
-//       cld_data_scope(test::CreateCldDataHarness()) {
+//       cld_data_scope(
+//         test::CldDataHarnessFactory::Get->CreateCldDataHarness()) {
+//       // (your additional setup code here)
 //     }
 //     virtual void SetUpOnMainThread() override {
 //       cld_data_scope->Init();
@@ -51,11 +61,36 @@ class CldDataHarness {
   // immediately deleted.
   // If dynamic data is not currently available for any reason, this method has
   // no effect.
+  // The default implementation does nothing.
   virtual ~CldDataHarness() {}
 
   // Call this method, wrapping it in ASSERT_NO_FATAL_FAILURE, to initialize
   // the harness and trigger test failure if initialization fails.
-  virtual void Init() = 0;
+  // IMPORTANT: This method will unconditionally set the CLD data source using
+  // translate::CldDataSource::Set(...). Any previously-configured CLD data
+  // source will be lost. This helps ensure a consistent test environment where
+  // the configured data source matches definitely matches the harness.
+  virtual void Init() {};
+
+  // Create and return a new instance of a data harness whose Init() method
+  // will configure the "none" CldDataSource.
+  static scoped_ptr<CldDataHarness> NONE();

[Takashi Toyoshima, 2014/10/15 08:45:07]

This kind of name conversion looks non-standard of Chromium I think.
How about CreateFooBarHarness() or something?

Also, do you have any reason not to use the factory class directly?

[Andrew Hayden (chromium.org), 2014/10/30 16:56:31]

This has been greatly cleaned up. I think you'll find it much easier to read now
and more compliant.

+
+  // Create and return a new instance of a data harness whose Init() method
+  // will configure the "static" CldDataSource.
+  static scoped_ptr<CldDataHarness> STATIC();
+
+  // Create and return a new instance of a data harness whose Init() method
+  // will configure the "standalone" CldDataSource.
+  // Unlike NONE() and STATIC(), this data hardness will perform work to allow
+  // CLD to load data from a file.
+  static scoped_ptr<CldDataHarness> STANDALONE();
+
+  // Create and return a new instance of a data harness whose Init() method
+  // will configure the "component" CldDataSource.
+  // Unlike NONE() and STATIC(), this data hardness will perform work to allow
+  // CLD to load data from a file.
+  static scoped_ptr<CldDataHarness> COMPONENT();
 
  protected:
   // Returns the version number of the Component Updater "extension" in the
@@ -71,10 +106,6 @@ class CldDataHarness {
   void GetTestDataSourceDirectory(base::FilePath* out_path);
 };
 
-// Static factory function that returns a data harness defined by the
-// implementation, which must be a subclass of CldDataHarness.
-scoped_ptr<CldDataHarness> CreateCldDataHarness();
-
 }  // namespace test
 
 #endif  // CHROME_BROWSER_TRANSLATE_CLD_DATA_HARNESS_H_