Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2919343007:
Check |errorCode| of translate.js and notify to Browser (Closed)
DescriptionCheck |errorCode| of translate.js and notify to Browser
CheckTranslateStatus() always returns TranslateErrors::TRANSLATION_ERROR when
there is an error during translation. Instead, get the error code from
translate.js and convert to TranslateErrors::Type to notify actual error type.
BUG=301568
Review-Url: https://codereview.chromium.org/2919343007
Cr-Commit-Position: refs/heads/master@{#484223}
Committed: https://chromium.googlesource.com/chromium/src/+/5bcb90a3e2cf481e8fcfca0b5573effab5479a49
Patch Set 1 #Patch Set 2 #Patch Set 3 #
Total comments: 2
Patch Set 4 : Addressing review comments : Use static_cast for enum conversion instead of switch case. #
Total comments: 2
Patch Set 5 : Added test cases to check for translate error types and a minor refactoring. #
Total comments: 25
Patch Set 6 : Addressed review comments (2 comments yet to be addressed). Test case code simplification. #Patch Set 7 : Addressed review comments. #
Total comments: 7
Patch Set 8 : Addressed review comments. #Patch Set 9 : Update comment in translate_errors.h #
Total comments: 1
Patch Set 10 : Mock ExecuteScriptAndGetIntegerResult function #Patch Set 11 : Addressed review comment #Patch Set 12 : Removed fallback argument of ExecuteScriptAndGetIntegerResult function #Patch Set 13 : nit #Patch Set 14 : Rebase #
Total comments: 3
Patch Set 15 : Probably fixes mac failures. This patch is to check dryrun. If passes, will request for review. #
Total comments: 2
Patch Set 16 : Addressed review comment. Proper order of enum variables in switch case. #
Messages
Total messages: 73 (22 generated)
Description was changed from ========== Check |errorCode| of translate.js and notify to Browser CheckTranslateStatus() always returns TranslateErrors::TRANSLATION_ERROR when there is an error during translation. Insteadi, get the error code from translate.js and convert to TranslateErrors::Type to notify actual error type. BUG=None ========== to ========== Check |errorCode| of translate.js and notify to Browser CheckTranslateStatus() always returns TranslateErrors::TRANSLATION_ERROR when there is an error during translation. Instead, get the error code from translate.js and convert to TranslateErrors::Type to notify actual error type. BUG=None ==========
gajendra.n@samsung.com changed reviewers: + toyoshim@chromium.org
Please take a look.
Sorry, I overlooked this review request. main change looks good, but there is a minor suggestion. https://codereview.chromium.org/2919343007/diff/40001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/40001/components/translate/co... components/translate/content/renderer/translate_helper.cc:67: static TranslateErrors::Type ErrorCodeToTranslateErrorType(int64_t error_code) { can be in anonymous namespace above. But just using static_cast should be ok here because ERROR in components/translate/core/browser/resources/translate.js is JS version of TranslateErrors.
On 2017/06/13 11:58:29, Takashi Toyoshima wrote: > Sorry, I overlooked this review request. That's OK :). I have addressed your comments, please take a look at new patch set.
https://codereview.chromium.org/2919343007/diff/40001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/40001/components/translate/co... components/translate/content/renderer/translate_helper.cc:67: static TranslateErrors::Type ErrorCodeToTranslateErrorType(int64_t error_code) { On 2017/06/13 11:58:29, Takashi Toyoshima wrote: > can be in anonymous namespace above. > > But just using static_cast should be ok here because ERROR in > components/translate/core/browser/resources/translate.js is JS version of > TranslateErrors. Done.
You would find related crbugs that would be written in BUG= line. For instance, 301570 and 301568 are related? (Sorry, I filed these bugs, but could not remember details)
Also one more comment https://codereview.chromium.org/2919343007/diff/60001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/60001/components/translate/co... components/translate/content/renderer/translate_helper.cc:357: NotifyBrowserTranslationFailed(ErrorCodeToTranslateErrorType(error_code)); Probably it's consistent to use static_cast<TranslateErrors> directly here. Because you already use static_cast<int> for converting TranslateErros to errorCode. There is no reason to do the reversed conversion in a different way.
By the way do you think we could have a test to check if this error code is correctly translated into string and shown in a right UI?
On 2017/06/14 03:59:45, Takashi Toyoshima wrote: > You would find related crbugs that would be written in BUG= line. > For instance, 301570 and 301568 are related? (Sorry, I filed these bugs, but > could not remember details) Ok, I will find a related bug and mention it in BUG= line. On 2017/06/14 04:06:43, Takashi Toyoshima wrote: > By the way do you think we could have a test to check if this error code is > correctly translated into string and shown in a right UI? AFAIK, currently the translate UI (bubble) does not show exact error reason that caused translation fail. It only says that "The page could not be translated". However, we could have a test to check if error code returned by translate script is correctly converted to TranslateErrors::Type. I will try to add a test for checking error code conversion.
https://codereview.chromium.org/2919343007/diff/60001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/60001/components/translate/co... components/translate/content/renderer/translate_helper.cc:357: NotifyBrowserTranslationFailed(ErrorCodeToTranslateErrorType(error_code)); On 2017/06/14 04:00:02, Takashi Toyoshima wrote: > Probably it's consistent to use static_cast<TranslateErrors> directly here. > > Because you already use static_cast<int> for converting TranslateErros to > errorCode. There is no reason to do the reversed conversion in a different way. Acknowledged. Ok, this seems better.
gajendra.n@samsung.com changed reviewers: + napper@chromium.org
@Takashi Toyoshima I have added test cases that checks for TranslateErrors::Type enum values. Please take a look at new patch set. Adding @napper, who's owner for most the translate related bugs. Please let me know if I am supposed to add another owner mentioned in OWNERS file (translate internals: groby@chromium.org)
Description was changed from ========== Check |errorCode| of translate.js and notify to Browser CheckTranslateStatus() always returns TranslateErrors::TRANSLATION_ERROR when there is an error during translation. Instead, get the error code from translate.js and convert to TranslateErrors::Type to notify actual error type. BUG=None ========== to ========== Check |errorCode| of translate.js and notify to Browser CheckTranslateStatus() always returns TranslateErrors::TRANSLATION_ERROR when there is an error during translation. Instead, get the error code from translate.js and convert to TranslateErrors::Type to notify actual error type. BUG=301568 ==========
https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:372: if (actual_source_lang == target_lang_) { What is the motivation to move here? |actual_source_lang| was set only when kAutoDetectionLanguage is specified. Original place looks correct to me. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:402: // There was an error during initialization of library. 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?) https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:413: NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_TIMEOUT); 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.
https://docs.google.com/document/d/1W8tlFxoMTkspgYyCN0WQyzUkId0v8hbzEkvW08_r3... FYI, this document explains how translate.js works with v8 and server provided library.
https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... chrome/browser/translate/translate_manager_browsertest.cc:27: " var google = {};" indent looks wrong https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... chrome/browser/translate/translate_manager_browsertest.cc:27: " var google = {};" indent looks wrong https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... chrome/browser/translate/translate_manager_browsertest.cc:289: error_subscription = why not just use the constructor? https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:354: static_cast<translate::TranslateErrors::Type>(GetErrorCode())); Consider checking that GetErrorCode() < TRANSLATE_ERROR_MAX https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... File components/translate/content/renderer/translate_helper.h (right): https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.h:68: virtual int64_t GetErrorCode(); Can this method (and the one below) be const?
https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:354: static_cast<translate::TranslateErrors::Type>(GetErrorCode())); maybe DCHECK inside GetErrorCode impl? https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... File components/translate/content/renderer/translate_helper.h (right): https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.h:68: virtual int64_t GetErrorCode(); IMHO, the method causes a side effect should not be const even though it does not change member variables. Especially for the method below, it executes arbitrary JavaScript code on v8. That should not be const.
https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... chrome/browser/translate/translate_manager_browsertest.cc:27: " var google = {};" On 2017/06/16 12:41:51, napper wrote: > indent looks wrong Acknowledged. https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... chrome/browser/translate/translate_manager_browsertest.cc:289: error_subscription = On 2017/06/16 12:41:51, napper wrote: > why not just use the constructor? Acknowledged. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:354: static_cast<translate::TranslateErrors::Type>(GetErrorCode())); On 2017/06/16 16:14:17, Takashi Toyoshima wrote: > maybe DCHECK inside GetErrorCode impl? Acknowledged. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:372: if (actual_source_lang == target_lang_) { On 2017/06/16 04:51:29, Takashi Toyoshima wrote: > What is the motivation to move here? > |actual_source_lang| was set only when kAutoDetectionLanguage is specified. > Original place looks correct to me. 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. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:402: // There was an error during initialization of library. On 2017/06/16 04:51:28, Takashi Toyoshima wrote: > 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?) 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. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:413: NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_TIMEOUT); On 2017/06/16 04:51:28, Takashi Toyoshima wrote: > 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. 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 :)
https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... chrome/browser/translate/translate_manager_browsertest.cc:27: " var google = {};" On 2017/06/16 12:41:51, napper wrote: > indent looks wrong Done. https://codereview.chromium.org/2919343007/diff/80001/chrome/browser/translat... chrome/browser/translate/translate_manager_browsertest.cc:289: error_subscription = On 2017/06/16 12:41:51, napper wrote: > why not just use the constructor? Done. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:354: static_cast<translate::TranslateErrors::Type>(GetErrorCode())); On 2017/06/16 16:14:17, Takashi Toyoshima wrote: > maybe DCHECK inside GetErrorCode impl? Done.
lgtm
looks good with one minor request. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:372: if (actual_source_lang == target_lang_) { 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. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:402: // There was an error during initialization of library. 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? https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:413: NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_TIMEOUT); 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 > TRANSLATION_TIMEOUT, // The library doesn't finish the translation. But it was wrong. Probably it should be "The library doesn't finish initialization."
@Takashi Toyoshima I have addressed your comments. Please take a look at new patch set. I hope you would be happy with new PS. Thanks. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:372: if (actual_source_lang == target_lang_) { 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. >You says "it may never reach line 343", but could you elaborate this >case? It might be a case I haven't notice. 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. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:402: // There was an error during initialization of library. 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. https://codereview.chromium.org/2919343007/diff/80001/components/translate/co... components/translate/content/renderer/translate_helper.cc:413: NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_TIMEOUT); Done.
I'm running out of time today, will take a look tomorrow.
Dear Takashi Toyoshima, Please take a look at latest patch set. Thanks.
https://codereview.chromium.org/2919343007/diff/120001/chrome/browser/transla... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/2919343007/diff/120001/chrome/browser/transla... chrome/browser/translate/translate_manager_browsertest.cc:329: EXPECT_EQ(translate::TranslateErrors::NONE, GetPageTranslatedResult()); This check fails because error_type_ wasn't initialized in TranslateManagerBrowserTest and OnTranslateError() wasn't called on successful case? https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... components/translate/content/renderer/translate_helper.cc:275: NOTREACHED(); 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.
@Takashi Toyoshima, Please check my replies to your comments and let me know your opinions and suggestions. Thanks for your patience. https://codereview.chromium.org/2919343007/diff/120001/chrome/browser/transla... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/2919343007/diff/120001/chrome/browser/transla... chrome/browser/translate/translate_manager_browsertest.cc:329: EXPECT_EQ(translate::TranslateErrors::NONE, GetPageTranslatedResult()); This check does not fail with your suggestion i.e "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?" All the test cases in translate_manager_browsertest.cc passes, but two of the test cases in translate_helper_browsertest.cc fails. They are 1) TranslateHelperBrowserTest.TranslateLibNeverReady & 2) TranslateHelperBrowserTest.TranslateSuccess The reason is that, in TranslateLibNeverReady test case : EXPECT_CALL(*translate_helper_, IsTranslateLibReady()) .Times(AtLeast(5)) // See kMaxTranslateInitCheckAttempts in // translate_helper.cc .WillRepeatedly(Return(false)); when false if returned for first call, it goes inside below 'if' check if (!IsTranslateLibReady()) { // There was an error during initialization of library. TranslateErrors::Type error = static_cast<translate::TranslateErrors::Type>(GetErrorCode()); if (error != TranslateErrors::NONE) { // Change suggested by you NotifyBrowserTranslationFailed(error); return; } The GetErrorCode function returns default fallback value i.e TRANSLATION_ERROR and exits from the function and test case fails with below error message. Actual function call count doesn't match EXPECT_CALL(*translate_helper_, IsTranslateLibReady())... Expected: to be called at least 5 times Actual: called once - unsatisfied and active This is the reason, why I added check only for INITIALIZATION_ERROR in Patch Set 6, like below, so that it does return from function on first call itself (!IsTranslateLibReady()). TranslateErrors::Type error = static_cast<translate::TranslateErrors::Type>(GetErrorCode()); if (error == TranslateErrors::INITIALIZATION_ERROR) { NotifyBrowserTranslationFailed(error); return; } GetErrorCode returns fallback value is because we are passing empty string as translate_script in the TranslateLibNeverReady test case. translate_helper_->TranslatePage("en", "fr", std::string()); Please correct me if I am wrong in any of the case above. I think that the Patch Set 6 was more appropriate in error handling for all cases (TranslateManagerBrowserTest & TranslateHelperBrowserTest), IMHO. Please let me know your opinions and suggestions. https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... components/translate/content/renderer/translate_helper.cc:275: NOTREACHED(); On 2017/06/27 12:04:55, Takashi Toyoshima wrote: > 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. 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.
https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... components/translate/content/renderer/translate_helper.cc:275: NOTREACHED(); Sorry, I made a mistake here. > With Patch Set 6, it will not reach this line for any test case. 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).
https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... components/translate/content/renderer/translate_helper.cc:275: NOTREACHED(); 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().
@Takashi Toyoshima, Thanks for your comments and guidance. Please take a look at latest patch set. Thanks. https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/120001/components/translate/c... components/translate/content/renderer/translate_helper.cc:275: NOTREACHED(); On 2017/06/28 07:25:32, Takashi Toyoshima wrote: > 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. Okay. > Unit tests were just written as simple as possible. You can just fixed such test > to be aligned with the latest production code. Done. I understand that test cases should be written/modified according to latest production code and not vice versa :). > Probably, you can just pass > std::string("cr = { googleTranslate: { errorCode: 0 } }") instead of > std::string(). 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.
Ah, that's right. Since almost all methods to touch v8 are mocked in that test, it should be the right way. Sorry, I overlooked that. Probably it would be better to mock ExecuteScriptAndGetIntegerResult too because this should be the reason why it's a virtual method.
On 2017/06/29 11:48:42, Takashi Toyoshima wrote: > Ah, that's right. > Since almost all methods to touch v8 are mocked in that test, it should be the > right way. Sorry, I overlooked that. > > Probably it would be better to mock ExecuteScriptAndGetIntegerResult too because > this should be the reason why it's a virtual method. Done.
Probably this will be the last comment. Others look fine. I confirmed that if error_type_ in TranslateManagerBrowserTest is initialized to 0 correctly, all tests pass even if we remove the fallback argument. https://codereview.chromium.org/2919343007/diff/160001/chrome/renderer/transl... File chrome/renderer/translate/translate_helper_browsertest.cc (right): https://codereview.chromium.org/2919343007/diff/160001/chrome/renderer/transl... chrome/renderer/translate/translate_helper_browsertest.cc:270: .WillOnce(Return(6)); // TRANSLATION_ERROR = 6 why don't you use translate::TranslationErrors::TRANSLATION_ERROR? Ditto for other returning 0 cases.
> why don't you use translate::TranslationErrors::TRANSLATION_ERROR? > > Ditto for other returning 0 cases. Done. > I confirmed that if error_type_ in TranslateManagerBrowserTest is initialized to > 0 correctly, all tests pass even if we remove the fallback argument. Should I do this change as well?
yeah, could you?
On 2017/06/29 12:39:37, Takashi Toyoshima wrote: > yeah, could you? Done.
On 2017/06/29 13:02:25, Gaja wrote: > On 2017/06/29 12:39:37, Takashi Toyoshima wrote: > > yeah, could you? > > Done. Dear Takashi Toyoshima, I have addressed your last review comment. Could you please take a look at latest patch set? Thanks.
one minor request against newly added code. https://codereview.chromium.org/2919343007/diff/260001/components/translate/c... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/260001/components/translate/c... components/translate/content/renderer/translate_helper.cc:468: base::GetQuotedJSONString(source_lang) + "," + I feel we do not need to be careful about these lang strings. We can trust these lang strings. If you really want this check, you may wrap results in GetOriginalPageLanguage. But it runs inside an isolated v8 world and only Google managed script runs there. So, the check is still optional, but up to you.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Also let me kick tryrun for the final check.
https://codereview.chromium.org/2919343007/diff/260001/components/translate/c... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/260001/components/translate/c... components/translate/content/renderer/translate_helper.cc:468: base::GetQuotedJSONString(source_lang) + "," + On 2017/07/03 12:12:52, Takashi Toyoshima wrote: > I feel we do not need to be careful about these lang strings. We can trust these > lang strings. > If you really want this check, you may wrap results in GetOriginalPageLanguage. > But it runs inside an isolated v8 world and only Google managed script runs > there. So, the check is still optional, but up to you. I am little confused about your comment here. This code was not added by me. This was added by https://chromium-review.googlesource.com/c/544481/. My CL was rebased to latest codebase. Please let me know if I am supposed to do anything here. (Sorry, if I have mistaken your comment).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM. But failed tests may be related to this CL (translation related browser tests failing on mac). If it fails on CQ again, please fix and take another review. https://codereview.chromium.org/2919343007/diff/260001/components/translate/c... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2919343007/diff/260001/components/translate/c... components/translate/content/renderer/translate_helper.cc:468: base::GetQuotedJSONString(source_lang) + "," + Oh, sorry, I added this comment when I'm reviewing diff lines between ps7 and the last one. OK, please ignore this comment.
On 2017/07/03 13:28:58, Takashi Toyoshima wrote: > LGTM. > > But failed tests may be related to this CL (translation related browser tests > failing on mac). If it fails on CQ again, please fix and take another review. > Oh, I do not have MAC setup at my workstation :(. Sorry about that. I will try CQ once. If it fails again, then I will try to statically analyse the code to find the cause and try dry run. Please also let me know if its okay to place those test cases under #if defined(USE_AURA) and put a TODO there? or am I supposed to mandatorily fix it on mac to get this CL land? Please let me know your opinion.
The CQ bit was checked by gajendra.n@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org Link to the patchset: https://codereview.chromium.org/2919343007/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I went through the logs and noticed that : 1) Test case : TranslateManagerBrowserTest.PageTranslationIdenticalLanguagesError is failing for DCHECK [12987:771:0703/092028.459655:FATAL:translate_infobar_delegate.cc(78)] Check failed: TranslateDownloadManager::IsSupportedLanguage(target_language) and 2) Test cases : TranslateManagerBrowserTest.PageTranslationScriptLoadError TranslateManagerBrowserTest.PageTranslationUnexpectedScriptError TranslateManagerBrowserTest.PageTranslationTimeoutError TranslateManagerBrowserTest.PageTranslationBadOriginError [71944:771:0703/092026.812753:FATAL:translate_infobar_delegate.cc(244)] Check failed: false. ---------------------------- I will try to fix and run tryjobs. If it passes, I will request for review.
The CQ bit was checked by gajendra.n@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dear Takashi Toyoshima, I have uploaded changes to fix failures on mac. Please review the latest patch (ps15). Thanks.
With PS15, CQ dry run passed all tryjobs including mac.
https://codereview.chromium.org/2919343007/diff/280001/components/translate/c... File components/translate/core/browser/translate_infobar_delegate.cc (right): https://codereview.chromium.org/2919343007/diff/280001/components/translate/c... components/translate/core/browser/translate_infobar_delegate.cc:228: IDS_TRANSLATE_INFOBAR_ERROR_CANT_CONNECT); coding style nits: Basically the "case" is placed in the order of enum declaration though TRANSLATE_ERROR was placed together with INITIALIZATION_ERROR to return the same value. So, could you re-oreder SCRIPT_LOAD_ERROR through TRANSLATION_ERROR below to follow the enum declaration order? It should be INITIALIZATION_ERROR TRANSLATION_ERROR TRANSLATION_TIMEOUT UNEXPECTED_SCRIPT_ERROR BAD_ORIGIN SCRIPT_LOAD_ERROR
Please take a look. https://codereview.chromium.org/2919343007/diff/280001/components/translate/c... File components/translate/core/browser/translate_infobar_delegate.cc (right): https://codereview.chromium.org/2919343007/diff/280001/components/translate/c... components/translate/core/browser/translate_infobar_delegate.cc:228: IDS_TRANSLATE_INFOBAR_ERROR_CANT_CONNECT); On 2017/07/05 05:37:17, Takashi Toyoshima wrote: > coding style nits: > Basically the "case" is placed in the order of enum declaration though > TRANSLATE_ERROR was placed together with INITIALIZATION_ERROR to return the same > value. > > So, could you re-oreder SCRIPT_LOAD_ERROR through TRANSLATION_ERROR below to > follow the enum declaration order? > > It should be > > INITIALIZATION_ERROR > TRANSLATION_ERROR > TRANSLATION_TIMEOUT > UNEXPECTED_SCRIPT_ERROR > BAD_ORIGIN > SCRIPT_LOAD_ERROR Done.
thank you for continuous efforts to make this finished. LGTM again!
On 2017/07/05 06:52:35, Takashi Toyoshima wrote: > thank you for continuous efforts to make this finished. > LGTM again! Thanks for your guidance throughout. CQing the CL.
The CQ bit was checked by gajendra.n@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org Link to the patchset: https://codereview.chromium.org/2919343007/#ps300001 (title: "Addressed review comment. Proper order of enum variables in switch case.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by toyoshim@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1499242032906240,
"parent_rev": "652fc568ad930f51c62da7f06ba187ea3e37dd00", "commit_rev":
"c3ae0e51f6657abc8d7fa5f1490c5d80f7e66e52"}
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1499242032906240,
"parent_rev": "d20c161048a7c3db249389de7ebd0bd13dde66d4", "commit_rev":
"5bcb90a3e2cf481e8fcfca0b5573effab5479a49"}
Message was sent while issue was closed.
Description was changed from ========== Check |errorCode| of translate.js and notify to Browser CheckTranslateStatus() always returns TranslateErrors::TRANSLATION_ERROR when there is an error during translation. Instead, get the error code from translate.js and convert to TranslateErrors::Type to notify actual error type. BUG=301568 ========== to ========== Check |errorCode| of translate.js and notify to Browser CheckTranslateStatus() always returns TranslateErrors::TRANSLATION_ERROR when there is an error during translation. Instead, get the error code from translate.js and convert to TranslateErrors::Type to notify actual error type. BUG=301568 Review-Url: https://codereview.chromium.org/2919343007 Cr-Commit-Position: refs/heads/master@{#484223} Committed: https://chromium.googlesource.com/chromium/src/+/5bcb90a3e2cf481e8fcfca0b5573... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/5bcb90a3e2cf481e8fcfca0b5573... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
