[spell] Remove spelling service feedback from behind the flag.

After this patch, all English-speaking users that have 'Ask Google for
Suggestions' enabled will send feedback with API version "v2". The users
in the "SpellingServiceFeedback.Enabled" field trial that append the
"--enable-spelling-feedback-field-trial" command-line flag will send
feedback with API version "v2-internal".

BUG=170514

Review URL: https://codereview.chromium.org/26558006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@237437 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 199 insertions, 51 deletions

diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index 2c4f0d2..39f19b5 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -14977,12 +14977,12 @@ Do you accept?
       The Simple Cache for HTTP is a new cache. It relies on the filesystem for disk space allocation.
     </message>
 
-    <!-- Spelling service feedback. -->
-    <message name="IDS_FLAGS_ENABLE_SPELLING_SERVICE_FEEDBACK_NAME" desc="Name of about:flags option to turn on sending feedback to spelling service.">
-      Spelling Service Feedback.
+    <!-- Spelling feedback field trial. -->
+    <message name="IDS_FLAGS_ENABLE_SPELLING_FEEDBACK_FIELD_TRIAL_NAME" desc="Name of about:flags option to enable the field trial for sending feedback to spelling service.">
+      Spelling Feedback Field Trial.
     </message>
-    <message name="IDS_FLAGS_ENABLE_SPELLING_SERVICE_FEEDBACK_DESCRIPTION" desc="Description of about:flags option to turn on sending feedback to spelling service.">
-      Send user feedback to spelling service. For example, if spelling service marks a misspelled word, but the user adds it to the custom dictionary, then Chrome will send a feedback message to spelling service. Spelling service uses this feedback to improve spelling suggestions.
+    <message name="IDS_FLAGS_ENABLE_SPELLING_FEEDBACK_FIELD_TRIAL_DESCRIPTION" desc="Description of about:flags option to enable the field trial for sending feedback to spelling service.">
+      Enable the field trial for sending user feedback to spelling service.
     </message>
 
     <!-- Web MIDI API. -->
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index c6cf711..b6fb23f 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -1604,11 +1604,11 @@ const Experiment kExperiments[] = {
   },
 #endif
   {
-    "enable-spelling-service-feedback",
-    IDS_FLAGS_ENABLE_SPELLING_SERVICE_FEEDBACK_NAME,
-    IDS_FLAGS_ENABLE_SPELLING_SERVICE_FEEDBACK_DESCRIPTION,
+    "enable-spelling-feedback-field-trial",
+    IDS_FLAGS_ENABLE_SPELLING_FEEDBACK_FIELD_TRIAL_NAME,
+    IDS_FLAGS_ENABLE_SPELLING_FEEDBACK_FIELD_TRIAL_DESCRIPTION,
     kOsAll,
-    SINGLE_VALUE_TYPE(switches::kEnableSpellingServiceFeedback)
+    SINGLE_VALUE_TYPE(switches::kEnableSpellingFeedbackFieldTrial)
   },
   {
     "enable-webgl-draft-extensions",
diff --git a/chrome/browser/spellchecker/feedback_sender.cc b/chrome/browser/spellchecker/feedback_sender.cc
index 8943667..406fa0b 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::kEnableSpellingFeedbackFieldTrial)) {
+    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,40 @@ void FeedbackSender::OnLanguageCountryChange(const std::string& language,
   country_ = country;
 }
 
+void FeedbackSender::StartFeedbackCollection() {
+  if (timer_.IsRunning())
+    return;
+
+  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;
+    static const int kSessionSeconds =
+        chrome::spellcheck_common::kSessionHours * 60 * 60;
+    if (interval_seconds >  kSessionSeconds)
+      interval_seconds = kSessionSeconds;
+  }
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromSeconds(interval_seconds),
+               this,
+               &FeedbackSender::RequestDocumentMarkers);
+}
+
+void FeedbackSender::StopFeedbackCollection() {
+  if (!timer_.IsRunning())
+    return;
+
+  FlushFeedback();
+  timer_.Stop();
+}
+
 void FeedbackSender::OnURLFetchComplete(const net::URLFetcher* source) {
   for (ScopedVector<net::URLFetcher>::iterator sender_it = senders_.begin();
        sender_it != senders_.end();
@@ -377,7 +399,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);
 
diff --git a/chrome/browser/spellchecker/feedback_sender.h b/chrome/browser/spellchecker/feedback_sender.h
index ea647ad..1ec5f3f 100644
--- a/chrome/browser/spellchecker/feedback_sender.h
+++ b/chrome/browser/spellchecker/feedback_sender.h
@@ -103,6 +103,13 @@ class FeedbackSender : public base::SupportsWeakPtr<FeedbackSender>,
   void OnLanguageCountryChange(const std::string& language,
                                const std::string& country);
 
+  // Starts collecting feedback, if it's not already being collected.
+  void StartFeedbackCollection();
+
+  // Sends out all previously collected data and stops collecting feedback, if
+  // it was being collected.
+  void StopFeedbackCollection();
+
  private:
   friend class FeedbackSenderTest;
 
@@ -125,6 +132,9 @@ class FeedbackSender : public base::SupportsWeakPtr<FeedbackSender>,
   // URL request context for the feedback senders.
   scoped_refptr<net::URLRequestContextGetter> request_context_;
 
+  // The feedback API version.
+  const std::string api_version_;
+
   // The language of text. The string is a BCP 47 language tag.
   std::string language_;
 
@@ -149,8 +159,9 @@ class FeedbackSender : public base::SupportsWeakPtr<FeedbackSender>,
   GURL feedback_service_url_;
 
   // A timer to periodically request a list of document spelling markers from
-  // all of the renderers. The timer runs while an instance of this class is
-  // alive.
+  // all of the renderers. The timer starts in StartFeedbackCollection() and
+  // stops in StopFeedbackCollection(). The timer stops and abandons its tasks
+  // on destruction.
   base::RepeatingTimer<FeedbackSender> timer_;
 
   // Feedback senders that need to stay alive for the duration of sending data.
diff --git a/chrome/browser/spellchecker/feedback_sender_unittest.cc b/chrome/browser/spellchecker/feedback_sender_unittest.cc
index ed6ac5be..6bc5c25 100644
--- a/chrome/browser/spellchecker/feedback_sender_unittest.cc
+++ b/chrome/browser/spellchecker/feedback_sender_unittest.cc
@@ -62,21 +62,37 @@ int CountOccurences(const std::string& haystack, const std::string& needle) {
 class FeedbackSenderTest : public testing::Test {
  public:
   FeedbackSenderTest() : ui_thread_(content::BrowserThread::UI, &loop_) {
-    // The command-line switch and the field trial are temporary.
-    // TODO(rouslan): Remove the command-line switch and the field trial.
-    // http://crbug.com/247726
+    feedback_.reset(new FeedbackSender(NULL, kLanguage, kCountry));
+    feedback_->StartFeedbackCollection();
+  }
+
+  virtual ~FeedbackSenderTest() {}
+
+ protected:
+  // Appends the "--enable-spelling-service-feedback" switch to the
+  // command-line.
+  void AppendCommandLineSwitch() {
+    // The command-line switch is temporary.
+    // TODO(rouslan): Remove the command-line switch. http://crbug.com/247726
     CommandLine::ForCurrentProcess()->AppendSwitch(
-        switches::kEnableSpellingServiceFeedback);
+        switches::kEnableSpellingFeedbackFieldTrial);
+    feedback_.reset(new FeedbackSender(NULL, kLanguage, kCountry));
+    feedback_->StartFeedbackCollection();
+  }
+
+  // Enables the "SpellingServiceFeedback.Enabled" field trial.
+  void EnableFieldTrial() {
+    // The field trial is temporary.
+    // TODO(rouslan): Remove the field trial. http://crbug.com/247726
     field_trial_list_.reset(
         new base::FieldTrialList(new metrics::SHA1EntropyProvider("foo")));
     field_trial_ = base::FieldTrialList::CreateFieldTrial(
         kFeedbackFieldTrialName, kFeedbackFieldTrialEnabledGroupName);
     field_trial_->group();
     feedback_.reset(new FeedbackSender(NULL, kLanguage, kCountry));
+    feedback_->StartFeedbackCollection();
   }
-  virtual ~FeedbackSenderTest() {}
 
- protected:
   uint32 AddPendingFeedback() {
     std::vector<SpellCheckResult> results(1, BuildSpellCheckResult());
     feedback_->OnSpellcheckResults(kRendererProcessId,
@@ -433,6 +449,55 @@ TEST_F(FeedbackSenderTest, FeedbackAPI) {
       << "\nActual data:   " << actual_data;
 }
 
+// The default API version is "v2".
+TEST_F(FeedbackSenderTest, DefaultApiVersion) {
+  AddPendingFeedback();
+  feedback_->OnReceiveDocumentMarkers(kRendererProcessId,
+                                      std::vector<uint32>());
+  EXPECT_TRUE(UploadDataContains("\"apiVersion\":\"v2\""));
+  EXPECT_FALSE(UploadDataContains("\"apiVersion\":\"v2-internal\""));
+}
+
+// The API version should not change for field-trial participants that do not
+// append the command-line switch.
+TEST_F(FeedbackSenderTest, FieldTrialAloneHasSameApiVersion) {
+  EnableFieldTrial();
+
+  AddPendingFeedback();
+  feedback_->OnReceiveDocumentMarkers(kRendererProcessId,
+                                      std::vector<uint32>());
+
+  EXPECT_TRUE(UploadDataContains("\"apiVersion\":\"v2\""));
+  EXPECT_FALSE(UploadDataContains("\"apiVersion\":\"v2-internal\""));
+}
+
+// The API version should not change if the command-line switch is appended, but
+// the user is not participating in the field-trial.
+TEST_F(FeedbackSenderTest, CommandLineSwitchAloneHasSameApiVersion) {
+  AppendCommandLineSwitch();
+
+  AddPendingFeedback();
+  feedback_->OnReceiveDocumentMarkers(kRendererProcessId,
+                                      std::vector<uint32>());
+
+  EXPECT_TRUE(UploadDataContains("\"apiVersion\":\"v2\""));
+  EXPECT_FALSE(UploadDataContains("\"apiVersion\":\"v2-internal\""));
+}
+
+// The API version should be different for field-trial participants that also
+// append the command-line switch.
+TEST_F(FeedbackSenderTest, InternalApiVersion) {
+  AppendCommandLineSwitch();
+  EnableFieldTrial();
+
+  AddPendingFeedback();
+  feedback_->OnReceiveDocumentMarkers(kRendererProcessId,
+                                      std::vector<uint32>());
+
+  EXPECT_FALSE(UploadDataContains("\"apiVersion\":\"v2\""));
+  EXPECT_TRUE(UploadDataContains("\"apiVersion\":\"v2-internal\""));
+}
+
 // Duplicate spellcheck results should be matched to the existing markers.
 TEST_F(FeedbackSenderTest, MatchDupliateResultsWithExistingMarkers) {
   uint32 hash = AddPendingFeedback();
@@ -524,4 +589,36 @@ TEST_F(FeedbackSenderTest, IgnoreOutOfBounds) {
   EXPECT_FALSE(IsUploadingData());
 }
 
+// FeedbackSender does not collect and upload feedback when instructed to stop.
+TEST_F(FeedbackSenderTest, CanStopFeedbackCollection) {
+  feedback_->StopFeedbackCollection();
+  AddPendingFeedback();
+  feedback_->OnReceiveDocumentMarkers(kRendererProcessId,
+                                      std::vector<uint32>());
+  EXPECT_FALSE(IsUploadingData());
+}
+
+// FeedbackSender resumes collecting and uploading feedback when instructed to
+// start after stopping.
+TEST_F(FeedbackSenderTest, CanResumeFeedbackCollection) {
+  feedback_->StopFeedbackCollection();
+  feedback_->StartFeedbackCollection();
+  AddPendingFeedback();
+  feedback_->OnReceiveDocumentMarkers(kRendererProcessId,
+                                      std::vector<uint32>());
+  EXPECT_TRUE(IsUploadingData());
+}
+
+// FeedbackSender does not collect data while being stopped and upload it later.
+TEST_F(FeedbackSenderTest, NoFeedbackCollectionWhenStopped) {
+  feedback_->StopFeedbackCollection();
+  AddPendingFeedback();
+  feedback_->OnReceiveDocumentMarkers(kRendererProcessId,
+                                      std::vector<uint32>());
+  feedback_->StartFeedbackCollection();
+  feedback_->OnReceiveDocumentMarkers(kRendererProcessId,
+                                      std::vector<uint32>());
+  EXPECT_FALSE(IsUploadingData());
+}
+
 }  // namespace spellcheck
diff --git a/chrome/browser/spellchecker/spellcheck_service.cc b/chrome/browser/spellchecker/spellcheck_service.cc
index 848dfc7c..2bb13ae 100644
--- a/chrome/browser/spellchecker/spellcheck_service.cc
+++ b/chrome/browser/spellchecker/spellcheck_service.cc
@@ -13,6 +13,7 @@
 #include "chrome/browser/spellchecker/spellcheck_host_metrics.h"
 #include "chrome/browser/spellchecker/spellcheck_hunspell_dictionary.h"
 #include "chrome/browser/spellchecker/spellcheck_platform_mac.h"
+#include "chrome/browser/spellchecker/spelling_service_client.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/spellcheck_messages.h"
 #include "components/user_prefs/user_prefs.h"
@@ -307,6 +308,7 @@ void SpellcheckService::OnSpellCheckDictionaryChanged() {
   chrome::spellcheck_common::GetISOLanguageCountryCodeFromLocale(
       dictionary, &language_code, &country_code);
   feedback_sender_->OnLanguageCountryChange(language_code, country_code);
+  UpdateFeedbackSenderState();
 }
 
 void SpellcheckService::OnUseSpellingServiceChanged() {
@@ -314,4 +316,14 @@ void SpellcheckService::OnUseSpellingServiceChanged() {
       prefs::kSpellCheckUseSpellingService);
   if (metrics_)
     metrics_->RecordSpellingServiceStats(enabled);
+  UpdateFeedbackSenderState();
+}
+
+void SpellcheckService::UpdateFeedbackSenderState() {
+  if (SpellingServiceClient::IsAvailable(
+          context_, SpellingServiceClient::SPELLCHECK)) {
+    feedback_sender_->StartFeedbackCollection();
+  } else {
+    feedback_sender_->StopFeedbackCollection();
+  }
 }
diff --git a/chrome/browser/spellchecker/spellcheck_service.h b/chrome/browser/spellchecker/spellcheck_service.h
index 8f000fd..ad5438b 100644
--- a/chrome/browser/spellchecker/spellcheck_service.h
+++ b/chrome/browser/spellchecker/spellcheck_service.h
@@ -147,6 +147,10 @@ class SpellcheckService : public BrowserContextKeyedService,
   // Notification handler for changes to prefs::kSpellCheckUseSpellingService.
   void OnUseSpellingServiceChanged();
 
+  // Enables the feedback sender if spelling server is available and enabled.
+  // Otherwise disables the feedback sender.
+  void UpdateFeedbackSenderState();
+
   PrefChangeRegistrar pref_change_registrar_;
   content::NotificationRegistrar registrar_;
 
diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc
index 19f61da..087c1a4 100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -735,9 +735,10 @@ const char kEnableSpdy4a2[]                 = "enable-spdy4a2";
 // Enables auto correction for misspelled words.
 const char kEnableSpellingAutoCorrect[]     = "enable-spelling-auto-correct";
 
-// Enables sending user feedback to spelling service.
-const char kEnableSpellingServiceFeedback[] =
-    "enable-spelling-service-feedback";
+// Enables participation in the field trial for user feedback to spelling
+// service.
+const char kEnableSpellingFeedbackFieldTrial[] =
+    "enable-spelling-feedback-field-trial";
 
 // Enables the stacked tabstrip.
 const char kEnableStackedTabStrip[]         = "enable-stacked-tab-strip";
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index 4fec5f2..f5d2ef7 100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -211,7 +211,7 @@ extern const char kEnableSpdy2[];
 extern const char kDisableSpdy31[];
 extern const char kEnableSpdy4a2[];
 extern const char kEnableSpellingAutoCorrect[];
-extern const char kEnableSpellingServiceFeedback[];
+extern const char kEnableSpellingFeedbackFieldTrial[];
 extern const char kEnableStackedTabStrip[];
 extern const char kEnableStreamlinedHostedApps[];
 extern const char kEnableSuggestionsTabPage[];