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 bae7a5984c1eafd9be8b712c9d2192326930bed0..c1e174828ffcc2d76abfadd9f367df4c17583520 100644
--- a/chrome/browser/translate/translate_tab_helper.cc
+++ b/chrome/browser/translate/translate_tab_helper.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/browser/translate/translate_tab_helper.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"
@@ -15,19 +19,32 @@
 #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/14 15:30:58]

nit: NULL is probably cleaerer

[Andrew Hayden (chromium.org), 2014/03/20 16:12:53]

The compiler disagrees with you. I shall side with the compiler :P


../../chrome/browser/translate/translate_tab_helper.cc:40:66: error: converting
to non-pointer type 'int' from NULL [-Werror=conversion-null]

+base::Lock TranslateTabHelper::s_file_lock_;
+#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)) {}
 
@@ -112,6 +129,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()
 
@@ -133,6 +153,90 @@ void TranslateTabHelper::WebContentsDestroyed(
   translate_manager_.reset();
 }
 
+#if defined(CLD2_DYNAMIC_MODE)
+void TranslateTabHelper::OnCLDDataRequested() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  // 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_.Try())

[bulach, 2014/03/14 15:30:58]

nit: use base::AutoLock, is a more common pattern.

[Andrew Hayden (chromium.org), 2014/03/19 12:22:23]

I explicitly chose Lock.try() instead. AutoLock will call acquire(). I don't
want this thread to block in the event that the file is currently being opened;
it's not worth it. The renderer will call again later. Make sense?

+    return; // We'll get another request later, whatevs!
+  const base::PlatformFile handle = s_cached_platform_file_;
+  s_file_lock_.Release();
+
+  if (handle == 0) {

[bulach, 2014/03/14 15:30:58]

nit: NULL

[Andrew Hayden (chromium.org), 2014/03/20 16:12:53]

See previous comment, re: compiler.

+    // We don't have the data file yet. Queue an asynchronous caching attempt.
+    // The caching attempt happens in the blocking pool because it may involve
+    // arbitrary filesystem access.
+    content::BrowserThread::PostBlockingPoolTask(
+        FROM_HERE,
+        base::Bind(&TranslateTabHelper::HandleCLDDataRequest,
+                   weak_pointer_factory_.GetWeakPtr()));

[bulach, 2014/03/14 15:30:58]

hmm... WeakPtr can't be dereferenced across threads. :(
it looks like HandleCLDDataRequest is only dealing with static, right?
in that case, make that function static so that this closure won't need any
param?

[bulach, 2014/03/19 11:18:51]

btw, I forgot to mention "Post(BlockingPool)TaskAndReply".
the idea being:
- blocking pool will be just a static method, not dealing with this object at
all.
- the reply is a callback with WeakPtr: it'll be executed back in the caller
thread's task runner so it won't have any problems since it won't be
dereferenced across different threads.

[Andrew Hayden (chromium.org), 2014/03/19 12:22:23]

But I was under the impression that the weakptr was necessary to provide the
object reference upon which the target method is invoked. Guess I need to do
some more reading. But yeah, static all the way, so I guess? The method itself
could presumably be static, too.

+  } else {
+    // We do have the data available. Respond to the request.
+    SendCLDDataAvailable(handle);
+  }
+}
+
+void TranslateTabHelper::SendCLDDataAvailable(const base::PlatformFile handle) {
+  // WARNING: This method can be invoked from either the IO thread or the
+  // blocking pool. Do not use the synchronous invocation style.
+
+  // We have the data available, respond to the request.
+  IPC::PlatformFileForTransit ipc_platform_file =
+      IPC::GetFileHandleForProcess(
+          handle,
+          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_);

[bulach, 2014/03/14 15:30:58]

locks are expensive, make them scoped:
{
  base::AutoLock auto_lock(s_file_lock_);
  if (s_cached_platform_file_ != 0)
    return;
}
...
..
..
{
  base::AutoLock auto_lock(s_file_lock_);
  if (s_cached_platform_file_ == 0)
    s_cached_platform_file_ = base::CreatePlatformFile(....);
}

[Andrew Hayden (chromium.org), 2014/03/19 12:22:23]

The only other code that depends upon this lock is the Try() call in
OnCLDDataRequested, which should make this into a non-issue. It's simpler to
keep it scoped the way it is, and it's ok if it ends up running long because
nothing else will attempt to Acquire() the lock.

+  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(
+      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;
+  }
+
+  // If we get this far, we've just cached the file. Respond immediately.
+  lock.~AutoLock(); // Release the lock now rather than after responding

[bulach, 2014/03/14 15:30:58]

as above, create smaller scopes instead..

[Andrew Hayden (chromium.org), 2014/03/19 12:22:23]

See reply above.

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