[spell] Remove spelling service feedback from behind the flag

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 8943667f175b0551b893541c095acb419f90402a..4613e726a93bf9125b68561c9b4be3e21d864119 100644
--- a/chrome/browser/spellchecker/feedback_sender.cc
+++ b/chrome/browser/spellchecker/feedback_sender.cc
@@ -120,11 +120,12 @@ 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) {
+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;
 }
 
@@ -138,26 +139,31 @@ bool IsInBounds(int misspelling_location,
              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
@@ -167,24 +173,6 @@ FeedbackSender::FeedbackSender(net::URLRequestContextGetter* request_context,
         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() {
@@ -318,6 +306,30 @@ void FeedbackSender::OnLanguageCountryChange(const std::string& 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();
@@ -377,7 +389,8 @@ void FeedbackSender::SendFeedback(const std::vector<Misspelling>& feedback_data,
   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);