Use a check item instead of changing a menu-item text.

This change changes a menu item "Ask Google for suggestions" to a check item as suggested in privacy review.

BUG=110362
TEST=browser_tests --gtest_filter=SpellingMenuObserverTest.EnableSpellingService

Review URL: http://codereview.chromium.org/9562002

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

chrome/app/generated_resources.grd

@@ -500,9 +500,6 @@ are declared in build/common.gypi.
         <message name="IDS_CONTENT_CONTEXT_SPELLING_BUBBLE_DISABLE" desc="The button text that disallows integrating the spelling service of Google.">
           No thanks
         </message>
-        <message name="IDS_CONTENT_CONTEXT_SPELLING_STOP_ASKING_GOOGLE" desc="The context-menu item that stops integrating the spelling service.">
-          Stop asking Google for suggestions
-        </message>
         <message name="IDS_CONTENT_CONTEXT_SPELLING_CHECKING" desc="The place-holder message shown while the Spelling service is checking text">
           Loading suggestion
         </message>
@@ -709,9 +706,6 @@ are declared in build/common.gypi.
         <message name="IDS_CONTENT_CONTEXT_SPELLING_BUBBLE_DISABLE" desc="In Title Case: The button text that disallows integrating the spelling service of Google.">
           No Thanks
         </message>
-        <message name="IDS_CONTENT_CONTEXT_SPELLING_STOP_ASKING_GOOGLE" desc="The context-menu item that stops integrating the spelling service.">
-          Stop asking Google for suggestions
-        </message>
         <message name="IDS_CONTENT_CONTEXT_SPELLING_CHECKING" desc="The place-holder message shown while the Spelling service is checking text">
           Loading suggestion
         </message>

chrome/browser/tab_contents/render_view_context_menu.cc

@@ -679,6 +679,11 @@ void RenderViewContextMenu::AddMenuItem(int command_id,
   menu_model_.AddItem(command_id, title);
 }
 
+void RenderViewContextMenu::AddCheckItem(int command_id,
+                                         const string16& title) {
+  menu_model_.AddCheckItem(command_id, title);
+}
+
 void RenderViewContextMenu::AddSeparator() {
   menu_model_.AddSeparator();
 }

chrome/browser/tab_contents/render_view_context_menu.h

