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..ebd97ab5b4e631fe46e9d80da1987079b895b5c1 100644
--- a/components/translate/content/renderer/translate_helper.cc
+++ b/components/translate/content/renderer/translate_helper.cc
@@ -166,6 +166,12 @@ bool TranslateHelper::HasTranslationFailed() {
   return ExecuteScriptAndGetBoolResult("cr.googleTranslate.error", true);
 }
 
+int64_t TranslateHelper::GetErrorCode() {
+  return ExecuteScriptAndGetIntegerResult(
+      "cr.googleTranslate.errorCode",
+      static_cast<int>(TranslateErrors::TRANSLATION_ERROR));
+}
+
 bool TranslateHelper::StartTranslation() {
   std::string script = "cr.googleTranslate.translate('" +
                        source_lang_ +
@@ -252,6 +258,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();
+    return fallback;
+  }
+
+  return results[0]->IntegerValue();
+}
+
 // mojom::Page implementations.
 void TranslateHelper::Translate(const std::string& translate_script,
                                 const std::string& source_lang,
@@ -325,8 +350,8 @@ 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>(GetErrorCode()));

[napper, 2017/06/16 12:41:51]

Consider checking that GetErrorCode() < TRANSLATE_ERROR_MAX

[Takashi Toyoshima, 2017/06/16 16:14:17]

maybe DCHECK inside GetErrorCode impl?

[Gaja, 2017/06/17 06:41:46]

Acknowledged.

[Gaja, 2017/06/17 13:25:48]

Done.

     return;  // There was an error.
   }
 
@@ -339,14 +364,16 @@ void TranslateHelper::CheckTranslateStatus() {
       if (actual_source_lang.empty()) {
         NotifyBrowserTranslationFailed(TranslateErrors::UNKNOWN_LANGUAGE);
         return;
-      } else if (actual_source_lang == target_lang_) {
-        NotifyBrowserTranslationFailed(TranslateErrors::IDENTICAL_LANGUAGES);
-        return;
       }
     } else {
       actual_source_lang = source_lang_;
     }
 
