Cheer up spell-checking code

chrome/browser/spellchecker/feedback_sender.cc

Index: chrome/browser/spellchecker/feedback_sender.cc
diff --git a/chrome/browser/spellchecker/feedback_sender.cc b/chrome/browser/spellchecker/feedback_sender.cc
index 4bfb8244462af3a4acc84f9c73cee25741e68eff..9a226a5624ec317cb91691747f48e5cd403e165b 100644
--- a/chrome/browser/spellchecker/feedback_sender.cc
+++ b/chrome/browser/spellchecker/feedback_sender.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Copyright 2013 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.
 //
@@ -93,30 +93,29 @@ Misspelling BuildFeedback(const SpellCheckResult& result,
                      result.hash);
 }
 
-// Builds suggestion info from |suggestions|. The caller owns the result.
-base::ListValue* BuildSuggestionInfo(
-    const std::vector<Misspelling>& suggestions,
+// Builds suggestion info from |suggestions|.
+scoped_ptr<base::ListValue> BuildSuggestionInfo(
+    const std::vector<Misspelling>& misspellings,
     bool is_first_feedback_batch) {
-  base::ListValue* list = new base::ListValue;
-  for (std::vector<Misspelling>::const_iterator suggestion_it =
-           suggestions.begin();
-       suggestion_it != suggestions.end();
-       ++suggestion_it) {
-    base::DictionaryValue* suggestion = SerializeMisspelling(*suggestion_it);
-    suggestion->SetBoolean("isFirstInSession", is_first_feedback_batch);
-    suggestion->SetBoolean("isAutoCorrection", false);
-    list->Append(suggestion);
+  scoped_ptr<base::ListValue> list(new base::ListValue);
+  for (const auto& raw_misspelling : misspellings) {
+    scoped_ptr<base::DictionaryValue> misspelling(
+        SerializeMisspelling(raw_misspelling));
+    misspelling->SetBoolean("isFirstInSession", is_first_feedback_batch);
+    misspelling->SetBoolean("isAutoCorrection", false);
+    list->Append(misspelling.release());
   }
   return list;
 }
 
 // Builds feedback parameters from |suggestion_info|, |language|, and |country|.
-// Takes ownership of |suggestion_list|. The caller owns the result.
-base::DictionaryValue* BuildParams(base::ListValue* suggestion_info,
-                                   const std::string& language,
-                                   const std::string& country) {
-  base::DictionaryValue* params = new base::DictionaryValue;
-  params->Set("suggestionInfo", suggestion_info);
+// Takes ownership of |suggestion_list|.
+scoped_ptr<base::DictionaryValue> BuildParams(
+    scoped_ptr<base::ListValue> suggestion_info,
+    const std::string& language,
+    const std::string& country) {
+  scoped_ptr<base::DictionaryValue> params(new base::DictionaryValue);
+  params->Set("suggestionInfo", suggestion_info.release());
   params->SetString("key", google_apis::GetAPIKey());
   params->SetString("language", language);
   params->SetString("originCountry", country);
@@ -124,15 +123,15 @@ base::DictionaryValue* BuildParams(base::ListValue* suggestion_info,
   return params;
 }
 
-// Builds feedback data from |params|. Takes ownership of |params|. The caller
-// owns the result.
-base::Value* BuildFeedbackValue(base::DictionaryValue* params,
-                                const std::string& api_version) {
-  base::DictionaryValue* result = new base::DictionaryValue;
-  result->Set("params", params);
+// Builds feedback data from |params|. Takes ownership of |params|.
+scoped_ptr<base::Value> BuildFeedbackValue(
+    scoped_ptr<base::DictionaryValue> params,
+    const std::string& api_version) {
+  scoped_ptr<base::DictionaryValue> result(new base::DictionaryValue);
+  result->Set("params", params.release());
   result->SetString("method", "spelling.feedback");
   result->SetString("apiVersion", api_version);
-  return result;
+  return std::move(result);

[groby-ooo-7-16, 2016/02/09 20:20:26]

Does this need to be std::move? The move ctor _should_ handle upcasts.

[Kevin Bailey, 2016/02/09 22:26:39]

../../chrome/browser/spellchecker/feedback_sender.cc:134:10: error: no viable
conversion from returned value of type 'scoped_ptr<base::DictionaryValue>' to
function return type 'scoped_ptr<base::Value>'
  return result;

 }
 
 // Returns true if the misspelling location is within text bounds.
@@ -206,9 +205,8 @@ void FeedbackSender::AddedToDictionary(uint32_t hash) {
   misspelling->timestamp = base::Time::Now();
   const std::set<uint32_t>& hashes =
       feedback_.FindMisspellings(GetMisspelledString(*misspelling));
-  for (std::set<uint32_t>::const_iterator hash_it = hashes.begin();
-       hash_it != hashes.end(); ++hash_it) {
-    Misspelling* duplicate_misspelling = feedback_.GetMisspelling(*hash_it);
+  for (uint32_t hash : hashes) {
+    Misspelling* duplicate_misspelling = feedback_.GetMisspelling(hash);
     if (!duplicate_misspelling || duplicate_misspelling->action.IsFinal())
       continue;
     duplicate_misspelling->action.set_type(SpellcheckAction::TYPE_ADD_TO_DICT);
@@ -283,25 +281,22 @@ void FeedbackSender::OnSpellcheckResults(
   for (size_t i = 0; i < markers.size(); ++i)
     marker_map[markers[i].offset] = markers[i].hash;
 
-  for (std::vector<SpellCheckResult>::iterator result_it = results->begin();
-       result_it != results->end();
-       ++result_it) {
-    if (!IsInBounds(result_it->location, result_it->length, text.length()))
+  for (auto& result : *results) {
+    if (!IsInBounds(result.location, result.length, text.length()))
       continue;
-    MarkerMap::const_iterator marker_it = marker_map.find(result_it->location);
+    MarkerMap::const_iterator marker_it = marker_map.find(result.location);
     if (marker_it != marker_map.end() &&
         feedback_.HasMisspelling(marker_it->second)) {
       // If the renderer already has a marker for this spellcheck result, then
       // set the hash of the spellcheck result to be the same as the marker.
-      result_it->hash = marker_it->second;
+      result.hash = marker_it->second;
     } else {
       // If the renderer does not yet have a marker for this spellcheck result,
       // then generate a new hash for the spellcheck result.
-      result_it->hash = BuildHash(session_start_, ++misspelling_counter_);
+      result.hash = BuildHash(session_start_, ++misspelling_counter_);
     }
     // Save the feedback data for the spellcheck result.
-    feedback_.AddMisspelling(renderer_process_id,
-                             BuildFeedback(*result_it, text));
+    feedback_.AddMisspelling(renderer_process_id, BuildFeedback(result, text));
   }
 }
 
@@ -375,14 +370,12 @@ void FeedbackSender::RequestDocumentMarkers() {
   std::vector<int> known_renderers = feedback_.GetRendersWithMisspellings();
   std::sort(known_renderers.begin(), known_renderers.end());
   std::vector<int> dead_renderers =
-      base::STLSetDifference<std::vector<int> >(known_renderers,
-                                                alive_renderers);
-  for (std::vector<int>::const_iterator it = dead_renderers.begin();
-       it != dead_renderers.end();
-       ++it) {
+      base::STLSetDifference<std::vector<int>>(known_renderers,
+                                               alive_renderers);
+  for (int renderer : dead_renderers) {

[groby-ooo-7-16, 2016/02/09 20:20:26]

nit: renderer_process_id (I wish we had a type for this, but since we don't,
let's make clear what the int means)

[Kevin Bailey, 2016/02/09 22:26:39]

Done.

     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&FeedbackSender::OnReceiveDocumentMarkers,
-                              AsWeakPtr(), *it, std::vector<uint32_t>()));
+                              AsWeakPtr(), renderer, std::vector<uint32_t>()));
   }
 }
 
@@ -420,7 +413,7 @@ void FeedbackSender::SendFeedback(const std::vector<Misspelling>& feedback_data,
   sender->SetUploadData("application/json", feedback);
   senders_.push_back(sender);
 
-  // Request context is NULL in testing.
+  // Request context is nullptr in testing.
   if (request_context_.get()) {
     sender->SetRequestContext(request_context_.get());
     sender->Start();