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

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

This patch adds back the "Spellcheck" submenu to the context menu on
Windows, ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also adds
support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck in
all of the languages that are listed in the submenu. The language list
can be modified in chrome://settings/languages.

TBR=brettw@chromium.org
BUG=544234
Committed: https://crrev.com/e59900a51f10891ee99368eda83ddec185f2b007
Cr-Commit-Position: refs/heads/master@{#375955}

[please use gerrit instead, 2016-01-27 23:24:20]

rouslan@chromium.org changed reviewers:
+ groby@chromium.org

[please use gerrit instead, 2016-01-27 23:24:20]

Rachel, PTAL.

Video of the submenu operation here:

https://drive.google.com/a/google.com/file/d/0BwnAxYoFTeYPUGNGZFJOamhYZGM/vie...

[please use gerrit instead, 2016-01-27 23:29:27]

Istiaque, ptal chrome/browser/renderer_context_menu/.

Most of this code is a revert of
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144. I am adding
"Multilingual spell-checker" submenu item in spellchecker_submenu_observer.cc,
though.

Video of the submenu operation here:
https://drive.google.com/a/google.com/file/d/0BwnAxYoFTeYPUGNGZFJOamhYZGM/vie...

[please use gerrit instead, 2016-01-27 23:29:42]

rouslan@chromium.org changed reviewers:
+ lazyboy@chromium.org

[please use gerrit instead, 2016-01-27 23:29:42]

Istiaque, ptal chrome/browser/renderer_context_menu/.

Most of this code is a revert of
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144. I am adding
"Multilingual spell-checker" submenu item in spellchecker_submenu_observer.cc,
though.

Video of the submenu operation here:
https://drive.google.com/a/google.com/file/d/0BwnAxYoFTeYPUGNGZFJOamhYZGM/vie...

[please use gerrit instead, 2016-01-27 23:31:46]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2016-01-27 23:32:38]

rouslan@chromium.org changed reviewers:
+ isherman@chromium.org

[please use gerrit instead, 2016-01-27 23:32:39]

Ilya, ptal histograms.xml

[please use gerrit instead, 2016-01-27 23:33:55]

rouslan@chromium.org changed reviewers:
+ grt@chromium.org

[please use gerrit instead, 2016-01-27 23:33:55]

Greg, ptal chrome_command_ids.h

[Ilya Sherman, 2016-01-28 01:39:19]

histograms.xml lgtm

[grt (UTC plus 2), 2016-01-28 13:56:17]

chrome/app/chrome_command_ids.h lgtm

[lazyboy, 2016-01-28 20:36:25]

