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..e4bc213ca9027d16b6e41918993e9be4587653b5 100644
--- a/chrome/browser/spellchecker/feedback_sender.cc
+++ b/chrome/browser/spellchecker/feedback_sender.cc
@@ -43,6 +43,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/values.h"
+#include "chrome/browser/spellchecker/set_difference_container.h"
 #include "chrome/browser/spellchecker/word_trimmer.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/spellcheck_common.h"
@@ -70,11 +71,9 @@ const int kMinIntervalSeconds = 5;
 // Returns a hash of |session_start|, the current timestamp, and
 // |suggestion_index|.
 uint32_t BuildHash(const base::Time& session_start, size_t suggestion_index) {
-  return base::Hash(
-      base::StringPrintf("%" PRId64 "%" PRId64 "%" PRIuS,
-                         session_start.ToInternalValue(),
-                         base::Time::Now().ToInternalValue(),
-                         suggestion_index));
+  return base::Hash(base::StringPrintf(
+      "%" PRId64 "%" PRId64 "%" PRIuS, session_start.ToInternalValue(),
+      base::Time::Now().ToInternalValue(), suggestion_index));
 }
 
 // Returns a pending feedback data structure for the spellcheck |result| and
@@ -82,41 +81,38 @@ uint32_t BuildHash(const base::Time& session_start, size_t suggestion_index) {
 Misspelling BuildFeedback(const SpellCheckResult& result,
                           const base::string16& text) {
   size_t start = result.location;
-  base::string16 context = TrimWords(&start,
-                               start + result.length,
-                               text,
-                               chrome::spellcheck_common::kContextWordCount);
-  return Misspelling(context,
-                     start,
-                     result.length,
+  base::string16 context =
+      TrimWords(&start, start + result.length, text,
+                chrome::spellcheck_common::kContextWordCount);
+  return Misspelling(context, start, result.length,
                      std::vector<base::string16>(1, result.replacement),
                      result.hash);
 }
 
 // Builds suggestion info from |suggestions|. The caller owns the result.
-base::ListValue* BuildSuggestionInfo(
-    const std::vector<Misspelling>& suggestions,
-    bool is_first_feedback_batch) {
-  base::ListValue* list = new base::ListValue;
+scoped_ptr<base::ListValue> BuildSuggestionInfo(
+    const std::vector<Misspelling>& suggestions, bool is_first_feedback_batch) {
+  scoped_ptr<base::ListValue> list(new base::ListValue);
   for (std::vector<Misspelling>::const_iterator suggestion_it =

[please use gerrit instead, 2016/02/03 23:59:10]

Let's switch to "for (const auto& suggestion : suggestions)" loop in this or
later patch.

[Kevin Bailey, 2016/02/04 16:34:11]

This was probably the most dangerous change, from a fat-finger perspective. I
think I got them all except ones where it needed the iterator.

            suggestions.begin();
-       suggestion_it != suggestions.end();
-       ++suggestion_it) {
-    base::DictionaryValue* suggestion = SerializeMisspelling(*suggestion_it);
+       suggestion_it != suggestions.end(); ++suggestion_it) {
+    scoped_ptr<base::DictionaryValue> suggestion(
+        SerializeMisspelling(*suggestion_it));
     suggestion->SetBoolean("isFirstInSession", is_first_feedback_batch);
     suggestion->SetBoolean("isAutoCorrection", false);
-    list->Append(suggestion);
+    list->Append(suggestion.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);
+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);
@@ -126,18 +122,18 @@ base::DictionaryValue* BuildParams(base::ListValue* suggestion_info,
 
 // 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);
+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 scoped_ptr<base::Value>(result.release());

[please use gerrit instead, 2016/02/03 23:59:10]

Usually "return result;" gets the job done. If the compiler complains about
conversion from DicitonaryValue to Value, try using "return result.Pass();"
which does the upcasting for you. If ::Pass() is being deprecated (I do not
recall), try "result std::move(result);" as the last resort.

If none of these options work, fine, create an extra scoped_ptr :-)

[Kevin Bailey, 2016/02/04 16:34:11]

Ya, I was disappointed that it didn't automatically convert.

 }
 
 // Returns true if the misspelling location is within text bounds.
-bool IsInBounds(int misspelling_location,
-                int misspelling_length,
+bool IsInBounds(int misspelling_location, int misspelling_length,
                 size_t text_length) {
   return misspelling_location >= 0 && misspelling_length > 0 &&
          static_cast<size_t>(misspelling_location) < text_length &&
@@ -182,15 +178,13 @@ FeedbackSender::FeedbackSender(net::URLRequestContextGetter* request_context,
   }
 }
 
-FeedbackSender::~FeedbackSender() {
-}
+FeedbackSender::~FeedbackSender() {}
 
 void FeedbackSender::SelectedSuggestion(uint32_t hash, int suggestion_index) {
   Misspelling* misspelling = feedback_.GetMisspelling(hash);
   // GetMisspelling() returns null for flushed feedback. Feedback is flushed
   // when the session expires every |kSessionHours| hours.
-  if (!misspelling)
-    return;
+  if (!misspelling) return;

[please use gerrit instead, 2016/02/03 23:59:10]

Which clang-format are you using? I'm surprised it wants to do this. Perhaps
it's not using the Chromium style, which is specified in
https://code.google.com/p/chromium/codesearch#chromium/src/.clang-format

[Kevin Bailey, 2016/02/04 16:34:11]

I'm glad you asked. depot_tools was at the end of my PATH, not beginning, but in
the end, I'm not sure it made much difference.

   misspelling->action.set_type(SpellcheckAction::TYPE_SELECT);
   misspelling->action.set_index(suggestion_index);
   misspelling->timestamp = base::Time::Now();
@@ -200,8 +194,7 @@ void FeedbackSender::AddedToDictionary(uint32_t hash) {
   Misspelling* misspelling = feedback_.GetMisspelling(hash);
   // GetMisspelling() returns null for flushed feedback. Feedback is flushed
   // when the session expires every |kSessionHours| hours.
-  if (!misspelling)
-    return;
+  if (!misspelling) return;
   misspelling->action.set_type(SpellcheckAction::TYPE_ADD_TO_DICT);
   misspelling->timestamp = base::Time::Now();
   const std::set<uint32_t>& hashes =
@@ -220,8 +213,7 @@ void FeedbackSender::RecordInDictionary(uint32_t hash) {
   Misspelling* misspelling = feedback_.GetMisspelling(hash);
   // GetMisspelling() returns null for flushed feedback. Feedback is flushed
   // when the session expires every |kSessionHours| hours.
-  if (!misspelling)
-    return;
+  if (!misspelling) return;
   misspelling->action.set_type(SpellcheckAction::TYPE_IN_DICTIONARY);
 }
 
@@ -229,8 +221,7 @@ void FeedbackSender::IgnoredSuggestions(uint32_t hash) {
   Misspelling* misspelling = feedback_.GetMisspelling(hash);
   // GetMisspelling() returns null for flushed feedback. Feedback is flushed
   // when the session expires every |kSessionHours| hours.
-  if (!misspelling)
-    return;
+  if (!misspelling) return;
   misspelling->action.set_type(SpellcheckAction::TYPE_PENDING_IGNORE);
   misspelling->timestamp = base::Time::Now();
 }
@@ -240,24 +231,21 @@ void FeedbackSender::ManuallyCorrected(uint32_t hash,
   Misspelling* misspelling = feedback_.GetMisspelling(hash);
   // GetMisspelling() returns null for flushed feedback. Feedback is flushed
   // when the session expires every |kSessionHours| hours.
-  if (!misspelling)
-    return;
+  if (!misspelling) return;
   misspelling->action.set_type(SpellcheckAction::TYPE_MANUALLY_CORRECTED);
   misspelling->action.set_value(correction);
   misspelling->timestamp = base::Time::Now();
 }
 
 void FeedbackSender::OnReceiveDocumentMarkers(
-    int renderer_process_id,
-    const std::vector<uint32_t>& markers) {
+    int renderer_process_id, const std::vector<uint32_t>& markers) {
   if ((base::Time::Now() - session_start_).InHours() >=
       chrome::spellcheck_common::kSessionHours) {
     FlushFeedback();
     return;
   }
 
-  if (!feedback_.RendererHasMisspellings(renderer_process_id))
-    return;
+  if (!feedback_.RendererHasMisspellings(renderer_process_id)) return;
 
   feedback_.FinalizeRemovedMisspellings(renderer_process_id, markers);
   SendFeedback(feedback_.GetMisspellingsInRenderer(renderer_process_id),
@@ -267,13 +255,11 @@ void FeedbackSender::OnReceiveDocumentMarkers(
 }
 
 void FeedbackSender::OnSpellcheckResults(
-    int renderer_process_id,
-    const base::string16& text,
+    int renderer_process_id, const base::string16& text,
     const std::vector<SpellCheckMarker>& markers,
     std::vector<SpellCheckResult>* results) {
   // Don't collect feedback if not going to send it.
-  if (!timer_.IsRunning())
-    return;
+  if (!timer_.IsRunning()) return;
 
   // Generate a map of marker offsets to marker hashes. This map helps to
   // efficiently lookup feedback data based on the position of the misspelling
@@ -284,8 +270,7 @@ void FeedbackSender::OnSpellcheckResults(
     marker_map[markers[i].offset] = markers[i].hash;
 
   for (std::vector<SpellCheckResult>::iterator result_it = results->begin();
-       result_it != results->end();
-       ++result_it) {
+       result_it != results->end(); ++result_it) {
     if (!IsInBounds(result_it->location, result_it->length, text.length()))
       continue;
     MarkerMap::const_iterator marker_it = marker_map.find(result_it->location);
@@ -313,8 +298,7 @@ void FeedbackSender::OnLanguageCountryChange(const std::string& language,
 }
 
 void FeedbackSender::StartFeedbackCollection() {
-  if (timer_.IsRunning())
-    return;
+  if (timer_.IsRunning()) return;
 
   int interval_seconds = chrome::spellcheck_common::kFeedbackIntervalSeconds;
   // This command-line switch is for testing and temporary.
@@ -330,18 +314,14 @@ void FeedbackSender::StartFeedbackCollection() {
       interval_seconds = kMinIntervalSeconds;
     static const int kSessionSeconds =
         chrome::spellcheck_common::kSessionHours * 60 * 60;
-    if (interval_seconds >  kSessionSeconds)
-      interval_seconds = kSessionSeconds;
+    if (interval_seconds > kSessionSeconds) interval_seconds = kSessionSeconds;
   }
-  timer_.Start(FROM_HERE,
-               base::TimeDelta::FromSeconds(interval_seconds),
-               this,
+  timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(interval_seconds), this,
                &FeedbackSender::RequestDocumentMarkers);
 }
 
 void FeedbackSender::StopFeedbackCollection() {
-  if (!timer_.IsRunning())
-    return;
+  if (!timer_.IsRunning()) return;
 
   FlushFeedback();
   timer_.Stop();
@@ -349,8 +329,7 @@ void FeedbackSender::StopFeedbackCollection() {
 
 void FeedbackSender::OnURLFetchComplete(const net::URLFetcher* source) {
   for (ScopedVector<net::URLFetcher>::iterator sender_it = senders_.begin();
-       sender_it != senders_.end();
-       ++sender_it) {
+       sender_it != senders_.end(); ++sender_it) {
     if (*sender_it == source) {
       senders_.erase(sender_it);
       return;
@@ -364,8 +343,7 @@ void FeedbackSender::RequestDocumentMarkers() {
   std::set<int> alive_renderers;
   for (content::RenderProcessHost::iterator it(
            content::RenderProcessHost::AllHostsIterator());
-       !it.IsAtEnd();
-       it.Advance()) {
+       !it.IsAtEnd(); it.Advance()) {
     alive_renderers.insert(it.GetCurrentValue()->GetID());
     it.GetCurrentValue()->Send(new SpellCheckMsg_RequestDocumentMarkers());
   }
@@ -374,21 +352,17 @@ void FeedbackSender::RequestDocumentMarkers() {
   // longer alive.
   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) {
+  spellcheck::set_difference_container<std::vector<int>, std::set<int> >

[please use gerrit instead, 2016/02/03 23:59:10]

The ">>" is OK in C++11 now. No need to insert a space there anymore.

[Kevin Bailey, 2016/02/04 16:34:11]

Ok, I thought it was "allowed", not "mandated".

+      renderers(known_renderers, alive_renderers);

[please use gerrit instead, 2016/02/03 23:59:10]

s/renderers/dead_renderers/

[Kevin Bailey, 2016/02/04 16:34:11]

Done.

+  for (auto renderer : renderers) {

[please use gerrit instead, 2016/02/03 23:59:10]

for (const auto& renderer : dead_renderers) {
...

To make sure that "auto" does not attempt to create a copy or allow
modifications.

[Kevin Bailey, 2016/02/04 16:34:11]

If you wish, but it's just an int, and auto is not allowed to enable
modification.

     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&FeedbackSender::OnReceiveDocumentMarkers,
-                              AsWeakPtr(), *it, std::vector<uint32_t>()));
+                              AsWeakPtr(), renderer, std::vector<uint32_t>()));
   }
 }
 
 void FeedbackSender::FlushFeedback() {
-  if (feedback_.Empty())
-    return;
+  if (feedback_.Empty()) return;
   feedback_.FinalizeAllMisspellings();
   SendFeedback(feedback_.GetAllMisspellings(),
                renderers_sent_feedback_.empty());
@@ -402,8 +376,7 @@ void FeedbackSender::SendFeedback(const std::vector<Misspelling>& feedback_data,
                                   bool is_first_feedback_batch) {
   scoped_ptr<base::Value> feedback_value(BuildFeedbackValue(
       BuildParams(BuildSuggestionInfo(feedback_data, is_first_feedback_batch),
-                  language_,
-                  country_),
+                  language_, country_),
       api_version_));
   std::string feedback;
   base::JSONWriter::Write(*feedback_value, &feedback);
@@ -412,7 +385,8 @@ void FeedbackSender::SendFeedback(const std::vector<Misspelling>& feedback_data,
   static const int kUrlFetcherId = 0;
   net::URLFetcher* sender =
       net::URLFetcher::Create(kUrlFetcherId, feedback_service_url_,
-                              net::URLFetcher::POST, this).release();
+                              net::URLFetcher::POST, this)
+          .release();
   data_use_measurement::DataUseUserData::AttachToFetcher(
       sender, data_use_measurement::DataUseUserData::SPELL_CHECKER);
   sender->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
@@ -420,7 +394,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();