[spell] Remove spelling service feedback from behind the flag

chrome/browser/spellchecker/feedback_sender.cc

 // Copyright (c) 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.
 //
 // The |FeedbackSender| object stores the user feedback to spellcheck
 // suggestions in a |Feedback| object.
 //
 // When spelling service returns spellcheck results, these results first arrive
 // in |FeedbackSender| to assign hash identifiers for each
 // misspelling-suggestion pair. If the spelling service identifies the same
@@ skipping 102 matching lines @@
   params->Set("suggestionInfo", suggestion_info);
   params->SetString("key", google_apis::GetAPIKey());
   params->SetString("language", language);
   params->SetString("originCountry", country);
   params->SetString("clientName", "Chrome");
   return params;
 }
 
 // Builds feedback data from |params|. Takes ownership of |params|. The caller
 // owns the result.
-base::Value* BuildFeedbackValue(base::DictionaryValue* params) {
+base::Value* BuildFeedbackValue(base::DictionaryValue* params,
+                                const std::string& api_version) {
   base::DictionaryValue* result = new base::DictionaryValue;
   result->Set("params", params);
   result->SetString("method", "spelling.feedback");
-  result->SetString("apiVersion", "v2");
+  result->SetString("apiVersion", api_version);
   return result;
 }
 
 // Returns true if the misspelling location is within text bounds.
 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 &&
          static_cast<size_t>(misspelling_location + misspelling_length) <=
              text_length;
 }
 
+// Returns the feedback API version.
+std::string GetApiVersion() {
+  // This guard is temporary.
+  // TODO(rouslan): Remove the guard. http://crbug.com/247726
+  if (base::FieldTrialList::FindFullName(kFeedbackFieldTrialName) ==
+          kFeedbackFieldTrialEnabledGroupName &&
+      CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableSpellingServiceFeedback)) {

[groby-ooo-7-16, 2013/11/21 02:48:41]

This confuses me, since we have a "FeedbackEnabled" flag, but we always do
feedback. Is that really intentional?

[please use gerrit instead, 2013/11/22 22:06:31]

The flag is to switch from "v2" to "v2-internal" API version for field-trial
participants. I've renamed it to something less confusing:
--enable-spelling-feedback-dogfood.

[please use gerrit instead, 2013/11/22 22:14:22]

--enable-spelling-feedback-field-trial.

+    return "v2-internal";
+  }
+  return "v2";
+}
+
 }  // namespace
 
 FeedbackSender::FeedbackSender(net::URLRequestContextGetter* request_context,
                                const std::string& language,
                                const std::string& country)
     : request_context_(request_context),
+      api_version_(GetApiVersion()),
       language_(language),
       country_(country),
       misspelling_counter_(0),
       session_start_(base::Time::Now()),
       feedback_service_url_(kFeedbackServiceURL) {
-  // This guard is temporary.
-  // TODO(rouslan): Remove the guard. http://crbug.com/247726
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableSpellingServiceFeedback) ||
-      base::FieldTrialList::FindFullName(kFeedbackFieldTrialName) !=
-          kFeedbackFieldTrialEnabledGroupName) {
-    return;
-  }
-
   // The command-line switch is for testing and temporary.
   // TODO(rouslan): Remove the command-line switch when testing is complete.
   // http://crbug.com/247726
   if (CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSpellingServiceFeedbackUrl)) {
     feedback_service_url_ =
         GURL(CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
             switches::kSpellingServiceFeedbackUrl));
   }
-
-  int interval_seconds = chrome::spellcheck_common::kFeedbackIntervalSeconds;
-  // This command-line switch is for testing and temporary.
-  // TODO(rouslan): Remove the command-line switch when testing is complete.
-  // http://crbug.com/247726
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSpellingServiceFeedbackIntervalSeconds)) {
-    base::StringToInt(CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-                          switches::kSpellingServiceFeedbackIntervalSeconds),
-                      &interval_seconds);
-    if (interval_seconds < kMinIntervalSeconds)
-      interval_seconds = kMinIntervalSeconds;
-  }
-
-  timer_.Start(FROM_HERE,
-               base::TimeDelta::FromSeconds(interval_seconds),
-               this,
-               &FeedbackSender::RequestDocumentMarkers);
 }
 
 FeedbackSender::~FeedbackSender() {
 }
 
 void FeedbackSender::SelectedSuggestion(uint32 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)
@@ skipping 113 matching lines @@
   }
 }
 
 void FeedbackSender::OnLanguageCountryChange(const std::string& language,
                                              const std::string& country) {
   FlushFeedback();
   language_ = language;
   country_ = country;
 }
 
+void FeedbackSender::StopFeedbackCollection() {
+  FlushFeedback();
+  timer_.Stop();
+}
+
+void FeedbackSender::StartFeedbackCollection() {
+  int interval_seconds = chrome::spellcheck_common::kFeedbackIntervalSeconds;
+  // This command-line switch is for testing and temporary.
+  // TODO(rouslan): Remove the command-line switch when testing is complete.
+  // http://crbug.com/247726
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSpellingServiceFeedbackIntervalSeconds)) {
+    base::StringToInt(CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+                          switches::kSpellingServiceFeedbackIntervalSeconds),

[groby-ooo-7-16, 2013/11/21 02:48:41]

You might want to clamp to kMaxIntervalSeconds, too?

[please use gerrit instead, 2013/11/22 22:06:31]

Done.

+                      &interval_seconds);
+    if (interval_seconds < kMinIntervalSeconds)
+      interval_seconds = kMinIntervalSeconds;
+  }
+  timer_.Start(FROM_HERE,

[groby-ooo-7-16, 2013/11/21 02:48:41]

I suppose timer automatically cancels pending tasks on destruction, right? (Just
double-checking)

[please use gerrit instead, 2013/11/22 22:06:31]

Correct. Added a comment in feedback_sender.h.

+               base::TimeDelta::FromSeconds(interval_seconds),
+               this,
+               &FeedbackSender::RequestDocumentMarkers);
+}
+
 void FeedbackSender::OnURLFetchComplete(const net::URLFetcher* source) {
   for (ScopedVector<net::URLFetcher>::iterator sender_it = senders_.begin();
        sender_it != senders_.end();
        ++sender_it) {
     if (*sender_it == source) {
       senders_.erase(sender_it);
       return;
     }
   }
   delete source;
@@ skipping 39 matching lines @@
   renderers_sent_feedback_.clear();
   session_start_ = base::Time::Now();
   timer_.Reset();
 }
 
 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_)));
+                  country_),
+      api_version_));
   std::string feedback;
   base::JSONWriter::Write(feedback_value.get(), &feedback);
 
   // The tests use this identifier to mock the URL fetcher.
   static const int kUrlFetcherId = 0;
   net::URLFetcher* sender = net::URLFetcher::Create(
       kUrlFetcherId, feedback_service_url_, net::URLFetcher::POST, this);
   sender->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                        net::LOAD_DO_NOT_SAVE_COOKIES);
   sender->SetUploadData("application/json", feedback);
   senders_.push_back(sender);
 
   // Request context is NULL in testing.
   if (request_context_.get()) {
     sender->SetRequestContext(request_context_.get());
     sender->Start();
   }
 }
 
 }  // namespace spellcheck