Check |errorCode| of translate.js and notify to Browser

components/translate/content/renderer/translate_helper.cc

 // Copyright (c) 2012 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 "components/translate/content/renderer/translate_helper.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 148 matching lines @@
 }
 
 bool TranslateHelper::HasTranslationFinished() {
   return ExecuteScriptAndGetBoolResult("cr.googleTranslate.finished", true);
 }
 
 bool TranslateHelper::HasTranslationFailed() {
   return ExecuteScriptAndGetBoolResult("cr.googleTranslate.error", true);
 }
 
+int64_t TranslateHelper::GetErrorCodeWithFallback(
+    TranslateErrors::Type fallback) {
+  int64_t error_code = ExecuteScriptAndGetIntegerResult(
+      "cr.googleTranslate.errorCode", static_cast<int>(fallback));
+  DCHECK_LT(error_code, static_cast<int>(TranslateErrors::TRANSLATE_ERROR_MAX));
+  return error_code;
+}
+
 bool TranslateHelper::StartTranslation() {
   std::string script = "cr.googleTranslate.translate('" +
                        source_lang_ +
                        "','" +
                        target_lang_ +
                        "')";
   return ExecuteScriptAndGetBoolResult(script, false);
 }
 
 std::string TranslateHelper::GetOriginalPageLanguage() {
@@ skipping 66 matching lines @@
   WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
   main_frame->ExecuteScriptInIsolatedWorld(world_id_, &source, 1, &results);
   if (results.size() != 1 || results[0].IsEmpty() || !results[0]->IsNumber()) {
     NOTREACHED();
     return 0.0;
   }
 
   return results[0]->NumberValue();
 }
 
+int64_t TranslateHelper::ExecuteScriptAndGetIntegerResult(
+    const std::string& script,
+    int64_t fallback) {
+  WebLocalFrame* main_frame = render_frame()->GetWebFrame();
+  if (!main_frame)
+    return fallback;
+
+  v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
+  WebVector<v8::Local<v8::Value>> results;
+  WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
+  main_frame->ExecuteScriptInIsolatedWorld(world_id_, &source, 1, &results);
+  if (results.size() != 1 || results[0].IsEmpty() || !results[0]->IsNumber()) {
+    NOTREACHED();

[Takashi Toyoshima, 2017/06/27 12:04:55]

If this actually happens as you said, this NOTREACHED() should not be here.
But, does this actually happen?

We always call ExecuteScript(translate_script) at translate_helper.cc line 337.
Once the call is finished, cr.googleTranslate.errorCode should be set to 0
synchronously, IIUC.

[Gaja, 2017/06/28 03:43:31]

It reaches this point for test cases in TrnaslateHelperBrowserTest. That is
because we are passing empty string as translate_script like below. 

  translate_helper_->TranslatePage("en", "fr", std::string());

Previously i.e before my CL, we were not facing this issue because we were not
getting the error code from translate.js; instead, we were directly notifying
with default error TRANSLATION_ERROR :
  NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_ERROR);

With Patch Set 6, it will not reach this line for any test case.
Please let me if its okay to revert back to PS6.

[Gaja, 2017/06/28 04:16:00]

Sorry, I made a mistake here.
With Patch Set 6, it will still reach this line for few test cases because of
empty script as parameter value in TranslateLibNeverReady, TranslateSuccess &
TranslateFailure test cases (TranslateHelperBrowserTest). So please let me know
if this line should be removed (only in ExecuteScriptAndGetIntegerResult
function).

[Takashi Toyoshima, 2017/06/28 07:25:32]

So, we should still have this NOTREACHED() check and stop providing the fallback
parameter? That would help not to ship Chrome with a broken translate.js.

Unit tests were just written as simple as possible. You can just fixed such test
to be aligned with the latest production code. Probably, you can just pass
std::string("cr = { googleTranslate: { errorCode: 0 } }") instead of
std::string().

[Gaja, 2017/06/28 11:34:45]

Okay.
Done.
I understand that test cases should be written/modified according to latest
production code and not vice versa :).
I tried with std::string("cr = { googleTranslate: { errorCode: 0 } }"), but
still it was reaching NOTREACHED in ExecuteScriptAndGetIntegerResult. I have
fixed it by making use of EXPECT_CALL for GetErrorCode. I hope this is fine.

