[win/cros/lin] Add back the spellcheck menu.

chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/renderer_context_menu/spelling_options_submenu_observer.h"
+
+#include "base/logging.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/spellchecker/spellcheck_service.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/grit/generated_resources.h"
+#include "components/prefs/pref_member.h"
+#include "components/prefs/pref_service.h"
+#include "components/renderer_context_menu/render_view_context_menu_proxy.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/menu_separator_types.h"
+
+using content::BrowserThread;
+
+SpellingOptionsSubMenuObserver::SpellingOptionsSubMenuObserver(
+    RenderViewContextMenuProxy* proxy,
+    ui::SimpleMenuModel::Delegate* delegate,
+    int group_id)
+    : proxy_(proxy),
+      submenu_model_(delegate),
+      language_group_id_(group_id),
+      num_selected_languages_(0) {
+  DCHECK(proxy_);
+}
+
+SpellingOptionsSubMenuObserver::~SpellingOptionsSubMenuObserver() {}
+
+void SpellingOptionsSubMenuObserver::InitMenu(
+    const content::ContextMenuParams& params) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // Add available spell-checker languages to the sub menu.
+  content::BrowserContext* browser_context = proxy_->GetBrowserContext();
+  DCHECK(browser_context);
+  num_selected_languages_ =
+      SpellcheckService::GetSpellCheckLanguages(browser_context, &languages_);
+  DCHECK(languages_.size() <
+         IDC_SPELLCHECK_LANGUAGES_LAST - IDC_SPELLCHECK_LANGUAGES_FIRST);
+  const std::string app_locale = g_browser_process->GetApplicationLocale();
+
+  if (languages_.size() > 1) {
+    submenu_model_.AddRadioItemWithStringId(
+        IDC_SPELLCHECK_MULTI_LINGUAL,
+        IDS_CONTENT_CONTEXT_SPELLCHECK_MULTI_LINGUAL, language_group_id_);
+  }
+
+  const size_t kMaxLanguages = static_cast<size_t>(
+      IDC_SPELLCHECK_LANGUAGES_FIRST - IDC_SPELLCHECK_LANGUAGES_LAST);
+  for (size_t i = 0; i < languages_.size() && i < kMaxLanguages; ++i) {

[groby-ooo-7-16, 2016/02/04 18:59:10]

We've DCHECKED above that i will be below kMaxLanguages, so I'd suggest a C++11
loop

[please use gerrit instead, 2016/02/05 00:14:13]

I need "i" to calculate "IDC_SPELLCHECK_LANGUAGES_FIRST + i". I don't see a way
to calculate this inside of a C++11 loop. Did I miss something?

As for kMaxLanguages, I'm checking it at the suggestion of the owner who
reviewed this code:

"Should we skip items beyond 100 if languages_.size() > 100? It's better to keep
a deterministic behavior imo. I understand this is totally impractical though."

+    submenu_model_.AddRadioItem(
+        IDC_SPELLCHECK_LANGUAGES_FIRST + i,
+        l10n_util::GetDisplayNameForLocale(languages_[i], app_locale, true),
+        language_group_id_);
+  }
+
+  // Add an item that opens the 'fonts and languages options' page.
+  submenu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
+  submenu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_LANGUAGE_SETTINGS,
+                                     IDS_CONTENT_CONTEXT_LANGUAGE_SETTINGS);
+
+  if (num_selected_languages_ > 0) {
+    // Add a 'Check spelling while typing' item in the sub menu.

[groby-ooo-7-16, 2016/02/04 18:59:10]

This should be greyed out if no languages are selected, not removed. (In
general, it's bad form to have context menu items that only appear sometimes,
given the same context)

[please use gerrit instead, 2016/02/05 00:14:13]

========

1) If the user sees the following menu, I think it's clear that one of the
language radio buttons need to be selected. ("◯" is deselected radio button.)

CURRENT:

|Spellcheck ► || ◯ All languages                 |
               | ◯ English (United States)       |
               | ◯ Spanish                       |
               |---------------------------------|
               |   Settings...                   |
               |   Ask Google for suggestions    |

I'm afraid that graying out "Check spelling of text fields" can be confusing for
someone who unchecked all languages in chrome://settings/languages:

PROPOSED:

