Only show "Never translate this site" when site can be blacklisted.

The option to "Never translate this site" is always shown on the
translate dialog, even when the site cannot be blacklisted (for example
a file:/// URL). This patch changes the translate options combon so
"Never translate..." is only shown if it will have some effect.

Note that the 2016Q2 experimental UI has not been patched as the
currently plan is to not launch this UI.

Bug: 663934
Change-Id: I64b80d830e124e9372906bd0c4711568c46ff636
Reviewed-on: https://chromium-review.googlesource.com/572475Reviewed-by: Jia Meng <jiameng@chromium.org>
Commit-Queue: Jon Napper <napper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509607}

chrome/browser/ui/translate/translate_bubble_model.h

@@ -102,6 +102,9 @@ class TranslateBubbleModel {
   // Returns true if the page is translated in the currently selected source
   // and target language.
   virtual bool IsPageTranslatedInCurrentLanguages() const = 0;
+
+  // True if the site of the current page can be blacklisted.
+  virtual bool CanBlacklistSite() = 0;
 };
 
 #endif  // CHROME_BROWSER_UI_TRANSLATE_TRANSLATE_BUBBLE_MODEL_H_

chrome/browser/ui/translate/translate_bubble_model_impl.cc

@@ -100,6 +100,10 @@ void TranslateBubbleModelImpl::SetNeverTranslateSite(bool value) {
   ui_delegate_->SetSiteBlacklist(value);
 }
 
+bool TranslateBubbleModelImpl::CanBlacklistSite() {
+  return ui_delegate_->CanBlacklistSite();
+}
+
 bool TranslateBubbleModelImpl::ShouldAlwaysTranslate() const {
   return ui_delegate_->ShouldAlwaysTranslate();
 }

chrome/browser/ui/translate/translate_bubble_model_impl.h

@@ -50,6 +50,7 @@ class TranslateBubbleModelImpl : public TranslateBubbleModel {
   void RevertTranslation() override;
   void OnBubbleClosing() override;
   bool IsPageTranslatedInCurrentLanguages() const override;
+  bool CanBlacklistSite() override;
 
  private:
   std::unique_ptr<translate::TranslateUIDelegate> ui_delegate_;

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

@@ -645,15 +645,25 @@ views::View* TranslateBubbleView::CreateViewBeforeTranslate() {
     denial_menu_button_->SetStyleDeprecated(views::Button::STYLE_BUTTON);
     layout->AddView(denial_menu_button_);
   } else {
-    std::vector<base::string16> items(
-        static_cast<size_t>(DenialComboboxIndex::MENU_SIZE));
+    // Can the site of the current page be blacklisted? This will be false for
+    // example for file:///... URLs.
+    const bool can_blacklist_site = model_->CanBlacklistSite();
+
+    // Number of items in the combobox.
+    const size_t menu_size = static_cast<size_t>(
+        can_blacklist_site ? DenialComboboxIndex::MENU_SIZE
+                           : DenialComboboxIndex::MENU_SIZE_NO_BLACKLIST);
+
+    std::vector<base::string16> items(menu_size);
     items[static_cast<size_t>(DenialComboboxIndex::DONT_TRANSLATE)] =
         l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_DENY);
     items[static_cast<size_t>(DenialComboboxIndex::NEVER_TRANSLATE_LANGUAGE)] =
         l10n_util::GetStringFUTF16(IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_LANG,
                                    original_language_name);
-    items[static_cast<size_t>(DenialComboboxIndex::NEVER_TRANSLATE_SITE)] =
-        l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_SITE);
+    if (can_blacklist_site) {
+      items[static_cast<size_t>(DenialComboboxIndex::NEVER_TRANSLATE_SITE)] =
+          l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_SITE);
+    }
     denial_combobox_model_.reset(new ui::SimpleComboboxModel(items));
     denial_combobox_ = new views::Combobox(denial_combobox_model_.get(),
                                            views::Combobox::STYLE_ACTION);

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

@@ -49,10 +49,12 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
                             public content::WebContentsObserver {
  public:
   // Commands shown in the action-style combobox. The value corresponds to the
-  // position in the combobox menu. Gaps will become separators.
+  // position in the combobox menu.
   enum class DenialComboboxIndex {
     DONT_TRANSLATE = 0,
     NEVER_TRANSLATE_LANGUAGE = 1,
+    SEPARATOR = 2,
+    MENU_SIZE_NO_BLACKLIST = SEPARATOR,
     NEVER_TRANSLATE_SITE = 3,
     MENU_SIZE = 4,
   };
@@ -175,6 +177,8 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
                            CancelButtonReturningAfterTranslate);
   FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewTest, CancelButtonReturningError);
   FRIEND_TEST_ALL_PREFIXES(TranslateLanguageBrowserTest, TranslateAndRevert);
