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 85d4f2b4fc4ae272dfcc19decc2fb9158f61a1e4..7fef9c22171785fc6526571fbf3b9115505da68b 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,34 @@
 #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_
+  = base::kInvalidPlatformFileValue;

[bulach, 2014/03/25 09:29:26]

nit: move the = to the previous line, and indent by 4 since its a continuation
(same on 44 below).

[Andrew Hayden (chromium.org), 2014/03/26 15:34:11]

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)) {}
 
@@ -138,6 +158,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()
 
@@ -159,6 +182,113 @@ void TranslateTabHelper::WebContentsDestroyed(
   translate_manager_.reset();
 }
 
+#if defined(CLD2_DYNAMIC_MODE)
+void TranslateTabHelper::OnCLDDataRequested() {
+  // Quickly try to read s_cached_platform_file_. If valid, the file handle is
+  // cached and can be used immediately. Else, queue the caching task to the
+  // blocking pool.
+  s_file_lock_.Get().Acquire();

[bulach, 2014/03/25 09:29:26]

AutoLock in its own scope?

[Andrew Hayden (chromium.org), 2014/03/26 15:34:11]

Done.

+  const base::PlatformFile handle = s_cached_platform_file_;
+  s_file_lock_.Get().Release();
+
+  if (handle != base::kInvalidPlatformFileValue) {
+    // Cached data available. Respond to the request.
+    SendCLDDataAvailable(handle);
+    return;
+  }
+
+  // Else, we don't have the data file yet. Queue a caching 0attempt.

[palmer, 2014/03/24 19:01:47]

is "0attempt" a typo?

[Andrew Hayden (chromium.org), 2014/03/26 15:34:11]

yes, fixed.

+  // 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()));
+}
+
+void TranslateTabHelper::MaybeSendCLDDataAvailable() {
+  if (!s_file_lock_.Get().Try())

[bulach, 2014/03/25 09:29:26]

since the scope for the locks are much narrower now, using autolocks throughout
makes it simpler to follow..

[Andrew Hayden (chromium.org), 2014/03/26 15:34:11]

Done.

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

[bulach, 2014/03/25 09:29:26]

in this case, the early return is not buying much. could do:

if (handle != base::kInvalidPlatformFileValue)
  SendCLDDataAvailable(handle);

[Andrew Hayden (chromium.org), 2014/03/26 15:34:11]

Done.

+  SendCLDDataAvailable(handle);
+}
+
+void TranslateTabHelper::SendCLDDataAvailable(const base::PlatformFile handle) {
+  // Data available, respond to the request.
+  IPC::PlatformFileForTransit ipc_platform_file =
+      IPC::GetFileHandleForProcess(
+          handle,
+          GetWebContents()->GetRenderViewHost()->GetProcess()->GetHandle(),
+          false);
+  // In general, sending a response from within the code path that is processing
+  // a request is discouraged because there is potential for deadlock (if the
+  // methods are sent synchronously) or loops (if the response can trigger a
+  // new request). Neither of these concerns is relevant in this code, so
+  // sending the response from within the code path of the request handler is
+  // safe.
+  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());
+    if (s_cached_platform_file_ != base::kInvalidPlatformFileValue)
+      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.
+  bool created = false;
+  base::PlatformFileError error;
+  const base::PlatformFile file = base::CreatePlatformFile(
+      path,
+      base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ,
+      &created, &error);
+  DCHECK(!created);

[palmer, 2014/03/24 19:01:47]

I wonder if this should be a cause for LOG and return, rather than just a
DCHECK.

[Andrew Hayden (chromium.org), 2014/03/26 15:34:11]

Probably not. We're attempting to open a file for read-only access. Any creation
of the file by the opening attempt is a serious programming error that would
affect tons of code. The DCHECK is for sanity.

+  if (error != base::PLATFORM_FILE_OK) {
+    LOG(WARNING) << "CLD data file exists but cannot be opened";
+    return;
+  }
+
+  s_file_lock_.Get().Acquire();
+  if (s_cached_platform_file_ != base::kInvalidPlatformFileValue) {
+    // Idempotence: Racing another request on the blocking pool, abort.
+    s_file_lock_.Get().Release();

[palmer, 2014/03/24 19:01:47]

This, and line 288, make me think you want an Autolock.

[bulach, 2014/03/25 09:29:26]

yep, something like:

{
  AutoLock(...);
  if (s_cached_platform_file_ != base::kInvalidPlatformFileValue)
    s_cached_platform_file_ = file;
}

if (file != s_cached_platform_file_) {
  // Idempotence: racing another request on the blocking pool.
  base::ClosePlatformFile(file);
}

[Andrew Hayden (chromium.org), 2014/03/26 15:34:11]

Done.

+    base::ClosePlatformFile(file);
+    return;
+  }
+
+  // Else, this request has taken care of it all. Cache the handle and return.
+  s_cached_platform_file_ = file;
+  s_file_lock_.Get().Release();
+}
+#endif // defined(CLD2_DYNAMIC_MODE)
+
 void TranslateTabHelper::OnLanguageDetermined(
     const LanguageDetectionDetails& details,
     bool page_needs_translation) {