I haven't finished looking at the whole CL yet, sending some comments.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1238: if
(!spellchecker_submenu_observer_.get()) {
.get() is unnecessary.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1248: void
RenderViewContextMenu::AppendSpellcheckSuggestionItems() {
I always get confused by Spelling*Menu and Spellchecker*Menu stuff when I'm
reading this code...

btw, this method should have a "SubMenu" suffix like the other ones that add a
sub-menu item to RVCM (opposed to adding multiple items).

can we rename |spelling_menu_observer_| to
|spellcheck_suggestions_menu_observer_|, or something you think would be
clearer?

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc
(right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:47:
submenu_model_.AddRadioItemWithStringId(
Do we not want to guard this with >= 2 languages?

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:60: for
(size_t i = 0; i < languages_.size(); ++i) {
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.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:132:
DCHECK(profile);
This is probably not necessary, we'll crash on the next line anyway if profile
is NULL.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:167: if
(command_id == IDC_SPELLCHECK_MULTI_LINGUAL) {
Can we fold this into the switch below on line 186?

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:170:
dictionaries_pref.SetValue(languages_);
return?

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:175:
size_t language = command_id - IDC_SPELLCHECK_LANGUAGES_FIRST;
language_index

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h
(right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:17: // A
class that implements the 'spell-checker options' submenu. This class
I would call it so: SpellcheckerOptionsSubMenuObserver, also it'd be nice to be
consistent with Spellchecker vs SpellChecker in the CL.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:23: int
group);
group_id

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:38: //
submenu and add it to the parent menu.
nit: adds

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:41: // The
radio items representing languages available for spellchecking.
The id of radio items...

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:42: int
language_group_;
language_group_id_

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc
(right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc:145:
EXPECT_TRUE(menu->IsCommandIdChecked(IDC_CHECK_SPELLING_WHILE_TYPING));
Remove.

https://codereview.chromium.org/1647723002/diff/20001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1039: # Desktop Linux, ChromeOS, and Windows.
Android and ios?

https://codereview.chromium.org/1647723002/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:392:
'browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc',
Do you not need to guard this as well from mac?

[please use gerrit instead, 2016-01-28 21:42:00]

Rachel & Istiaque, ptal patch 4.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1238: if
(!spellchecker_submenu_observer_.get()) {
On 2016/01/28 20:36:24, lazyboy wrote:
> .get() is unnecessary.

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1248: void
RenderViewContextMenu::AppendSpellcheckSuggestionItems() {
On 2016/01/28 20:36:24, lazyboy wrote:
> I always get confused by Spelling*Menu and Spellchecker*Menu stuff when I'm
> reading this code...

Changed everything to use the word "Spelling" instead of "Spellchecker".

> btw, this method should have a "SubMenu" suffix like the other ones that add a
> sub-menu item to RVCM (opposed to adding multiple items).

This method does not involve the submenu. It adds the spelling suggestions at
the top of the context menu. Suppose you typed "helo", then this method adds
these items at the top of the context menu:

   --------------------------
   hello
   Add to dictionary
   Ask Google for suggestions
   --------------------------


So the suffix should be "Items", not "SubMenu". This is similar to the naming of
AppendPasswordItems() method below, for example.

> can we rename |spelling_menu_observer_| to
> |spellcheck_suggestions_menu_observer_|, or something you think would be
> clearer?

Renamed "spelling_menu_observer" to "spelling_suggestions_menu_observer".

Renamed "spellchecker_submenu_obvserver" to "spelling_options_submenu_observer".

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc
(right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:47:
submenu_model_.AddRadioItemWithStringId(
On 2016/01/28 20:36:24, lazyboy wrote:
> Do we not want to guard this with >= 2 languages?

Done in patch 2.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:60: for
(size_t i = 0; i < languages_.size(); ++i) {
On 2016/01/28 20:36:25, lazyboy wrote:
> 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.

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:132:
DCHECK(profile);
On 2016/01/28 20:36:25, lazyboy wrote:
> This is probably not necessary, we'll crash on the next line anyway if profile
> is NULL.

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:167: if
(command_id == IDC_SPELLCHECK_MULTI_LINGUAL) {
On 2016/01/28 20:36:25, lazyboy wrote:
> Can we fold this into the switch below on line 186?

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:170:
dictionaries_pref.SetValue(languages_);
On 2016/01/28 20:36:25, lazyboy wrote:
> return?

using "break" inside of the switch.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.cc:175:
size_t language = command_id - IDC_SPELLCHECK_LANGUAGES_FIRST;
On 2016/01/28 20:36:25, lazyboy wrote:
> language_index

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h
(right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:17: // A
class that implements the 'spell-checker options' submenu. This class
On 2016/01/28 20:36:25, lazyboy wrote:
> I would call it so: SpellcheckerOptionsSubMenuObserver, also it'd be nice to
be
> consistent with Spellchecker vs SpellChecker in the CL.

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:23: int
group);
On 2016/01/28 20:36:25, lazyboy wrote:
> group_id

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:38: //
submenu and add it to the parent menu.
On 2016/01/28 20:36:25, lazyboy wrote:
> nit: adds

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:41: // The
radio items representing languages available for spellchecking.
On 2016/01/28 20:36:25, lazyboy wrote:
> The id of radio items...

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:42: int
language_group_;
On 2016/01/28 20:36:25, lazyboy wrote:
> language_group_id_

Done.

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc
(right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc:145:
EXPECT_TRUE(menu->IsCommandIdChecked(IDC_CHECK_SPELLING_WHILE_TYPING));
On 2016/01/28 20:36:25, lazyboy wrote:
> Remove.

Why?

https://codereview.chromium.org/1647723002/diff/20001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1039: # Desktop Linux, ChromeOS, and Windows.
On 2016/01/28 20:36:25, lazyboy wrote:
> Android and ios?

Android and iOS do not have this submenu. I am indicating it by keyword
"desktop" in the group name.

https://codereview.chromium.org/1647723002/diff/20001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:3914: ['OS=="linux" or OS=="win"', {
Note that OS="linux" is also true for ChromeOS, but not true for Android,
although both of those are based on Linux.

https://codereview.chromium.org/1647723002/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1647723002/diff/20001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:392:
'browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc',
On 2016/01/28 20:36:25, lazyboy wrote:
> Do you not need to guard this as well from mac?

Done.

[groby-ooo-7-16, 2016-01-28 22:49:29]

https://codereview.chromium.org/1647723002/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1647723002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:993: +        <if expr="use_titlecase">
Do we ever use titlecase outside of OSX? If not (I don't think we do), kill
use_titlecase

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1231: #if
defined(USE_BROWSER_SPELLCHECKER)
MumbleGrumble. It's theoretically not related to using a browser-side
spellchecker, but specifically the OSX checker.

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc
(right):

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:32: :
proxy_(proxy),
Hm. I wonder if it's a better idea to

a) pass this in during InitMenu, where it's actually needed
b) not store it as a member, but store the profile as a member instead.

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:130:
return num_selected_languages_ >
What is this test supposed to achieve? Comment, please?

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:172:
command_id < IDC_SPELLCHECK_LANGUAGES_LAST) {
Do you want to dynamically adjust to LANGUAGES_FIRST + languages_.size()?

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:179:
std::vector<std::string>(1, languages_[language_index]));
Sneaky! ;) (The use of the initializer, that is)

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc
(right):

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc:24:
class MockRenderViewContextMenu : public ui::SimpleMenuModel::Delegate,
That looks somewhat similar to the one in spelling_menu_observer_browsertest.cc
- can you hoist this out?

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc:150:
for (const auto& dict : dictionaries) {
Why not  AppendStrings(dictionaries) ?

https://codereview.chromium.org/1647723002/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1647723002/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1042: 'chrome_browser_non_mac_desktop_sources': [
Eeew. Shouldn't we treat this as feature-based source? (I'm fine with this, I
just find the splitting by platforms we're doing here kind of appalling. )

[please use gerrit instead, 2016-01-29 20:09:35]

Rachel & Istiaque, ptal patch 6.

https://codereview.chromium.org/1647723002/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1647723002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:993: +        <if expr="use_titlecase">
On 2016/01/28 22:49:28, groby wrote:
> Do we ever use titlecase outside of OSX? If not (I don't think we do), kill
> use_titlecase

Done.

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1231: #if
defined(USE_BROWSER_SPELLCHECKER)
On 2016/01/28 22:49:28, groby wrote:
> MumbleGrumble. It's theoretically not related to using a browser-side
> spellchecker, but specifically the OSX checker.

OK. Using the OS_MACOSX macro.

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc
(right):

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:32: :
proxy_(proxy),
Both good ideas, but implementing the RenderViewContextMenuObserver interface
puts some constraints on us.

On 2016/01/28 22:49:28, groby wrote:
> Hm. I wonder if it's a better idea to
> 
> a) pass this in during InitMenu, where it's actually needed

InitMenu() is an ovveride of a method in RenderViewContextMenuObserver, so I
cannot change its signature. 

> b) not store it as a member, but store the profile as a member instead.

Need to store proxy as a member to call proxy__>AddSubMenu() from InitMenu().

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:130:
return num_selected_languages_ >
On 2016/01/28 22:49:28, groby wrote:
> What is this test supposed to achieve? Comment, please?

Commented and pulled out "command_id - IDC_SPELLCHECK_LANGUAGES_FIRST" into
"language_index" variable. I hope this will clarify. The comment is:

 // The first |num_selected_languages_| are used for spellchecking.

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:172:
command_id < IDC_SPELLCHECK_LANGUAGES_LAST) {
On 2016/01/28 22:49:28, groby wrote:
> Do you want to dynamically adjust to LANGUAGES_FIRST + languages_.size()?

Done.

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:179:
std::vector<std::string>(1, languages_[language_index]));
On 2016/01/28 22:49:28, groby wrote:
> Sneaky! ;) (The use of the initializer, that is)

Now that you mention it, we can start using initializer list here, like the rest
of the Chrome code.

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc
(right):

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc:24:
class MockRenderViewContextMenu : public ui::SimpleMenuModel::Delegate,
On 2016/01/28 22:49:28, groby wrote:
> That looks somewhat similar to the one in
spelling_menu_observer_browsertest.cc
> - can you hoist this out?

Done.

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc:150:
for (const auto& dict : dictionaries) {
On 2016/01/28 22:49:28, groby wrote:
> Why not  AppendStrings(dictionaries) ?

Done.

https://codereview.chromium.org/1647723002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc:229:
ExpectPreferences(true, {"en-US"});
Removed use of initializer lists, which are not allowed in Chrome yet.

https://chromium-cpp.appspot.com/

https://codereview.chromium.org/1647723002/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1647723002/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1042: 'chrome_browser_non_mac_desktop_sources': [
On 2016/01/28 22:49:28, groby wrote:
> Eeew. Shouldn't we treat this as feature-based source? (I'm fine with this, I
> just find the splitting by platforms we're doing here kind of appalling. )

We're hiding the scoped_ptr<SpellingOptionsSubMenuObserver> member variable
inside of defined(OS_MACOSX), so let's use the same terminology in the gyp file.

[lazyboy, 2016-02-01 19:23:33]

Mostly looks good from my side. Some comments.

https://chromiumcodereview.appspot.com/1647723002/diff/20001/chrome/browser/r...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/1647723002/diff/20001/chrome/browser/r...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1248: void
RenderViewContextMenu::AppendSpellcheckSuggestionItems() {
On 2016/01/28 21:41:59, Rouslan wrote:
> On 2016/01/28 20:36:24, lazyboy wrote:
> > I always get confused by Spelling*Menu and Spellchecker*Menu stuff when I'm
> > reading this code...
> 
> Changed everything to use the word "Spelling" instead of "Spellchecker".
> 
> > btw, this method should have a "SubMenu" suffix like the other ones that add
a
> > sub-menu item to RVCM (opposed to adding multiple items).
> 
> This method does not involve the submenu. It adds the spelling suggestions at
> the top of the context menu. Suppose you typed "helo", then this method adds
> these items at the top of the context menu:
> 
>    --------------------------
>    hello
>    Add to dictionary
>    Ask Google for suggestions
>    --------------------------
> 
> 
> So the suffix should be "Items", not "SubMenu". This is similar to the naming
of
> AppendPasswordItems() method below, for example.
Got it, thanks.
> 
> > can we rename |spelling_menu_observer_| to
> > |spellcheck_suggestions_menu_observer_|, or something you think would be
> > clearer?
> 
> Renamed "spelling_menu_observer" to "spelling_suggestions_menu_observer".
> 
> Renamed "spellchecker_submenu_obvserver" to
"spelling_options_submenu_observer".

Ty.

https://chromiumcodereview.appspot.com/1647723002/diff/20001/chrome/browser/r...
File
chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/1647723002/diff/20001/chrome/browser/r...
chrome/browser/renderer_context_menu/spellchecker_submenu_observer_browsertest.cc:145:
EXPECT_TRUE(menu->IsCommandIdChecked(IDC_CHECK_SPELLING_WHILE_TYPING));
On 2016/01/28 21:42:00, Rouslan wrote:
> On 2016/01/28 20:36:25, lazyboy wrote:
> > Remove.
> 
> Why?

My bad, I thought they were both the same :)

https://chromiumcodereview.appspot.com/1647723002/diff/20001/chrome/chrome_br...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/1647723002/diff/20001/chrome/chrome_br...
chrome/chrome_browser.gypi:3914: ['OS=="linux" or OS=="win"', {
On 2016/01/28 21:42:00, Rouslan wrote:
> Note that OS="linux" is also true for ChromeOS, but not true for Android,
> although both of those are based on Linux.

Acknowledged.

https://chromiumcodereview.appspot.com/1647723002/diff/80001/chrome/browser/r...
File
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/1647723002/diff/80001/chrome/browser/r...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc:41:
MockRenderViewContextMenu() : observer_(NULL), profile_(new TestingProfile) {}
Here and in other places: prefer nullptr instead of NULL

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
File chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc
(right):

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc:94: for
(std::vector<MockMenuItem>::iterator it = items_.begin();
nit: use range loop.

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc:106:
FAIL();
Instead of commenting, we can add the message here:
FAIL() << "Menu observer is trying to change a menu item it doesn't own." or
sth.

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc:110:
return NULL;
(Here and in all other places throughout the CL) use nullptr instead of NULL.

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
File chrome/browser/renderer_context_menu/mock_render_view_context_menu.h
(right):

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.h:74: bool
GetMenuItem(size_t i, MockMenuItem* item) const;
nit: s/i/index

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.h:81: //
test. This is a weak pointer, the test is responsible for deleting this
Instead of calling it a weak pointer ..., just say "This is owned by our owner
and the owner is responsible for its lifetime."

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
File chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc
(right):

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:60:
const int kMaxLanguages = 100;
kMaxLanguages = IDC_SPELLCHECK_LANGUAGES_LAST - ..FIRST

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
File
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/1647723002/diff/160001/chrome/browser/...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc:55:
dictionaries_value.AppendString(dict);
Could use AppendStrings here too.

[please use gerrit instead, 2016-02-01 20:20:57]

Rachel & Istiaque, ptal patch 9.

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc
(right):

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc:94: for
(std::vector<MockMenuItem>::iterator it = items_.begin();
On 2016/02/01 19:23:33, lazyboy wrote:
> nit: use range loop.

Done.

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc:106:
FAIL();
On 2016/02/01 19:23:33, lazyboy wrote:
> Instead of commenting, we can add the message here:
> FAIL() << "Menu observer is trying to change a menu item it doesn't own." or
> sth.

Done.

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc:110:
return NULL;
On 2016/02/01 19:23:33, lazyboy wrote:
> (Here and in all other places throughout the CL) use nullptr instead of NULL.

Done.

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/mock_render_view_context_menu.h
(right):

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.h:74: bool
GetMenuItem(size_t i, MockMenuItem* item) const;
On 2016/02/01 19:23:33, lazyboy wrote:
> nit: s/i/index

Done.

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/mock_render_view_context_menu.h:81: //
test. This is a weak pointer, the test is responsible for deleting this
On 2016/02/01 19:23:33, lazyboy wrote:
> Instead of calling it a weak pointer ..., just say "This is owned by our owner
> and the owner is responsible for its lifetime."

Done.

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc
(right):

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:60:
const int kMaxLanguages = 100;
On 2016/02/01 19:23:33, lazyboy wrote:
> kMaxLanguages = IDC_SPELLCHECK_LANGUAGES_LAST - ..FIRST

Done.

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
File
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc
(right):

https://codereview.chromium.org/1647723002/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc:55:
dictionaries_value.AppendString(dict);
On 2016/02/01 19:23:33, lazyboy wrote:
> Could use AppendStrings here too.

Done.

[lazyboy, 2016-02-01 22:58:12]

menu code lgtm.

Also, add a little bit more context in the CL description please. Thanks.

https://chromiumcodereview.appspot.com/1647723002/diff/200001/chrome/browser/...
File chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc
(right):

https://chromiumcodereview.appspot.com/1647723002/diff/200001/chrome/browser/...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:20:
#include "content/public/browser/render_view_host.h"
Do you really need these? It seems like we have too many includes, perhaps
because of copy/paste. I don't see the need for RenderWidgetHostView on the next
line either.
Can you go through a pass to remove not required includes in the CL? Ty.

[please use gerrit instead, 2016-02-02 00:28:07]

rouslan@chromium.org changed reviewers:
+ bauerb@chromium.org

[please use gerrit instead, 2016-02-02 00:28:08]

Bernhard, ptal chrome/browser/DEPS adding a dependency to components/prefs in
patch 11.

Rachel, ptal spelly things in patch 11.

[please use gerrit instead, 2016-02-02 00:28:51]

https://chromiumcodereview.appspot.com/1647723002/diff/200001/chrome/browser/...
File chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc
(right):

https://chromiumcodereview.appspot.com/1647723002/diff/200001/chrome/browser/...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:20:
#include "content/public/browser/render_view_host.h"
On 2016/02/01 22:58:12, lazyboy wrote:
> Do you really need these? It seems like we have too many includes, perhaps
> because of copy/paste. I don't see the need for RenderWidgetHostView on the
next
> line either.
> Can you go through a pass to remove not required includes in the CL? Ty.

Done.

[please use gerrit instead, 2016-02-02 00:41:25]

rouslan@chromium.org changed reviewers:
- bauerb@chromium.org

[please use gerrit instead, 2016-02-02 00:41:26]

Bernhard, actually I don't need your review after a rebase. Now chrome/browser/
can depend on all of components/.

[please use gerrit instead, 2016-02-02 18:12:37]

Description was changed from

==========
Add back the spellcheck menu.

This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144

BUG=544234
==========

to

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

This patch adds a "Spellcheck" submenu to the context menu on Windows,
ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also
adds support for multilingual spellcheck. It is supported by "All
languages" menu item in the "Spellcheck" submenu. Selecting this option
will spellcheck in all of the languages that are listed in the submenu.
These language list can be modified in chrome://settings/languages.

BUG=544234
==========

[please use gerrit instead, 2016-02-02 19:42:54]

Description was changed from

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

This patch adds a "Spellcheck" submenu to the context menu on Windows,
ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also
adds support for multilingual spellcheck. It is supported by "All
languages" menu item in the "Spellcheck" submenu. Selecting this option
will spellcheck in all of the languages that are listed in the submenu.
These language list can be modified in chrome://settings/languages.

BUG=544234
==========

to

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

This patch adds a "Spellcheck" submenu to the context menu on Windows,
ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also
adds support for multilingual spellcheck. It is supported by "All
languages" menu item in the "Spellcheck" submenu. Selecting this option
will spellcheck in all of the languages that are listed in the submenu.
These language list can be modified in chrome://settings/languages.

BUG=544234
==========

[please use gerrit instead, 2016-02-02 19:43:38]

Description was changed from

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

This patch adds a "Spellcheck" submenu to the context menu on Windows,
ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also
adds support for multilingual spellcheck. It is supported by "All
languages" menu item in the "Spellcheck" submenu. Selecting this option
will spellcheck in all of the languages that are listed in the submenu.
These language list can be modified in chrome://settings/languages.

BUG=544234
==========

to

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

This patch adds a "Spellcheck" submenu to the context menu on Windows,
ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also
adds support for multilingual spellcheck in the submenu. It is
supported by "All languages" menu item. Selecting this option will
spellcheck in all of the languages that are listed in the submenu.
The language list can be modified in chrome://settings/languages.

BUG=544234
==========

[please use gerrit instead, 2016-02-02 19:44:40]

Description was changed from

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

This patch adds a "Spellcheck" submenu to the context menu on Windows,
ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also
adds support for multilingual spellcheck in the submenu. It is
supported by "All languages" menu item. Selecting this option will
spellcheck in all of the languages that are listed in the submenu.
The language list can be modified in chrome://settings/languages.

BUG=544234
==========

to

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

This patch adds a "Spellcheck" submenu to the context menu on Windows,
ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also
adds support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck
in all of the languages that are listed in the submenu. The language
list can be modified in chrome://settings/languages.

BUG=544234
==========

[please use gerrit instead, 2016-02-02 20:21:24]

Description was changed from

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

This patch adds a "Spellcheck" submenu to the context menu on Windows,
ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also
adds support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck
in all of the languages that are listed in the submenu. The language
list can be modified in chrome://settings/languages.

BUG=544234
==========

to

==========
[win/cros/lin] Add back the spellcheck submenu.

This patch adds back the "Spellcheck" submenu to the context menu on
Windows, ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also adds
support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck in
all of the languages that are listed in the submenu. The language list
can be modified in chrome://settings/languages.

BUG=544234
==========

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

Pending the resolution of the larger radio button question, too.

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc
(right):

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:58:
for (size_t i = 0; i < languages_.size() && i < kMaxLanguages; ++i) {
We've DCHECKED above that i will be below kMaxLanguages, so I'd suggest a C++11
loop

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:71: //
Add a 'Check spelling while typing' item in the sub menu.
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)

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:80: //
on behalf of this class.)
Would it make sense to handle it here instead?

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:94:
command_id < IDC_SPELLCHECK_LANGUAGES_LAST) {
Should this gate on the number of languages in |languages_|?

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:101:
// is hard for this class to handle it.
No excuses! ;) Why is it hard?

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:125:
return false;
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]

Rachel, ptal patch 14.

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc
(right):

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:58:
for (size_t i = 0; i < languages_.size() && i < kMaxLanguages; ++i) {
On 2016/02/04 18:59:10, groby wrote:
> We've DCHECKED above that i will be below kMaxLanguages, so I'd suggest a
C++11
> loop

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."

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:71: //
Add a 'Check spelling while typing' item in the sub menu.
On 2016/02/04 18:59:10, groby wrote:
> 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)

========

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.

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:80: //
on behalf of this class.)
On 2016/02/04 18:59:10, groby wrote:
> Would it make sense to handle it here instead?

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.

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:94:
command_id < IDC_SPELLCHECK_LANGUAGES_LAST) {
On 2016/02/04 18:59:10, groby wrote:
> Should this gate on the number of languages in |languages_|?

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().

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:101:
// is hard for this class to handle it.
On 2016/02/04 18:59:10, groby wrote:
> No excuses! ;) Why is it hard?

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.

https://codereview.chromium.org/1647723002/diff/240001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:125:
return false;
On 2016/02/04 18:59:10, groby wrote:
> You might combine with previous if...
> if (size == num) return (size == 1);
> 
> But it's a personal preference. 

Done.

[please use gerrit instead, 2016-02-16 22:56:54]

Rachel, fyi patch 15 is rebased on top of the new GetDictionaries() function.

https://codereview.chromium.org/1647723002/diff/300001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc
(right):

https://codereview.chromium.org/1647723002/diff/300001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/spelling_options_submenu_observer.cc:44:
SpellcheckService::GetDictionaries(browser_context, &dictionaries_);
Using the new GetDictionaries() call.

[groby-ooo-7-16, 2016-02-17 01:20:37]

I wish you had kept the refactorings in separate CLs, but...

LGTM

[Ilya Sherman, 2016-02-17 01:24:09]

histograms.xml lgtm

[please use gerrit instead, 2016-02-17 01:42:46]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-02-17 01:42:46]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org,
lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/1647723002/#ps300001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-02-17 01:45:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1647723002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1647723002/300001

[commit-bot: I haz the power, 2016-02-17 01:59:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-17 01:59:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[please use gerrit instead, 2016-02-17 02:01:36]

rouslan@chromium.org changed reviewers:
+ brettw@chromium.org

[please use gerrit instead, 2016-02-17 02:01:36]

Brett, ptal chrome/browser/BUILD.gn in patch 15.

[brettw, 2016-02-17 19:26:37]

You don't need me to review build file changes unless there's some specific
question.

[please use gerrit instead, 2016-02-17 19:29:00]

No specific question, only a presubmit failure because of lack of LG from an
owner. I will TBR you instead.

[please use gerrit instead, 2016-02-17 19:29:44]

Description was changed from

==========
[win/cros/lin] Add back the spellcheck submenu.

This patch adds back the "Spellcheck" submenu to the context menu on
Windows, ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also adds
support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck in
all of the languages that are listed in the submenu. The language list
can be modified in chrome://settings/languages.

BUG=544234
==========

to

==========
[win/cros/lin] Add back the spellcheck submenu.

This patch adds back the "Spellcheck" submenu to the context menu on
Windows, ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also adds
support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck in
all of the languages that are listed in the submenu. The language list
can be modified in chrome://settings/languages.

TBR=brettw@chromium.org
BUG=544234
==========

[please use gerrit instead, 2016-02-17 19:29:52]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2016-02-17 19:34:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1647723002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1647723002/300001

[commit-bot: I haz the power, 2016-02-17 19:45:51]

Description was changed from

==========
[win/cros/lin] Add back the spellcheck submenu.

This patch adds back the "Spellcheck" submenu to the context menu on
Windows, ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also adds
support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck in
all of the languages that are listed in the submenu. The language list
can be modified in chrome://settings/languages.

TBR=brettw@chromium.org
BUG=544234
==========

to

==========
[win/cros/lin] Add back the spellcheck submenu.

This patch adds back the "Spellcheck" submenu to the context menu on
Windows, ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also adds
support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck in
all of the languages that are listed in the submenu. The language list
can be modified in chrome://settings/languages.

TBR=brettw@chromium.org
BUG=544234
==========

[commit-bot: I haz the power, 2016-02-17 19:45:53]

Committed patchset #15 (id:300001)

[commit-bot: I haz the power, 2016-02-17 19:48:07]

Description was changed from

==========
[win/cros/lin] Add back the spellcheck submenu.

This patch adds back the "Spellcheck" submenu to the context menu on
Windows, ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also adds
support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck in
all of the languages that are listed in the submenu. The language list
can be modified in chrome://settings/languages.

TBR=brettw@chromium.org
BUG=544234
==========

to

==========
[win/cros/lin] Add back the spellcheck submenu.

This patch adds back the "Spellcheck" submenu to the context menu on
Windows, ChromeOS, and Linux. This partially reverts commit
http://crrev.com/5414139041953f2b558cfb0fdf8a4927a19a3144, but also adds
support for multilingual spellcheck in the submenu via the "All
languages" radio button. Selecting this radio button will spellcheck in
all of the languages that are listed in the submenu. The language list
can be modified in chrome://settings/languages.

TBR=brettw@chromium.org
BUG=544234

Committed: https://crrev.com/e59900a51f10891ee99368eda83ddec185f2b007
Cr-Commit-Position: refs/heads/master@{#375955}
==========

[commit-bot: I haz the power, 2016-02-17 19:48:08]

Patchset 15 (id:??) landed as
https://crrev.com/e59900a51f10891ee99368eda83ddec185f2b007
Cr-Commit-Position: refs/heads/master@{#375955}

[brettw, 2016-02-17 20:17:29]

Ah, I think you need owner review for a bunch of files then, not just the build
file. I only glanced at this briefly, but LGTM.