Allow browser tests to run with dynamic CLD data.

chrome/browser/translate/translate_browser_test_utils.cc

Index: chrome/browser/translate/translate_browser_test_utils.cc
diff --git a/chrome/browser/translate/translate_browser_test_utils.cc b/chrome/browser/translate/translate_browser_test_utils.cc
new file mode 100644
index 0000000000000000000000000000000000000000..18ac640b816f10ed244fd2ac875e82af1ca4732b
--- /dev/null
+++ b/chrome/browser/translate/translate_browser_test_utils.cc
@@ -0,0 +1,150 @@
+// 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/translate_browser_test_utils.h"
+
+#include "base/base_paths.h"
+#include "base/file_util.h"
+#include "base/path_service.h"
+#include "base/platform_file.h"
+#include "base/synchronization/lock.h"
+#include "chrome/browser/component_updater/cld_component_installer.h"
+#include "chrome/browser/translate/translate_tab_helper.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_paths.h"
+
+namespace {
+
+// This constant yields the version of the CRX that has been extracted into
+// the test data directory, and must be kept in sync with what is there.
+// A reciprocal comment has been placed in
+// chrome/test/data/cld2_component/README.chromium; don't update one without
+// updating the other.
+const base::FilePath::CharType kCrxVersion[] = FILE_PATH_LITERAL("160");
+
+base::FilePath GetTestDataSourceDirectory() {
+  base::FilePath result;
+  DCHECK(PathService::Get(chrome::DIR_TEST_DATA, &result));

[Paweł Hajdan Jr., 2014/05/19 15:06:56]

Please don't use DCHECK for code with side-effects. This won't run in Release
mode and lead to breakage.

There seem to be many more instances of this throughout the CL.

+  return result.Append(FILE_PATH_LITERAL("cld2_component")).Append(kCrxVersion);
+}
+
+base::FilePath GetStandaloneFileSource() {

[Takashi Toyoshima, 2014/05/19 14:32:24]

GetStandaloneDataFileSource() is consistent with the next *Destination()?

[Andrew Hayden (chromium.org), 2014/05/19 15:14:21]

Standardized on "DataFile" wherever I was using "*File".

+  return GetTestDataSourceDirectory()
+      .Append(FILE_PATH_LITERAL("_platform_specific"))
+      .Append(FILE_PATH_LITERAL("all"))
+      .Append(chrome::kCLDDataFilename);
+}
+
+base::FilePath GetStandaloneDataFileDestination() {
+  base::FilePath result;
+  DCHECK(PathService::Get(chrome::DIR_USER_DATA, &result));

[Takashi Toyoshima, 2014/05/19 14:32:24]

Just a question to check my understanding.
The data file is copied to DIR_USER_DATA, therefore it is isolated in each test.
So these tests can run in parallel safely, right?

[Paweł Hajdan Jr., 2014/05/19 15:06:56]

Yes.

[Andrew Hayden (chromium.org), 2014/05/19 15:14:21]

Asked Pawel to confirm this. I've added comments to clarify. Thanks, Pawel! And
great question, Takashi. I appreciate the diligence.

+  return result.Append(chrome::kCLDDataFilename);
+}
+
+base::FilePath GetExtractedComponentDestination() {
+  base::FilePath result;
+  DCHECK(PathService::Get(chrome::DIR_COMPONENT_CLD2, &result));

[Takashi Toyoshima, 2014/05/19 14:32:24]

Same question. Actually I'm not familiar with where DIR_COMPONENT_* are created,
but assume that it is under DIR_USER_DATA or something like that. Is this true?

[Andrew Hayden (chromium.org), 2014/05/19 15:14:21]

Yes, and I've added a comment to make this clear.

+  return result;
+}
+
+base::FilePath GetComponentDataFileDestination() {
+  return GetExtractedComponentDestination()
+      .Append(kCrxVersion)
+      .Append(FILE_PATH_LITERAL("_platform_specific"))
+      .Append(FILE_PATH_LITERAL("all"))
+      .Append(chrome::kCLDDataFilename);
+}
+
+void DeleteStandaloneFile() {

[Takashi Toyoshima, 2014/05/19 14:32:24]

DeleteStandaloneDataFile() is consistent with others?

[Andrew Hayden (chromium.org), 2014/05/19 15:14:21]

Standardized all use of "*File" to "DataFile"

+  base::DeleteFile(GetStandaloneDataFileDestination(), false);
+}

[Takashi Toyoshima, 2014/05/19 14:32:24]

empty line between line 61 and 62.

[Andrew Hayden (chromium.org), 2014/05/19 15:14:21]

Done.

+void CopyStandaloneFile() {

[Takashi Toyoshima, 2014/05/19 14:32:24]

CopyStandaloneDataFile()?
It you feel naming functions as *DataFile() is redundant, naming them as *File()
is also ok even if all names are consistent.

[Andrew Hayden (chromium.org), 2014/05/19 15:14:21]

Made consistent.

+  DeleteStandaloneFile();  // sanity: blow away any old copies
+  const base::FilePath target_file = GetStandaloneDataFileDestination();
+  const base::FilePath target_dir = target_file.DirName();
+  DCHECK(base::CreateDirectoryAndGetError(target_dir, NULL));
+  DCHECK(base::CopyFile(GetStandaloneFileSource(), target_file));
+  DCHECK(base::PathExists(target_file));
+}
+
+void DeleteComponentTree() {
+  base::DeleteFile(GetExtractedComponentDestination(), true);
+}
+
+void CopyComponentTree() {
+  DeleteComponentTree();  // sanity: blow away any old copies
+  const base::FilePath target_dir = GetExtractedComponentDestination();
+  const base::FilePath source_dir = GetTestDataSourceDirectory();
+  DCHECK(base::CreateDirectoryAndGetError(target_dir, NULL));
+  DCHECK(base::CopyDirectory(source_dir, target_dir, true));
+  DCHECK(base::PathExists(target_dir));
+  DCHECK(base::PathExists(GetComponentDataFileDestination()));
+}
+
+}  // namespace
+
+namespace test {
+
+ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() {
+  MakeDynamicCLDDataAvailableForTest();
+}
+
+ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() {
+  MakeDynamicCLDDataUnavailableForTest();
+}
+
+#if defined(CLD2_DYNAMIC_MODE)
+void ScopedCLDDynamicDataHarness::ClearStandaloneFileState() {
+  base::AutoLock lock(TranslateTabHelper::s_file_lock_.Get());
+  if (TranslateTabHelper::s_cached_file_) {
+    // Leaks any open handle, no way to avoid safely.
+    TranslateTabHelper::s_cached_file_ = NULL;
+    TranslateTabHelper::s_cached_data_offset_ = 0;
+    TranslateTabHelper::s_cached_data_length_ = 0;
+  }
+}
+#else
+void ScopedCLDDynamicDataHarness::ClearStandaloneFileState() {

[Takashi Toyoshima, 2014/05/19 14:32:24]

If we move ClearStandaloneFileState() to anonymous namespace, empty
implementation for other conditions are not needed.

[Andrew Hayden (chromium.org), 2014/05/19 15:14:21]

Can't be done because it accesses "friend"-only data members of the
TranslateTabHelper. I added a comment to this effect.

[Takashi Toyoshima, 2014/05/19 16:14:20]

Oh, sorry I missed it.
Agreed.

+}
+#endif
+
+#if (CLD_VERSION == 1) || !defined(CLD2_DYNAMIC_MODE)
+
+// No dynamic data capabilities; methods do nothing.
+void ScopedCLDDynamicDataHarness::MakeDynamicCLDDataAvailableForTest() {
+}
+void ScopedCLDDynamicDataHarness::MakeDynamicCLDDataUnavailableForTest() {
+}
+
+#elif defined(CLD2_IS_COMPONENT)
+
+// Dynamic data mode is enabled and we are using the component updater
+void ScopedCLDDynamicDataHarness::MakeDynamicCLDDataAvailableForTest() {
+  CopyComponentTree();
+  component_updater::CldComponentInstallerTraits::SetLatestCldDataFile(
+      GetComponentDataFileDestination());
+}
+
+void ScopedCLDDynamicDataHarness::MakeDynamicCLDDataUnavailableForTest() {
+  component_updater::CldComponentInstallerTraits::SetLatestCldDataFile(
+      base::FilePath());
+  DeleteComponentTree();
+}
+
+#else

[Takashi Toyoshima, 2014/05/19 14:32:24]

This #if/#elif/#else conditions seem to need a comment. Is the last #else
equivalent to defined(CLD2_DYNAMIC_MODE)?

Also, how about removing MakeDynamicCLDDataAvailableForTest() and
MakeDynamicCLDDataUnavailableForTest(), then implementing ctor and dtor directly
here. I feel it will be more straight-forward and easy to read.

e.g.,
#if defined(CLD2_IS_COMPONENT)
namespace {
void ClearStandaloneFileState() {
  ...
}
}
#endif

#if (CLD_VERSION == 1) || !defined(CLD2_DYNAMIC_MODE)

ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() {
  ...
}

ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() {
  ...
}

#elif defined(CLD2_IS_COMPONENT)

ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() {
  ...
}

ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() {
  ...
}

#elif defined(CLD2_DYNAMIC_MODE)

ScopedCLDDynamicDataHarness::ScopedCLDDynamicDataHarness() {
  ClearStandaloneFileState();
  ...
}

ScopedCLDDynamicDataHarness::~ScopedCLDDynamicDataHarness() {
  ClearStandaloneFileState();
  ...
}

#else

#error

#endif

[Andrew Hayden (chromium.org), 2014/05/19 15:14:21]

I've got comments right inside the #ifdefs. I think those should be sufficient.
I've done as you requested with the ctor/dtor code, though, good thought.

+
+// Dynamic data mode is enabled and we are using a standalone file
+void ScopedCLDDynamicDataHarness::MakeDynamicCLDDataAvailableForTest() {
+  ClearStandaloneFileState();
+  CopyStandaloneFile();
+}
+
+void ScopedCLDDynamicDataHarness::MakeDynamicCLDDataUnavailableForTest() {
+  ClearStandaloneFileState();
+  DeleteStandaloneFile();
+}
+
+#endif
+
+}  // namespace test