Handle typographical apostrophe with spelling service client

chrome/renderer/spellchecker/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 "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"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/spellcheck_common.h"
 #include "chrome/common/spellcheck_messages.h"
 #include "chrome/common/spellcheck_result.h"
 #include "chrome/renderer/spellchecker/spellcheck_language.h"
 #include "chrome/renderer/spellchecker/spellcheck_provider.h"
 #include "content/public/renderer/render_thread.h"
@@ skipping 68 matching lines @@
   for (size_t i = 0; i < words.size(); ++i)
     words_[i] = WebString::fromUTF8(words[i]);
 }
 
 bool DocumentMarkersRemover::Visit(content::RenderView* render_view) {
   if (render_view && render_view->GetWebView())
     render_view->GetWebView()->removeSpellingMarkersUnderWords(words_);
   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 {
  public:
   SpellcheckRequest(const base::string16& text,
                     blink::WebTextCheckingCompletion* completion)
       : text_(text), completion_(completion) {
     DCHECK(completion);
   }
   ~SpellcheckRequest() {}
@@ skipping 271 matching lines @@
 
 void SpellCheck::CreateTextCheckingResults(
     ResultFilter filter,
     int line_offset,
     const base::string16& line_text,
     const std::vector<SpellCheckResult>& spellcheck_results,
     WebVector<WebTextCheckingResult>* textcheck_results) {
   // 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;
-      }
+        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;
     }
-    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));
-    }
+
+    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);
 }