Fixing the translate menu behavior.

The correct behavior is to always have the menu showing,
it is disabled when the page is translated or a Chrome page
(new tab page, history...).
Selecting the translate menu triggers a translation of the
page to the Chrome UI language.

BUG=35480
TEST=Open the page context menu on the new tab page. There
     should be a translate menu and it should be disabled.
     Navigate to a page in a foreign language. Opne the
     context menu. The translate menu should be enabled.
     Select it. The page should be translated. Open the
     context menu again on the translated page. The
     translate menu should be disabled.
     
Review URL: http://codereview.chromium.org/1402003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@42932 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 143 insertions, 71 deletions

diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc
index edfff450..137311d 100644
--- a/chrome/browser/tab_contents/render_view_context_menu.cc
+++ b/chrome/browser/tab_contents/render_view_context_menu.cc
@@ -233,6 +233,8 @@ void RenderViewContextMenu::AppendExtensionItems(
 void RenderViewContextMenu::AppendAllExtensionItems() {
   extension_item_map_.clear();
   ExtensionsService* service = profile_->GetExtensionsService();
+  if (!service)
+    return;  // In unit-tests, we may not have an ExtensionService.
   ExtensionMenuManager* menu_manager = service->menu_manager();
 
   // Get a list of extension id's that have context menu items, and sort it by
@@ -402,7 +404,8 @@ void RenderViewContextMenu::AppendPageItems() {
   AppendSeparator();
   AppendMenuItem(IDS_CONTENT_CONTEXT_SAVEPAGEAS);
   AppendMenuItem(IDS_CONTENT_CONTEXT_PRINT);
-  if (TranslationService::IsTranslationEnabled()) {
+  if (TranslationService::IsTranslationEnabled() ||
+      TranslateManager::test_enabled()) {
     std::string locale = g_browser_process->GetApplicationLocale();
     locale = TranslationService::GetLanguageCode(locale);
     string16 language =
@@ -626,12 +629,10 @@ bool RenderViewContextMenu::IsItemCommandEnabled(int id) const {
     case IDS_CONTENT_CONTEXT_VIEWPAGEINFO:
       return IsDevCommandEnabled(id);
 
-    case IDS_CONTENT_CONTEXT_TRANSLATE: {
-      TranslateManager* translate_manager = Singleton<TranslateManager>::get();
-      return !source_tab_contents_->interstitial_page() &&
-          translate_manager->IsTranslatableURL(params_.page_url) &&
-          !translate_manager->IsShowingTranslateInfobar(source_tab_contents_);
-    }
+    case IDS_CONTENT_CONTEXT_TRANSLATE:
+      return !source_tab_contents_->language_state().IsPageTranslated() &&
+          !source_tab_contents_->interstitial_page() &&
+          TranslateManager::IsTranslatableURL(params_.page_url);
 
     case IDS_CONTENT_CONTEXT_OPENLINKNEWTAB:
     case IDS_CONTENT_CONTEXT_OPENLINKNEWWINDOW:
@@ -1017,9 +1018,20 @@ void RenderViewContextMenu::ExecuteItemCommand(int id) {
       break;
     }
 
-    case IDS_CONTENT_CONTEXT_TRANSLATE:
-      TranslateManager::ShowInfoBar(source_tab_contents_);
+    case IDS_CONTENT_CONTEXT_TRANSLATE: {
+      // A translation might have been triggered by the time the menu got
+      // selected, do nothing in that case.
+      if (source_tab_contents_->language_state().IsPageTranslated() ||
+          source_tab_contents_->language_state().translation_pending()) {
+        return;
+      }
+      std::string locale = g_browser_process->GetApplicationLocale();
+      locale = TranslationService::GetLanguageCode(locale);
+      source_tab_contents_->TranslatePage(
+          source_tab_contents_->language_state().original_language(),
+          locale);
       break;
+    }
 
     case IDS_CONTENT_CONTEXT_RELOADFRAME:
       source_tab_contents_->render_view_host()->ReloadFrame();
diff --git a/chrome/browser/translate/translate_manager.cc b/chrome/browser/translate/translate_manager.cc
index 2e7dd2f..5665c43 100644
--- a/chrome/browser/translate/translate_manager.cc
+++ b/chrome/browser/translate/translate_manager.cc
@@ -30,6 +30,7 @@ bool TranslateManager::test_enabled_ = false;
 TranslateManager::~TranslateManager() {
 }
 
+// static
 bool TranslateManager::IsTranslatableURL(const GURL& url) {
   return !url.SchemeIs("chrome");
 }
@@ -129,30 +130,6 @@ void TranslateManager::Observe(NotificationType type,
 }
 
 // static
-bool TranslateManager::ShowInfoBar(TabContents* tab) {
-  NavigationEntry* nav_entry = tab->controller().GetActiveEntry();
-  if (!nav_entry || IsShowingTranslateInfobar(tab))
-    return false;
-
-  LanguageState& language_state = tab->language_state();
-  std::string target_lang;
-  TranslateInfoBarDelegate::TranslateState state;
-  if (language_state.IsPageTranslated()) {
-    state = TranslateInfoBarDelegate::kAfterTranslate;
-    target_lang = language_state.current_language();
-  } else {
-    state = TranslateInfoBarDelegate::kBeforeTranslate;
-    target_lang = GetTargetLanguage();
-    if (target_lang.empty())
-      return false;  // The language Chrome is in is not supported.
-  }
-
-  AddTranslateInfoBar(tab, state, nav_entry->url(),
-                      language_state.original_language(), target_lang);
-  return true;
-}
-
-// static
 bool TranslateManager::IsShowingTranslateInfobar(TabContents* tab) {
   for (int i = 0; i < tab->infobar_delegate_count(); ++i) {
     if (tab->GetInfoBarDelegateAt(i)->AsTranslateInfoBarDelegate())
diff --git a/chrome/browser/translate/translate_manager.h b/chrome/browser/translate/translate_manager.h
index 704d928..fed522d 100644
--- a/chrome/browser/translate/translate_manager.h
+++ b/chrome/browser/translate/translate_manager.h
@@ -28,27 +28,21 @@ class TranslateManager : public NotificationObserver {
  public:
   virtual ~TranslateManager();
 
-  // Returns true if the URL can be translated, if it is not an internal URL
-  // (chrome:// and others).
-  bool IsTranslatableURL(const GURL& url);
-
   // NotificationObserver implementation:
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
-  // Shows the translate infobar if it's not already showing.  The state and
-  // languages are determined based on the current state of the page.
-  // Returns true if a new infobar was shown as a result of this call, false
-  // otherwise (if there was already a translate infobar or if there is no
-  // current navigation entry).
-  static bool ShowInfoBar(TabContents* tab);
-
   // Convenience method to know if a tab is showing a translate infobar.
   static bool IsShowingTranslateInfobar(TabContents* tab);
 
+  // Returns true if the URL can be translated, if it is not an internal URL
+  // (chrome:// and others).
+  static bool IsTranslatableURL(const GURL& url);
+
   // Used by unit-test to enable the TranslateManager for testing purpose.
   static void set_test_enabled(bool enabled) { test_enabled_ = enabled; }
+  static bool test_enabled() { return test_enabled_; }
 
  protected:
   TranslateManager();
diff --git a/chrome/browser/translate/translate_manager_unittest.cc b/chrome/browser/translate/translate_manager_unittest.cc
index 13278c5..30bc4c5 100644
--- a/chrome/browser/translate/translate_manager_unittest.cc
+++ b/chrome/browser/translate/translate_manager_unittest.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/renderer_host/test/test_render_view_host.h"
 
 #include "chrome/browser/renderer_host/mock_render_process_host.h"
+#include "chrome/browser/tab_contents/render_view_context_menu.h"
 #include "chrome/browser/translate/translate_infobars_delegates.h"
 #include "chrome/browser/translate/translate_manager.h"
 #include "chrome/common/ipc_test_sink.h"
@@ -13,6 +14,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/test/testing_browser_process.h"
+#include "grit/generated_resources.h"
 #include "third_party/cld/languages/public/languages.h"
 
 class TestTranslateManager : public TranslateManager {
@@ -178,6 +180,65 @@ class NavEntryCommittedObserver : public NotificationObserver {
   DISALLOW_COPY_AND_ASSIGN(NavEntryCommittedObserver);
 };
 
+class TestRenderViewContextMenu : public RenderViewContextMenu {
+ public:
+  static TestRenderViewContextMenu* CreateContextMenu(
+      TabContents* tab_contents) {
+    ContextMenuParams params;
+    params.media_type = WebKit::WebContextMenuData::MediaTypeNone;
+    params.x = 0;
+    params.y = 0;
+    params.is_image_blocked = false;
+    params.media_flags = 0;
+    params.spellcheck_enabled = false;;
+    params.is_editable = false;
+    params.page_url = tab_contents->controller().GetActiveEntry()->url();
+#if defined(OS_MACOSX)
+    params.writing_direction_default = 0;
+    params.writing_direction_left_to_right = 0;
+    params.writing_direction_right_to_left = 0;
+#endif  // OS_MACOSX
+    params.edit_flags = 0;
+    return new TestRenderViewContextMenu(tab_contents, params);
+  }
+
+  bool IsItemPresent(int id) {
+    return std::find(item_ids_.begin(), item_ids_.end(), id) != item_ids_.end();
+  }
+
+  bool TestIsItemCommandEnabled(int id) const {
+    return IsItemCommandEnabled(id);
+  }
+  void TestExecuteItemCommand(int id) { return ExecuteItemCommand(id); }
+
+ protected:
+  virtual void AppendMenuItem(int id) { item_ids_.push_back(id); }
+  virtual void AppendMenuItem(int id, const string16& label) {
+    item_ids_.push_back(id);
+  }
+  virtual void AppendRadioMenuItem(int id, const string16& label) {
+    item_ids_.push_back(id);
+  }
+  virtual void AppendCheckboxMenuItem(int id, const string16& label) {
+    item_ids_.push_back(id);
+  }
+  virtual void AppendSeparator() {}
+  virtual void StartSubMenu(int id, const string16& label) {
+    item_ids_.push_back(id);
+  }
+  virtual void FinishSubMenu() {}
+
+ private:
+  TestRenderViewContextMenu(TabContents* tab_contents,
+                            const ContextMenuParams& params)
+      : RenderViewContextMenu(tab_contents, params) {
+  }
+
+  std::vector<int> item_ids_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestRenderViewContextMenu);
+};
+
 TEST_F(TranslateManagerTest, NormalTranslate) {
   // Simulate navigating to a page.
   SimulateNavigation(GURL("http://www.google.fr"), 0, L"Le Google", "fr");
@@ -682,41 +743,69 @@ TEST_F(TranslateManagerTest, AlwaysTranslateLanguagePref) {
   EXPECT_TRUE(GetTranslateInfoBar() != NULL);
 }
 
-// Tests TranslateManager::ShowInfoBar.
-TEST_F(TranslateManagerTest, ShowInfoBar) {
-  // Simulate navigating to a page and getting its language.
+// Context menu.
+TEST_F(TranslateManagerTest, ContextMenu) {
+  // Simulate navigating to a page in French. The translate menu should show.
   SimulateNavigation(GURL("http://www.google.fr"), 0, L"Le Google", "fr");
-  TranslateInfoBarDelegate* infobar = GetTranslateInfoBar();
-  EXPECT_TRUE(infobar != NULL);
-
-  // ShowInfobar should have no effect since a bar is already showing.
-  EXPECT_FALSE(TranslateManager::ShowInfoBar(contents()));
-  // The infobar should still be showing.
-  EXPECT_EQ(infobar, GetTranslateInfoBar());
+  EXPECT_TRUE(GetTranslateInfoBar() != NULL);
+  scoped_ptr<TestRenderViewContextMenu> menu(
+      TestRenderViewContextMenu::CreateContextMenu(contents()));
+  menu->Init();
+  EXPECT_TRUE(menu->IsItemPresent(IDS_CONTENT_CONTEXT_TRANSLATE));
+  EXPECT_TRUE(menu->TestIsItemCommandEnabled(IDS_CONTENT_CONTEXT_TRANSLATE));
 
-  // Close the infobar.
-  EXPECT_TRUE(CloseTranslateInfoBar());
+  // Use the menu to translate the page.
+  menu->TestExecuteItemCommand(IDS_CONTENT_CONTEXT_TRANSLATE);
 
-  // ShowInfoBar should bring back the infobar.
-  EXPECT_TRUE(TranslateManager::ShowInfoBar(contents()));
-  infobar = GetTranslateInfoBar();
-  ASSERT_TRUE(infobar != NULL);
+  // That should have triggered a translation.
+  int page_id = 0;
+  std::string original_lang, target_lang;
+  EXPECT_TRUE(GetTranslateMessage(&page_id, &original_lang, &target_lang));
+  EXPECT_EQ(0, page_id);
+  EXPECT_EQ("fr", original_lang);
+  EXPECT_EQ("en", target_lang);
+  process()->sink().ClearMessages();
 
-  // Translate.
-  infobar->Translate();
+  // Let's simulate the page being translated.
   rvh()->TestOnMessageReceived(ViewHostMsg_PageTranslated(0, 0, "fr", "en"));
 
-  // Test again that ShowInfobar has no effect since a bar is already showing.
-  EXPECT_FALSE(TranslateManager::ShowInfoBar(contents()));
-  EXPECT_EQ(infobar, GetTranslateInfoBar());
+  // The translate menu should now be disabled.
+  menu.reset(TestRenderViewContextMenu::CreateContextMenu(contents()));
+  menu->Init();
+  EXPECT_TRUE(menu->IsItemPresent(IDS_CONTENT_CONTEXT_TRANSLATE));
+  EXPECT_FALSE(menu->TestIsItemCommandEnabled(IDS_CONTENT_CONTEXT_TRANSLATE));
 
-  // Close the infobar.
-  EXPECT_TRUE(CloseTranslateInfoBar());
+  // Test that selecting translate in the context menu WHILE the page is being
+  // translated does nothing (this could happen if autotranslate kicks-in and
+  // the user selects the menu while the translation is being performed).
+  SimulateNavigation(GURL("http://www.google.es"), 1, L"El Google", "es");
+  TranslateInfoBarDelegate* infobar = GetTranslateInfoBar();
+  ASSERT_TRUE(infobar != NULL);
+  infobar->Translate();
+  EXPECT_TRUE(GetTranslateMessage(&page_id, &original_lang, &target_lang));
+  EXPECT_EQ(1, page_id);
+  process()->sink().ClearMessages();
+  menu.reset(TestRenderViewContextMenu::CreateContextMenu(contents()));
+  menu->Init();
+  EXPECT_TRUE(menu->TestIsItemCommandEnabled(IDS_CONTENT_CONTEXT_TRANSLATE));
+  menu->TestExecuteItemCommand(IDS_CONTENT_CONTEXT_TRANSLATE);
+  // No message expected since the translation should have been ignored.
+  EXPECT_FALSE(GetTranslateMessage(&page_id, &original_lang, &target_lang));
 
-  // ShowInfoBar should bring back the infobar, with the right languages.
-  EXPECT_TRUE(TranslateManager::ShowInfoBar(contents()));
+  // Now test that selecting translate in the context menu AFTER the page has
+  // been translated does nothing.
+  SimulateNavigation(GURL("http://www.google.de"), 2, L"Das Google", "de");
   infobar = GetTranslateInfoBar();
   ASSERT_TRUE(infobar != NULL);
-  EXPECT_EQ("fr", infobar->original_lang_code());
-  EXPECT_EQ("en", infobar->target_lang_code());
+  infobar->Translate();
+  EXPECT_TRUE(GetTranslateMessage(&page_id, &original_lang, &target_lang));
+  EXPECT_EQ(2, page_id);
+  process()->sink().ClearMessages();
+  menu.reset(TestRenderViewContextMenu::CreateContextMenu(contents()));
+  menu->Init();
+  EXPECT_TRUE(menu->TestIsItemCommandEnabled(IDS_CONTENT_CONTEXT_TRANSLATE));
+  rvh()->TestOnMessageReceived(ViewHostMsg_PageTranslated(0, 0, "de", "en"));
+  menu->TestExecuteItemCommand(IDS_CONTENT_CONTEXT_TRANSLATE);
+  // No message expected since the translation should have been ignored.
+  EXPECT_FALSE(GetTranslateMessage(&page_id, &original_lang, &target_lang));
 }