[Spellcheck] Clean up Spelling Service Finch experiment

The Spelling Service ("Enhanced Spell Check") was migrated from a
JSON-RPC endpoint to a REST endpoint. This migration was rolled out
as a finch experiment to ensure nothing had regressed.

Now that everything has been working well for a few months, it's time to
clean up everything related to the Finch experiment and the old JSON-RPC
endpoint.

Metrics about the new endpoint:
https://uma.googleplex.com/histograms?sid=53ae9a4522ca75cb8f945bf4422baafe

Migration bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=943108

Migration mini design doc:
https://docs.google.com/document/d/1tmqz3QTF-lhbbkBxqOSYMdNZ-OOhlMIhb8hczhbZVhQ

Bug: 943108
Change-Id: If46ffbbbdf956f7ed7000ade6c3bd9c5252361f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1932095Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Commit-Queue: Guillaume Jenkins <gujen@google.com>
Cr-Commit-Position: refs/heads/master@{#719592}

chrome/browser/spellchecker/spelling_service_client_unittest.cc

@@ -16,12 +16,10 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
-#include "base/test/scoped_feature_list.h"
 #include "base/values.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/prefs/pref_service.h"
 #include "components/spellcheck/browser/pref_names.h"
-#include "components/spellcheck/common/spellcheck_features.h"
 #include "components/spellcheck/common/spellcheck_result.h"
 #include "content/public/test/browser_task_environment.h"
 #include "net/base/load_flags.h"
@@ -43,7 +41,6 @@ struct SpellingServiceTestCase {
   bool success;
   const char* corrected_text;
   std::string language;
-  bool restEndpoint;
 };
 
 // A class derived from the SpellingServiceClient class used by the
@@ -129,7 +126,6 @@ class SpellingServiceClientTest
   content::BrowserTaskEnvironment task_environment_;
   TestingSpellingServiceClient client_;
   TestingProfile profile_;
-  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 }  // namespace
@@ -151,15 +147,6 @@ using Redirects = std::vector<
 
 TEST_P(SpellingServiceClientTest, RequestTextCheck) {
   auto test_case = GetParam();
-  bool is_rest = test_case.restEndpoint;
-
-  if (is_rest) {
-    scoped_feature_list_.InitAndEnableFeature(
-        spellcheck::kSpellingServiceRestApi);
-  } else {
-    scoped_feature_list_.InitAndDisableFeature(
-        spellcheck::kSpellingServiceRestApi);
-  }
 
   PrefService* pref = profile_.GetPrefs();
   pref->SetBoolean(spellcheck::prefs::kSpellCheckEnable, true);
@@ -228,32 +215,21 @@ TEST_P(SpellingServiceClientTest, RequestTextCheck) {
   ASSERT_TRUE(value.get());
 
   std::string method;
-  std::string version;
-  if (is_rest) {
   EXPECT_FALSE(value->GetString("method", &method));
+  std::string version;
   EXPECT_FALSE(value->GetString("apiVersion", &version));
-  } else {
-    EXPECT_TRUE(value->GetString("method", &method));
-    EXPECT_EQ("spelling.check", method);
-    EXPECT_TRUE(value->GetString("apiVersion", &version));
-    EXPECT_EQ(base::StringPrintf("v%d", test_case.request_type), version);
-  }
-
   std::string sanitized_text;
-  EXPECT_TRUE(
-      value->GetString(is_rest ? "text" : "params.text", &sanitized_text));
+  EXPECT_TRUE(value->GetString("text", &sanitized_text));
   EXPECT_EQ(test_case.sanitized_request_text, sanitized_text);
   std::string language;
-  EXPECT_TRUE(
-      value->GetString(is_rest ? "language" : "params.language", &language));
+  EXPECT_TRUE(value->GetString("language", &language));
   std::string expected_language =
       test_case.language.empty() ? std::string("en") : test_case.language;
   EXPECT_EQ(expected_language, language);
   std::string expected_country;
   ASSERT_TRUE(GetExpectedCountry(language, &expected_country));
   std::string country;
-  EXPECT_TRUE(value->GetString(
-      is_rest ? "originCountry" : "params.originCountry", &country));
+  EXPECT_TRUE(value->GetString("originCountry", &country));
   EXPECT_EQ(expected_country, country);
 }
 
@@ -261,7 +237,7 @@ INSTANTIATE_TEST_SUITE_P(
     SpellingService,
     SpellingServiceClientTest,
     testing::Values(
-        // Test cases for the RPC endpoint
+        // Test cases for the REST endpoint
         SpellingServiceTestCase{
             L"",
             "",
@@ -271,7 +247,6 @@ INSTANTIATE_TEST_SUITE_P(
             false,
             "",
             "af",
-            false,
         },
         SpellingServiceTestCase{
             L"chromebook",
@@ -282,7 +257,6 @@ INSTANTIATE_TEST_SUITE_P(
             true,
             "chromebook",
             "af",
-            false,
         },
         SpellingServiceTestCase{
             L"chrombook",
@@ -290,7 +264,6 @@ INSTANTIATE_TEST_SUITE_P(
             SpellingServiceClient::SUGGEST,
             net::HttpStatusCode(200),
             "{\n"
-            "  \"result\": {\n"
             "  \"spellingCheckResponse\": {\n"
             "    \"misspellings\": [{\n"
             "      \"charStart\": 0,\n"
@@ -299,12 +272,10 @@ INSTANTIATE_TEST_SUITE_P(
             "      \"canAutoCorrect\": false\n"
             "    }]\n"
             "  }\n"
-            "  }\n"
             "}",
             true,
             "chromebook",
             "af",
-            false,
         },
         SpellingServiceTestCase{
             L"",
@@ -315,111 +286,7 @@ INSTANTIATE_TEST_SUITE_P(
             false,
             "",
             "en",
-            false,
-        },
-        SpellingServiceTestCase{
-            L"I have been to USA.",
-            "I have been to USA.",
-            SpellingServiceClient::SPELLCHECK,
-            net::HttpStatusCode(200),
-            "{}",
-            true,
-            "I have been to USA.",
-            "en",
-            false,
-        },
-        SpellingServiceTestCase{
-            L"I have bean to USA.",
-            "I have bean to USA.",
-            SpellingServiceClient::SPELLCHECK,
-            net::HttpStatusCode(200),
-            "{\n"
-            "  \"result\": {\n"
-            "    \"spellingCheckResponse\": {\n"
-            "      \"misspellings\": [{\n"
-            "        \"charStart\": 7,\n"
-            "        \"charLength\": 4,\n"
-            "        \"suggestions\": [{ \"suggestion\": \"been\" }],\n"
-            "        \"canAutoCorrect\": false\n"
-            "      }]\n"
-            "    }\n"
-            "  }\n"
-            "}",
-            true,
-            "I have been to USA.",
-            "en",
-            false,
-        },
-        SpellingServiceTestCase{
-            L"I\x2019mattheIn'n'Out.",
-            "I'mattheIn'n'Out.",
-            SpellingServiceClient::SPELLCHECK,
-            net::HttpStatusCode(200),
-            "{\n"
-            "  \"result\": {\n"
-            "    \"spellingCheckResponse\": {\n"
-            "      \"misspellings\": [{\n"
-            "        \"charStart\": 0,\n"
-            "        \"charLength\": 16,\n"
-            "        \"suggestions\":"
-            " [{ \"suggestion\": \"I'm at the In'N'Out\" }],\n"
-            "        \"canAutoCorrect\": false\n"
-            "      }]\n"
-            "    }\n"
-            "  }\n"
-            "}",
-            true,
-            "I'm at the In'N'Out.",
-            "en",
-            false,
-        },
-
-        // Test cases for the REST endpoint
-        SpellingServiceTestCase{
-            L"",
-            "",
-            SpellingServiceClient::SUGGEST,
-            net::HttpStatusCode(500),
-            "",
-            false,
-            "",
-            "af",
-            true,
-        },
-        SpellingServiceTestCase{
-            L"chromebook",
-            "chromebook",
-            SpellingServiceClient::SUGGEST,
-            net::HttpStatusCode(200),
-            "{}",
-            true,
-            "chromebook",
-            "af",
-            true,
-        },
-        SpellingServiceTestCase{
-            L"chrombook",
-            "chrombook",
-            SpellingServiceClient::SUGGEST,
-            net::HttpStatusCode(200),
-            "{\n"
-            "  \"spellingCheckResponse\": {\n"
-            "    \"misspellings\": [{\n"
-            "      \"charStart\": 0,\n"
-            "      \"charLength\": 9,\n"
-            "      \"suggestions\": [{ \"suggestion\": \"chromebook\" }],\n"
-            "      \"canAutoCorrect\": false\n"
-            "    }]\n"
-            "  }\n"
-            "}",
-            true,
-            "chromebook",
-            "af",
-            true,
         },
-        SpellingServiceTestCase{L"", "", SpellingServiceClient::SPELLCHECK,
-                                net::HttpStatusCode(500), "", false, "", "en",
-                                true},
         SpellingServiceTestCase{
             L"I have been to USA.",
             "I have been to USA.",
@@ -429,7 +296,6 @@ INSTANTIATE_TEST_SUITE_P(
             true,
             "I have been to USA.",
             "en",
-            true,
         },
         SpellingServiceTestCase{
             L"I have bean to USA.",
@@ -449,7 +315,6 @@ INSTANTIATE_TEST_SUITE_P(
             true,
             "I have been to USA.",
             "en",
-            true,
         },
         SpellingServiceTestCase{
             L"I\x2019mattheIn'n'Out.",
@@ -470,7 +335,6 @@ INSTANTIATE_TEST_SUITE_P(
             true,
             "I'm at the In'N'Out.",
             "en",
-            true,
         }));
 
 // Verify that SpellingServiceClient::IsAvailable() returns true only when it

components/spellcheck/browser/spelling_service_client.cc

@@ -10,7 +10,6 @@
 #include <memory>
 
 #include "base/bind.h"
-#include "base/feature_list.h"
 #include "base/json/json_reader.h"
 #include "base/json/string_escape.h"
 #include "base/memory/ptr_util.h"
@@ -24,7 +23,6 @@
 #include "components/prefs/pref_service.h"
 #include "components/spellcheck/browser/pref_names.h"
 #include "components/spellcheck/common/spellcheck_common.h"
-#include "components/spellcheck/common/spellcheck_features.h"
 #include "components/spellcheck/common/spellcheck_result.h"
 #include "components/user_prefs/user_prefs.h"
 #include "content/public/browser/browser_context.h"
@@ -38,21 +36,12 @@
 
 namespace {
 
-// The old JSON-RPC endpoint for requesting spell checking and sending user
-// feedback.
-const char kSpellingServiceRpcURL[] = "https://www.googleapis.com/rpc";
-
-// The new REST endpoint for requesting spell checking and sending user
-// feedback.
+// The REST endpoint for requesting spell checking and sending user feedback.
 const char kSpellingServiceRestURL[] =
     "https://www.googleapis.com/spelling/v%d/spelling/check?key=%s";
 
 // The spellcheck suggestions object key in the JSON response from the spelling
-// service when using the JSON-RPC endpoint.
-const char kMisspellingsRpcPath[] = "result.spellingCheckResponse.misspellings";
-
-// The spellcheck suggestions object key in the JSON response from the spelling
-// service when using the REST endpoint.
+// service.
 const char kMisspellingsRestPath[] = "spellingCheckResponse.misspellings";
 
 // The location of error messages in JSON response from spelling service.
@@ -99,9 +88,7 @@ bool SpellingServiceClient::RequestTextCheck(
 
   std::string api_key = google_apis::GetAPIKey();
   std::string encoded_text = base::GetQuotedJSONString(text_copy);
-  std::string request_body;
 
-  if (base::FeatureList::IsEnabled(spellcheck::kSpellingServiceRestApi)) {
   static const char kSpellingRequestRestBodyTemplate[] =
       "{"
       "\"text\":%s,"
@@ -109,27 +96,9 @@ bool SpellingServiceClient::RequestTextCheck(
       "\"originCountry\":\"%s\""
       "}";
 
-    request_body = base::StringPrintf(
-        kSpellingRequestRestBodyTemplate, encoded_text.c_str(),
+  std::string request_body =
+      base::StringPrintf(kSpellingRequestRestBodyTemplate, encoded_text.c_str(),
                          language_code.c_str(), country_code.c_str());
-  } else {
-    static const char kSpellingRequestRpcBodyTemplate[] =
-        "{"
-        "\"method\":\"spelling.check\","
-        "\"apiVersion\":\"v%d\","
-        "\"params\":{"
-        "\"text\":%s,"
-        "\"language\":\"%s\","
-        "\"originCountry\":\"%s\","
-        "\"key\":%s"
-        "}"
-        "}";
-
-    request_body = base::StringPrintf(
-        kSpellingRequestRpcBodyTemplate, type, encoded_text.c_str(),
-        language_code.c_str(), country_code.c_str(),
-        base::GetQuotedJSONString(api_key).c_str());
-  }
 
   // Create traffic annotation tag.
   net::NetworkTrafficAnnotationTag traffic_annotation =
@@ -197,8 +166,8 @@ bool SpellingServiceClient::IsAvailable(content::BrowserContext* context,
                                         ServiceType type) {
   const PrefService* pref = user_prefs::UserPrefs::Get(context);
   DCHECK(pref);
-  // If prefs don't allow spellchecking, if the context is off the record, or if
-  // multilingual spellchecking is enabled the spelling service should be
+  // If prefs don't allow spell checking, if enhanced spell check is disabled,
+  // or if the context is off the record, the spelling service should be
   // unavailable.
   if (!pref->GetBoolean(spellcheck::prefs::kSpellCheckEnable) ||
       !pref->GetBoolean(spellcheck::prefs::kSpellCheckUseSpellingService) ||
@@ -232,19 +201,14 @@ void SpellingServiceClient::SetURLLoaderFactoryForTesting(
 }
 
 GURL SpellingServiceClient::BuildEndpointUrl(int type) {
-  if (base::FeatureList::IsEnabled(spellcheck::kSpellingServiceRestApi)) {
     return GURL(base::StringPrintf(kSpellingServiceRestURL, type,
                                    google_apis::GetAPIKey().c_str()));
-  } else {
-    return GURL(kSpellingServiceRpcURL);
-  }
 }
 
 bool SpellingServiceClient::ParseResponse(
     const std::string& data,
     std::vector<SpellCheckResult>* results) {
   // Data is in the following format:
-  //  * result: (only in the RPC API; skipped for the REST API) A root object
   //  * spellingCheckResponse: A wrapper object containing the response
   //    * mispellings: (optional Array<object>) A list of mistakes for the
   //      requested text, with the following format:
@@ -258,7 +222,6 @@ bool SpellingServiceClient::ParseResponse(
   //
   // Example response for "duck goes quisk":
   //  {
-  //    "result": {                   // (Only in the RPC API)
   //    "spellingCheckResponse": {
   //      "misspellings": [{
   //        "charStart": 10,
@@ -270,7 +233,6 @@ bool SpellingServiceClient::ParseResponse(
   //      }]
   //    }
   //  }
-  //  }
   //
   // If the service is not available, the Spelling service returns JSON with an
   // error:
@@ -299,11 +261,8 @@ bool SpellingServiceClient::ParseResponse(
   // have misspelled words, it returns an empty JSON. (In this case, its HTTP
   // status is 200.) We just return true for this case.
   base::ListValue* misspellings = nullptr;
-  std::string mispellingsPath =
-      base::FeatureList::IsEnabled(spellcheck::kSpellingServiceRestApi)
-          ? kMisspellingsRestPath
-          : kMisspellingsRpcPath;
-  if (!value->GetList(mispellingsPath, &misspellings))
+
+  if (!value->GetList(kMisspellingsRestPath, &misspellings))
     return true;
 
   for (size_t i = 0; i < misspellings->GetSize(); ++i) {

components/spellcheck/common/spellcheck_features.cc

@@ -13,9 +13,6 @@ namespace spellcheck {
 
 #if BUILDFLAG(ENABLE_SPELLCHECK)
 
-const base::Feature kSpellingServiceRestApi{"SpellingServiceRestApi",
-                                            base::FEATURE_ENABLED_BY_DEFAULT};
-
 #if defined(OS_WIN)
 const base::Feature kWinUseBrowserSpellChecker{
     "WinUseBrowserSpellChecker", base::FEATURE_DISABLED_BY_DEFAULT};

components/spellcheck/common/spellcheck_features.h

@@ -12,26 +12,23 @@
 namespace spellcheck {
 
 #if BUILDFLAG(ENABLE_SPELLCHECK)
-extern const base::Feature kSpellingServiceRestApi;
-
-#if defined(OS_WIN)
-extern const base::Feature kWinUseBrowserSpellChecker;
-#endif  // defined(OS_WIN)
 
 bool UseBrowserSpellChecker();
 
 #if defined(OS_WIN)
+extern const base::Feature kWinUseBrowserSpellChecker;
+
 bool WindowsVersionSupportsSpellchecker();
 #endif  // defined(OS_WIN)
 
-#endif  // BUILDFLAG(ENABLE_SPELLCHECK)
-
-#if BUILDFLAG(ENABLE_SPELLCHECK) && defined(OS_ANDROID)
+#if defined(OS_ANDROID)
 extern const base::Feature kAndroidSpellChecker;
 extern const base::Feature kAndroidSpellCheckerNonLowEnd;
 
 bool IsAndroidSpellCheckFeatureEnabled();
-#endif  // BUILDFLAG(ENABLE_SPELLCHECK) && defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID)
+
+#endif  // BUILDFLAG(ENABLE_SPELLCHECK)
 
 }  // namespace spellcheck
 

testing/variations/fieldtrial_testing_config.json

@@ -6064,25 +6064,6 @@
             ]
         }
     ],
-    "SpellingServiceRestEndpoint": [
-        {
-            "platforms": [
-                "android",
-                "chromeos",
-                "linux",
-                "mac",
-                "windows"
-            ],
-            "experiments": [
-                {
-                    "name": "Enabled",
-                    "enable_features": [
-                        "SpellingServiceRestApi"
-                    ]
-                }
-            ]
-        }
-    ],
     "SplitCacheByNetworkIsolationKey": [
         {
             "platforms": [