Make it possible to read CLD data from a file

chrome/browser/translate/translate_tab_helper.cc

Index: chrome/browser/translate/translate_tab_helper.cc
diff --git a/chrome/browser/translate/translate_tab_helper.cc b/chrome/browser/translate/translate_tab_helper.cc
index 8cf93f32beadf62b351e251df5b5f10bae13e93a..0096f4e024c3f86a6e64cc0bbf0bba66f20db753 100644
--- a/chrome/browser/translate/translate_tab_helper.cc
+++ b/chrome/browser/translate/translate_tab_helper.cc
@@ -4,7 +4,12 @@
 
 #include "chrome/browser/translate/translate_tab_helper.h"
 
+#include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/path_service.h"
+#include "base/platform_file.h"
+#include "base/synchronization/lock.h"
+#include "base/task_runner.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/translate/translate_accept_languages_factory.h"
@@ -16,19 +21,33 @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/translate/translate_bubble_factory.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/render_messages.h"
 #include "components/translate/core/browser/page_translated_details.h"
 #include "components/translate/core/browser/translate_accept_languages.h"
 #include "components/translate/core/browser/translate_prefs.h"
 #include "components/translate/core/common/language_detection_details.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(TranslateTabHelper);
 
+#if defined(CLD2_DYNAMIC_MODE)
+// Statics defined in the .h file:
+base::PlatformFile TranslateTabHelper::s_cached_platform_file_ = 0;

[bulach, 2014/03/23 20:57:02]

nit: sorry, I just found that it should be kInvalidPlatformFileValue rather than
my previous NULL suggestion :)

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

Done.

+base::LazyInstance<base::Lock> TranslateTabHelper::s_file_lock_
+  = LAZY_INSTANCE_INITIALIZER;
+#endif
+
 TranslateTabHelper::TranslateTabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
+#if defined(CLD2_DYNAMIC_MODE)
+      weak_pointer_factory_(this),
+#endif
       translate_driver_(&web_contents->GetController()),
       translate_manager_(new TranslateManager(this, prefs::kAcceptLanguages)) {}
 