+  FRIEND_TEST_ALL_PREFIXES(TranslateBubbleViewBrowserTest,
+                           CheckNeverTranslateThisSiteBlacklist);
 
   TranslateBubbleView(views::View* anchor_view,
                       const gfx::Point& anchor_point,

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

@@ -9,6 +9,8 @@
 
 #include "base/command_line.h"
 #include "base/macros.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/translate/chrome_translate_client.h"
 #include "chrome/browser/ui/browser.h"
@@ -16,11 +18,15 @@
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/translate/core/browser/translate_manager.h"
 #include "components/translate/core/common/language_detection_details.h"
 #include "content/public/browser/notification_details.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/simple_combobox_model.h"
+#include "ui/views/controls/combobox/combobox.h"
 
 class TranslateBubbleViewBrowserTest : public InProcessBrowserTest {
  public:
@@ -28,6 +34,7 @@ class TranslateBubbleViewBrowserTest : public InProcessBrowserTest {
   ~TranslateBubbleViewBrowserTest() override {}
 
   void SetUp() override {
+    feature_list_.InitAndDisableFeature(translate::kTranslateUI2016Q2);
     set_open_about_blank_on_browser_launch(true);
     translate::TranslateManager::SetIgnoreMissingKeyForTesting(true);
     InProcessBrowserTest::SetUp();
@@ -48,6 +55,7 @@ class TranslateBubbleViewBrowserTest : public InProcessBrowserTest {
 
  private:
   std::string expected_lang_;
+  base::test::ScopedFeatureList feature_list_;
 
   bool OnLanguageDetermined(const content::NotificationSource& source,
                             const content::NotificationDetails& details) {
@@ -124,3 +132,37 @@ IN_PROC_BROWSER_TEST_F(TranslateBubbleViewBrowserTest,
                            browser()->tab_strip_model()->GetActiveWebContents(),
                            false);
 }
+
+IN_PROC_BROWSER_TEST_F(TranslateBubbleViewBrowserTest,
+                       CheckNeverTranslateThisSiteBlacklist) {
+  EXPECT_FALSE(TranslateBubbleView::GetCurrentBubble());
+
+  // Show a French page and wait until the bubble is shown.
+  GURL french_url = ui_test_utils::GetTestUrl(
+      base::FilePath(), base::FilePath(FILE_PATH_LITERAL("french_page.html")));
+  NavigateAndWaitForLanguageDetection(french_url, "fr");
+  const TranslateBubbleView* const bubble =
+      TranslateBubbleView::GetCurrentBubble();
+  EXPECT_TRUE(bubble);
+
+  // Since this is a file:/// URL, we should not be able to blacklist.
+  EXPECT_FALSE(bubble->model_->CanBlacklistSite());
+
+  ASSERT_FALSE(base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2));
+  EXPECT_TRUE(bubble->denial_combobox_);
+
+  // Check the combobox contains the correct items.
+  EXPECT_EQ(
+      bubble->denial_combobox_->GetRowCount(),
+      static_cast<int>(
+          TranslateBubbleView::DenialComboboxIndex::MENU_SIZE_NO_BLACKLIST));
+  EXPECT_EQ(bubble->denial_combobox_->GetTextForRow(0),
+            l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_DENY));
+  EXPECT_EQ(
+      bubble->denial_combobox_->GetTextForRow(1),
+      l10n_util::GetStringFUTF16(IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_LANG,
+                                 base::ASCIIToUTF16("French")));
+
+  chrome::CloseWindow(browser());
+  EXPECT_FALSE(TranslateBubbleView::GetCurrentBubble());
+}

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

@@ -12,8 +12,10 @@
 #include "chrome/browser/ui/translate/translate_bubble_model.h"
 #include "chrome/browser/ui/translate/translate_bubble_view_state_transition.h"
 #include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/translate/core/browser/translate_prefs.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/events/event_constants.h"
 #include "ui/events/event_utils.h"
 #include "ui/gfx/range/range.h"
@@ -43,7 +45,8 @@ class MockTranslateBubbleModel : public TranslateBubbleModel {
         revert_translation_called_(false),
         translation_declined_(false),
         original_language_index_on_translation_(-1),
-        target_language_index_on_translation_(-1) {}
+        target_language_index_on_translation_(-1),
+        can_blacklist_site_(true) {}
 
   TranslateBubbleModel::ViewState GetViewState() const override {
     return view_state_transition_.view_state();
@@ -120,6 +123,10 @@ class MockTranslateBubbleModel : public TranslateBubbleModel {
            target_language_index_on_translation_ == target_language_index_;
   }
 
+  bool CanBlacklistSite() override { return can_blacklist_site_; }
+
+  void SetCanBlacklistSite(bool value) { can_blacklist_site_ = value; }
+
   TranslateBubbleViewStateTransition view_state_transition_;
   translate::TranslateErrors::Type error_type_;
   int original_language_index_;
@@ -134,6 +141,7 @@ class MockTranslateBubbleModel : public TranslateBubbleModel {
   bool translation_declined_;
   int original_language_index_on_translation_;
   int target_language_index_on_translation_;
+  bool can_blacklist_site_;
 };
 
 }  // namespace
@@ -469,3 +477,45 @@ TEST_F(TranslateBubbleViewTest, CancelButtonReturningError) {
   bubble_->HandleButtonPressed(TranslateBubbleView::BUTTON_ID_CANCEL);
   EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_ERROR, bubble_->GetViewState());
 }
+
+TEST_F(TranslateBubbleViewTest, ComboboxCanBlacklistSite) {
+  EXPECT_TRUE(mock_model_->CanBlacklistSite());
+  CreateAndShowBubble();
+  views::Combobox* const combobox = denial_combobox();
+  EXPECT_FALSE(denial_button_clicked());
+  EXPECT_FALSE(bubble_->GetWidget()->IsClosed());
+  // Check menu rows are DENY, NEVER_TRANSLATE_LANG, [SEPARATOR], and
+  // NEVER_TRANSLATE_SITE.
+  EXPECT_EQ(4, combobox->GetRowCount());
+  EXPECT_EQ(l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_DENY),
+            combobox->GetTextForRow(static_cast<size_t>(
+                TranslateBubbleView::DenialComboboxIndex::DONT_TRANSLATE)));
+  EXPECT_EQ(
+      l10n_util::GetStringFUTF16(IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_LANG,
+                                 base::ASCIIToUTF16("English")),
+      combobox->GetTextForRow(static_cast<size_t>(
+          TranslateBubbleView::DenialComboboxIndex::NEVER_TRANSLATE_LANGUAGE)));
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_SITE),
+      combobox->GetTextForRow(static_cast<size_t>(
+          TranslateBubbleView::DenialComboboxIndex::NEVER_TRANSLATE_SITE)));
+}
+
+TEST_F(TranslateBubbleViewTest, ComboboxCantBlacklistSite) {
+  mock_model_->SetCanBlacklistSite(false);
+  CreateAndShowBubble();
+  views::Combobox* const combobox = denial_combobox();
+  //  views::test::ComboboxTestApi test_api(combobox);
+  EXPECT_FALSE(denial_button_clicked());
+  EXPECT_FALSE(bubble_->GetWidget()->IsClosed());
+  // Check the menu rows are DENY and NEVER_TRANSLATE_LANG.
+  EXPECT_EQ(2, combobox->GetRowCount());
+  EXPECT_EQ(l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_DENY),
+            combobox->GetTextForRow(static_cast<size_t>(
+                TranslateBubbleView::DenialComboboxIndex::DONT_TRANSLATE)));
+  EXPECT_EQ(
+      l10n_util::GetStringFUTF16(IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_LANG,
+                                 base::ASCIIToUTF16("English")),
+      combobox->GetTextForRow(static_cast<size_t>(
+          TranslateBubbleView::DenialComboboxIndex::NEVER_TRANSLATE_LANGUAGE)));
+}

components/translate/core/browser/translate_ui_delegate.cc

@@ -291,6 +291,10 @@ bool TranslateUIDelegate::IsSiteBlacklisted() {
   return !host.empty() && prefs_->IsSiteBlacklisted(host);
 }
 
+bool TranslateUIDelegate::CanBlacklistSite() {
+  return !GetPageHost().empty();
+}
+
 void TranslateUIDelegate::SetSiteBlacklist(bool value) {
   std::string host = GetPageHost();
   if (host.empty())

components/translate/core/browser/translate_ui_delegate.h

@@ -108,7 +108,12 @@ class TranslateUIDelegate {
   // Returns true if the current webpage is blacklisted.
   bool IsSiteBlacklisted();
 
-  // Sets the value if the current webpage is blacklisted.
+  // Returns true if the site of the current webpage can be blacklisted.
+  bool CanBlacklistSite();
+
+  // Sets the blacklisted state for the host of the current page. If
+  // value is true, the current host will be blacklisted and translations
+  // will not be offered for that site.
   void SetSiteBlacklist(bool value);
 
   // Returns true if the webpage in the current original language should be