Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(68)

Issue 2900603003: Successful translation erroneously reported on page with too little text (Closed)

Created:
3 years, 7 months ago by Gaja
Modified:
3 years, 6 months ago
Reviewers:
napper, groby-ooo-7-16, msw
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Successful translation erroneously reported on page with too little text Before showing the translation bubble, check if there was an error during previous translation. The previous error status will be set in LanguageState as bool value and is checked when browser command Translate is executed. This sets TRANSLATE_STEP_TRANSLATE_ERROR as next step to show appropriate error message in the popup. TEST= (1) Load a page with little-to-no text (e.g. about:blank). (2) Translate the page (through the right-click menu). (3) Observe the error message bubble. (4) Click the translate icon at the right of the omnibar. (5) Error message bubble seen in (3) should be seen again. BUG=721596 R=groby@chromium.org Review-Url: https://codereview.chromium.org/2900603003 Cr-Commit-Position: refs/heads/master@{#478895} Committed: https://chromium.googlesource.com/chromium/src/+/f5ec8bec81e8f54e11e0367836294357dd99c915

Patch Set 1 #

Patch Set 2 : Added interactive ui test for translate bubble view. (one of the test case is still failing) #

Total comments: 12

Patch Set 3 : Removed interactive_uitest and added test cases in translate_manager_browsertest #

Total comments: 2

Patch Set 4 : Fixed a nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+150 lines, -1 line) Patch
M chrome/browser/translate/translate_manager_browsertest.cc View 1 2 3 4 chunks +135 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser_commands.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M components/translate/core/browser/language_state.h View 2 chunks +7 lines, -0 lines 0 comments Download
M components/translate/core/browser/language_state.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M components/translate/core/browser/translate_manager.cc View 1 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 28 (9 generated)
Gaja
Please take a look.
3 years, 7 months ago (2017-05-22 10:32:45 UTC) #5
groby-ooo-7-16
The fix seems good, but adding napper@, who's looking at all new translate work. Please ...
3 years, 7 months ago (2017-05-22 14:41:08 UTC) #7
msw
This seems fine, but I don't see the option to translate pages in my web ...
3 years, 7 months ago (2017-05-22 19:42:53 UTC) #8
napper
lgtm
3 years, 7 months ago (2017-05-23 02:57:53 UTC) #9
Gaja
On 2017/05/22 14:41:08, groby wrote: > The fix seems good, but adding napper@, who's looking ...
3 years, 7 months ago (2017-05-23 03:14:18 UTC) #10
Gaja
On 2017/05/22 19:42:53, msw wrote: > This seems fine, but I don't see the option ...
3 years, 7 months ago (2017-05-23 03:20:18 UTC) #11
Gaja
On 2017/05/22 19:42:53, msw wrote: > This seems fine, but I don't see the option ...
3 years, 7 months ago (2017-05-23 04:54:50 UTC) #12
msw
On 2017/05/23 04:54:50, Gaja wrote: > On 2017/05/22 19:42:53, msw wrote: > > This seems ...
3 years, 7 months ago (2017-05-23 05:53:02 UTC) #13
Gaja
@msw, Thanks. @groby I was checking the files where can I add a unit test ...
3 years, 7 months ago (2017-05-23 07:15:44 UTC) #14
groby-ooo-7-16
On 2017/05/23 07:15:44, Gaja wrote: > @msw, Thanks. > > @groby > I was checking ...
3 years, 7 months ago (2017-05-25 21:48:10 UTC) #15
Gaja
On 2017/05/25 21:48:10, groby wrote: > On 2017/05/23 07:15:44, Gaja wrote: > > @msw, Thanks. ...
3 years, 6 months ago (2017-05-31 15:28:09 UTC) #16
groby-ooo-7-16
It's possible that the missing key fails the second test (see inline comment) If that's ...
3 years, 6 months ago (2017-05-31 19:51:44 UTC) #17
Gaja
@groby, Thanks for your comments and patience. I will address your comments in new patch ...
3 years, 6 months ago (2017-06-01 12:24:02 UTC) #18
Gaja
On 2017/06/01 12:24:02, Gaja wrote: > @groby, > > Thanks for your comments and patience. ...
3 years, 6 months ago (2017-06-08 07:39:32 UTC) #19
Gaja
https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translate/translate_manager_browsertest.cc File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translate/translate_manager_browsertest.cc#newcode40 chrome/browser/translate/translate_manager_browsertest.cc:40: void SimulateURLFetch(bool success) { Please let me know if ...
3 years, 6 months ago (2017-06-08 07:41:52 UTC) #20
groby-ooo-7-16
lgtm https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translate/translate_manager_browsertest.cc File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translate/translate_manager_browsertest.cc#newcode40 chrome/browser/translate/translate_manager_browsertest.cc:40: void SimulateURLFetch(bool success) { On 2017/06/08 07:41:52, Gaja ...
3 years, 6 months ago (2017-06-13 00:33:00 UTC) #21
Gaja
On 2017/06/13 00:33:00, groby wrote: > lgtm > > https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translate/translate_manager_browsertest.cc > File chrome/browser/translate/translate_manager_browsertest.cc (right): > ...
3 years, 6 months ago (2017-06-13 02:32:52 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2900603003/60001
3 years, 6 months ago (2017-06-13 02:33:30 UTC) #25
commit-bot: I haz the power
3 years, 6 months ago (2017-06-13 03:57:13 UTC) #28
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f5ec8bec81e8f54e11e036783629...

Powered by Google App Engine
This is Rietveld 408576698