Modularize Compact Language Detector 2 (CLD2) data sources

chrome/browser/component_updater/cld_component_installer.cc

Index: chrome/browser/component_updater/cld_component_installer.cc
diff --git a/chrome/browser/component_updater/cld_component_installer.cc b/chrome/browser/component_updater/cld_component_installer.cc
index 3ec1f595e63dec2dfdbf71ac4ce59db7796bc49d..92878874692d079da7dd0e9f2bb179d407e08057 100644
--- a/chrome/browser/component_updater/cld_component_installer.cc
+++ b/chrome/browser/component_updater/cld_component_installer.cc
@@ -15,8 +15,8 @@
 #include "base/path_service.h"
 #include "base/platform_file.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
+#include "components/translate/content/browser/data_file_browser_cld_data_provider.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/ssl/ssl_config_service.h"
 
@@ -28,11 +28,13 @@ namespace {
 // Once we have acquired a valid file from the component installer, we need to
 // make the path available to other parts of the system such as the
 // translation libraries. We create a global to hold onto the path, and a
-// lock to guard it. See GetLatestCldDataFile(...) for more info.
+// lock to guard it.
 base::LazyInstance<base::Lock> cld_file_lock = LAZY_INSTANCE_INITIALIZER;

[Takashi Toyoshima, 2014/06/23 08:49:59]

These two LazyInstance objects looks not needed. See my comment on
SetLatestCldDataFile() below.

[Andrew Hayden (chromium.org), 2014/06/23 13:20:34]

Yes, you're right. Now that this class configures the CLD data provider, we no
longer need to hold state here.

I've removed the variables and the lock, as well as the getter method. 

 base::LazyInstance<base::FilePath> cld_file = LAZY_INSTANCE_INITIALIZER;
+const base::FilePath::CharType kCLDDataFilename[] =

[Takashi Toyoshima, 2014/06/23 08:50:00]

You are changing other CLD* to Cld*, so kCldDataFilename looks better to be
consistent with others.

[Andrew Hayden (chromium.org), 2014/06/23 13:20:34]

Done.

+    FILE_PATH_LITERAL("cld2_data.bin");
 
-}
+}  // namespace
 
 namespace component_updater {
 
@@ -67,7 +69,7 @@ base::FilePath CldComponentInstallerTraits::GetInstalledPath(
   // NB: This may change when 64-bit is officially supported.
   return base.Append(FILE_PATH_LITERAL("_platform_specific"))
       .Append(FILE_PATH_LITERAL("all"))
-      .Append(chrome::kCLDDataFilename);
+      .Append(kCLDDataFilename);
 }
 
 void CldComponentInstallerTraits::ComponentReady(
@@ -118,14 +120,16 @@ void RegisterCldComponent(ComponentUpdateService* cus) {
 void CldComponentInstallerTraits::SetLatestCldDataFile(
     const base::FilePath& path) {
   VLOG(1) << "Setting CLD data file location: " << path.value();
-  base::AutoLock lock(cld_file_lock.Get());
-  cld_file.Get() = path;
+  {
+    base::AutoLock lock(cld_file_lock.Get());
+    cld_file.Get() = path;

[Takashi Toyoshima, 2014/06/23 08:50:00]

Do you really need to have a copy of path inside this class? It seems that
|cld_file| is not used in any other places.

[Andrew Hayden (chromium.org), 2014/06/23 13:20:34]

Good catch, as described above I've removed all of this leftover logic.

+  }
+  translate::DataFileBrowserCldDataProvider::SetCldDataFilePath(path);
 }
 
-base::FilePath GetLatestCldDataFile() {
-  base::AutoLock lock(cld_file_lock.Get());
-  // cld_file is an empty path by default, meaning "file not available yet".
-  return cld_file.Get();
+const base::FilePath::CharType*
+  CldComponentInstallerTraits::GetCldDataFileName() {

[Takashi Toyoshima, 2014/06/23 08:50:00]

no two spaces.

Recently, another reviewer tell me 'git cl format' that automatically fixes
style nits. It's really useful.

[Andrew Hayden (chromium.org), 2014/06/23 13:20:34]

Yeah, I always forget this. I'll do a final formatting pass.

+  return kCLDDataFilename;
 }
 
 }  // namespace component_updater