Offers suggestions for each language that marks a word misspelled.

Offers suggestions for each language that marks a word misspelled.

As evenly as possible, offers suggestions for each language that marks
a word as misspelled.

TEST=*Multi*Spell*Suggestions*

BUG=5102
Committed: https://crrev.com/452a849ab75bd6d231cc1afcd145a0806b82142f
Cr-Commit-Position: refs/heads/master@{#344647}

[Julius, 2015-08-19 23:09:55]

Patchset #2 (id:20001) has been deleted

[Julius, 2015-08-20 17:02:19]

Patchset #3 (id:60001) has been deleted

[Julius, 2015-08-20 17:24:49]

juliusa@google.com changed reviewers:
+ rouslan@chromium.org

[Julius, 2015-08-20 17:24:50]

Rouslan, PTAL at Patch Set #3.

[please use gerrit instead, 2015-08-20 18:40:23]

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck.cc (right):

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:307: suggestions_list.clear();
If optional_suggestions is null, then suggestions_list will be empty already.
Calling suggestions_list on an empty vector is cheap for the computer. Thinking
about why you're checking optional_suggestions for null here is cognitive load
for the reader. Therefore, remove "if (optional_suggestions)". Just
"suggestions_list.clear();" is enough.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:326: // suggestions twice.
I don't think this comment explains much, although the code seems correct.
Remove the comment.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:327: if (optional_suggestions)
Remove the if. Just push_back.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:347: void
SpellCheck::FillSuggestions(
Your function does nothing if |optional_suggestions| is null. This type of
functions is usually called MaybeDoSomething(), but I would avoid this. Instead,
assume that |optinal_suggestions| is never null. When you call this function
call it like so:

if (optional_suggestions)
  FillSuggestions(suggestions_list, optional_suggestions);

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:364: base::string16 suggestion =
suggestions_list[i][indices[i]];
Avoid copying |suggestion| by making it const-ref. This is not const-happiness,
because the purpose of const-ref is to avoid copying, not to avoid modifying.
Copying would be bad here, because you're doing it inside of a loop, so possibly
many-many times.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:366: if
(std::find_if(optional_suggestions->begin(),
You don't need a lambda and std::find_if. Use std::find() instead.

http://www.cplusplus.com/reference/algorithm/find/

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:374: if (indices[i] ==
suggestions_list[i].size())
You can reduce lines 373-374 into a single line:

if (++indicies[i] == suggesions_list[i].size())

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck.h (right):

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.h:154: // |suggestions_list|.
Specify the max size of optional suggestions.

Specify whether optional_suggestions is allowed to be null. (It's not.)

Specify the format for suggestions_list. (suggestions_list[i][j] is the j-th
suggestion from i-th language.)

Make this method static, because it does not access |this|. Static methods go to
the top of a section, so below FRIEND_TEST_ALL_PREFIXES(SpellCheckTest, ...).

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_multilingual_unittest.cc (right):

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_multilingual_unittest.cc:140:
EXPECT_EQ(expected_suggestions.size(), suggestions.size());
Does EXPECT_EQ(expected_suggestions, suggestions); work?

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_multilingual_unittest.cc:258:
ExpectSpellCheckSuggestionResults(kTestCases, arraysize(kTestCases));
Inline this function, because it's used only once.

[Julius, 2015-08-20 21:51:02]

Patchset #4 (id:90001) has been deleted

[Julius, 2015-08-20 21:52:51]

Rouslan, PTAL at Patch Set #4.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck.cc (right):

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:307: suggestions_list.clear();
On 2015/08/20 18:40:23, Rouslan wrote:
> If optional_suggestions is null, then suggestions_list will be empty already.
> Calling suggestions_list on an empty vector is cheap for the computer.
Thinking
> about why you're checking optional_suggestions for null here is cognitive load
> for the reader. Therefore, remove "if (optional_suggestions)". Just
> "suggestions_list.clear();" is enough.

Done.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:326: // suggestions twice.
On 2015/08/20 18:40:23, Rouslan wrote:
> I don't think this comment explains much, although the code seems correct.
> Remove the comment.

Done.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:327: if (optional_suggestions)
On 2015/08/20 18:40:23, Rouslan wrote:
> Remove the if. Just push_back.

Done.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:347: void
SpellCheck::FillSuggestions(
On 2015/08/20 18:40:23, Rouslan wrote:
> Your function does nothing if |optional_suggestions| is null. This type of
> functions is usually called MaybeDoSomething(), but I would avoid this.
Instead,
> assume that |optinal_suggestions| is never null. When you call this function
> call it like so:
> 
> if (optional_suggestions)
>   FillSuggestions(suggestions_list, optional_suggestions);

Done.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:364: base::string16 suggestion =
suggestions_list[i][indices[i]];
On 2015/08/20 18:40:23, Rouslan wrote:
> Avoid copying |suggestion| by making it const-ref. This is not
const-happiness,
> because the purpose of const-ref is to avoid copying, not to avoid modifying.
> Copying would be bad here, because you're doing it inside of a loop, so
possibly
> many-many times.

Done.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:366: if
(std::find_if(optional_suggestions->begin(),
On 2015/08/20 18:40:23, Rouslan wrote:
> You don't need a lambda and std::find_if. Use std::find() instead.
> 
> http://www.cplusplus.com/reference/algorithm/find/

Wow, duh.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.cc:374: if (indices[i] ==
suggestions_list[i].size())
On 2015/08/20 18:40:23, Rouslan wrote:
> You can reduce lines 373-374 into a single line:
> 
> if (++indicies[i] == suggesions_list[i].size())

Done.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck.h (right):

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck.h:154: // |suggestions_list|.
On 2015/08/20 18:40:23, Rouslan wrote:
> Specify the max size of optional suggestions.
> 
> Specify whether optional_suggestions is allowed to be null. (It's not.)
> 
> Specify the format for suggestions_list. (suggestions_list[i][j] is the j-th
> suggestion from i-th language.)
> 
> Make this method static, because it does not access |this|. Static methods go
to
> the top of a section, so below FRIEND_TEST_ALL_PREFIXES(SpellCheckTest, ...).

Done.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_multilingual_unittest.cc (right):

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_multilingual_unittest.cc:140:
EXPECT_EQ(expected_suggestions.size(), suggestions.size());
On 2015/08/20 18:40:23, Rouslan wrote:
> Does EXPECT_EQ(expected_suggestions, suggestions); work?

expected_suggestions is of type std::vector<base::string16> while suggestions is
blink::WebVector<blink::WebString>. They don't seem to be directly comparable.
Also, doing it this way will show which suggestion has an error which I think
could be valuable.

https://codereview.chromium.org/1300213002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_multilingual_unittest.cc:258:
ExpectSpellCheckSuggestionResults(kTestCases, arraysize(kTestCases));
On 2015/08/20 18:40:23, Rouslan wrote:
> Inline this function, because it's used only once.

Done.

[please use gerrit instead, 2015-08-20 22:14:27]

lgtm % comments

https://codereview.chromium.org/1300213002/diff/110001/chrome/renderer/spellc...
File chrome/renderer/spellchecker/spellcheck.cc (right):

https://codereview.chromium.org/1300213002/diff/110001/chrome/renderer/spellc...
chrome/renderer/spellchecker/spellcheck.cc:346: void
SpellCheck::FillSuggestions(
Match the order of functions in the header and the source file. When another
engineers reads the header and the source side-by-side, it would be jarring to
scroll one window up and the other down.

https://codereview.chromium.org/1300213002/diff/110001/chrome/renderer/spellc...
File chrome/renderer/spellchecker/spellcheck.h (right):

https://codereview.chromium.org/1300213002/diff/110001/chrome/renderer/spellc...
chrome/renderer/spellchecker/spellcheck.h:124: // Evenly fill
|optional_suggestions| with a maximum of 5 suggestions from
Specify the constant name instead of 5. If we update the constant to 4, no one
will bother to update this comment, so it will be out of date.

[Julius, 2015-08-21 00:36:43]

Patchset #5 (id:130001) has been deleted

[Julius, 2015-08-21 00:39:19]

Fixed nits and committing.

https://codereview.chromium.org/1300213002/diff/110001/chrome/renderer/spellc...
File chrome/renderer/spellchecker/spellcheck.cc (right):

https://codereview.chromium.org/1300213002/diff/110001/chrome/renderer/spellc...
chrome/renderer/spellchecker/spellcheck.cc:346: void
SpellCheck::FillSuggestions(
On 2015/08/20 22:14:27, Rouslan wrote:
> Match the order of functions in the header and the source file. When another
> engineers reads the header and the source side-by-side, it would be jarring to
> scroll one window up and the other down.

Done.

https://codereview.chromium.org/1300213002/diff/110001/chrome/renderer/spellc...
File chrome/renderer/spellchecker/spellcheck.h (right):

https://codereview.chromium.org/1300213002/diff/110001/chrome/renderer/spellc...
chrome/renderer/spellchecker/spellcheck.h:124: // Evenly fill
|optional_suggestions| with a maximum of 5 suggestions from
On 2015/08/20 22:14:27, Rouslan wrote:
> Specify the constant name instead of 5. If we update the constant to 4, no one
> will bother to update this comment, so it will be out of date.

Done.

[Julius, 2015-08-21 00:39:28]

The CQ bit was checked by juliusa@google.com

[Julius, 2015-08-21 00:39:28]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/1300213002/#ps150001
(title: "Fixed final nits.")

[commit-bot: I haz the power, 2015-08-21 00:41:23]

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

[commit-bot: I haz the power, 2015-08-21 01:48:46]

Committed patchset #5 (id:150001)

[commit-bot: I haz the power, 2015-08-21 01:49:34]

Patchset 5 (id:??) landed as
https://crrev.com/452a849ab75bd6d231cc1afcd145a0806b82142f
Cr-Commit-Position: refs/heads/master@{#344647}