|Spellcheck ► || ◯ All languages                 |
               | ◯ English (United States)       |
               | ◯ Spanish                       |
               |---------------------------------|
               |   Settings...                   |
               | ✓ Check spelling of text fields | <-- GRAYED OUT
               |   Ask Google for suggestions    |

"Check spelling of text fields" is grayed out and checked, but spellcheck is not
working. The user needs to select one of the radio buttons above, but that's not
immediately clear. 

========

2) Unchecking "Check spelling of text fields" currently disables "All
languages", "English (United States)", and "Spanish" radio buttons. This way,
the 
user has only one possible action: select the "Check spelling of text fields"
menu item.

CURRENT:

|Spellcheck ► || ◯ All languages                 | <-- GRAYED OUT
               | ◯ English (United States)       | <-- GRAYED OUT
               | ◯ Spanish                       | <-- GRAYED OUT
               |---------------------------------|
               |   Settings...                   |
               |   Check spelling of text fields |
               |   Ask Google for suggestions    |

If deselecting all languages grays out "Check spelling of text fields" and vice
versa, then the user might get in the state where all of the items are grayed
out. Only chrome://settings/languages can fix this state.

PROPOSED:

|Spellcheck ► || ◯ All languages                 | <-- GRAYED OUT
               | ◯ English (United States)       | <-- GRAYED OUT
               | ◯ Spanish                       | <-- GRAYED OUT
               |---------------------------------|
               |   Settings...                   |
               |   Check spelling of text fields | <-- GRAYED OUT
               |   Ask Google for suggestions    |

It's not immediately clear to the user that clicking "Settings..." is necessary
to enable spellcheck.

========

Note that "Enable spellcheck" checkbox in chrome://settings/languages is never
grayed out or hidden.

+    submenu_model_.AddCheckItem(
+        IDC_CHECK_SPELLING_WHILE_TYPING,
+        l10n_util::GetStringUTF16(
+            IDS_CONTENT_CONTEXT_CHECK_SPELLING_WHILE_TYPING));
+  }
+
+  // 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.)

[groby-ooo-7-16, 2016/02/04 18:59:10]

Would it make sense to handle it here instead?

[please use gerrit instead, 2016/02/05 00:14:13]

This class is not used on Mac, which also has "Ask Google for suggestions" menu
item. SpellingMenuObserver is used on all desktop platforms, so it makes sense
to handle "Ask Google for suggestions" in there.

