Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2900603003:
Successful translation erroneously reported on page with too little text (Closed)
DescriptionSuccessful 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 #
Messages
Total messages: 28 (9 generated)
Description was changed from ========== 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 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
gajendra.n@samsung.com changed reviewers: + msw@chromium.org
gajendra.n@samsung.com changed reviewers: + msw@chromium.org
Please take a look.
groby@chromium.org changed reviewers: + napper@chromium.org
The fix seems good, but adding napper@, who's looking at all new translate work. Please do add a unit test.
This seems fine, but I don't see the option to translate pages in my web content context (right-click) menu. How do I get that? I'd like to repro before stamping this.
lgtm
On 2017/05/22 14:41:08, groby wrote: > The fix seems good, but adding napper@, who's looking at all new translate work. > > Please do add a unit test. Sure. I will add a unit test in next Patch Set.
On 2017/05/22 19:42:53, msw wrote: > This seems fine, but I don't see the option to translate pages in my web content > context (right-click) menu. How do I get that? I'd like to repro before stamping > this. Please correct me if I am wrong, AFAIK, (1) In about:blank case, since the translation fails every time, the context menu option should always be enabled and bubble is expected to show error message on every click, which is not happening. (2) In other sites where translation becomes successful (say https://zh.wikipedia.org), the context menu option gets disabled after translation. If we revert translation using "Show original" button in the bubble, then the context menu option is enabled back. Case (2) is already working fine. With Patch Set 2, case (1) will be fixed. Please let me know your opinion.
On 2017/05/22 19:42:53, msw wrote: > This seems fine, but I don't see the option to translate pages in my web content > context (right-click) menu. How do I get that? I'd like to repro before stamping > this. (Sorry for manually adding newline chars between phrases in previous comment, please ignore it) @msw Please correct me if I am wrong, AFAIK, (1) In about:blank case, since the translation fails every time, the context menu option should always be enabled and bubble is expected to show error message on every click, which is not happening. (2) In other sites where translation becomes successful (say https://zh.wikipedia.org), the context menu option gets disabled after translation. If we revert translation using "Show original" button in the bubble, then the context menu option is enabled back. Case (2) is already working fine. With Patch Set 1, case (1) will be fixed. Please let me know your opinion.
On 2017/05/23 04:54:50, Gaja wrote: > On 2017/05/22 19:42:53, msw wrote: > > This seems fine, but I don't see the option to translate pages in my web > content > > context (right-click) menu. How do I get that? I'd like to repro before > stamping > > this. > > (Sorry for manually adding newline chars between phrases in previous comment, > please ignore it) > > @msw > Please correct me if I am wrong, AFAIK, > > (1) In about:blank case, since the translation fails every time, the context > menu option should always be enabled and bubble is expected to show error > message on every click, which is not happening. > (2) In other sites where translation becomes successful (say > https://zh.wikipedia.org), the context menu option gets disabled after > translation. If we revert translation using "Show original" button in the > bubble, then the context menu option is enabled back. > > Case (2) is already working fine. With Patch Set 1, case (1) will be fixed. > Please let me know your opinion. lgtm. I just didn't see any translation option in my context menus on about:version. I tried about:blank, and that does repro as described.
@msw, Thanks. @groby I was checking the files where can I add a unit test for this CL and came across file chrome/browser/translate/translate_manager_render_view_host_unittest.cc that checks state after translation like below (as required for this patch) : EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_ERROR, bubble->GetViewState()); But, in the file almost all the test cases are placed under flag #if !defined(USE_AURA) which does not compile for Linux. Seems the test cases written in the file are for TranslateInfoBarDelegate (non Windows/Linux). Can you please help me know where can I add a test case for this patch? Thanks in advance.
On 2017/05/23 07:15:44, Gaja wrote: > @msw, Thanks. > > @groby > I was checking the files where can I add a unit test for this CL and came across > file chrome/browser/translate/translate_manager_render_view_host_unittest.cc > that checks state after translation like below (as required for this patch) : > > EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_ERROR, bubble->GetViewState()); > > But, in the file almost all the test cases are placed under flag #if > !defined(USE_AURA) which does not compile for Linux. > Seems the test cases written in the file are for TranslateInfoBarDelegate (non > Windows/Linux). > > Can you please help me know where can I add a test case for this patch? Thanks > in advance. There's both translate_manager_unittest.cc, translate_manager_browsertest.cc. For a unittest, just set up a translate manager with mock ranker and mock translate client (both provided in the test fixture), set the object in the desired state, call PageTranslated, and observe the state changed as desired. If you'd rather test the full machinery, including how state is preserved across multiple interactions, you'll want a browsertest (which provides almost a full Chrome instance). The PageLanguageDetection test shows you how to navigate to two separate pages. You'll need to figure out a way how to wait for the bubble changing state, and how to simulate clickjng on the icon. It's possible that translate_browsertest.cc contains more of the helpers needed to run your test.
On 2017/05/25 21:48:10, groby wrote: > On 2017/05/23 07:15:44, Gaja wrote: > > @msw, Thanks. > > > > @groby > > I was checking the files where can I add a unit test for this CL and came > across > > file chrome/browser/translate/translate_manager_render_view_host_unittest.cc > > that checks state after translation like below (as required for this patch) : > > > > EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_ERROR, bubble->GetViewState()); > > > > But, in the file almost all the test cases are placed under flag #if > > !defined(USE_AURA) which does not compile for Linux. > > Seems the test cases written in the file are for TranslateInfoBarDelegate (non > > Windows/Linux). > > > > Can you please help me know where can I add a test case for this patch? Thanks > > in advance. > > There's both translate_manager_unittest.cc, translate_manager_browsertest.cc. > > For a unittest, just set up a translate manager with mock ranker and mock > translate client (both provided in the test fixture), set the object in the > desired state, call PageTranslated, and observe the state changed as desired. > > If you'd rather test the full machinery, including how state is preserved across > multiple interactions, you'll want a browsertest (which provides almost a full > Chrome instance). The PageLanguageDetection test shows you how to navigate to > two separate pages. You'll need to figure out a way how to wait for the bubble > changing state, and how to simulate clickjng on the icon. It's possible that > translate_browsertest.cc contains more of the helpers needed to run your test. Thanks for the detailed guide. I have added interactive ui test for translation bubble view. The test file contains 2 test cases : 1) TranslateSuccess, 2) TranslateError TranslateSuccess is passing, but however TranslateError case is failing. When run in Browser, translation fails for 'about:blank' url (as expected), but while running test, the translation becomes success. Not sure why this behaviour, I debugged a lot but couldn't figure out the root cause. Would you please have a look into the test file and let me know if there is any problem in TranslateError test case. Thanks in advance.
It's possible that the missing key fails the second test (see inline comment) If that's not it, it's probably best to instrument the flow with lots of LOG(), and see where they differ. https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. Why did you name this interactive_uitest instead of browser_test? Intentional, or based on the autofill test? If the latter, please don't - there are very specific reasons why something should be an interactive_ui_test, and we're trying to avoid them. (See https://www.chromium.org/developers/testing/browser-tests for more details) https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:47: void SimulateURLFetch(bool success) { It's probably worth collecting those test functions in a separate file - I assume this is duplicated code from autofill_interactive_uitest? (Sorry, can't verify a lot of things because code search is broken right now:( ) https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:97: translate::TranslateManager::SetIgnoreMissingKeyForTesting(true); You probably want to do that in the SetUp code - it's likely you need this for all test cases. (And this might possibly cause the failure) https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:107: // Set up an observer to be able to wait for the bubble to be shown. Since this is shared across tests, might belong on the fixture. https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:131: content::WindowedNotificationObserver translation_observer( This is probably sharable too https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:163: content::ContextMenuParams params; You might want to use translate::test_utils::PressTranslate(browser());
@groby, Thanks for your comments and patience. I will address your comments in new patch set. https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/31 19:51:44, groby wrote: > Why did you name this interactive_uitest instead of browser_test? Intentional, > or based on the autofill test? > > If the latter, please don't - there are very specific reasons why something > should be an interactive_ui_test, and we're trying to avoid them. (See > https://www.chromium.org/developers/testing/browser-tests for more details) I noticed that few bubble_view/icon_view related tests, especially with respect to URL bar, were interactive_uitests. So decided to name it the same. Also influenced by autofill test. If interactive_ui_tests are being avoided or not preferred, then I will switch to browser_test. https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:47: void SimulateURLFetch(bool success) { On 2017/05/31 19:51:44, groby wrote: > It's probably worth collecting those test functions in a separate file - I > assume this is duplicated code from autofill_interactive_uitest? > > (Sorry, can't verify a lot of things because code search is broken right now:( ) OK, I will try moving it to separate file. Yes, most of the code is duplicated from autofill_interactive_uitest, which I thought that test was suitable for checking translate bubble states. https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:97: translate::TranslateManager::SetIgnoreMissingKeyForTesting(true); On 2017/05/31 19:51:44, groby wrote: > You probably want to do that in the SetUp code - it's likely you need this for > all test cases. (And this might possibly cause the failure) Acknowledged. https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:107: // Set up an observer to be able to wait for the bubble to be shown. On 2017/05/31 19:51:44, groby wrote: > Since this is shared across tests, might belong on the fixture. Acknowledged. https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:131: content::WindowedNotificationObserver translation_observer( On 2017/05/31 19:51:44, groby wrote: > This is probably sharable too Acknowledged. https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:163: content::ContextMenuParams params; On 2017/05/31 19:51:44, groby wrote: > You might want to use translate::test_utils::PressTranslate(browser()); As I observed, for about:blank case, the translate icon and bubble does not appear in location bar. But the "Translate to English" option will be available in Context menu. As per my understanding, the above util function does press action on "Translate" button in bubble. Since the bubble isn't shown, I was sceptical to use PressTranslate at this point. Please let me know your opinion.
On 2017/06/01 12:24:02, Gaja wrote: > @groby, > > Thanks for your comments and patience. > I will address your comments in new patch set. > > https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... > File > chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc > (right): > > https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:1: > // Copyright (c) 2017 The Chromium Authors. All rights reserved. > On 2017/05/31 19:51:44, groby wrote: > > Why did you name this interactive_uitest instead of browser_test? Intentional, > > or based on the autofill test? > > > > If the latter, please don't - there are very specific reasons why something > > should be an interactive_ui_test, and we're trying to avoid them. (See > > https://www.chromium.org/developers/testing/browser-tests for more details) > > I noticed that few bubble_view/icon_view related tests, especially with respect > to URL bar, were interactive_uitests. So decided to name it the same. Also > influenced by autofill test. If interactive_ui_tests are being avoided or not > preferred, then I will switch to browser_test. > > https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:47: > void SimulateURLFetch(bool success) { > On 2017/05/31 19:51:44, groby wrote: > > It's probably worth collecting those test functions in a separate file - I > > assume this is duplicated code from autofill_interactive_uitest? > > > > (Sorry, can't verify a lot of things because code search is broken right now:( > ) > > OK, I will try moving it to separate file. > Yes, most of the code is duplicated from autofill_interactive_uitest, which I > thought that test was suitable for checking translate bubble states. > > https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:97: > translate::TranslateManager::SetIgnoreMissingKeyForTesting(true); > On 2017/05/31 19:51:44, groby wrote: > > You probably want to do that in the SetUp code - it's likely you need this for > > all test cases. (And this might possibly cause the failure) > > Acknowledged. > > https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:107: > // Set up an observer to be able to wait for the bubble to be shown. > On 2017/05/31 19:51:44, groby wrote: > > Since this is shared across tests, might belong on the fixture. > > Acknowledged. > > https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:131: > content::WindowedNotificationObserver translation_observer( > On 2017/05/31 19:51:44, groby wrote: > > This is probably sharable too > > Acknowledged. > > https://codereview.chromium.org/2900603003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc:163: > content::ContextMenuParams params; > On 2017/05/31 19:51:44, groby wrote: > > You might want to use translate::test_utils::PressTranslate(browser()); > > As I observed, for about:blank case, the translate icon and bubble does not > appear in location bar. But the "Translate to English" option will be available > in Context menu. As per my understanding, the above util function does press > action on "Translate" button in bubble. Since the bubble isn't shown, I was > sceptical to use PressTranslate at this point. Please let me know your opinion. Dear groby, As per your suggestions and comments, I have removed interactive_uitest and added test cases in browser_test. For about:blank url, the translate bubble isn't shown automatically once the Language is determined. So I have managed to test the translation error case using translation_error() getter function which is being added in this patch. Please take a look at new patch set. Thanks.
https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translat... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translat... chrome/browser/translate/translate_manager_browsertest.cc:40: void SimulateURLFetch(bool success) { Please let me know if this function should be moved to a test util.
lgtm https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translat... File chrome/browser/translate/translate_manager_browsertest.cc (right): https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translat... chrome/browser/translate/translate_manager_browsertest.cc:40: void SimulateURLFetch(bool success) { On 2017/06/08 07:41:52, Gaja wrote: > Please let me know if this function should be moved to a test util. Ideally, we can make this a shared test fixture, not just a util. But that can be a follow up CL
On 2017/06/13 00:33:00, groby wrote: > lgtm > > https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translat... > File chrome/browser/translate/translate_manager_browsertest.cc (right): > > https://codereview.chromium.org/2900603003/diff/40001/chrome/browser/translat... > chrome/browser/translate/translate_manager_browsertest.cc:40: void > SimulateURLFetch(bool success) { > On 2017/06/08 07:41:52, Gaja wrote: > > Please let me know if this function should be moved to a test util. > > Ideally, we can make this a shared test fixture, not just a util. But that can > be a follow up CL Sure. Thank You.
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, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2900603003/#ps60001 (title: "Fixed a nit")
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": 60001, "attempt_start_ts": 1497321185467210,
"parent_rev": "239d4798dfeee56d5b6feb44060a086c88e00557", "commit_rev":
"f5ec8bec81e8f54e11e0367836294357dd99c915"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/f5ec8bec81e8f54e11e036783629... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f5ec8bec81e8f54e11e036783629... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
