Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1923143003:
Implement the 2016Q2 Translate UI designe spec out in (Closed)
DescriptionImplement the 2016Q2 Translate UI designe spec out in
https://goto.google.com/chrometranslateui2016q3
1. removed ‘Nope’ and replaced it with ‘x’ (close button)
2. show detected and page languages on the initial bubble- Makes it clearer that this is a translation feature, even for users who can’t perfectly read their UI language
3. Clicking on language names takes user to the language edit view (already exists)
4. increased the visibility of Always Translate
5. Instead of Nope, relabeled the button Options and made it a simple drop down
6. If the same language is translated 2 times, next time, the ‘always translate’ option is checked by default.
7. After 10 consecutive clicks on ‘x’ or ‘outside the box’ for the same language across sites: Stop showing the bubble, but show the omnibar icon
8. After 3 consecutive clicks on ‘x’ for the same language across sites: Stop showing the bubble, but show the omnibar icon
9. Translate events reset all counters
10. After 2 consecutive clicks on Translate for the same language: Show bubble with ‘Always do this’ checked But if users manually uncheck the checkbox, we will never automatically check it
BUG=607170
Committed: https://crrev.com/7223e2c7d071d74bcd66c5a113e97fe21a82bb68
Cr-Commit-Position: refs/heads/master@{#391540}
Patch Set 1 #Patch Set 2 : fix test breakage #
Total comments: 37
Patch Set 3 : add unit test for TranslateBubbleView and fix test breakage #Patch Set 4 : address review comment. Add unit tests. fix test breakages #Patch Set 5 : fix test breakages in TranslateUIDelegateTest #Patch Set 6 : 2nd try - fix test breakages in TranslateUIDelegateTest #
Total comments: 1
Patch Set 7 : Move close button tracking code to OnWidgetClosing() #
Total comments: 8
Patch Set 8 : fix namespace in .h and other minor review comments #
Total comments: 165
Patch Set 9 : change according to msw review comments #
Total comments: 40
Patch Set 10 : 2nd change according to msw review comments #
Total comments: 7
Patch Set 11 : change to use MenuButton with MenuRunner for 'Options' menu in new UI based on msw review request. #
Total comments: 6
Patch Set 12 : add 200 icon png. remove unnecessary include in .cc file, remove unnecessary reset and remove expli… #
Total comments: 57
Patch Set 13 : change based on msw comments. #
Total comments: 5
Patch Set 14 : minor changes in comments #Messages
Total messages: 108 (38 generated)
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/1
ftang@chromium.org changed reviewers: + sky@chromium.org, toyoshim@chromium.org
Try to get in the code before the freeze. This implement the new design of TranslateUI behind a flag. See https://goto.google.com/chrometranslateui2016q3 for the UI specification.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
fix test breakage
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/10017 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/10017
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
add unit test for TranslateBubbleView and fix test breakage
groby@chromium.org changed reviewers: + groby@chromium.org
Thank you for tackling this! If you can, please give it a slightly clearer title, and add a quick outline of behavior changes in the description. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:207: if (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2)) { I'd prefer it if we could resolve the TODO, instead of adding _more_ logic into this code. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:54: enum UIActionType { Please use TranslateBubbleUiEvent and expand that. Translate already has a lot of histograms, and each new one comes at a cost. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:208: I don't understand why you chose to encapsulate this one, but use |use_blue_button_|, |show_always_checkbox_|, and |show_icon_| as bare members. Also, since they never change value, can we get rid of them? https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:216: model_->DeclineTranslation(); This is invoked when the widget is closed for _any_ reason, IIRC. That does include dismissal by focus loss, so probably this is a mistake. Can we extract all this logic into a controller class that's actually testable for proper behavior? https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:353: UMA_HISTOGRAM_ENUMERATION(kTranslateUIAction, I'd suggest factoring this out into a ReportUiAction() function. (Ideally, on the controller mentioned above :) https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:448: model_->DeclineTranslation(); Why is this common code factored out into every single branch? https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:535: gfx::Range(static_cast<uint32_t>(offsets[0]), Can we amend gfx::Range to take a size_t? Because this is ugly :) https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:627: layout->AddView(styled_label); styled_label leaks if we're not using the new UI. Why not create it on demand only? https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:807: if (show_icon_) { Given that this is common code, I'm inclined to suggest factoring out. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:31: always_translate_checked_(false), Any tests for the new features? https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_prefs.cc:191: ResetTranslationIgnoredCount(language); This always makes me queasy, because it spams a _lot_ of update notifications that could be batched. https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_prefs.cc:391: if (base::FeatureList::IsEnabled(kTranslateUI2016Q2)) { This technically does not belong on prefs. It's behavioral logic. We should separate this from the prefs, because alternate behaviors can then implemented via separate implementation classes. https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_prefs.cc:393: // 3 times or user ignore more than 10 times. nit: ignored. https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_prefs.h:133: // translation bubble for a specific language. I'm really wondering if this belongs into components/ - but that is probably a question beyond this CL. (I.e. is this the universal behavior for translate on all platforms, or does it depend on the embedder) https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:229: if (explicitly_closed) { Can we get unit tests? https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:257: nit:Please remove https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:262: if (translate_manager_) { Why are we not resetting TranslateEnabled for the new UI?
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/30001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/30001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
address review comment. Add unit tests. fix test breakage
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/50001
groby- PTAL https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:207: if (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2)) { On 2016/04/27 19:08:24, groby wrote: > I'd prefer it if we could resolve the TODO, instead of adding _more_ logic into > this code. Not quit sure what you ask me to do. I try not to change the logic while the flag is not on to preserve the old behavior. Do you want me to remove that if/return for both cases? https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:54: enum UIActionType { On 2016/04/27 19:08:24, groby wrote: > Please use TranslateBubbleUiEvent and expand that. Translate already has a lot > of histograms, and each new one comes at a cost. ok. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:208: On 2016/04/27 19:08:24, groby wrote: > I don't understand why you chose to encapsulate this one, but use > |use_blue_button_|, |show_always_checkbox_|, and |show_icon_| as bare members. > > Also, since they never change value, can we get rid of them? Done. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:216: model_->DeclineTranslation(); On 2016/04/27 19:08:25, groby wrote: > This is invoked when the widget is closed for _any_ reason, IIRC. That does > include dismissal by focus loss, so probably this is a mistake. > > Can we extract all this logic into a controller class that's actually testable > for proper behavior? Notice this is wrap inside "if (GetBubbleFrameView()->close_button_clicked())" which only happen when it is closed due to the close button clicked. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:353: UMA_HISTOGRAM_ENUMERATION(kTranslateUIAction, On 2016/04/27 19:08:25, groby wrote: > I'd suggest factoring this out into a ReportUiAction() function. (Ideally, on > the controller mentioned above :) Done. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:448: model_->DeclineTranslation(); On 2016/04/27 19:08:24, groby wrote: > Why is this common code factored out into every single branch? Done. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:535: gfx::Range(static_cast<uint32_t>(offsets[0]), On 2016/04/27 19:08:24, groby wrote: > Can we amend gfx::Range to take a size_t? Because this is ugly :) I agree this is ugly but I think that is out of the scope of this cl. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:627: layout->AddView(styled_label); On 2016/04/27 19:08:24, groby wrote: > styled_label leaks if we're not using the new UI. Why not create it on demand > only? Done. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:807: if (show_icon_) { On 2016/04/27 19:08:25, groby wrote: > Given that this is common code, I'm inclined to suggest factoring out. Done. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:31: always_translate_checked_(false), On 2016/04/27 19:08:25, groby wrote: > Any tests for the new features? Done. https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_prefs.cc:191: ResetTranslationIgnoredCount(language); On 2016/04/27 19:08:25, groby wrote: > This always makes me queasy, because it spams a _lot_ of update notifications > that could be batched. Acknowledged. https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_prefs.cc:391: if (base::FeatureList::IsEnabled(kTranslateUI2016Q2)) { On 2016/04/27 19:08:25, groby wrote: > This technically does not belong on prefs. It's behavioral logic. We should > separate this from the prefs, because alternate behaviors can then implemented > via separate implementation classes. I agree. could I do it later in a different cl? https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_prefs.cc:393: // 3 times or user ignore more than 10 times. On 2016/04/27 19:08:25, groby wrote: > nit: ignored. Done. https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_prefs.h:133: // translation bubble for a specific language. On 2016/04/27 19:08:25, groby wrote: > I'm really wondering if this belongs into components/ - but that is probably a > question beyond this CL. (I.e. is this the universal behavior for translate on > all platforms, or does it depend on the embedder) Acknowledged. https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:257: On 2016/04/27 19:08:25, groby wrote: > nit:Please remove Done. https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:262: if (translate_manager_) { On 2016/04/27 19:08:25, groby wrote: > Why are we not resetting TranslateEnabled for the new UI? In the new ui, the omnibar icon is always shown, regardless the site/language is set to never or not. This give the user a chance to "change their mind" in the new ui. Without that, there are no easy way for user to figure out how to "undo" what they set. see slide 20 "but show the omnibar icon"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
fix test breakage in TranslateUIDelegateTest
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/70001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
2nd try - fix test breakage in TranslateUIDelegateTest
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/90001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
looks good, but can you add more information to the change description, or in crbug? groby: how many changes did you submit or review on translate? I guess you already achieved much things to be an owner in this area. https://codereview.chromium.org/1923143003/diff/90001/chrome/browser/ui/views... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:225: bubble_->WindowClosing(); ButtonPressed calls this internally, and causing test failures?
Description was changed from ========== Implement the 2016Q2 Translate UI designe spec out in https://goto.google.com/chrometranslateui2016q3 BUG=607170 ========== to ========== Implement the 2016Q2 Translate UI designe spec out in https://goto.google.com/chrometranslateui2016q3 1. removed ‘Nope’ and replaced it with ‘x’ (close button) 2. show detected and page languages on the initial bubble- Makes it clearer that this is a translation feature, even for users who can’t perfectly read their UI language 3. Clicking on language names takes user to the language edit view (already exists) 4. increased the visibility of Always Translate 5. Instead of Nope, relabeled the button Options and made it a simple drop down 6. If the same language is translated 2 times, next time, the ‘always translate’ option is checked by default. 7. After 10 consecutive clicks on ‘x’ or ‘outside the box’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 8. After 3 consecutive clicks on ‘x’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 9. Translate events reset all counters 10. After 2 consecutive clicks on Translate for the same language: Show bubble with ‘Always do this’ checked But if users manually uncheck the checkbox, we will never automatically check it BUG=607170 ==========
fix test breakage. Move the code to track close button from WindowClosing() to OnWidgetClosing()
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/110001
PTAL- this should fix the test
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM - you'll still need OWNERs, though https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:207: if (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2)) { On 2016/04/27 21:43:15, ftang wrote: > On 2016/04/27 19:08:24, groby wrote: > > I'd prefer it if we could resolve the TODO, instead of adding _more_ logic > into > > this code. > > Not quit sure what you ask me to do. I try not to change the logic while the > flag is not on to preserve the old behavior. Do you want me to remove that > if/return for both cases? I'm not sure what to do with this, either - it's unfortunate that it's tied to a specific UI. I've tried pulling it out, and it's not completely trivial. So I'm fine with leaving it in this CL (because moving it out will probably trigger more changes), but we should have a concrete bug for this milestone to get this resolved. https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/10017/chrome/browser/ui/views... chrome/browser/ui/views/translate/translate_bubble_view.cc:216: model_->DeclineTranslation(); On 2016/04/27 21:43:15, ftang wrote: > On 2016/04/27 19:08:25, groby wrote: > > This is invoked when the widget is closed for _any_ reason, IIRC. That does > > include dismissal by focus loss, so probably this is a mistake. > > > > Can we extract all this logic into a controller class that's actually testable > > for proper behavior? > > Notice this is wrap inside "if (GetBubbleFrameView()->close_button_clicked())" > which only happen when it is closed due to the close button clicked. I suppose so. I'd rather call DeclineTranslation in the handler for the close button, but since that's in the BubbleFrameView itself... I guess I'm OK with it. https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_prefs.cc:391: if (base::FeatureList::IsEnabled(kTranslateUI2016Q2)) { On 2016/04/27 21:43:15, ftang wrote: > On 2016/04/27 19:08:25, groby wrote: > > This technically does not belong on prefs. It's behavioral logic. We should > > separate this from the prefs, because alternate behaviors can then implemented > > via separate implementation classes. > > I agree. could I do it later in a different cl? Later CL is perfectly fine. It's not like this *introduces* the idea of logic into prefs :) https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/10017/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:262: if (translate_manager_) { On 2016/04/27 21:43:16, ftang wrote: > On 2016/04/27 19:08:25, groby wrote: > > Why are we not resetting TranslateEnabled for the new UI? > > In the new ui, the omnibar icon is always shown, regardless the site/language is > set to never or not. This give the user a chance to "change their mind" in the > new ui. Without that, there are no easy way for user to figure out how to "undo" > what they set. see slide 20 "but show the omnibar icon" Ah, I understand - thanks for the explanation. Would you mind leaving a short comment that this is the desired effect? It's not obvious at first glance that SetTranslateEnabled(false) keeps the icon visible :) https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_view_state_transition.h (right): https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:11: namespace { Please don't use unnamed namespaces in .h files (see https://google.github.io/styleguide/cppguide.html#Namespaces) https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:423: // Do not noop on 2016q2 UI. I do not understand what "do not noop" means here - could you explain? https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:506: if (use_2016_q2_ui_ && !is_in_incognito_window_) { I'm curious - does that mean we should get rid of "Never" for incognito as well? https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:178: Please remove second blank line
ftang@chromium.org changed reviewers: + msw@chromium.org, oshim@chromium.org
oshim- need your approval for changes in ui/resources/ sky- need your review for changes in many directories which have no OWNERS file but you are in the parent directory OWNERS file. msw- need your review for the ui/views/bubble/bubble_frame_view.h changes for testing code. toyoshim- please approve since you are in the OWNERS of most of the translate file. https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_view_state_transition.h (right): https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:11: namespace { On 2016/04/29 05:26:14, groby wrote: > Please don't use unnamed namespaces in .h files (see > https://google.github.io/styleguide/cppguide.html#Namespaces) Done. https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:423: // Do not noop on 2016q2 UI. On 2016/04/29 05:26:14, groby wrote: > I do not understand what "do not noop" means here - could you explain? sorry, remove "not" https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:506: if (use_2016_q2_ui_ && !is_in_incognito_window_) { On 2016/04/29 05:26:14, groby wrote: > I'm curious - does that mean we should get rid of "Never" for incognito as well? good question. I will raise that to pendar. It does not make sense to me. But since that will change not only the new UI but existing one. I would like to work on that separately. https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:178: On 2016/04/29 05:26:14, groby wrote: > Please remove second blank line Done.
fix namespace in .h and other minor review comment
The CQ bit was checked by ftang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org Link to the patchset: https://codereview.chromium.org/1923143003/#ps130001 (title: "fix namespace in .h and other minor review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
This is fairly messy, please self-review first if you need something reviewed urgently. https://codereview.chromium.org/1923143003/diff/130001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/app/generated_r... chrome/app/generated_resources.grd:11611: + <message name="IDS_TRANSLATE_BUBBLE_BEFORE_TRANSLATE2" desc="Text to show for the translate bubble label when that page is in specified language and ask if should translate."> nit: How about IDS_TRANSLATE_BUBBLE_BEFORE_TRANSLATE_NEW instead of *2? (or something like IDS_TRANSLATE_BUBBLE_BEFORE_TRANSLATE_LANGUAGES) https://codereview.chromium.org/1923143003/diff/130001/chrome/app/generated_r... chrome/app/generated_resources.grd:11612: + Do you want Google to translate this page from <ph name="source_language">$1<ex>Spanish</ex></ph> to <ph name="target_language">$2<ex>English</ex></ph>? Remove the extra space here: "to <ph" https://codereview.chromium.org/1923143003/diff/130001/chrome/app/generated_r... chrome/app/generated_resources.grd:11612: + Do you want Google to translate this page from <ph name="source_language">$1<ex>Spanish</ex></ph> to <ph name="target_language">$2<ex>English</ex></ph>? Not sure why we changed from "Would you like" to "Do you want"... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:207: if (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2)) { nit: combine with nested if statement using &&... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_model.h (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model.h:40: // Returns the Always Translate Checked. This comment isn't well formed, consider "Returns true if the 'Always Translate' checkbox is checked." (or maybe 'should be checked'???) Maybe clarify how this relates to ShouldAlwaysTranslate()... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model.h:41: virtual bool GetAlwaysTranslateChecked() const = 0; Rename this "IsAlwaysTranslateChecked" or "ShouldAlwaysTranslateBeChecked" or similar and move it down or up, I'm not sure why you put it between GetViewState and SetViewState... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_model_impl.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model_impl.cc:50: bool TranslateBubbleModelImpl::GetAlwaysTranslateChecked() const { Remove extra space after bool; reorder to match decl. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_model_impl.h (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model_impl.h:34: bool GetAlwaysTranslateChecked() const override; nit: reorder override to match reordering in base class. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_view_state_transition.h (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:11: namespace translate { nit: add a blank line between the namespace and the enum (ditto on close) https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:13: // Bubble enters the options state. Give a more descriptive comment, maybe "The bubble started showing advanced options."; ditto below. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:14: SET_STATE_OPTIONS = 1, Why do you give explicit values to the enums? Remove those. (if needed for consistency with old values, you can just leave the '= 1' for the first entry and remove the rest...) https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:19: // User click the advanced link Add trailing periods on this comment and others below. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:19: // User click the advanced link It'd be nice if you used the same subject and tense in the comments throughout ("Bubble enters" vs. "User click")... I'd suggest "The user clicked", etc. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:46: // User click the "Closed" [X] button nit: s/Closed/Close/ and s/[X]/(x)/ https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:62: TARGET_LANGUAGE_MENU_CLICKED = 17 , nit: remove the space before the comma. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:53: const int kQuestionWidth = 200; nit: move to usage in CreateViewBeforeTranslate. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:57: const base::string16& label, nit: fix indent, ditto below. Use 'git cl format' & self-review to ease review. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:74: base::string16 text, const ref https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:81: views::Link* CreateLink(views::LinkListener* listener, nit: add a blank line above https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:84: return CreateLink(listener, This all fits on one line... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:92: icon->SetImage( This all fits on one line... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:195: void TranslateBubbleView::ReportUiAction(int action) { Order the definition to match the declaration. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:198: translate::TRANSLATE_BUBBLE_UI_EVENT_MAX); nit: fits on line above; 'git cl format' https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:201: bool TranslateBubbleView::ShouldShowCloseButton() const { Order the definition to match the declaration. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:210: void TranslateBubbleView::OnWidgetClosing(views::Widget* widget) { Order the definition to match the declaration. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:387: translate::ALWAYS_TRANSLATE_CHECKED : Is this stat useful? It also counts users checking/unchecking the box before they actually submit the 'form' value... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:423: // Do noop on 2016q2 UI. "Do nothing"... move comment above the conditional and drop the curlies. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:485: items[static_cast<size_t>(DenialComboboxIndex::DONT_TRANSLATE)] = nit: items[static_cast<size_t>(DenialComboboxIndex::DONT_TRANSLATE)] = l10n_util::GetStringUTF16(use_2016_q2_ui_ ? IDS_TRANSLATE_BUBBLE_ADVANCED : IDS_TRANSLATE_BUBBLE_DENY) https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:486: l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_ADVANCED); Using advanced for don't translate is a bit odd... can you add an explanatory comment as to what this is actually trying to do? https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:497: remove https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:500: remove https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:505: // In an incognito window, "Always translate" checkbox shouldn't be shown. nit: , the "Alwa... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:506: if (use_2016_q2_ui_ && !is_in_incognito_window_) { Merge this block into the one below where you actually add the view. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:507: always_translate_checkbox_ = new views::Checkbox(base::string16()); Pass l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_ALWAYS) here and remove the SetText call... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:527: if (use_2016_q2_ui_) { Revert this change; if and else do the same thing... please self-review... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:572: styled_label->SetBounds(styled_label->x(), Use Label::SizeToFit(kQuestionWidth) or at least use SetSize to avoid passing current x/y location. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:579: auto message_label = new views::Label( inline this in the AddView call below, |message_label| isn't otherwise needed. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:582: auto link = CreateLink(this, ditto: inline this in the AddView call below. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:587: remove extra blank line... self-review https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:633: 0, views::GridLayout::USE_PREF, 0, 0); nit: remove views:: here (and elsewhere applicable) https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:646: if (use_2016_q2_ui_) { nit: remove curlies https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:682: 0, views::GridLayout::USE_PREF, 0, 0); ditto: remove views:: here (and elsewhere applicable) https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:698: if (use_2016_q2_ui_) { ditto: remove curlies. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:751: if (use_2016_q2_ui_) { ditto: remove curlies. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:906: use_2016_q2_ui_ ? git cl format https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:107: bool ShouldShowCloseButton() const override; Order with the other WidgetDelegate method, WindowClosing, in decl order. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:110: void StyledLabelLinkClicked(views::StyledLabel* label, Order just after the LinkListener override and before the WebContentsObserver override to match the subclass ordering. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:115: void OnWidgetClosing(views::Widget* widget) override; Order before non-override function GetViewState. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:237: // Whether one of denial buttons is clicked. nit: 'the', 'was': "Whether one of the denial was clicked." https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:240: // Use 2016 Q2 Translate UI nit: trailing period; "Use the new (2016 Q2) translate UI." https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:94: bool ShouldAlwaysTranslate() const override { nit: blank line above https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:166: translate::kTranslateUI2016Q2.name, ""); nit: s/""/std::string()/ https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:220: bubble_->GetBubbleFrameView()->ButtonPressed( nit: maybe use EventGenerator. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:251: // In the 2016Q2 UI, we should not close nor take action. Isn't the 'nope' button simply not available in the new UI? I'm not sure it makes sense to test your workaround to no-op when the non-existent button is pressed... https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:330: // call the StyledLabelLinkClicked(); nit: "Click the styled label link." https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:331: views::StyledLabel styled_label(base::ASCIIToUTF16("test"), nullptr); You don't need to supply a valid styled label here; pass nullptr instead. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:103: nit remove extra blank line https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:346: update.Get()->SetInteger(language, 0); Perhaps you can actually clear the pref value here... but it's not necessary. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:392: // In the new logic, we only hide the Bubble if user denied for more than nit: no caps for 'bubble'; 'if the user denied it more than 3 times or the user ignored it more'... https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:394: return (GetTranslationDeniedCount(language) > 3) || Should this ever propagate to kPrefTranslateTooOftenDeniedForLanguage? https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:404: void TranslatePrefs::ResetDenialState() { Shouldn't this also have some effect on the new UI? https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.h:35: // Feature flag for "Translate UI 2016 Q2" project trailing period. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.h:132: // These methods are used to track how many times the user has ignore the ignored https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs_unittest.cc (left): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:9: #include <vector> this file uses vector, leave this include here. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:169: translate::kTranslateUI2016Q2.name, ""); std::string https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:185: ASSERT_FALSE(translate_prefs_->IsTooOftenDenied(kTestLanguage)); nit: assert is a bit heavy-handed; expect would be okay throughout these tests. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:196: TEST_F(TranslatePrefTest, IsTooOftenIgnoredIn2016Q2UI) { Maybe add some test that intermixes denied and ignored? Is there some special behavior for that? https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:260: if (!base::FeatureList::IsEnabled(kTranslateUI2016Q2)) { nit: combine nested if with && https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:284: if (!base::FeatureList::IsEnabled(kTranslateUI2016Q2)) { nit: combine nested if with && https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:303: // After 2 clicks on Translate for the same language. Why does this comment reference 2, but the check is for 3? I'm not really sure this comment makes much sense... consider rewriting it for clarity. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:304: // it is == 2 not >2 because if the user translate with always on Capitalize 'it', '> 2'... 'translates' https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:306: // bubble will show up is if the user uncheck the Always translate, unchecks https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:307: // but in that case since user explictly uncheck, we should show 'unchecked it', 'show it as unchecked'... https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:122: // Show "Always translate" as checked in the bubble UI trailing period https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:123: bool ShowAlwaysTranslateChecked(); Maybe rename this (and/or make the comment) something more descriptive, like "ShouldAlwaysTranslateBeCheckedByDefault" or similar? https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:86: "settings.language.preferred_languages", ""); std::string() here and below. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:110: translate::kTranslateUI2016Q2.name, ""); std::string() https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:174: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedFalse) { Combine this test with the one above? you're toggling the same value... https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:187: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedTrueIn2016Q2UI) { Maybe use a parameterized gtest here for old/new? TEST_P... https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:201: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedFalseIn2016Q2UI) { Ditto https://codereview.chromium.org/1923143003/diff/130001/ui/resources/ui_resour... File ui/resources/ui_resources.grd (right): https://codereview.chromium.org/1923143003/diff/130001/ui/resources/ui_resour... ui/resources/ui_resources.grd:196: <structure type="chrome_scaled_image" name="IDR_TRANSLATE_ICON_V2" file="common/translate.png" /> There is no "IDR_TRANSLATE_ICON"... so I'm not sure why you'd postfix '_V2'. It seems like the current icon is vectorized, it'd be nice if this one was similarly vectorized, but for now, maybe just drop the _V2? https://codereview.chromium.org/1923143003/diff/130001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1923143003/diff/130001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:101: FRIEND_TEST_ALL_PREFIXES(::TranslateBubbleViewTest, CloseButtonIn2016Q2UI); Can you avoid this and instead fire an [Escape] key or similar in the test?
Also, did you mean oshima? (not oshim)
change according to msw review comments
ftang@chromium.org changed reviewers: + oshima@chromium.org - oshim@chromium.org
msw- PTAL https://codereview.chromium.org/1923143003/diff/130001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/app/generated_r... chrome/app/generated_resources.grd:11611: + <message name="IDS_TRANSLATE_BUBBLE_BEFORE_TRANSLATE2" desc="Text to show for the translate bubble label when that page is in specified language and ask if should translate."> On 2016/04/29 19:59:59, msw wrote: > nit: How about IDS_TRANSLATE_BUBBLE_BEFORE_TRANSLATE_NEW instead of *2? > (or something like IDS_TRANSLATE_BUBBLE_BEFORE_TRANSLATE_LANGUAGES) Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/app/generated_r... chrome/app/generated_resources.grd:11612: + Do you want Google to translate this page from <ph name="source_language">$1<ex>Spanish</ex></ph> to <ph name="target_language">$2<ex>English</ex></ph>? On 2016/04/29 19:59:59, msw wrote: > Not sure why we changed from "Would you like" to "Do you want"... It is designed by our UI designer spec out in page 11 of https://docs.google.com/presentation/d/1QECg9ZPANcOISaiS3tEDgOj4yJNSFnPhq2VWE... https://codereview.chromium.org/1923143003/diff/130001/chrome/app/generated_r... chrome/app/generated_resources.grd:11612: + Do you want Google to translate this page from <ph name="source_language">$1<ex>Spanish</ex></ph> to <ph name="target_language">$2<ex>English</ex></ph>? On 2016/04/29 19:59:59, msw wrote: > Remove the extra space here: "to <ph" Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:207: if (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2)) { On 2016/04/29 19:59:59, msw wrote: > nit: combine with nested if statement using &&... Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_model.h (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model.h:40: // Returns the Always Translate Checked. On 2016/04/29 19:59:59, msw wrote: > This comment isn't well formed, consider "Returns true if the 'Always Translate' > checkbox is checked." (or maybe 'should be checked'???) Maybe clarify how this > relates to ShouldAlwaysTranslate()... Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model.h:41: virtual bool GetAlwaysTranslateChecked() const = 0; On 2016/04/29 19:59:59, msw wrote: > Rename this "IsAlwaysTranslateChecked" or "ShouldAlwaysTranslateBeChecked" or > similar and move it down or up, I'm not sure why you put it between GetViewState > and SetViewState... Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_model_impl.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model_impl.cc:50: bool TranslateBubbleModelImpl::GetAlwaysTranslateChecked() const { On 2016/04/29 20:00:00, msw wrote: > Remove extra space after bool; reorder to match decl. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_model_impl.h (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model_impl.h:34: bool GetAlwaysTranslateChecked() const override; On 2016/04/29 20:00:00, msw wrote: > nit: reorder override to match reordering in base class. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_view_state_transition.h (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:11: namespace translate { On 2016/04/29 20:00:00, msw wrote: > nit: add a blank line between the namespace and the enum (ditto on close) Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:13: // Bubble enters the options state. On 2016/04/29 20:00:00, msw wrote: > Give a more descriptive comment, maybe "The bubble started showing advanced > options."; ditto below. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:19: // User click the advanced link On 2016/04/29 20:00:00, msw wrote: > Add trailing periods on this comment and others below. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:19: // User click the advanced link On 2016/04/29 20:00:00, msw wrote: > It'd be nice if you used the same subject and tense in the comments throughout > ("Bubble enters" vs. "User click")... I'd suggest "The user clicked", etc. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:53: const int kQuestionWidth = 200; On 2016/04/29 20:00:00, msw wrote: > nit: move to usage in CreateViewBeforeTranslate. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:57: const base::string16& label, On 2016/04/29 20:00:01, msw wrote: > nit: fix indent, ditto below. Use 'git cl format' & self-review to ease review. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:74: base::string16 text, On 2016/04/29 20:00:01, msw wrote: > const ref Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:81: views::Link* CreateLink(views::LinkListener* listener, On 2016/04/29 20:00:00, msw wrote: > nit: add a blank line above Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:84: return CreateLink(listener, On 2016/04/29 20:00:01, msw wrote: > This all fits on one line... Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:92: icon->SetImage( On 2016/04/29 20:00:01, msw wrote: > This all fits on one line... Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:195: void TranslateBubbleView::ReportUiAction(int action) { On 2016/04/29 20:00:01, msw wrote: > Order the definition to match the declaration. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:198: translate::TRANSLATE_BUBBLE_UI_EVENT_MAX); On 2016/04/29 20:00:02, msw wrote: > nit: fits on line above; 'git cl format' Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:201: bool TranslateBubbleView::ShouldShowCloseButton() const { On 2016/04/29 20:00:01, msw wrote: > Order the definition to match the declaration. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:210: void TranslateBubbleView::OnWidgetClosing(views::Widget* widget) { On 2016/04/29 20:00:01, msw wrote: > Order the definition to match the declaration. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:387: translate::ALWAYS_TRANSLATE_CHECKED : On 2016/04/29 20:00:00, msw wrote: > Is this stat useful? It also counts users checking/unchecking the box before > they actually submit the 'form' value... that is exactly what the UI designer ask me to track. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:423: // Do noop on 2016q2 UI. On 2016/04/29 20:00:01, msw wrote: > "Do nothing"... move comment above the conditional and drop the curlies. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:485: items[static_cast<size_t>(DenialComboboxIndex::DONT_TRANSLATE)] = On 2016/04/29 20:00:02, msw wrote: > nit: items[static_cast<size_t>(DenialComboboxIndex::DONT_TRANSLATE)] = > l10n_util::GetStringUTF16(use_2016_q2_ui_ ? IDS_TRANSLATE_BUBBLE_ADVANCED : > IDS_TRANSLATE_BUBBLE_DENY) Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:486: l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_ADVANCED); On 2016/04/29 20:00:01, msw wrote: > Using advanced for don't translate is a bit odd... can you add an explanatory > comment as to what this is actually trying to do? Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:497: On 2016/04/29 20:00:00, msw wrote: > remove Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:500: On 2016/04/29 20:00:01, msw wrote: > remove Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:505: // In an incognito window, "Always translate" checkbox shouldn't be shown. On 2016/04/29 20:00:01, msw wrote: > nit: , the "Alwa... Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:507: always_translate_checkbox_ = new views::Checkbox(base::string16()); On 2016/04/29 20:00:01, msw wrote: > Pass l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_ALWAYS) here and remove the > SetText call... Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:527: if (use_2016_q2_ui_) { On 2016/04/29 20:00:00, msw wrote: > Revert this change; if and else do the same thing... please self-review... Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:572: styled_label->SetBounds(styled_label->x(), On 2016/04/29 20:00:00, msw wrote: > Use Label::SizeToFit(kQuestionWidth) or at least use SetSize to avoid passing > current x/y location. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:579: auto message_label = new views::Label( On 2016/04/29 20:00:01, msw wrote: > inline this in the AddView call below, |message_label| isn't otherwise needed. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:582: auto link = CreateLink(this, On 2016/04/29 20:00:00, msw wrote: > ditto: inline this in the AddView call below. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:646: if (use_2016_q2_ui_) { On 2016/04/29 20:00:01, msw wrote: > nit: remove curlies Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:682: 0, views::GridLayout::USE_PREF, 0, 0); On 2016/04/29 20:00:00, msw wrote: > ditto: remove views:: here (and elsewhere applicable) Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:698: if (use_2016_q2_ui_) { On 2016/04/29 20:00:01, msw wrote: > ditto: remove curlies. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:751: if (use_2016_q2_ui_) { On 2016/04/29 20:00:01, msw wrote: > ditto: remove curlies. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:107: bool ShouldShowCloseButton() const override; On 2016/04/29 20:00:02, msw wrote: > Order with the other WidgetDelegate method, WindowClosing, in decl order. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:110: void StyledLabelLinkClicked(views::StyledLabel* label, On 2016/04/29 20:00:02, msw wrote: > Order just after the LinkListener override and before the WebContentsObserver > override to match the subclass ordering. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:115: void OnWidgetClosing(views::Widget* widget) override; On 2016/04/29 20:00:02, msw wrote: > Order before non-override function GetViewState. Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:237: // Whether one of denial buttons is clicked. On 2016/04/29 20:00:02, msw wrote: > nit: 'the', 'was': "Whether one of the denial was clicked." this is merge mistake. removed https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:240: // Use 2016 Q2 Translate UI On 2016/04/29 20:00:02, msw wrote: > nit: trailing period; "Use the new (2016 Q2) translate UI." Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:94: bool ShouldAlwaysTranslate() const override { On 2016/04/29 20:00:02, msw wrote: > nit: blank line above Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:166: translate::kTranslateUI2016Q2.name, ""); On 2016/04/29 20:00:02, msw wrote: > nit: s/""/std::string()/ Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:220: bubble_->GetBubbleFrameView()->ButtonPressed( On 2016/04/29 20:00:02, msw wrote: > nit: maybe use EventGenerator. Acknowledged. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:251: // In the 2016Q2 UI, we should not close nor take action. On 2016/04/29 20:00:02, msw wrote: > Isn't the 'nope' button simply not available in the new UI? I'm not sure it > makes sense to test your workaround to no-op when the non-existent button is > pressed... no, that is not true. the menu item will be there but the clicking should have no effect. User will still see it. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:330: // call the StyledLabelLinkClicked(); On 2016/04/29 20:00:02, msw wrote: > nit: "Click the styled label link." Done. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:331: views::StyledLabel styled_label(base::ASCIIToUTF16("test"), nullptr); On 2016/04/29 20:00:02, msw wrote: > You don't need to supply a valid styled label here; pass nullptr instead. no, I tried it. It won't work and will give me error. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:103: On 2016/04/29 20:00:02, msw wrote: > nit remove extra blank line Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:346: update.Get()->SetInteger(language, 0); On 2016/04/29 20:00:02, msw wrote: > Perhaps you can actually clear the pref value here... but it's not necessary. Acknowledged. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:392: // In the new logic, we only hide the Bubble if user denied for more than On 2016/04/29 20:00:02, msw wrote: > nit: no caps for 'bubble'; 'if the user denied it more than 3 times or the user > ignored it more'... Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:394: return (GetTranslationDeniedCount(language) > 3) || On 2016/04/29 20:00:02, msw wrote: > Should this ever propagate to kPrefTranslateTooOftenDeniedForLanguage? No https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:404: void TranslatePrefs::ResetDenialState() { On 2016/04/29 20:00:02, msw wrote: > Shouldn't this also have some effect on the new UI? no https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.h:35: // Feature flag for "Translate UI 2016 Q2" project On 2016/04/29 20:00:02, msw wrote: > trailing period. Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.h:132: // These methods are used to track how many times the user has ignore the On 2016/04/29 20:00:02, msw wrote: > ignored Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs_unittest.cc (left): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:9: #include <vector> On 2016/04/29 20:00:03, msw wrote: > this file uses vector, leave this include here. Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:169: translate::kTranslateUI2016Q2.name, ""); On 2016/04/29 20:00:02, msw wrote: > std::string Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:185: ASSERT_FALSE(translate_prefs_->IsTooOftenDenied(kTestLanguage)); On 2016/04/29 20:00:02, msw wrote: > nit: assert is a bit heavy-handed; expect would be okay throughout these tests. Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:196: TEST_F(TranslatePrefTest, IsTooOftenIgnoredIn2016Q2UI) { On 2016/04/29 20:00:03, msw wrote: > Maybe add some test that intermixes denied and ignored? Is there some special > behavior for that? Acknowledged. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:260: if (!base::FeatureList::IsEnabled(kTranslateUI2016Q2)) { On 2016/04/29 20:00:03, msw wrote: > nit: combine nested if with && Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:284: if (!base::FeatureList::IsEnabled(kTranslateUI2016Q2)) { On 2016/04/29 20:00:03, msw wrote: > nit: combine nested if with && Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:303: // After 2 clicks on Translate for the same language. On 2016/04/29 20:00:03, msw wrote: > Why does this comment reference 2, but the check is for 3? I'm not really sure > this comment makes much sense... consider rewriting it for clarity. Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:304: // it is == 2 not >2 because if the user translate with always on On 2016/04/29 20:00:03, msw wrote: > Capitalize 'it', '> 2'... 'translates' Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:306: // bubble will show up is if the user uncheck the Always translate, On 2016/04/29 20:00:03, msw wrote: > unchecks Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:307: // but in that case since user explictly uncheck, we should show On 2016/04/29 20:00:03, msw wrote: > 'unchecked it', 'show it as unchecked'... Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:122: // Show "Always translate" as checked in the bubble UI On 2016/04/29 20:00:03, msw wrote: > trailing period Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:123: bool ShowAlwaysTranslateChecked(); On 2016/04/29 20:00:03, msw wrote: > Maybe rename this (and/or make the comment) something more descriptive, like > "ShouldAlwaysTranslateBeCheckedByDefault" or similar? Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:86: "settings.language.preferred_languages", ""); On 2016/04/29 20:00:03, msw wrote: > std::string() here and below. Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:110: translate::kTranslateUI2016Q2.name, ""); On 2016/04/29 20:00:03, msw wrote: > std::string() Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:174: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedFalse) { On 2016/04/29 20:00:03, msw wrote: > Combine this test with the one above? you're toggling the same value... Done. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:187: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedTrueIn2016Q2UI) { On 2016/04/29 20:00:03, msw wrote: > Maybe use a parameterized gtest here for old/new? TEST_P... Acknowledged. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:201: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedFalseIn2016Q2UI) { On 2016/04/29 20:00:03, msw wrote: > Ditto Acknowledged. https://codereview.chromium.org/1923143003/diff/130001/ui/resources/ui_resour... File ui/resources/ui_resources.grd (right): https://codereview.chromium.org/1923143003/diff/130001/ui/resources/ui_resour... ui/resources/ui_resources.grd:196: <structure type="chrome_scaled_image" name="IDR_TRANSLATE_ICON_V2" file="common/translate.png" /> On 2016/04/29 20:00:03, msw wrote: > There is no "IDR_TRANSLATE_ICON"... so I'm not sure why you'd postfix '_V2'. It > seems like the current icon is vectorized, it'd be nice if this one was > similarly vectorized, but for now, maybe just drop the _V2? I cannot drop _V2 because it will cause name conflict. There already a IDR_TRANSLATE_ICON somewhere. This one is for the icon inside the bubble. https://codereview.chromium.org/1923143003/diff/130001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1923143003/diff/130001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:101: FRIEND_TEST_ALL_PREFIXES(::TranslateBubbleViewTest, CloseButtonIn2016Q2UI); On 2016/04/29 20:00:03, msw wrote: > Can you avoid this and instead fire an [Escape] key or similar in the test? no, I cannot.
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/150001
https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:387: translate::ALWAYS_TRANSLATE_CHECKED : On 2016/04/29 22:37:01, ftang wrote: > On 2016/04/29 20:00:00, msw wrote: > > Is this stat useful? It also counts users checking/unchecking the box before > > they actually submit the 'form' value... > > that is exactly what the UI designer ask me to track. Acknowledged. https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:251: // In the 2016Q2 UI, we should not close nor take action. On 2016/04/29 22:37:02, ftang wrote: > On 2016/04/29 20:00:02, msw wrote: > > Isn't the 'nope' button simply not available in the new UI? I'm not sure it > > makes sense to test your workaround to no-op when the non-existent button is > > pressed... > > no, that is not true. the menu item will be there but the clicking should have > no effect. User will still see it. Sorry, in the mocks it doesn't show a 'Nope' button. It shows [Translate], [Never (site)], [Never (lang)] under the [Options] button, a close (x), and an [Always] checkbox; where is the [Nope] button? Can you post a screenshot with this button? https://docs.google.com/presentation/d/1QECg9ZPANcOISaiS3tEDgOj4yJNSFnPhq2VWE... It sounds like you saying that there is a [Nope] button that won't even close the bubble, so users can repeatedly click [Nope] and the bubble will do *nothing*, even though users (myself included) would expect it to close the bubble. I hope I'm just misunderstanding your meaning... can you clarify? https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:394: return (GetTranslationDeniedCount(language) > 3) || On 2016/04/29 22:37:02, ftang wrote: > On 2016/04/29 20:00:02, msw wrote: > > Should this ever propagate to kPrefTranslateTooOftenDeniedForLanguage? > > No I'm not really sure how this is supposed to work, and it's non-obvious that the 'translation denied count' shouldn't affect the 'denied too often' flag, please explain in a comment. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs.cc:404: void TranslatePrefs::ResetDenialState() { On 2016/04/29 22:37:02, ftang wrote: > On 2016/04/29 20:00:02, msw wrote: > > Shouldn't this also have some effect on the new UI? > > no Ditto, I don't know understand why 'reset denial state' shouldn't change the recorded 'translation denied count', please explain in a comment. https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_prefs_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_prefs_unittest.cc:196: TEST_F(TranslatePrefTest, IsTooOftenIgnoredIn2016Q2UI) { On 2016/04/29 22:37:03, ftang wrote: > On 2016/04/29 20:00:03, msw wrote: > > Maybe add some test that intermixes denied and ignored? Is there some special > > behavior for that? > > Acknowledged. I guess this means 'no'... https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:187: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedTrueIn2016Q2UI) { On 2016/04/29 22:37:03, ftang wrote: > On 2016/04/29 20:00:03, msw wrote: > > Maybe use a parameterized gtest here for old/new? TEST_P... > > Acknowledged. I'm not sure why you wouldn't do this... the new UI won't have the same test coverage as the old UI here. Using a parameterized test would make coverage equal and let you avoid duplicating test fixtures in a one-off manner. https://codereview.chromium.org/1923143003/diff/130001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1923143003/diff/130001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:101: FRIEND_TEST_ALL_PREFIXES(::TranslateBubbleViewTest, CloseButtonIn2016Q2UI); On 2016/04/29 22:37:03, ftang wrote: > On 2016/04/29 20:00:03, msw wrote: > > Can you avoid this and instead fire an [Escape] key or similar in the test? > > no, I cannot. Please explain why; it's unfortunate if we need to make some random Chrome bubble's test a friend of a common UI element... I'd rather you add GetCloseButtonForTest() to this class. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_model.h (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model.h:85: // Returns true if the Always Translate should be Checked by default. nit: 'Translate button should' and do not capitalize 'checked' https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model.h:86: virtual bool ShouldAlwaysTranslateCheckedByDefault() const = 0; nit: TranslateBeChecked https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_view_state_transition.h (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:17: // The user clicked down and went back from the advanced option. nit: I'm not sure what 'clicked down' means, do you just mean 'clicked'? Otherwise, what does the user actually click to return to the normal view? https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:29: // The user selected the "Nope" in option menu. nit: grammar here and elsewhere, but whatever... https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:38: // The user clicked "Translate" button. nit: 'clicked the' https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:56: // The user clicked the "Setting" link. nit: Settings https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:57: SETTING_LINK_CLICKED, nit: SETTINGS https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:409: // Do noop on 2016q2 UI. nit: s/noop/nothing/ Explain why we have a button that does nothing? (this is very confusing to me...) https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:471: // The text of IDS_TRANSLATE_BUBBLE_ADVANCED is "Option" now. nit: s/IDS_TRANSLATE_BUBBLE_ADVANCED/DONT_TRANSLATE/ and add a TODO to change the identifier name once we deprecate the old bubble style (we shouldn't continue using 'DONT_TRANSLATE' as 'Options' in the future). https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:472: // In the 2016 Q2 UI, the menu item DONT_TRANSLATE is change to "option" nit: s/option/Options/ https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:473: // and won't perform any action. Doesn't it show the advanced view? That's quite different than not performing any action... I'm somewhat confused by this part of the change. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:91: // views::WidgetDelegate method. nit: Remove redundant comment and line above (merge "// views::WidgetDelegate method." sections and optionally change 'method' to 'methods'.) https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:213: TurnOnTranslate2016Q2UIFlag(); It would also be very nice if these tests were parameterized so the old bubble and the new bubble would have the same test coverage (allowing for expected differences as needed). As is, the new UI has far less coverage than the old UI. https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:389: // 3 times or the user ignored more than 10 times. nit: 'ignored it' https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:298: // After 2 clicks on Translate for the same language. nit: I still don't understand this comment, it'd be great if you could clarify, but please at least move it up out of the return statement and fix the indent. https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:124: bool ShouldAlwaysTranslateCheckedByDefault(); nit: BeChecked https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:165: manager_->GetLanguageState().SetTranslateEnabled(true); nit: remove if not needed (called on 155) https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:187: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedFalseIn2016Q2UI) { Combine this test with the one above, so the new and old versions of the test follow the same pattern. https://codereview.chromium.org/1923143003/diff/150001/ui/resources/ui_resour... File ui/resources/ui_resources.grd (right): https://codereview.chromium.org/1923143003/diff/150001/ui/resources/ui_resour... ui/resources/ui_resources.grd:196: <structure type="chrome_scaled_image" name="IDR_TRANSLATE_ICON_V2" file="common/translate.png" /> nit: Replace 'V2' with 'BUBBLE' (or 'NEW' to match the new IDS).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
2nd change according to msw review comments
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/170001
msw- PTAL https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_model.h (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model.h:85: // Returns true if the Always Translate should be Checked by default. On 2016/04/29 23:43:40, msw wrote: > nit: 'Translate button should' and do not capitalize 'checked' Done. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_model.h:86: virtual bool ShouldAlwaysTranslateCheckedByDefault() const = 0; On 2016/04/29 23:43:40, msw wrote: > nit: TranslateBeChecked Done. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_view_state_transition.h (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:17: // The user clicked down and went back from the advanced option. On 2016/04/29 23:43:40, msw wrote: > nit: I'm not sure what 'clicked down' means, do you just mean 'clicked'? > Otherwise, what does the user actually click to return to the normal view? Done. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:29: // The user selected the "Nope" in option menu. On 2016/04/29 23:43:40, msw wrote: > nit: grammar here and elsewhere, but whatever... Done. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:38: // The user clicked "Translate" button. On 2016/04/29 23:43:40, msw wrote: > nit: 'clicked the' Done. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:56: // The user clicked the "Setting" link. On 2016/04/29 23:43:40, msw wrote: > nit: Settings Done. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:57: SETTING_LINK_CLICKED, On 2016/04/29 23:43:40, msw wrote: > nit: SETTINGS Done. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:409: // Do noop on 2016q2 UI. On 2016/04/29 23:43:40, msw wrote: > nit: s/noop/nothing/ Explain why we have a button that does nothing? (this is > very confusing to me...) I agree. That is what the UI designer change it to. Originally, it lead to not translate and close the bubble but the UI designer want to remove that behavior. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:471: // The text of IDS_TRANSLATE_BUBBLE_ADVANCED is "Option" now. On 2016/04/29 23:43:40, msw wrote: > nit: s/IDS_TRANSLATE_BUBBLE_ADVANCED/DONT_TRANSLATE/ and add a TODO to change > the identifier name once we deprecate the old bubble style (we shouldn't > continue using 'DONT_TRANSLATE' as 'Options' in the future). Acknowledged. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:472: // In the 2016 Q2 UI, the menu item DONT_TRANSLATE is change to "option" On 2016/04/29 23:43:40, msw wrote: > nit: s/option/Options/ Done. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:473: // and won't perform any action. On 2016/04/29 23:43:40, msw wrote: > Doesn't it show the advanced view? That's quite different than not performing > any action... I'm somewhat confused by this part of the change. No, it only show "Options" and the menu drop down show "Never translate this language" and "Never translate this site". It does not lead to the advanced UI view. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:91: // views::WidgetDelegate method. On 2016/04/29 23:43:40, msw wrote: > nit: Remove redundant comment and line above (merge "// views::WidgetDelegate > method." sections and optionally change 'method' to 'methods'.) Done. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:213: TurnOnTranslate2016Q2UIFlag(); On 2016/04/29 23:43:40, msw wrote: > It would also be very nice if these tests were parameterized so the old bubble > and the new bubble would have the same test coverage (allowing for expected > differences as needed). As is, the new UI has far less coverage than the old UI. Acknowledged. https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... File components/translate/core/browser/translate_prefs.cc (right): https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_prefs.cc:389: // 3 times or the user ignored more than 10 times. On 2016/04/29 23:43:40, msw wrote: > nit: 'ignored it' Done. https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:298: // After 2 clicks on Translate for the same language. On 2016/04/29 23:43:40, msw wrote: > nit: I still don't understand this comment, it'd be great if you could clarify, > but please at least move it up out of the return statement and fix the indent. Done. https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:124: bool ShouldAlwaysTranslateCheckedByDefault(); On 2016/04/29 23:43:40, msw wrote: > nit: BeChecked Done. https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:165: manager_->GetLanguageState().SetTranslateEnabled(true); On 2016/04/29 23:43:40, msw wrote: > nit: remove if not needed (called on 155) no. Since I merged the two tests as you suggested, I have to reset the SeTranslateEnabled(true) after I called delegate_->SetLanguageBlocked(true); because that will turn it to false. https://codereview.chromium.org/1923143003/diff/150001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:187: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedFalseIn2016Q2UI) { On 2016/04/29 23:43:41, msw wrote: > Combine this test with the one above, so the new and old versions of the test > follow the same pattern. Done. https://codereview.chromium.org/1923143003/diff/150001/ui/resources/ui_resour... File ui/resources/ui_resources.grd (right): https://codereview.chromium.org/1923143003/diff/150001/ui/resources/ui_resour... ui/resources/ui_resources.grd:196: <structure type="chrome_scaled_image" name="IDR_TRANSLATE_ICON_V2" file="common/translate.png" /> On 2016/04/29 23:43:41, msw wrote: > nit: Replace 'V2' with 'BUBBLE' (or 'NEW' to match the new IDS). Done.
https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/130001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:187: TEST_F(TranslateUIDelegateTest, SetLanguageBlockedTrueIn2016Q2UI) { On 2016/04/29 23:43:40, msw wrote: > On 2016/04/29 22:37:03, ftang wrote: > > On 2016/04/29 20:00:03, msw wrote: > > > Maybe use a parameterized gtest here for old/new? TEST_P... > > > > Acknowledged. > > I'm not sure why you wouldn't do this... the new UI won't have the same test > coverage as the old UI here. Using a parameterized test would make coverage > equal and let you avoid duplicating test fixtures in a one-off manner. Please add TODOs and/or file a bug, this regresses test coverage. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:473: // and won't perform any action. On 2016/04/30 01:24:50, ftang wrote: > On 2016/04/29 23:43:40, msw wrote: > > Doesn't it show the advanced view? That's quite different than not performing > > any action... I'm somewhat confused by this part of the change. > > No, it only show "Options" and the menu drop down show "Never translate this > language" and "Never translate this site". It does not lead to the advanced UI > view. Does clicking the "Options" part of the button at least show the drop-down menu? If so say: "In the 2016 Q2 UI, the menu item DONT_TRANSLATE is changed to "Options", and it shows a drop-down menu when clicked." If not, make it do that or replace the combobox with a menu button. https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:213: TurnOnTranslate2016Q2UIFlag(); On 2016/04/30 01:24:50, ftang wrote: > On 2016/04/29 23:43:40, msw wrote: > > It would also be very nice if these tests were parameterized so the old bubble > > and the new bubble would have the same test coverage (allowing for expected > > differences as needed). As is, the new UI has far less coverage than the old > UI. > > Acknowledged. Please add TODOs and/or file a bug, this regresses test coverage. https://codereview.chromium.org/1923143003/diff/170001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:409: // Do nothing on 2016q2 UI. We should open the combobox menu here, instead of actually doing nothing. If that's not possible, replace the combobox with a menu button. https://codereview.chromium.org/1923143003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:471: // The text of DONT_TRANSLATE is "Options" now. Add a TODO comment here to rename the DONT_TRANSLATE enum entry to ADVANCED or OPTIONS or similar. https://codereview.chromium.org/1923143003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:472: // In the 2016 Q2 UI, the menu item DONT_TRANSLATE is change to "Options" nit: changed https://codereview.chromium.org/1923143003/diff/170001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/170001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:298: // We check it == 2 instead of >= 2 because if the user translates with the nit: s/it/for/ ... this comment makes a little more sense now, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
change to use MenuButton with MenuRunner for 'Options' menu in new UI based on msw request.
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/190001
toyoshim- please review ASAP. msw- I changed to use MenuButton and it looks much better. PTAL https://codereview.chromium.org/1923143003/diff/170001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:409: // Do nothing on 2016q2 UI. On 2016/04/30 02:12:59, msw wrote: > We should open the combobox menu here, instead of actually doing nothing. > If that's not possible, replace the combobox with a menu button. Done. https://codereview.chromium.org/1923143003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:471: // The text of DONT_TRANSLATE is "Options" now. On 2016/04/30 02:12:59, msw wrote: > Add a TODO comment here to rename the DONT_TRANSLATE enum entry to ADVANCED or > OPTIONS or similar. I think this is too confusing. Instead of recycle the ID, I use a new resources to make it clean now. https://codereview.chromium.org/1923143003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:472: // In the 2016 Q2 UI, the menu item DONT_TRANSLATE is change to "Options" On 2016/04/30 02:12:59, msw wrote: > nit: changed no longer needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just minor comments when reviewing diff to patch set 10. I'll take another pass over the whole CL this afternoon. https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:37: #include "ui/base/models/simple_menu_model.h" nit: not needed, included by header. https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:297: denial_menu_runner_.reset(); Why reset the runner here? Isn't it okay to re-use the same menu for the same bubble instance? https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:68: NEVER_TRANSLATE_LANGUAGE = 0, nit: explicit enum values (= 0, = 1) are not needed.
don't you need 200 assets?
On 2016/05/03 18:30:26, oshima wrote: > don't you need 200 assets? Or just use vector icon instead.
add 200 icon png. remove unnecessary include in .cc file. Remvoe unncessary reset() and remove explict value for enum
PTAL https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:37: #include "ui/base/models/simple_menu_model.h" On 2016/05/03 18:16:41, msw wrote: > nit: not needed, included by header. Done. https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:297: denial_menu_runner_.reset(); On 2016/05/03 18:16:41, msw wrote: > Why reset the runner here? Isn't it okay to re-use the same menu for the same > bubble instance? Done. https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/1923143003/diff/190001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:68: NEVER_TRANSLATE_LANGUAGE = 0, On 2016/05/03 18:16:41, msw wrote: > nit: explicit enum values (= 0, = 1) are not needed. Done.
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/210001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:203: if (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2) && Why does this only bail for the old UI? Maybe add a comment? https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_view_state_transition.h (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:20: // The user clicked the advanced link. How is this different from SET_STATE_OPTIONS? Should there only be one metric for "clicking the advanced link and going to the advanced options view"? https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:29: // The user selected the "Nope" in the "Options" menu. nit: selected "Nope" https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:32: // The user selected the "Never translate language" in the "Options" menu. nit: selected "Never https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:35: // The user selected the "Never translate this site" in the "Options" menu. nit: selected "Never https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:274: denial_menu_model_.reset(new ui::SimpleMenuModel(this)); optional nit: move this above the decl of |original_language_name|. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:372: use_2016_q2_ui_( This isn't actually needed as a class member, instead inline the IsEnabled check, or add a file-local helper function in the anon namespace, and use that. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:437: ReportUiAction((always_translate_checkbox_->checked() nit: remove extra parentheses around the ternary https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:470: model_->DeclineTranslation(); optional nit: move this back up above the decl of |index|. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:30: class PrefService; optional nit: Remove this; it's not used. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:33: class Range; Remove this; it should be included via styled_label_listener.h https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:38: class GridLayout; optional nit: Remove this; it's not used. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:40: class Link; optional nit: Remove this; it should be included via link_listener.h https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:42: class StyledLabel; Remove this; it should be included via styled_label_listener.h https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:67: enum DenialMenuItem { NEVER_TRANSLATE_LANGUAGE, NEVER_TRANSLATE_SITE }; nit: add a comment "// Item IDs for the denial button's menu." https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:225: void ReportUiAction(int action); nit: add a comment "Logs metrics for the user's TranslateBubbleUiEvent |action|." https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:225: void ReportUiAction(int action); Pass |action| as a TranslateBubbleUiEvent, not an int. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:249: nit: remove blank line https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:251: nit: remove blank line https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_prefs.h:36: extern const base::Feature kTranslateUI2016Q2; Does this belong in chrome/common/chrome_features.[h|cc]? https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:257: if (!base::FeatureList::IsEnabled(kTranslateUI2016Q2) && Why is this disabled for the new ui? Maybe add a comment? https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:280: if (!base::FeatureList::IsEnabled(kTranslateUI2016Q2) && Why is this disabled for the new ui? Maybe add a comment? https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:122: // Return true if "Always translate" should be checked by default in the optional nit: copy the comment from translate_bubble_model.h: // Returns true if the Always Translate checkbox should be checked by default. https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:80: pref_service_->registry()->RegisterStringPref( Why do we now need to register these two prefs? (I'm mostly just curious) https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:81: "settings.language.preferred_languages", std::string()); Should this use preferred_languages_prefs? (maybe doesn't work on non-cros?) https://codereview.chromium.org/1923143003/diff/210001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1923143003/diff/210001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:19: FORWARD_DECLARE_TEST(TranslateBubbleViewTest, CloseButtonIn2016Q2UI); Instead of adding a friend test, add a GetCloseButtonForTest() accessor.
change based on msw comments.
PATL https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:203: if (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2) && On 2016/05/03 22:33:34, msw wrote: > Why does this only bail for the old UI? Maybe add a comment? The problem is currently it stop offer translate if user click to the next page with the same language- which itself is unreasonable at first. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_view_state_transition.h (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:20: // The user clicked the advanced link. On 2016/05/03 22:33:34, msw wrote: > How is this different from SET_STATE_OPTIONS? Should there only be one metric > for "clicking the advanced link and going to the advanced options view"? ADVANCED_LINK_CLICKED record the link (the UI toward that state) got clicked. SET_STATE_OPTIONS record the view showing that state AFTER the link got clicked (or came from other UI action). It represent two different steps. ADVANCED_LINK_CLICKED should always lead to SET_STATE_OPTIONS but SET_STATE_OPTIONS may not always came from ADVANCED_LINK_CLICKED. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:29: // The user selected the "Nope" in the "Options" menu. On 2016/05/03 22:33:34, msw wrote: > nit: selected "Nope" Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:32: // The user selected the "Never translate language" in the "Options" menu. On 2016/05/03 22:33:34, msw wrote: > nit: selected "Never Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:35: // The user selected the "Never translate this site" in the "Options" menu. On 2016/05/03 22:33:34, msw wrote: > nit: selected "Never Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:274: denial_menu_model_.reset(new ui::SimpleMenuModel(this)); On 2016/05/03 22:33:34, msw wrote: > optional nit: move this above the decl of |original_language_name|. Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:372: use_2016_q2_ui_( On 2016/05/03 22:33:34, msw wrote: > This isn't actually needed as a class member, instead inline the IsEnabled > check, or add a file-local helper function in the anon namespace, and use that. Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:437: ReportUiAction((always_translate_checkbox_->checked() On 2016/05/03 22:33:34, msw wrote: > nit: remove extra parentheses around the ternary Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:470: model_->DeclineTranslation(); On 2016/05/03 22:33:34, msw wrote: > optional nit: move this back up above the decl of |index|. Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:30: class PrefService; On 2016/05/03 22:33:34, msw wrote: > optional nit: Remove this; it's not used. Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:33: class Range; On 2016/05/03 22:33:35, msw wrote: > Remove this; it should be included via styled_label_listener.h Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:38: class GridLayout; On 2016/05/03 22:33:35, msw wrote: > optional nit: Remove this; it's not used. Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:40: class Link; On 2016/05/03 22:33:35, msw wrote: > optional nit: Remove this; it should be included via link_listener.h Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:42: class StyledLabel; On 2016/05/03 22:33:35, msw wrote: > Remove this; it should be included via styled_label_listener.h Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:67: enum DenialMenuItem { NEVER_TRANSLATE_LANGUAGE, NEVER_TRANSLATE_SITE }; On 2016/05/03 22:33:35, msw wrote: > nit: add a comment "// Item IDs for the denial button's menu." Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:225: void ReportUiAction(int action); On 2016/05/03 22:33:34, msw wrote: > nit: add a comment "Logs metrics for the user's TranslateBubbleUiEvent > |action|." Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:225: void ReportUiAction(int action); On 2016/05/03 22:33:34, msw wrote: > Pass |action| as a TranslateBubbleUiEvent, not an int. Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:249: On 2016/05/03 22:33:34, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.h:251: On 2016/05/03 22:33:34, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_prefs.h:36: extern const base::Feature kTranslateUI2016Q2; On 2016/05/03 22:33:35, msw wrote: > Does this belong in chrome/common/chrome_features.[h|cc]? this is not really a feature flag, but a short term experimental flag inside the translate feature. I don't think it is a good idea to put that into the chrome/common/chrome_features.[h|cc] https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:257: if (!base::FeatureList::IsEnabled(kTranslateUI2016Q2) && On 2016/05/03 22:33:35, msw wrote: > Why is this disabled for the new ui? Maybe add a comment? Done. https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:280: if (!base::FeatureList::IsEnabled(kTranslateUI2016Q2) && On 2016/05/03 22:33:35, msw wrote: > Why is this disabled for the new ui? Maybe add a comment? Done. https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate.h:122: // Return true if "Always translate" should be checked by default in the On 2016/05/03 22:33:35, msw wrote: > optional nit: copy the comment from translate_bubble_model.h: > // Returns true if the Always Translate checkbox should be checked by default. Done. https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:80: pref_service_->registry()->RegisterStringPref( On 2016/05/03 22:33:35, msw wrote: > Why do we now need to register these two prefs? (I'm mostly just curious) without doing so, the unit test will break in some platform while it try to read them. https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:81: "settings.language.preferred_languages", std::string()); On 2016/05/03 22:33:35, msw wrote: > Should this use preferred_languages_prefs? (maybe doesn't work on non-cros?) ChromeOS use one of them and non ChromeOS use another one. I just register both in all cases to make the unit test simpler and won't break on any platform. https://codereview.chromium.org/1923143003/diff/210001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1923143003/diff/210001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:19: FORWARD_DECLARE_TEST(TranslateBubbleViewTest, CloseButtonIn2016Q2UI); On 2016/05/03 22:33:35, msw wrote: > Instead of adding a friend test, add a GetCloseButtonForTest() accessor. Done.
PATL
toyoshim: Sorry, I missed the "how many changes" question amongst the other messages. AFAICT, I've done 17 CLs so far, as measured by git shortlog --author= groby@chromium.org --grep="Translate" I have no idea on the review count, though :) On Wed, Apr 27, 2016 at 10:01 PM, <toyoshim@chromium.org> wrote: > looks good, but can you add more information to the change description, or > in > crbug? > > groby: how many changes did you submit or review on translate? I guess you > already achieved much things to be an owner in this area. > > > > https://codereview.chromium.org/1923143003/diff/90001/chrome/browser/ui/views... > File chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc > (right): > > > https://codereview.chromium.org/1923143003/diff/90001/chrome/browser/ui/views... > chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc:225: > bubble_->WindowClosing(); > ButtonPressed calls this internally, and causing test failures? > > https://codereview.chromium.org/1923143003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM with minor nits and confusion about why the new UI continues to offer translation after the user navigates to another page. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:203: if (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2) && On 2016/05/03 23:55:41, ftang wrote: > On 2016/05/03 22:33:34, msw wrote: > > Why does this only bail for the old UI? Maybe add a comment? > > The problem is currently it stop offer translate if user click to > the next page with the same language- which itself is unreasonable > at first. I don't understand why this is okay for the new UI, but not the old UI. Please seek review from someone in chrome/browser/translate/OWNERS Also, please add an explanatory comment here, I like your other comments. https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... File chrome/browser/ui/translate/translate_bubble_view_state_transition.h (right): https://codereview.chromium.org/1923143003/diff/210001/chrome/browser/ui/tran... chrome/browser/ui/translate/translate_bubble_view_state_transition.h:20: // The user clicked the advanced link. On 2016/05/03 23:55:41, ftang wrote: > On 2016/05/03 22:33:34, msw wrote: > > How is this different from SET_STATE_OPTIONS? Should there only be one metric > > for "clicking the advanced link and going to the advanced options view"? > ADVANCED_LINK_CLICKED record the link (the UI toward that state) got clicked. > SET_STATE_OPTIONS record the view showing that state AFTER the link got clicked > (or came from other UI action). It represent two different steps. > ADVANCED_LINK_CLICKED should always lead to SET_STATE_OPTIONS but > SET_STATE_OPTIONS may not always came from ADVANCED_LINK_CLICKED. Acknowledged. https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_prefs.h (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_prefs.h:36: extern const base::Feature kTranslateUI2016Q2; On 2016/05/03 23:55:42, ftang wrote: > On 2016/05/03 22:33:35, msw wrote: > > Does this belong in chrome/common/chrome_features.[h|cc]? > > this is not really a feature flag, but a short term experimental flag inside the > translate feature. I don't think it is a good idea to put that into the > chrome/common/chrome_features.[h|cc] Acknowledged. https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:80: pref_service_->registry()->RegisterStringPref( On 2016/05/03 23:55:42, ftang wrote: > On 2016/05/03 22:33:35, msw wrote: > > Why do we now need to register these two prefs? (I'm mostly just curious) > > without doing so, the unit test will break in some platform while it try to read > them. Acknowledged. https://codereview.chromium.org/1923143003/diff/210001/components/translate/c... components/translate/core/browser/translate_ui_delegate_unittest.cc:81: "settings.language.preferred_languages", std::string()); On 2016/05/03 23:55:42, ftang wrote: > On 2016/05/03 22:33:35, msw wrote: > > Should this use preferred_languages_prefs? (maybe doesn't work on non-cros?) > > ChromeOS use one of them and non ChromeOS use another one. I just register both > in all cases to make the unit test simpler and won't break on any platform. Acknowledged. https://codereview.chromium.org/1923143003/diff/230001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/230001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:203: // In the new UI, keep offer the translation after user navigate to grammar nit: "continue offering translation after the user navigates to another page"... I'm not sure why this makes sense for the new UI and not the old UI, but I presume someone more familiar with translation will review this CL. https://codereview.chromium.org/1923143003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:98: } // namespace nit: add a blank line above. https://codereview.chromium.org/1923143003/diff/230001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1923143003/diff/230001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:258: // even if the language is blocked so in case the user just want to nit: wants
oshima- I need your LGTM to check into the resource directory https://codereview.chromium.org/1923143003/diff/230001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/1923143003/diff/230001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:203: // In the new UI, keep offer the translation after user navigate to On 2016/05/04 00:19:20, msw wrote: > grammar nit: "continue offering translation after the user navigates to another > page"... I'm not sure why this makes sense for the new UI and not the old UI, > but I presume someone more familiar with translation will review this CL. Done. from my point of view, the old UI does not make sense but different people have different opinion and there will be just endless arguments base on what the people believe. So, instead of argue that for both new and old UI, we just change that in the new UI. https://codereview.chromium.org/1923143003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/translate/translate_bubble_view.cc (right): https://codereview.chromium.org/1923143003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/translate/translate_bubble_view.cc:98: } // namespace On 2016/05/04 00:19:21, msw wrote: > nit: add a blank line above. Done.
minor changes in comments
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/250001
ui/resources lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ftang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1923143003/#ps250001 (title: "minor changes in comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
translate lgtm
The CQ bit was checked by ftang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923143003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923143003/250001
Message was sent while issue was closed.
Description was changed from ========== Implement the 2016Q2 Translate UI designe spec out in https://goto.google.com/chrometranslateui2016q3 1. removed ‘Nope’ and replaced it with ‘x’ (close button) 2. show detected and page languages on the initial bubble- Makes it clearer that this is a translation feature, even for users who can’t perfectly read their UI language 3. Clicking on language names takes user to the language edit view (already exists) 4. increased the visibility of Always Translate 5. Instead of Nope, relabeled the button Options and made it a simple drop down 6. If the same language is translated 2 times, next time, the ‘always translate’ option is checked by default. 7. After 10 consecutive clicks on ‘x’ or ‘outside the box’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 8. After 3 consecutive clicks on ‘x’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 9. Translate events reset all counters 10. After 2 consecutive clicks on Translate for the same language: Show bubble with ‘Always do this’ checked But if users manually uncheck the checkbox, we will never automatically check it BUG=607170 ========== to ========== Implement the 2016Q2 Translate UI designe spec out in https://goto.google.com/chrometranslateui2016q3 1. removed ‘Nope’ and replaced it with ‘x’ (close button) 2. show detected and page languages on the initial bubble- Makes it clearer that this is a translation feature, even for users who can’t perfectly read their UI language 3. Clicking on language names takes user to the language edit view (already exists) 4. increased the visibility of Always Translate 5. Instead of Nope, relabeled the button Options and made it a simple drop down 6. If the same language is translated 2 times, next time, the ‘always translate’ option is checked by default. 7. After 10 consecutive clicks on ‘x’ or ‘outside the box’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 8. After 3 consecutive clicks on ‘x’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 9. Translate events reset all counters 10. After 2 consecutive clicks on Translate for the same language: Show bubble with ‘Always do this’ checked But if users manually uncheck the checkbox, we will never automatically check it BUG=607170 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== Implement the 2016Q2 Translate UI designe spec out in https://goto.google.com/chrometranslateui2016q3 1. removed ‘Nope’ and replaced it with ‘x’ (close button) 2. show detected and page languages on the initial bubble- Makes it clearer that this is a translation feature, even for users who can’t perfectly read their UI language 3. Clicking on language names takes user to the language edit view (already exists) 4. increased the visibility of Always Translate 5. Instead of Nope, relabeled the button Options and made it a simple drop down 6. If the same language is translated 2 times, next time, the ‘always translate’ option is checked by default. 7. After 10 consecutive clicks on ‘x’ or ‘outside the box’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 8. After 3 consecutive clicks on ‘x’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 9. Translate events reset all counters 10. After 2 consecutive clicks on Translate for the same language: Show bubble with ‘Always do this’ checked But if users manually uncheck the checkbox, we will never automatically check it BUG=607170 ========== to ========== Implement the 2016Q2 Translate UI designe spec out in https://goto.google.com/chrometranslateui2016q3 1. removed ‘Nope’ and replaced it with ‘x’ (close button) 2. show detected and page languages on the initial bubble- Makes it clearer that this is a translation feature, even for users who can’t perfectly read their UI language 3. Clicking on language names takes user to the language edit view (already exists) 4. increased the visibility of Always Translate 5. Instead of Nope, relabeled the button Options and made it a simple drop down 6. If the same language is translated 2 times, next time, the ‘always translate’ option is checked by default. 7. After 10 consecutive clicks on ‘x’ or ‘outside the box’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 8. After 3 consecutive clicks on ‘x’ for the same language across sites: Stop showing the bubble, but show the omnibar icon 9. Translate events reset all counters 10. After 2 consecutive clicks on Translate for the same language: Show bubble with ‘Always do this’ checked But if users manually uncheck the checkbox, we will never automatically check it BUG=607170 Committed: https://crrev.com/7223e2c7d071d74bcd66c5a113e97fe21a82bb68 Cr-Commit-Position: refs/heads/master@{#391540} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/7223e2c7d071d74bcd66c5a113e97fe21a82bb68 Cr-Commit-Position: refs/heads/master@{#391540} |