+  submenu_model_.AddCheckItem(
+      IDC_CONTENT_CONTEXT_SPELLING_TOGGLE,
+      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_SPELLING_ASK_GOOGLE));
+
+  proxy_->AddSubMenu(
+      IDC_SPELLCHECK_MENU,
+      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_SPELLCHECK_MENU),
+      &submenu_model_);
+}
+
+bool SpellingOptionsSubMenuObserver::IsCommandIdSupported(int command_id) {
+  // Allow Spell Check language items on sub menu for text area context menu.
+  if (command_id >= IDC_SPELLCHECK_LANGUAGES_FIRST &&
+      command_id < IDC_SPELLCHECK_LANGUAGES_LAST) {

[groby-ooo-7-16, 2016/02/04 18:59:10]

Should this gate on the number of languages in |languages_|?

[please use gerrit instead, 2016/02/05 00:14:13]

I would like to avoid the extra calculation of "IDC_SPELLCHECK_LANGUAGES_FIRST +
languages_.size()" in production code, but a DCHECK() for that condition is a
good idea. Added the DCHECK().

+    return true;
+  }
+
+  switch (command_id) {
+    case IDC_CONTENT_CONTEXT_LANGUAGE_SETTINGS:
+      // Return false so RenderViewContextMenu can handle this item because it
+      // is hard for this class to handle it.

[groby-ooo-7-16, 2016/02/04 18:59:10]

No excuses! ;) Why is it hard?

[please use gerrit instead, 2016/02/05 00:14:13]

Agreed that this is worded badly. This is similar to
IDC_CONTENT_CONTEXT_SPELLING_TOGGLE, which is handled in the top-level context
menu (not this sub menu) for cross platform reasons. Mac has this menu item in
the top-level context menu, which is where it's handled. The
spelling_options_submenu_observer.cc file is not used on Mac.

Note that IDC_CONTENT_CONTEXT_LANGUAGE_SETTINGS is handled in
RenderViewContextMenu instead of SpellingMenuObserver, because it is
RenderViewContextMenu that opens up new tabs.

The best option is to actually remove this whole case and fall through to
returning false at the end of the method.

+      return false;
+
+    case IDC_CHECK_SPELLING_WHILE_TYPING:
+    case IDC_SPELLCHECK_MENU:
+    case IDC_SPELLCHECK_MULTI_LINGUAL:
+      return true;
+  }
+
+  return false;
+}
+
+bool SpellingOptionsSubMenuObserver::IsCommandIdChecked(int command_id) {
+  DCHECK(IsCommandIdSupported(command_id));
+
+  if (command_id == IDC_SPELLCHECK_MULTI_LINGUAL)
+    return num_selected_languages_ == languages_.size();
+
+  if (command_id >= IDC_SPELLCHECK_LANGUAGES_FIRST &&
+      command_id < IDC_SPELLCHECK_LANGUAGES_LAST) {
+    if (languages_.size() == 1 && num_selected_languages_ == 1)
+      return true;
+
+    if (languages_.size() == num_selected_languages_)
+      return false;

[groby-ooo-7-16, 2016/02/04 18:59:10]

You might combine with previous if...
if (size == num) return (size == 1);

But it's a personal preference. 

[please use gerrit instead, 2016/02/05 00:14:13]

Done.

+
+    size_t language_index =
+        static_cast<size_t>(command_id - IDC_SPELLCHECK_LANGUAGES_FIRST);
+    // The first |num_selected_languages_| are used for spellchecking.
+    return num_selected_languages_ > language_index;
+  }
+
+  // Check box for 'Check Spelling while typing'.
+  if (command_id == IDC_CHECK_SPELLING_WHILE_TYPING) {
+    Profile* profile = Profile::FromBrowserContext(proxy_->GetBrowserContext());
+    return profile->GetPrefs()->GetBoolean(prefs::kEnableContinuousSpellcheck);
+  }
+
+  return false;
+}
+
+bool SpellingOptionsSubMenuObserver::IsCommandIdEnabled(int command_id) {
+  DCHECK(IsCommandIdSupported(command_id));
+
+  Profile* profile = Profile::FromBrowserContext(proxy_->GetBrowserContext());
+  DCHECK(profile);
+  const PrefService* pref = profile->GetPrefs();
+  if ((command_id >= IDC_SPELLCHECK_LANGUAGES_FIRST &&
+       command_id < IDC_SPELLCHECK_LANGUAGES_LAST) ||
+      command_id == IDC_SPELLCHECK_MULTI_LINGUAL) {
+    return pref->GetBoolean(prefs::kEnableContinuousSpellcheck);
+  }
+
+  switch (command_id) {
+    case IDC_CHECK_SPELLING_WHILE_TYPING:
+    case IDC_SPELLCHECK_MENU:
+      return true;
+  }
+
+  return false;
+}
+
+void SpellingOptionsSubMenuObserver::ExecuteCommand(int command_id) {
+  DCHECK(IsCommandIdSupported(command_id));
+
+  // Check to see if one of the spell check language ids have been clicked.
+  Profile* profile = Profile::FromBrowserContext(proxy_->GetBrowserContext());
+  DCHECK(profile);
+
+  if (command_id >= IDC_SPELLCHECK_LANGUAGES_FIRST &&
+      static_cast<size_t>(command_id) <
+          IDC_SPELLCHECK_LANGUAGES_FIRST + languages_.size()) {
+    size_t language_index = command_id - IDC_SPELLCHECK_LANGUAGES_FIRST;
+    StringListPrefMember dictionaries_pref;
+    dictionaries_pref.Init(prefs::kSpellCheckDictionaries, profile->GetPrefs());
+    dictionaries_pref.SetValue({languages_[language_index]});
+    return;
+  }
+
+  switch (command_id) {
+    case IDC_CHECK_SPELLING_WHILE_TYPING:
+      profile->GetPrefs()->SetBoolean(
+          prefs::kEnableContinuousSpellcheck,
+          !profile->GetPrefs()->GetBoolean(prefs::kEnableContinuousSpellcheck));
+      break;
+
+    case IDC_SPELLCHECK_MULTI_LINGUAL:
+      StringListPrefMember dictionaries_pref;
+      dictionaries_pref.Init(prefs::kSpellCheckDictionaries,
+                             profile->GetPrefs());
+      dictionaries_pref.SetValue(languages_);
+      break;
+  }
+}