+    if (actual_source_lang == target_lang_) {

[Takashi Toyoshima, 2017/06/16 04:51:29]

What is the motivation to move here?
|actual_source_lang| was set only when kAutoDetectionLanguage is specified.
Original place looks correct to me.

[Gaja, 2017/06/17 06:41:46]

Please correct me if I am wrong.
As per my analysis, valid languages (e.g. "fr") are not indicated as
kAutoDetectionLanguage. In case, if both source and target languages are valid
(manager->TranslatePage("fr", "fr")) then it may never reach line 343. So I
assumed that, the error IDENTICAL_LANGUAGES should also be applied to valid
languages as well, and moved out of the if-else block. If this is not the right
way, then I will revert this and update my test case accordingly. Please let me
know your opinion.

[Takashi Toyoshima, 2017/06/20 11:49:54]

Here, |source_lang| is the language Chrome's CLD detects.
kAutoDetectionLanguage is used when CLD fails to detect. In such case, the
server returns a server detected source language. That's what
GetOriginalPageLanguage() returns.

If the server think source and target are the same language, it won't translate
the content. So, returning IDENTICAL_LANGUAGES is reasonable for
kAutoDetectionLanguage cases.

But even if Chrome think the |source_lang| is "en", and the |target_lang| is
specified as "en", server could translate contents partially if the page has a
mixed language contents, e.g. mainly written in English, but some parts are in
Japanese.

So, even though |source_lang| == |target_lang|, we should not assume it as
IDENTICAL_LANGUAGES.

You says "it may never reach line 343", but could you elaborate this case? It
might be a case I haven't notice.

[Gaja, 2017/06/22 05:22:23]

Thanks for the detailed explanation.

Now I understand that why we should throw IDENTICAL_LANGUAGES error only when
CLD fails to detect, i.e for kAutoDetectionLanguage.
Actually, I meant, for valid languages it never reach line 343. But that makes
sense with your explanation :

"But even if Chrome think the |source_lang| is "en", and the |target_lang| is
specified as "en", server could translate contents partially if the page has a
mixed language contents, e.g. mainly written in English, but some parts are in
Japanese."

Sorry for not considering this case.

+      NotifyBrowserTranslationFailed(TranslateErrors::IDENTICAL_LANGUAGES);
+      return;
+    }
+
     if (!translate_callback_pending_) {
       NOTREACHED();
       return;
@@ -372,10 +399,18 @@ void TranslateHelper::CheckTranslateStatus() {
 void TranslateHelper::TranslatePageImpl(int count) {
   DCHECK_LT(count, kMaxTranslateInitCheckAttempts);
   if (!IsTranslateLibReady()) {
+    // There was an error during initialization of library.

[Takashi Toyoshima, 2017/06/16 04:51:28]

I don't think errorCode is ready here, because IsTranslateLibReady() means we
are not ready to run translate js library (it may be still downloading the
server providied library?)

[Gaja, 2017/06/17 06:41:46]

I added this check considering the way the errorCode is set in translate.js.

      try {
        lib = google.translate.TranslateService({
          // translateApiKey is predefined by translate_script.cc.
          'key': translateApiKey,
          'serverParams': serverParams,
          'timeInfo': gtTimeInfo,
          'useSecureConnection': true
        });
        translateApiKey = undefined;
        serverParams = undefined;
        gtTimeInfo = undefined;
      } catch (err) {
        errorCode = ERROR['INITIALIZATION_ERROR'];
        translateApiKey = undefined;
        serverParams = undefined;
        gtTimeInfo = undefined;
        return;
      }

If an error is caught in try {} block, it would set INITIALIZATION_ERROR code.
So I simulated the error by adding below test string as translate library in
browsertest.cc, which results in INITIALIZATION_ERROR.

static const char kTestScriptInitializationError[] =
    " var google = {};"
    "google.translate = (function() {"
    "  return {"
    "    TranslateService: function() {"
    "      return error;"   //  causes error
    "    }"
    "  };"
    "})();"
    "cr.googleTranslate.onTranslateElementLoad();";


Please let me know if my understanding is wrong.

[Takashi Toyoshima, 2017/06/20 11:49:54]

I see, that makes sense.
So, could you change the following condition not to check "error =
TranslateErrors::INITIALIZATION_ERROR" but to check "error !=
TranslateErrors:NONE" so that it can be robust for translate.js side code
changes?

[Gaja, 2017/06/22 05:22:23]

I made the change suggested by you and all the test cases added in
translate_manager_browsertest.cc were passed. But test case :
TranslateHelperBrowserTest.TranslateFailure was failing.

GetErrorCode() was returning TRANSLATION_ERROR as fallback value even if there
was not any error while the lib is not yet ready. So we need NONE as fallback
value here.

If we make NONE as default fallback in GetErrorCode, then, at line 353 (right
side), it does not make sense if we get NONE value as fallback because the code
is inside HasTranslationFailed().

So I thought that its better to add fallback parameter to GetErrorCode() to
address your comment and as well fixing
TranslateHelperBrowserTest.TranslateFailure test case.

Please let me know if this is NOT okay.

+    TranslateErrors::Type error =
+        static_cast<translate::TranslateErrors::Type>(GetErrorCode());
+    if (error == TranslateErrors::INITIALIZATION_ERROR) {
+      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);

[Takashi Toyoshima, 2017/06/16 04:51:28]

We use the timeout error only when actual translation process takes too long
time. But here, it just waits for server provided javascript library being
ready. This is unexpected situation in the setup process, and should be
classified as the initialization error, IMHO.

[Gaja, 2017/06/17 06:41:46]

I added this check considering the way the errorCode is set in translate.js.

    if (lib.isAvailable()) {
      readyTime = performance.now();
      libReady = true;
      return;
    }
    if (checkReadyCount++ > 5) {
      errorCode = ERROR['TRANSLATION_TIMEOUT'];
      return;
    }

I added the below test string in browsertest.cc to simulate TRANSLATION_TIMEOUT
by always returning false.

    " var google = {};"
    "google.translate = (function() {"
    "  return {"
    "    TranslateService: function() {"
    "      return {"
    "        isAvailable : function() {"
    "          return false;"
    "        },"
    "      };"
    "    }"
    "  };"
    "})();"
    "cr.googleTranslate.onTranslateElementLoad();";

If this change is not proper, then I will update my test to throw
INITIALIZATION_ERROR.

For TRANSLATION_TIMEOUT, I will need your guidance to simulate the error :)

[Takashi Toyoshima, 2017/06/20 11:49:54]

Oops, I was wrong. We already used it for library loading timeout in the
translate.js. So errorCode should be set to TRANSLATION_TIMEOUT after 100-ms x
5-tries, and we enter this "if" clause after 150-ms x 5-tries. This is a kind of
NOTREACHED case, but could happen in real use-cases on e.g., low memory pressure
cases.

So, TRANSLATE_TIMEOUT is fine. Sorry for confusion. Also, could you update
comments at components/translate/core/common/translate_errors.h? It says
But it was wrong. Probably it should be "The library doesn't finish
initialization."

[Gaja, 2017/06/22 05:22:23]

Done.

       return;
     }
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
@@ -393,7 +428,7 @@ void TranslateHelper::TranslatePageImpl(int count) {
       ExecuteScriptAndGetDoubleResult("cr.googleTranslate.loadTime"));
 
   if (!StartTranslation()) {
-    NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_ERROR);
+    CheckTranslateStatus();
     return;
   }
   // Check the status of the translation.