Allow storing multiple replacements on SpellCheckResult

components/spellcheck/renderer/spellcheck.cc

 // Copyright (c) 2012 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 "components/spellcheck/renderer/spellcheck.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <algorithm>
 #include <utility>
@@ skipping 443 matching lines @@
   std::vector<WebTextCheckingResult> results;
   for (const SpellCheckResult& spellcheck_result : spellcheck_results) {
     DCHECK_LE(static_cast<size_t>(spellcheck_result.location),
               line_text.length());
     DCHECK_LE(static_cast<size_t>(spellcheck_result.location +
                                   spellcheck_result.length),
               line_text.length());
 
     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.
     if (custom_dictionary_.SpellCheckWord(misspelled_word, 0,
                                           misspelled_word.length())) {
       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) {
       // Double-check misspelled words with out spellchecker and attach grammar
       // markers to them if our spellchecker tells us they are correct words,
       // i.e. they are probably contextually-misspelled words.
       int unused_misspelling_start = 0;
       int unused_misspelling_length = 0;
       if (decoration == SpellCheckResult::SPELLING &&
           SpellCheckWord(misspelled_word.c_str(), kNoOffset,
                          misspelled_word.length(), kNoTag,
                          &unused_misspelling_start, &unused_misspelling_length,
                          nullptr)) {
         decoration = SpellCheckResult::GRAMMAR;
       }
     }
 
-    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);
 }
 
 bool SpellCheck::IsSpellcheckEnabled() {
 #if defined(OS_ANDROID)
   if (!spellcheck::IsAndroidSpellCheckFeatureEnabled()) return false;
 #endif
   return spellcheck_enabled_;
 }