Allow storing multiple replacements on SpellCheckResult

components/spellcheck/renderer/spellcheck.cc

Index: components/spellcheck/renderer/spellcheck.cc
diff --git a/components/spellcheck/renderer/spellcheck.cc b/components/spellcheck/renderer/spellcheck.cc
index cc9b238245aaccd1cec612b44ef5483e055b8b77..fe6f68e43cefa88c4ec24527e8aad104032fe542 100644
--- a/components/spellcheck/renderer/spellcheck.cc
+++ b/components/spellcheck/renderer/spellcheck.cc
@@ -461,7 +461,8 @@ void SpellCheck::CreateTextCheckingResults(
 
     const base::string16& misspelled_word =
         line_text.substr(spellcheck_result.location, spellcheck_result.length);
-    base::string16 replacement = spellcheck_result.replacement;
+    const std::vector<base::string16>& replacements =
+        spellcheck_result.replacements;
     SpellCheckResult::Decoration decoration = spellcheck_result.decoration;
 
     // Ignore words in custom dictionary.
@@ -470,11 +471,22 @@ void SpellCheck::CreateTextCheckingResults(
       continue;
     }
 
-    // Use the same types of appostrophes as in the mispelled word.
-    PreserveOriginalApostropheTypes(misspelled_word, &replacement);
+    bool skip_these_replacements = false;
+    std::vector<WebString> replacements_adjusted;
+    for (base::string16 replacement : replacements) {
+      // Use the same types of appostrophes as in the mispelled word.
+      PreserveOriginalApostropheTypes(misspelled_word, &replacement);
 
-    // Ignore misspellings due the typographical apostrophe.
-    if (misspelled_word == replacement)
+      // Ignore misspellings due the typographical apostrophe.
+      if (replacement == misspelled_word) {
+        skip_these_replacements = true;
+        continue;
+      }
+
+      replacements_adjusted.push_back(WebString::FromUTF16(replacement));
+    }
+
+    if (skip_these_replacements)

[groby-ooo-7-16, 2017/04/28 23:45:55]

I believe this is wrong - with your change, if *any* of the suggestions is
identical except for apostrophe fixes, *all* are discarded.

This is complex enough it probably deserves a test case :)

[rlanday, 2017/05/01 20:40:01]

My thinking was that if the spellchecker thinks the word can be made to be
correctly spelled just by changing the apostrophe, we should just ignore the
misspelling.

E.g. we might have a case like

word:
"John’s"

suggestions:
"John's"
"Jon's"

where one of the suggestions is just changing the apostrophe, and some of the
others are unnecessary changes to an already correctly-misspelled word. In this
case, the code I wrote probably does the most correct thing and ignores the
"misspelling".

I suppose it's also possible in principle to have a case like this:

word:
Johhn’s

suggestions:
Johhn's (which we can correct to Johhn’s and throw out)
John's (which we can correct to John’s and keep)

In this case, would do want to keep all other suggested replacements. However,
something like this seems like an oddball case that shouldn't happen because it
requires the spellchecker to return a suggestion that changes only the
apostrophe, despite the word still being misspelled. This shouldn't happen if
the dictionary to pull replacements out of is the same for checking the word in
the first place, but might happen if one of the rules for generating suggestions
is "replace curly quotes with straight quotes".

If we're really worried about this second case, we could probably switch out the
apostrophes to straight quotes before running the spellchecker, but then we
could theoretically have the same problem in the opposite direction where some
spellchecker might think words are only spelled correctly if they use *curly*
quotes.


What do you think the behavior should be?

[groby-ooo-7-16, 2017/05/08 15:31:22]

What we currently do is that for all replacements, we restore the original
apostrophes after spell checking. If after that replacement the suggestion
equals the original word, we drop *that suggestion*.

There might be multiple other suggestions that still apply. Let's assume we try
the possessive of "mart", "mart‘s" with a 'smart quote'

The suggestions coming back will be "mart's", "mark's", "mare's" - all with
straight apostrophes. We do the back substitution on the first. It's equal to
the original, it should be dropped. But all further suggestions should still be
included, with the apostrophe substitution. I.e. We'd keep "mark's" and "mare's"
with smart quotes.

We can certainly debate which is the better choice - you've opted for "it's
correct, let's drop it". I'm wondering if that's the right choice, since we also
mix results with the server-side check. (Below, filter==USE_NATIVE_CHECKER). 

I.e. if server-side suggested "mare's" as a contextually better result, I
believe it should still get a red underline. In your case, it'd get a grey one.

Yes, I know this is the edge case of an edge case :) If you'd prefer to stick
with your choice, I won't keep objecting - but please document the choice in a
comment, and add a unit test that verifies that specific behavior. 

       continue;
 
     if (filter == USE_NATIVE_CHECKER) {
@@ -492,10 +504,10 @@ void SpellCheck::CreateTextCheckingResults(
       }
     }
 
-    results.push_back(WebTextCheckingResult(
-        static_cast<WebTextDecorationType>(decoration),
-        line_offset + spellcheck_result.location, spellcheck_result.length,
-        blink::WebString::FromUTF16(replacement)));
+    results.push_back(
+        WebTextCheckingResult(static_cast<WebTextDecorationType>(decoration),
+                              line_offset + spellcheck_result.location,
+                              spellcheck_result.length, replacements_adjusted));
   }
 
   textcheck_results->Assign(results);