Fix Translate Tab UI bugs

Fix three Tab UI bugs:
1. The always translate checkbox in the menu was retranslating an
already translated page when toggled.

2. When reverting a translation from the tab UI, the model's view state
was not being updated. This meant if a user untranslated the page after
clicking the always translate button in the menu, then clicked always
translate again, the page wouldn't translate since the model's state is
incorrectly "translated" rather than "before translate".

3. The 'always translate' button beneath the tab UI was not being
updated when the always translate menu item was being clicked.

Additionally this CL removes some unused code that was missed in the
experiment cleanup.

Bug: 1099571
Change-Id: I0b3545463acc257ab69cd616bce2e9e833cf2ac4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2269751Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Commit-Queue: Megan Jablonski <megjablon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785801}

chrome/browser/ui/views/translate/translate_bubble_view.cc

@@ -219,7 +219,7 @@ void TranslateBubbleView::CloseBubble() {
 
 base::string16 TranslateBubbleView::GetWindowTitle() const {
   int id = 0;
-  switch (model_->GetViewState()) {
+  switch (GetViewState()) {
     case TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE:
       id = IDS_TRANSLATE_BUBBLE_BEFORE_TRANSLATE_TITLE;
       break;
@@ -270,7 +270,7 @@ void TranslateBubbleView::Init() {
 
   UpdateChildVisibilities();
 
-  if (model_->GetViewState() == TranslateBubbleModel::VIEW_STATE_ERROR)
+  if (GetViewState() == TranslateBubbleModel::VIEW_STATE_ERROR)
     model_->ShowError(error_type_);
 }
 
@@ -292,8 +292,7 @@ void TranslateBubbleView::ButtonPressed(views::Button* sender,
       should_always_translate_ = always_checkbox->GetChecked();
       // In the tab UI the always translate button should apply immediately
       // except for in an advanced view.
-      if (model_->GetViewState() !=
-          TranslateBubbleModel::VIEW_STATE_SOURCE_LANGUAGE) {
+      if (GetViewState() != TranslateBubbleModel::VIEW_STATE_SOURCE_LANGUAGE) {
         model_->SetAlwaysTranslate(should_always_translate_);
       }
       translate::ReportUiAction(should_always_translate_
@@ -314,13 +313,6 @@ void TranslateBubbleView::ButtonPressed(views::Button* sender,
       ResetLanguage();
       break;
     }
-    case BUTTON_ID_RETURN: {
-      SwitchView(TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE);
-      tabbed_pane_->SelectTabAt(1);
-      UpdateChildVisibilities();
-      SizeToContents();
-      break;
-    }
   }
 }
 
@@ -337,8 +329,7 @@ bool TranslateBubbleView::ShouldShowWindowTitle() const {
 }
 
 void TranslateBubbleView::ResetLanguage() {
-  if (model_->GetViewState() ==
-      TranslateBubbleModel::VIEW_STATE_SOURCE_LANGUAGE) {
+  if (GetViewState() == TranslateBubbleModel::VIEW_STATE_SOURCE_LANGUAGE) {
     if (source_language_combobox_->GetSelectedIndex() ==
         previous_source_language_index_ + 1) {
       return;
@@ -379,7 +370,7 @@ void TranslateBubbleView::WindowClosing() {
 
 bool TranslateBubbleView::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
-  switch (model_->GetViewState()) {
+  switch (GetViewState()) {
     case TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE: {
       if (accelerator.key_code() == ui::VKEY_RETURN) {
         Translate();
@@ -495,13 +486,13 @@ void TranslateBubbleView::ExecuteCommand(int command_id, int event_flags) {
       if (should_always_translate_) {
         should_never_translate_language_ = false;
         model_->SetNeverTranslateLanguage(should_never_translate_language_);
-        if (((model_->GetViewState() ==
-              TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE) ||
-             IsEquivalentState(model_->GetViewState()))) {
+        if (GetViewState() ==
+            TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE) {
           model_->Translate();
           SwitchView(TranslateBubbleModel::VIEW_STATE_TRANSLATING);
         }
       }
+      UpdateChildVisibilities();
       break;
 
     case OptionsMenuItem::NEVER_TRANSLATE_LANGUAGE:
@@ -579,7 +570,7 @@ TranslateBubbleView::TranslateBubbleView(
 }
 
 views::View* TranslateBubbleView::GetCurrentView() const {
-  switch (model_->GetViewState()) {
+  switch (GetViewState()) {
     case TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE:
       return translate_view_;
     case TranslateBubbleModel::VIEW_STATE_TRANSLATING:
@@ -599,11 +590,13 @@ views::View* TranslateBubbleView::GetCurrentView() const {
 
 void TranslateBubbleView::Translate() {
   model_->Translate();
+  model_->SetViewState(TranslateBubbleModel::VIEW_STATE_TRANSLATING);
   translate::ReportUiAction(translate::TRANSLATE_BUTTON_CLICKED);
 }
 
 void TranslateBubbleView::ShowOriginal() {
   model_->RevertTranslation();
+  model_->SetViewState(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE);
   translate::ReportUiAction(translate::SHOW_ORIGINAL_BUTTON_CLICKED);
 }
 
@@ -1082,24 +1075,14 @@ std::unique_ptr<views::Button> TranslateBubbleView::CreateCloseButton() {
   return close_button;
 }
 
-bool TranslateBubbleView::IsEquivalentState(
-    TranslateBubbleModel::ViewState view_state) {
-  return view_state == TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE ||
-         view_state == TranslateBubbleModel::VIEW_STATE_TRANSLATING ||
-         view_state == TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE;
-}
-
 views::Checkbox* TranslateBubbleView::GetAlwaysTranslateCheckbox() {
-  if (model_->GetViewState() ==
-          TranslateBubbleModel::VIEW_STATE_SOURCE_LANGUAGE ||
-      model_->GetViewState() ==
-          TranslateBubbleModel::VIEW_STATE_TARGET_LANGUAGE) {
+  if (GetViewState() == TranslateBubbleModel::VIEW_STATE_SOURCE_LANGUAGE ||
+      GetViewState() == TranslateBubbleModel::VIEW_STATE_TARGET_LANGUAGE) {
     return advanced_always_translate_checkbox_;
-  } else if (model_->GetViewState() ==
+  } else if (GetViewState() ==
                  TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE ||
-             model_->GetViewState() ==
-                 TranslateBubbleModel::VIEW_STATE_TRANSLATING ||
-             model_->GetViewState() ==
+             GetViewState() == TranslateBubbleModel::VIEW_STATE_TRANSLATING ||
+             GetViewState() ==
                  TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE) {
     return always_translate_checkbox_;
   } else {

chrome/browser/ui/views/translate/translate_bubble_view.h

@@ -117,8 +117,7 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
     BUTTON_ID_ALWAYS_TRANSLATE,
     BUTTON_ID_OPTIONS_MENU,
     BUTTON_ID_CLOSE,
-    BUTTON_ID_RESET,
-    BUTTON_ID_RETURN
+    BUTTON_ID_RESET
   };
 
   enum ComboboxID {
@@ -155,6 +154,8 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
                            TabSelectedAfterTranslation);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
                            AlwaysTranslateTriggerTranslation);
+  FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest,
+                           ShowOriginalUpdatesViewState);
 
   TranslateBubbleView(views::View* anchor_view,
                       std::unique_ptr<TranslateBubbleModel> model,
@@ -200,9 +201,6 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
   // takes ownership of the returned view.
   std::unique_ptr<views::View> CreateViewAdvancedTarget();
 
-  // Tab UI presents the same view for before/during/after translate state.
-  bool IsEquivalentState(TranslateBubbleModel::ViewState view_state);
-
   // Creates the 'advanced' view to show source/target language combobox. Caller
   // takes ownership of the returned view.
   std::unique_ptr<views::View> CreateViewAdvanced(

chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc

@@ -287,6 +287,7 @@ TEST_F(TranslateBubbleViewTest, AlwaysTranslateCheckboxShortcut) {
   PressButton(TranslateBubbleView::BUTTON_ID_ALWAYS_TRANSLATE);
   EXPECT_TRUE(mock_model_->should_always_translate_);
   EXPECT_EQ(1, mock_model_->set_always_translate_called_count_);
+  EXPECT_TRUE(bubble_->always_translate_checkbox_->GetChecked());
 }
 
 TEST_F(TranslateBubbleViewTest, AlwaysTranslateCheckboxAndCloseButton) {
@@ -434,6 +435,16 @@ TEST_F(TranslateBubbleViewTest, AlwaysTranslateLanguageMenuItem) {
   EXPECT_TRUE(mock_model_->ShouldAlwaysTranslate());
   EXPECT_TRUE(mock_model_->translate_called_);
 
+  // Toggle ShouldAlwaysTranslate and make sure Translation doesn't happen
+  // again.
+  mock_model_->translate_called_ = false;
+  // Revert Always Translate.
+  bubble_->options_menu_model_->ActivatedAt(index);
+  EXPECT_FALSE(mock_model_->translate_called_);
+  // Recheck Always Translate.
+  bubble_->options_menu_model_->ActivatedAt(index);
+  EXPECT_FALSE(mock_model_->translate_called_);
+
   // Go back to untranslated page, since the *language* should still always
   // be translated (and this "untranslate" is temporary) the option should now
   // be checked and it should be possible to disable it from the menu.
@@ -448,7 +459,6 @@ TEST_F(TranslateBubbleViewTest, AlwaysTranslateLanguageMenuItem) {
   // not currently in a translated state and nothing needs to be reverted.
   // translate_called_ is set back to false just to make sure it's not being
   // called again.
-  mock_model_->translate_called_ = false;
   bubble_->options_menu_model_->ActivatedAt(index);
   EXPECT_FALSE(mock_model_->ShouldAlwaysTranslate());
   EXPECT_FALSE(mock_model_->translate_called_);
@@ -497,3 +507,16 @@ TEST_F(TranslateBubbleViewTest, TabSelectedAfterTranslation) {
   EXPECT_EQ(bubble_->tabbed_pane_->GetSelectedTabIndex(),
             static_cast<size_t>(1));
 }
+
+TEST_F(TranslateBubbleViewTest, ShowOriginalUpdatesViewState) {
+  CreateAndShowBubble();
+  // Translate.
+  bubble_->TabSelectedAt(1);
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_TRANSLATING,
+            bubble_->GetViewState());
+
+  // Show Original.
+  bubble_->TabSelectedAt(0);
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
+            bubble_->GetViewState());
+}