Handle typographical apostrophe with spelling service client

chrome/renderer/spellchecker/spellcheck.cc

Index: chrome/renderer/spellchecker/spellcheck.cc
diff --git a/chrome/renderer/spellchecker/spellcheck.cc b/chrome/renderer/spellchecker/spellcheck.cc
index e8e283a0bec76c02dd91554c1e1362b32b065ab6..5f5f02d662b3f5caceb1c4436a772aae6ea0ff32 100644
--- a/chrome/renderer/spellchecker/spellcheck.cc
+++ b/chrome/renderer/spellchecker/spellcheck.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/renderer/spellchecker/spellcheck.h"
 
+#include <algorithm>
+
 #include "base/bind.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/strings/utf_string_conversions.h"
@@ -92,6 +94,26 @@ bool DocumentMarkersRemover::Visit(content::RenderView* render_view) {
   return true;
 }
 
+// Makes sure that the apostrophes in |to| are the same type as in the range
+// |from_begin| to |from_end|, in the same order.
+void PreserveOriginalApostropheTypes(
+    const base::string16::const_iterator& from_begin,
+    const base::string16::const_iterator& from_end,
+    base::string16* to) {
+  const base::string16 kApostrophes(base::WideToUTF16(L"\x2019'"));

[groby-ooo-7-16, 2015/04/25 00:19:02]

Please declare as constant

[please use gerrit instead, 2015/06/17 03:52:07]

No longer applicable after using literals in IsApostrophe().

+  base::string16::const_iterator from_it = std::find_first_of(
+           from_begin, from_end, kApostrophes.begin(), kApostrophes.end());
+  for (base::string16::iterator to_it = std::find_first_of(

[groby-ooo-7-16, 2015/04/25 00:19:02]

You'll need to handle the case of differing apostrophe counts somehow. (I.e. if
a spelling correction inserts/removes an apostrophe) 

[please use gerrit instead, 2015/06/17 03:52:07]

I think that's an edge case that requires some more thinking on how to properly
handle it. As such, it's out of scope of this quick fix, but let's discuss it
further in https://goo.gl/em8lwm.

+           to->begin(), to->end(), kApostrophes.begin(), kApostrophes.end());
+       from_it != from_end && to_it != to->end();

[groby-ooo-7-16, 2015/04/25 00:19:02]

Also, this is the for-loop from hell.

For readability purposes, I'd suggest something like

for (base::char16& c : to_string) {
  if (isApostrophe(c)) {
    // Find the next one in from_string, replace
  }
}

(If you use find_if, you can reuse IsApostrophe :)

[please use gerrit instead, 2015/06/17 03:52:07]

Done.

+       from_it = std::find_first_of(
+           from_it + 1, from_end, kApostrophes.begin(), kApostrophes.end()),
+       to_it = std::find_first_of(
+           to_it + 1, to->end(), kApostrophes.begin(), kApostrophes.end())) {
+    *to_it = *from_it;
+  }
+}
+
 }  // namespace
 
 class SpellCheck::SpellcheckRequest {
@@ -383,30 +405,41 @@ void SpellCheck::CreateTextCheckingResults(
   // Double-check misspelled words with our spellchecker and attach grammar
   // markers to them if our spellchecker tells they are correct words, i.e. they
   // are probably contextually-misspelled words.
-  const base::char16* text = line_text.c_str();
-  std::vector<WebTextCheckingResult> list;
+  std::vector<WebTextCheckingResult> result;
   for (size_t i = 0; i < spellcheck_results.size(); ++i) {

[groby-ooo-7-16, 2015/04/25 00:19:02]

While you're here:
for (const SpellCheckResult& spellcheck_result : spellcheck_results) {

[please use gerrit instead, 2015/06/17 03:52:07]

Done.

+    if (custom_dictionary_.SpellCheckWord(line_text,

[groby-ooo-7-16, 2015/04/25 00:19:02]

Makes sense to move this up - thanks! (If this CL drags on, might want to do
this separately)

[please use gerrit instead, 2015/06/17 03:52:07]

Ack

+                                          spellcheck_results[i].location,
+                                          spellcheck_results[i].length)) {
+      continue;
+    }
+
+    base::string16 replacement = spellcheck_results[i].replacement;
+    PreserveOriginalApostropheTypes(
+        line_text.begin() + spellcheck_results[i].location,

[groby-ooo-7-16, 2015/04/25 00:19:02]

Given that we refer to the substring several times, I'm wondering if we
shouldn't simply copy it out into a separate string.

[please use gerrit instead, 2015/06/17 03:52:07]

Done.

+        line_text.begin() + spellcheck_results[i].location +
+            spellcheck_results[i].length,
+        &replacement);
+    if (line_text.compare(spellcheck_results[i].location,
+                          spellcheck_results[i].length, replacement) == 0) {
+      continue;
+    }
+
     SpellCheckResult::Decoration decoration = spellcheck_results[i].decoration;
-    int word_location = spellcheck_results[i].location;
-    int word_length = spellcheck_results[i].length;
     int misspelling_start = 0;
     int misspelling_length = 0;
     if (decoration == SpellCheckResult::SPELLING &&
-        filter == USE_NATIVE_CHECKER) {
-      if (SpellCheckWord(text + word_location, word_length, 0,
-                         &misspelling_start, &misspelling_length, NULL)) {
-        decoration = SpellCheckResult::GRAMMAR;
-      }
-    }
-    if (!custom_dictionary_.SpellCheckWord(
-            line_text, word_location, word_length)) {
-      list.push_back(WebTextCheckingResult(
-          static_cast<WebTextDecorationType>(decoration),
-          word_location + line_offset,
-          word_length,
-          spellcheck_results[i].replacement,
-          spellcheck_results[i].hash));
+        filter == USE_NATIVE_CHECKER &&
+        SpellCheckWord(line_text.c_str() + spellcheck_results[i].location,
+                       spellcheck_results[i].length, 0, &misspelling_start,
+                       &misspelling_length, NULL)) {
+      decoration = SpellCheckResult::GRAMMAR;
     }
+
+    result.push_back(WebTextCheckingResult(
+        static_cast<WebTextDecorationType>(decoration),
+        line_offset + spellcheck_results[i].location,
+        spellcheck_results[i].length, replacement, spellcheck_results[i].hash));
   }
-  textcheck_results->assign(list);
+
+  textcheck_results->assign(result);
 }