@@ -119,6 +138,9 @@ bool TranslateTabHelper::OnMessageReceived(const IPC::Message& message) {
     IPC_MESSAGE_HANDLER(ChromeViewHostMsg_TranslateLanguageDetermined,
                         OnLanguageDetermined)
     IPC_MESSAGE_HANDLER(ChromeViewHostMsg_PageTranslated, OnPageTranslated)
+#if defined(CLD2_DYNAMIC_MODE)
+    IPC_MESSAGE_HANDLER(ChromeViewHostMsg_NeedCLDData, OnCLDDataRequested)
+#endif
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 
@@ -140,6 +162,99 @@ void TranslateTabHelper::WebContentsDestroyed(
   translate_manager_.reset();
 }
 
+#if defined(CLD2_DYNAMIC_MODE)
+void TranslateTabHelper::OnCLDDataRequested() {
+  // Quickly try to read s_cached_platform_file_. If it's non-zero, we have
+  // cached the CLD data file and can answer immediately; else, we'll make
+  // a request to the blocking pool to look up the file and cache it.
+  // The value of s_cached_platform_file_ becomes non-zero is only set once,
+  // so when it becomes non-zero we are guaranteed it will not change again.
+  if (!s_file_lock_.Get().Try())
+    return; // We'll get another request later, whatevs!
+  const base::PlatformFile handle = s_cached_platform_file_;
+  s_file_lock_.Get().Release();
+
+  if (handle == 0) {

[bulach, 2014/03/23 20:57:02]

nit: s/0/kInvalidPlatformFileValue/

I've seen the following pattern in some other places (i.e., early return and no
else):

if (handle != kInvalidPlatformFileValue) {
  SendCLDDataAvailable(handle);
  return;
}
// No data file yet, queue an asynchronous caching attempt.
// ...

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

Done.

+    // We don't have the data file yet. Queue an asynchronous caching attempt.

[bulach, 2014/03/23 20:57:02]

nit: I've got reviewers asking to not have pronouns, like my suggestion above..

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

Done, and reviewed the other .h/.cc files as well to try and clean up the
grammar.

+    // The caching attempt happens in the blocking pool because it may involve
+    // arbitrary filesystem access.
+    // After the caching attempt is made, we call MaybeSendCLDDataAvailable
+    // to pass the file handle to the renderer. This only results in an IPC
+    // message if the caching attempt was successful.
+    content::BrowserThread::PostBlockingPoolTaskAndReply(
+        FROM_HERE,
+        base::Bind(&TranslateTabHelper::HandleCLDDataRequest),
+        base::Bind(&TranslateTabHelper::MaybeSendCLDDataAvailable,
+                   weak_pointer_factory_.GetWeakPtr()));
+  } else {
+    // We do have the data available. Respond to the request.
+    SendCLDDataAvailable(handle);
+  }
+}
+
+void TranslateTabHelper::MaybeSendCLDDataAvailable() {
+  if (!s_file_lock_.Get().Try())

[bulach, 2014/03/23 20:57:02]

I don't quite follow why it's just trying rather than actually acquiring the
lock.. the AutoLock seems a fairly common pattern, so if this has extra
requirements preventing it from using, it should probably be documented?

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

As discussed offline, because in terms of performance the "try" with the widely
scoped blocking in the blocking pool mitigates performance concerns. We decided
offline to more narrowly scope the lock in the blocking pool, at which point
there should be no reason to need the try.

+    return; // We'll get another request later, whatevs!
+  const base::PlatformFile handle = s_cached_platform_file_;
+  s_file_lock_.Get().Release();
+  if (handle == 0)
+    return; // Nope, not ready yet
+  SendCLDDataAvailable(handle);
+}
+
+void TranslateTabHelper::SendCLDDataAvailable(const base::PlatformFile handle) {

[bulach, 2014/03/23 20:57:02]

nit: can this be a const &?

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

It's a file descriptor (integer) under the covers. Is there a strong reason to
prefer a reference? It seems simpler to me to just leave it as-is.

+  // WARNING: This method can be invoked from either the IO thread or the
+  // blocking pool. Do not use the synchronous invocation style.

[bulach, 2014/03/23 20:57:02]

not clear, synchronous invocation style of what?

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

Cleared up, I hope.

+
+  // We have the data available, respond to the request.
+  IPC::PlatformFileForTransit ipc_platform_file =
+      IPC::GetFileHandleForProcess(
+          handle,

[ulas, 2014/03/21 21:08:32]

Pls also allow offset+length for flexibility. Perhaps the cld2 data is part of a
larger "downloaded data" blob.

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

Will do this shortly, after I address Marcus' larger-scope comments.

+          GetWebContents()->GetRenderViewHost()->GetProcess()->GetHandle(),
+          false);
+  Send(new ChromeViewMsg_CLDDataAvailable(
+      GetWebContents()->GetRenderViewHost()->GetRoutingID(),
+      ipc_platform_file));
+}
+
+void TranslateTabHelper::HandleCLDDataRequest() {
+  // Because this function involves arbitrary file system access, it must run
+  // on the blocking pool.
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  base::AutoLock lock(s_file_lock_.Get());

[bulach, 2014/03/23 20:57:02]

nit: use a separate block here:
{
  base::AutoLock lock(s_file_lock_.get()));
  ....
}

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

Done.

+  if (s_cached_platform_file_ != 0)
+    return; // already done, duplicate request
+
+  base::FilePath path;
+  if (!PathService::Get(chrome::DIR_USER_DATA, &path)) {
+    LOG(WARNING) << "Unable to locate user data directory";
+    return; // Chrome isn't properly installed.
+  }
+
+  // If the file exists, we can send an IPC-safe construct back to the
+  // renderer process immediately.
+  path = path.Append(chrome::kCLDDataFilename);
+  if (!base::PathExists(path))
+    return;
+
+  // Attempt to open the file for reading. It exists, so we should succeed.
+  bool created = false;
+  base::PlatformFileError error;
+  s_cached_platform_file_ = base::CreatePlatformFile(

[bulach, 2014/03/23 20:57:02]

then here, use a local variable.

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

Done.

+      path,
+      base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ,
+      &created, &error);
+  DCHECK(!created);
+  if (error != base::PLATFORM_FILE_OK) {
+    s_cached_platform_file_ = 0;
+    LOG(WARNING) << "CLD data file exists but cannot be opened";
+    return;
+  }

[bulach, 2014/03/23 20:57:02]

so finally, only here use the lock again to actually set the platform_file.

if (platform_file) {
  base::AutoLock auto_lock(...);
  // Another blocking pool request got here first, close the file.
  if (s_cached_platform_file_) {
    ClosePlatformFile(platform_file);
    return;
  }
  // Set the global file.
  s_cached_platform_file_ = platform_file;
}

[Andrew Hayden (chromium.org), 2014/03/24 15:18:24]

Done.

+}
+#endif // defined(CLD2_DYNAMIC_MODE)
+
 void TranslateTabHelper::OnLanguageDetermined(
     const LanguageDetectionDetails& details,
     bool page_needs_translation) {