@@ -96,6 +96,7 @@ class RenderViewContextMenuProxy {
  public:
   // Add a menu item to a context menu.
   virtual void AddMenuItem(int command_id, const string16& title) = 0;
+  virtual void AddCheckItem(int command_id, const string16& title) = 0;
   virtual void AddSeparator() = 0;
 
   // Add a submenu item to a context menu.
@@ -142,6 +143,7 @@ class RenderViewContextMenu : public ui::SimpleMenuModel::Delegate,
 
   // RenderViewContextMenuDelegate implementation.
   virtual void AddMenuItem(int command_id, const string16& title) OVERRIDE;
+  virtual void AddCheckItem(int command_id, const string16& title) OVERRIDE;
   virtual void AddSeparator() OVERRIDE;
   virtual void AddSubMenu(int command_id,
                           const string16& label,

chrome/browser/tab_contents/spellchecker_submenu_observer_hunspell.cc

@@ -70,17 +70,11 @@ void SpellCheckerSubMenuObserver::InitMenu(
       l10n_util::GetStringUTF16(
           IDS_CONTENT_CONTEXT_CHECK_SPELLING_OF_THIS_FIELD));
 
-  // If we have not integrated the spelling service, we show an "Ask Google for
-  // spelling suggestions" item. On the other hand, if we have integrated the
-  // spelling service, we show "Stop asking Google for spelling suggestions"
-  // item.
-  bool integrate_spelling_service =
-      profile->GetPrefs()->GetBoolean(prefs::kSpellCheckUseSpellingService);
-  int spelling_message = integrate_spelling_service ?
-      IDS_CONTENT_CONTEXT_SPELLING_STOP_ASKING_GOOGLE :
-      IDS_CONTENT_CONTEXT_SPELLING_ASK_GOOGLE;
+  // Add a check item "Ask Google for spelling suggestions" item. (This class
+  // does not handle this item because the SpellingMenuObserver class handles it
+  // on behalf of this class.)
   submenu_model_.AddCheckItem(IDC_CONTENT_CONTEXT_SPELLING_TOGGLE,
-                              l10n_util::GetStringUTF16(spelling_message));
+      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_SPELLING_ASK_GOOGLE));
 
   proxy_->AddSubMenu(
       IDC_SPELLCHECK_MENU,

chrome/browser/tab_contents/spelling_menu_observer.cc

@@ -107,9 +107,8 @@ void SpellingMenuObserver::InitMenu(const content::ContextMenuParams& params) {
       spellcheck_host->GetMetrics()->RecordSuggestionStats(1);
   }
 
-  // If word is misspelled, give option for "Add to dictionary" and "Ask Google
-  // for suggestions". (The SpellCheckerSubMenuObserver class handles the "Ask
-  // Goole for suggestions" item so this class does not have to handle it.)
+  // If word is misspelled, give option for "Add to dictionary" and a check item
+  // "Ask Google for suggestions".
   if (!params.misspelled_word.empty()) {
     if (params.dictionary_suggestions.empty()) {
       proxy_->AddMenuItem(IDC_CONTENT_CONTEXT_NO_SPELLING_SUGGESTIONS,
@@ -122,11 +121,8 @@ void SpellingMenuObserver::InitMenu(const content::ContextMenuParams& params) {
 
     integrate_spelling_service_ =
         profile->GetPrefs()->GetBoolean(prefs::kSpellCheckUseSpellingService);
-    int spelling_message = integrate_spelling_service_ ?
-        IDS_CONTENT_CONTEXT_SPELLING_STOP_ASKING_GOOGLE :
-        IDS_CONTENT_CONTEXT_SPELLING_ASK_GOOGLE;
-    proxy_->AddMenuItem(IDC_CONTENT_CONTEXT_SPELLING_TOGGLE,
-                        l10n_util::GetStringUTF16(spelling_message));
+    proxy_->AddCheckItem(IDC_CONTENT_CONTEXT_SPELLING_TOGGLE,
+        l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_SPELLING_ASK_GOOGLE));
 
     proxy_->AddSeparator();
   }
@@ -150,6 +146,14 @@ bool SpellingMenuObserver::IsCommandIdSupported(int command_id) {
   return false;
 }
 
+bool SpellingMenuObserver::IsCommandIdChecked(int command_id) {
+  DCHECK(IsCommandIdSupported(command_id));
+
+  if (command_id == IDC_CONTENT_CONTEXT_SPELLING_TOGGLE)
+    return integrate_spelling_service_;
+  return false;
+}
+
 bool SpellingMenuObserver::IsCommandIdEnabled(int command_id) {
   DCHECK(IsCommandIdSupported(command_id));
 
@@ -219,10 +223,10 @@ void SpellingMenuObserver::ExecuteCommand(int command_id) {
   }
 
   if (command_id == IDC_CONTENT_CONTEXT_SPELLING_TOGGLE) {
-    // When a user chooses the "Ask Google for spelling suggestions" item, we
-    // show a bubble to confirm it. On the other hand, when a user chooses the
-    // "Stop asking Google for spelling suggestions" item, we directly update
-    // the profile and stop integrating the spelling service immediately.
+    // When a user enables the "Ask Google for spelling suggestions" item, we
+    // show a bubble to confirm it. On the other hand, when a user disables this
+    // item, we directly update/ the profile and stop integrating the spelling
+    // service immediately.
     if (!integrate_spelling_service_) {
       RenderViewHost* rvh = proxy_->GetRenderViewHost();
       gfx::Rect rect = rvh->view()->GetViewBounds();

chrome/browser/tab_contents/spelling_menu_observer.h

@@ -45,6 +45,7 @@ class SpellingMenuObserver : public RenderViewContextMenuObserver {
   // RenderViewContextMenuObserver implementation.
   virtual void InitMenu(const content::ContextMenuParams& params) OVERRIDE;
   virtual bool IsCommandIdSupported(int command_id) OVERRIDE;
+  virtual bool IsCommandIdChecked(int command_id) OVERRIDE;
   virtual bool IsCommandIdEnabled(int command_id) OVERRIDE;
   virtual void ExecuteCommand(int command_id) OVERRIDE;
 

chrome/browser/tab_contents/spelling_menu_observer_browsertest.cc

@@ -8,8 +8,10 @@
 
 #include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/tab_contents/render_view_context_menu.h"
 #include "chrome/browser/tab_contents/render_view_context_menu_observer.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/testing_profile.h"
 
@@ -23,8 +25,15 @@ class MockRenderViewContextMenu : public RenderViewContextMenuProxy {
   // A menu item used in this test. This test uses a vector of this struct to
   // hold menu items added by this test.
   struct MockMenuItem {
+    MockMenuItem()
+        : command_id(0),
+          enabled(false),
+          checked(false),
+          hidden(true) {
+    }
     int command_id;
     bool enabled;
+    bool checked;
     bool hidden;
     string16 title;
   };
@@ -34,6 +43,7 @@ class MockRenderViewContextMenu : public RenderViewContextMenuProxy {
 
   // RenderViewContextMenuProxy implementation.
   virtual void AddMenuItem(int command_id, const string16& title) OVERRIDE;
+  virtual void AddCheckItem(int command_id, const string16& title) OVERRIDE;
   virtual void AddSeparator() OVERRIDE;
   virtual void AddSubMenu(int command_id,
                           const string16& label,
@@ -85,6 +95,18 @@ void MockRenderViewContextMenu::AddMenuItem(int command_id,
   MockMenuItem item;
   item.command_id = command_id;
   item.enabled = observer_->IsCommandIdEnabled(command_id);
+  item.checked = false;
+  item.hidden = false;
+  item.title = title;
+  items_.push_back(item);
+}
+
+void MockRenderViewContextMenu::AddCheckItem(int command_id,
+                                             const string16& title) {
+  MockMenuItem item;
+  item.command_id = command_id;
+  item.enabled = observer_->IsCommandIdEnabled(command_id);
+  item.checked = observer_->IsCommandIdChecked(command_id);
   item.hidden = false;
   item.title = title;
   items_.push_back(item);
@@ -94,6 +116,7 @@ void MockRenderViewContextMenu::AddSeparator() {
   MockMenuItem item;
   item.command_id = -1;
   item.enabled = false;
+  item.checked = false;
   item.hidden = false;
   items_.push_back(item);
 }
@@ -104,6 +127,7 @@ void MockRenderViewContextMenu::AddSubMenu(int command_id,
   MockMenuItem item;
   item.command_id = -1;
   item.enabled = false;
+  item.checked = false;
   item.hidden = false;
   items_.push_back(item);
 }
@@ -145,6 +169,7 @@ bool MockRenderViewContextMenu::GetMenuItem(size_t i,
     return false;
   item->command_id = items_[i].command_id;
   item->enabled = items_[i].enabled;
+  item->checked = items_[i].checked;
   item->hidden = items_[i].hidden;
   item->title = items_[i].title;
   return true;
@@ -222,9 +247,37 @@ IN_PROC_BROWSER_TEST_F(SpellingMenuObserverTest, InitMenuWithMisspelledWord) {
   menu->GetMenuItem(2, &item);
   EXPECT_EQ(IDC_CONTENT_CONTEXT_SPELLING_TOGGLE, item.command_id);
   EXPECT_TRUE(item.enabled);
+  EXPECT_FALSE(item.checked);
   EXPECT_FALSE(item.hidden);
   menu->GetMenuItem(3, &item);
   EXPECT_EQ(-1, item.command_id);
   EXPECT_FALSE(item.enabled);
   EXPECT_FALSE(item.hidden);
 }
+
+// Tests that right-clicking a misspelled word when we enable spelling-service
+// integration to verify an item "Ask Google for suggestions" is checked. (This
+// test does not actually send JSON-RPC requests to the service because it makes
+// this test flaky.)
+IN_PROC_BROWSER_TEST_F(SpellingMenuObserverTest, EnableSpellingService) {
+  scoped_ptr<MockRenderViewContextMenu> menu(new MockRenderViewContextMenu);
+  scoped_ptr<SpellingMenuObserver> observer(
+      new SpellingMenuObserver(menu.get()));
+  menu->SetObserver(observer.get());
+  menu->GetPrefs()->SetBoolean(prefs::kSpellCheckUseSpellingService, true);
+
+  content::ContextMenuParams params;
+  params.is_editable = true;
+  params.misspelled_word = ASCIIToUTF16("wiimode");
+  observer->InitMenu(params);
+  EXPECT_EQ(static_cast<size_t>(4), menu->GetMenuSize());
+
+  // To avoid duplicates, this test reads only the "Ask Google for suggestions"
+  // item and verifies it is enabled and checked.
+  MockRenderViewContextMenu::MockMenuItem item;
+  menu->GetMenuItem(2, &item);
+  EXPECT_EQ(IDC_CONTENT_CONTEXT_SPELLING_TOGGLE, item.command_id);
+  EXPECT_TRUE(item.enabled);
+  EXPECT_TRUE(item.checked);
+  EXPECT_FALSE(item.hidden);
+}