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

components/translate/content/renderer/translate_helper.cc

Index: components/translate/content/renderer/translate_helper.cc
diff --git a/components/translate/content/renderer/translate_helper.cc b/components/translate/content/renderer/translate_helper.cc
index f1b8ce439604062e46d72094d4f05731725861dc..c2e1989e08202c8a5461b485bf6d4e27654ec60e 100644
--- a/components/translate/content/renderer/translate_helper.cc
+++ b/components/translate/content/renderer/translate_helper.cc
@@ -166,6 +166,14 @@ 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_ +
@@ -252,6 +260,25 @@ double TranslateHelper::ExecuteScriptAndGetDoubleResult(
   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,
@@ -325,8 +352,9 @@ void TranslateHelper::RevertTranslation() {
 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.
   }
 
@@ -372,10 +400,18 @@ void TranslateHelper::CheckTranslateStatus() {
 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(
@@ -393,7 +429,7 @@ void TranslateHelper::TranslatePageImpl(int count) {
       ExecuteScriptAndGetDoubleResult("cr.googleTranslate.loadTime"));
 
   if (!StartTranslation()) {
-    NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_ERROR);
+    CheckTranslateStatus();
     return;
   }
   // Check the status of the translation.