+    return fallback;
+  }
+
+  return results[0]->IntegerValue();
+}
+
 // mojom::Page implementations.
 void TranslateHelper::Translate(const std::string& translate_script,
                                 const std::string& source_lang,
                                 const std::string& target_lang,
                                 TranslateCallback callback) {
   WebLocalFrame* main_frame = render_frame()->GetWebFrame();
   if (!main_frame) {
     // Cancelled.
     std::move(callback).Run(true, source_lang, target_lang,
                             TranslateErrors::NONE);
@@ skipping 53 matching lines @@
   CancelPendingTranslation();
 
   ExecuteScript("cr.googleTranslate.revert()");
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // TranslateHelper, private:
 void TranslateHelper::CheckTranslateStatus() {
   // First check if there was an error.
   if (HasTranslationFailed()) {
-    // TODO(toyoshim): Check |errorCode| of translate.js and notify it here.
-    NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_ERROR);
+    NotifyBrowserTranslationFailed(
+        static_cast<translate::TranslateErrors::Type>(
+            GetErrorCodeWithFallback(TranslateErrors::TRANSLATION_ERROR)));
     return;  // There was an error.
   }
 
   if (HasTranslationFinished()) {
     std::string actual_source_lang;
     // Translation was successfull, if it was auto, retrieve the source
     // language the Translate Element detected.
     if (source_lang_ == kAutoDetectionLanguage) {
       actual_source_lang = GetOriginalPageLanguage();
       if (actual_source_lang.empty()) {
@@ skipping 25 matching lines @@
   // The translation is still pending, check again later.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, base::Bind(&TranslateHelper::CheckTranslateStatus,
                             weak_method_factory_.GetWeakPtr()),
       AdjustDelay(kTranslateStatusCheckDelayMs));
 }
 
 void TranslateHelper::TranslatePageImpl(int count) {
   DCHECK_LT(count, kMaxTranslateInitCheckAttempts);
   if (!IsTranslateLibReady()) {
+    // There was an error during initialization of library.
+    TranslateErrors::Type error = static_cast<translate::TranslateErrors::Type>(
+        GetErrorCodeWithFallback(TranslateErrors::NONE));
+    if (error != TranslateErrors::NONE) {
+      NotifyBrowserTranslationFailed(error);
+      return;
+    }
+
     // The library is not ready, try again later, unless we have tried several
     // times unsuccessfully already.
     if (++count >= kMaxTranslateInitCheckAttempts) {
-      NotifyBrowserTranslationFailed(TranslateErrors::INITIALIZATION_ERROR);
+      NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_TIMEOUT);
       return;
     }
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE, base::Bind(&TranslateHelper::TranslatePageImpl,
                               weak_method_factory_.GetWeakPtr(), count),
         AdjustDelay(count * kTranslateInitCheckDelayMs));
     return;
   }
 
   // The library is loaded, and ready for translation now.
   // Check JavaScript performance counters for UMA reports.
   ReportTimeToBeReady(
       ExecuteScriptAndGetDoubleResult("cr.googleTranslate.readyTime"));
   ReportTimeToLoad(
       ExecuteScriptAndGetDoubleResult("cr.googleTranslate.loadTime"));
 
   if (!StartTranslation()) {
-    NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_ERROR);
+    CheckTranslateStatus();
     return;
   }
   // Check the status of the translation.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, base::Bind(&TranslateHelper::CheckTranslateStatus,
                             weak_method_factory_.GetWeakPtr()),
       AdjustDelay(kTranslateStatusCheckDelayMs));
 }
 
 void TranslateHelper::NotifyBrowserTranslationFailed(
@@ skipping 17 matching lines @@
   binding_.Close();
   translate_callback_pending_.Reset();
   CancelPendingTranslation();
 }
 
 void TranslateHelper::OnDestruct() {
   delete this;
 }
 
 }  // namespace translate