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

chrome/browser/translate/cld_data_harness_factory.cc

Index: chrome/browser/translate/cld_data_harness_factory.cc
diff --git a/chrome/browser/translate/cld_data_harness_factory.cc b/chrome/browser/translate/cld_data_harness_factory.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d232a8be6aa0c106aa0a4ecb0c8b98bfd70e0f13
--- /dev/null
+++ b/chrome/browser/translate/cld_data_harness_factory.cc
@@ -0,0 +1,69 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/translate/cld_data_harness_factory.h"
+
+#include "base/lazy_instance.h"
+#include "chrome/browser/translate/component_cld_data_harness.h"
+#include "chrome/browser/translate/standalone_cld_data_harness.h"
+
+namespace {
+
+// This is the global instance managed by Get/Set
+test::CldDataHarnessFactory* g_instance = NULL;
+
+base::LazyInstance<test::CldDataHarnessFactory>::Leaky g_wrapped_component =
+    LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<test::CldDataHarnessFactory>::Leaky g_wrapped_standalone =
+    LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<test::CldDataHarnessFactory>::Leaky g_wrapped_static =
+    LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+namespace test {
+
+scoped_ptr<CldDataHarness> CldDataHarnessFactory::CreateCldDataHarness() {

[Takashi Toyoshima, 2014/11/06 14:34:28]

I may miss something, but this design looks redundant. Why can we implement them
simply like;

class Factory {
  virtual Instance* Create() { instance_->Create(); }

  void SetDefaultFactory(Factory* factory) { instance_.reset(factory); }  // or
  void SetStandardFactoryAsDefault() { instance_.reset(new StandardFactory); }

  scoped_ptr<Factory> instance_;
}

class StandardFactory : public Factory {
  virtual Instance* Create() { return new StandardInstance; }
}

If needed, you can add static methods to Factory, and make instance_ global
variable.

[Andrew Hayden (chromium.org), 2014/11/10 14:06:35]

Tricky question.
Each platform needs to be able to set its default differently, so
SetStandardFactoryAsDefault would only work if we had platform-specific #ifdefs
inside the method to customize the choice of the "standard" factory for each
platform.

The lazy instances up above here are basically lies. All of the factory
implementations for Chromium (i.e., open-source) are trivial, so instead of
actually having three separate *implementations* of the "Factory" class we just
have three lazily-initialized *instances* that cheat by looking at "this" to see
what they should do.

I actually used to have something that looked more like what you're requesting:
see patchset 29's version of this file. I was asked to simplify where possible
and get rid of pointless classes, so this was my solution. If patchset 29
doesn't look like what you're asking for, then I am misinterpreting and please
tell me how you'd like to see patchet 29's design for this file evolved.

I will add some comments clarifying this to the code, because I think that it is
confusing as it is written.

[Takashi Toyoshima, 2014/11/11 07:16:36]

Thank you for additional comments.
So, now I understand why you decide to do like this. I agreed.

+  if (this == GetStaticDataHarnessFactory()) {
+    return CldDataHarness::CreateStaticDataHarness();
+  }
+  if (this == GetStandaloneDataHarnessFactory()) {
+    return CldDataHarness::CreateStandaloneDataHarness();
+  }
+  if (this == GetComponentDataHarnessFactory()) {
+    return CldDataHarness::CreateComponentDataHarness();
+  }
+  NOTREACHED() << "Subclass failed to override CreateCldDataHarness()";
+  return CldDataHarness::CreateStaticDataHarness();
+}
+
+CldDataHarnessFactory* CldDataHarnessFactory::Get() {
+  DCHECK(g_instance != NULL) << "No CLD data harness factory was configured!";
+  if (g_instance != NULL) return g_instance;
+  return g_instance;
+}
+
+void CldDataHarnessFactory::Set(CldDataHarnessFactory* instance) {
+  g_instance = instance;
+}
+
+// static
+CldDataHarnessFactory*
+CldDataHarnessFactory::GetStaticDataHarnessFactory() {
+  return &g_wrapped_static.Get();
+}
+
+// static
+CldDataHarnessFactory*
+CldDataHarnessFactory::GetStandaloneDataHarnessFactory() {
+  return &g_wrapped_standalone.Get();
+}
+
+// static
+CldDataHarnessFactory*
+CldDataHarnessFactory::GetComponentDataHarnessFactory() {
+  return &g_wrapped_component.Get();
+}
+
+}  // namespace test