Revert "Spellcheck: Apply policy for languages that lack Hunspell support"

This reverts commit 1527cb7a.

Reason for revert: TestCases/SpellcheckLanguagePolicyHandlersTest.ApplyPolicySettings fails on win7 bot

See crbug.com/1068703 for more info.

Original change's description:
> Spellcheck: Apply policy for languages that lack Hunspell support
> 
> Spellcheck languages that have Windows platform support but not
> Hunspell support (Arabic, Finnish, ...) cannot be disabled or forced
> enabled by enterprise policy. This is because of the use of
> spellcheck::GetCorrespondingSpellCheckLanguage in
> SpellcheckLanguageBlacklistPolicyHandler::SortBlacklistedLanguages and
> SpellcheckLanguagePolicyHandler::SortForcedLanguages, a method that
> only recognizes Hunspell languages.
> 
> The fix is to instead check if a language with policy being enforced is
> on the list of supported accept languages, as these are what are used
> to populate the language list on the language settings page. Note that
> in the upcoming implementation of support for non-Hunspell languages,
> we will only allow Windows platform spellchecking for these accept
> languages (if they have a Windows spellcheck dictionary).
> 
> Instead of just searching for the accept language within the tag, the
> algorithm first tries for an exact match (so "en-CA" would be found),
> then searches for an exact match for the language without locale
> info, that is, what's to the left of the hyphen in a language tag
> (so "ar" would be found for "ar-SA").
> 
> Also adds unit test coverage for the spellcheck policy handlers, which
> previously had none.
> 
> Note that there is an interplay between forced and blocked languages,
> as well as the spellcheck-enabled policy:
> 
>   1) If a language is both forced and blocked, forced wins.
>   2) If spellcheck is disabled by policy, the forced/blocked languages
>      policy has no effect.
> 
> Test cases cover this interplay.
> 
> Bug: 1056850
> Change-Id: Iec54a851d38f00a7aa7597449f7acc30f3dd2de3
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2080450
> Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Bruce Long <brlong@microsoft.com>
> Cr-Commit-Position: refs/heads/master@{#757107}

TBR=avi@chromium.org,rouslan@chromium.org,pcupp@microsoft.com,brlong@microsoft.com,gujen@google.com

Change-Id: I4653ecce1cb2b7162d1a3eba5a5e7ef2238943ad
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1056850, 1068703
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2140672Reviewed-by: Chris Sharp <csharp@chromium.org>
Commit-Queue: Chris Sharp <csharp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757188}

chrome/browser/spellchecker/spellcheck_language_blacklist_policy_handler.cc

@@ -12,13 +12,12 @@
 #include "base/syslog_logging.h"
 #include "base/values.h"
 #include "build/build_config.h"
-#include "chrome/browser/spellchecker/spellcheck_service.h"
 #include "chrome/common/pref_names.h"
 #include "components/policy/core/browser/policy_error_map.h"
 #include "components/policy/policy_constants.h"
 #include "components/prefs/pref_value_map.h"
 #include "components/spellcheck/browser/pref_names.h"
-#include "components/spellcheck/common/spellcheck_features.h"
+#include "components/spellcheck/common/spellcheck_common.h"
 #include "components/strings/grit/components_strings.h"
 
 SpellcheckLanguageBlacklistPolicyHandler::
@@ -114,11 +113,9 @@ void SpellcheckLanguageBlacklistPolicyHandler::SortBlacklistedLanguages(
   // Separate the valid languages from the unknown / unsupported languages and
   // the languages that also appear in the SpellcheckLanguage policy.
   for (const base::Value& language : value->GetList()) {
-    std::string candidate_language =
-        base::TrimWhitespaceASCII(language.GetString(), base::TRIM_ALL)
-            .as_string();
     std::string current_language =
-        SpellcheckService::GetSupportedAcceptLanguageCode(candidate_language);
+        spellcheck::GetCorrespondingSpellCheckLanguage(
+            base::TrimWhitespaceASCII(language.GetString(), base::TRIM_ALL));
 
     if (current_language.empty()) {
       unknown->emplace_back(language.GetString());

chrome/browser/spellchecker/spellcheck_language_policy_handler.cc

@@ -11,13 +11,12 @@
 #include "base/syslog_logging.h"
 #include "base/values.h"
 #include "build/build_config.h"
-#include "chrome/browser/spellchecker/spellcheck_service.h"
 #include "chrome/common/pref_names.h"
 #include "components/policy/core/browser/policy_error_map.h"
 #include "components/policy/policy_constants.h"
 #include "components/prefs/pref_value_map.h"
 #include "components/spellcheck/browser/pref_names.h"
-#include "components/spellcheck/common/spellcheck_features.h"
+#include "components/spellcheck/common/spellcheck_common.h"
 #include "components/strings/grit/components_strings.h"
 
 SpellcheckLanguagePolicyHandler::SpellcheckLanguagePolicyHandler()
@@ -87,11 +86,9 @@ void SpellcheckLanguagePolicyHandler::SortForcedLanguages(
 
   // Separate the valid languages from the unknown / unsupported languages.
   for (const base::Value& language : value->GetList()) {
-    std::string candidate_language =
-        base::TrimWhitespaceASCII(language.GetString(), base::TRIM_ALL)
-            .as_string();
     std::string current_language =
-        SpellcheckService::GetSupportedAcceptLanguageCode(candidate_language);
+        spellcheck::GetCorrespondingSpellCheckLanguage(
+            base::TrimWhitespaceASCII(language.GetString(), base::TRIM_ALL));
 
     if (current_language.empty()) {
       unknown->emplace_back(language.GetString());

chrome/browser/spellchecker/spellcheck_language_policy_handlers_unittest.cc

-// Copyright 2020 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.
-
-// Chromium style is to have one unit test per one header file. However, the
-// applied blocked spellcheck language policy depends on the applied forced
-// language policy. If a language is both blocked and forced, forced wins. It is
-// only practical to test this interaction in a single unit test covering both
-// header files.
-#include "chrome/browser/spellchecker/spellcheck_language_blacklist_policy_handler.h"
-#include "chrome/browser/spellchecker/spellcheck_language_policy_handler.h"
-
-#include <ostream>
-#include <string>
-#include <vector>
-
-#include "base/strings/string_util.h"
-#include "base/test/scoped_feature_list.h"
-#include "base/values.h"
-#include "chrome/common/pref_names.h"
-#include "components/policy/core/common/policy_map.h"
-#include "components/policy/policy_constants.h"
-#include "components/prefs/pref_value_map.h"
-#include "components/spellcheck/browser/pref_names.h"
-#include "components/spellcheck/common/spellcheck_features.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace policy {
-
-struct TestCase {
-  TestCase(const std::vector<std::string>& blocked_languages,
-           const std::vector<std::string>& forced_languages,
-           const std::vector<std::string>& expected_blocked_languages,
-           const std::vector<std::string>& expected_forced_languages,
-           bool spellcheck_enabled,
-           bool windows_spellchecker_enabled)
-      : blocked_languages(blocked_languages),
-        forced_languages(forced_languages),
-        expected_blocked_languages(expected_blocked_languages),
-        expected_forced_languages(expected_forced_languages),
-        spellcheck_enabled(spellcheck_enabled),
-        windows_spellchecker_enabled(windows_spellchecker_enabled) {}
-
-  std::vector<std::string> blocked_languages;
-  std::vector<std::string> forced_languages;
-  std::vector<std::string> expected_blocked_languages;
-  std::vector<std::string> expected_forced_languages;
-  bool spellcheck_enabled;
-  bool windows_spellchecker_enabled;
-};
-
-std::ostream& operator<<(std::ostream& out, const TestCase& test_case) {
-  out << "blocked_languages=["
-      << base::JoinString(test_case.blocked_languages, ",")
-      << "], forced_languages=["
-      << base::JoinString(test_case.forced_languages, ",")
-      << "], expected_blocked_languages=["
-      << base::JoinString(test_case.expected_blocked_languages, ",")
-      << "], expected_forced_languages=["
-      << base::JoinString(test_case.expected_forced_languages, ",")
-      << "], spellcheck_enabled=" << test_case.spellcheck_enabled
-      << "], windows_spellchecker_enabled="
-      << test_case.windows_spellchecker_enabled;
-  return out;
-}
-
-class SpellcheckLanguagePolicyHandlersTest
-    : public testing::TestWithParam<TestCase> {
- public:
-  SpellcheckLanguagePolicyHandlersTest() = default;
-  ~SpellcheckLanguagePolicyHandlersTest() override = default;
-
-  void CheckPrefs(const PrefValueMap& prefs,
-                  const std::string& key,
-                  const std::vector<std::string>& expected) {
-    // Retrieve the spellcheck enabled pref (if it exists).
-    const base::Value* spellcheck_enabled_pref = nullptr;
-    const bool is_spellcheck_enabled_pref_set = prefs.GetValue(
-        spellcheck::prefs::kSpellCheckEnable, &spellcheck_enabled_pref);
-
-    const base::Value* languages_list = nullptr;
-    if (GetParam().spellcheck_enabled) {
-      EXPECT_TRUE(is_spellcheck_enabled_pref_set);
-      EXPECT_TRUE(spellcheck_enabled_pref->is_bool());
-      EXPECT_TRUE(spellcheck_enabled_pref->GetBool());
-
-      EXPECT_TRUE(prefs.GetValue(key, &languages_list));
-      EXPECT_TRUE(languages_list->is_list());
-      EXPECT_EQ(expected.size(), languages_list->GetList().size());
-
-      for (const auto& language : languages_list->GetList()) {
-        EXPECT_TRUE(language.is_string());
-        EXPECT_TRUE(std::find(expected.begin(), expected.end(),
-                              language.GetString()) != expected.end());
-      }
-    } else {
-      EXPECT_FALSE(is_spellcheck_enabled_pref_set);
-      // No language list should be added to prefs if spellchecking disabled.
-      EXPECT_FALSE(prefs.GetValue(key, &languages_list));
-    }
-  }
-};
-
-TEST_P(SpellcheckLanguagePolicyHandlersTest, ApplyPolicySettings) {
-#if BUILDFLAG(USE_WIN_HYBRID_SPELLCHECKER)
-  base::test::ScopedFeatureList feature_list;
-  if (GetParam().windows_spellchecker_enabled) {
-    // Force hybrid spellchecking to be enabled.
-    feature_list.InitWithFeatures(
-        /*enabled_features=*/{spellcheck::kWinUseBrowserSpellChecker,
-                              spellcheck::kWinUseHybridSpellChecker},
-        /*disabled_features=*/{});
-  } else {
-    // Hunspell-only spellcheck languages will be used.
-    feature_list.InitAndDisableFeature(spellcheck::kWinUseBrowserSpellChecker);
-  }
-#endif  // BUILDFLAG(USE_WIN_HYBRID_SPELLCHECKER)
-
-  PrefValueMap prefs;
-  policy::PolicyMap policy;
-
-  base::Value blocked_languages_list(base::Value::Type::LIST);
-  for (const auto& blocked_language : GetParam().blocked_languages) {
-    blocked_languages_list.Append(std::move(blocked_language));
-  }
-
-  base::Value forced_languages_list(base::Value::Type::LIST);
-  for (const auto& forced_language : GetParam().forced_languages) {
-    forced_languages_list.Append(std::move(forced_language));
-  }
-
-  policy.Set(policy::key::kSpellcheckLanguageBlacklist,
-             policy::POLICY_LEVEL_MANDATORY, policy::POLICY_SCOPE_USER,
-             policy::POLICY_SOURCE_ENTERPRISE_DEFAULT,
-             base::Value::ToUniquePtrValue(std::move(blocked_languages_list)),
-             nullptr);
-
-  policy.Set(
-      policy::key::kSpellcheckLanguage, policy::POLICY_LEVEL_MANDATORY,
-      policy::POLICY_SCOPE_USER, policy::POLICY_SOURCE_ENTERPRISE_DEFAULT,
-      base::Value::ToUniquePtrValue(std::move(forced_languages_list)), nullptr);
-
-  policy.Set(
-      policy::key::kSpellcheckEnabled, policy::POLICY_LEVEL_MANDATORY,
-      policy::POLICY_SCOPE_USER, policy::POLICY_SOURCE_ENTERPRISE_DEFAULT,
-      std::make_unique<base::Value>(GetParam().spellcheck_enabled), nullptr);
-
-  // Apply policy to the forced languages handler.
-  SpellcheckLanguagePolicyHandler forced_languages_handler;
-  forced_languages_handler.ApplyPolicySettings(policy, &prefs);
-
-  // Apply policy to the blocked languages handler.
-  SpellcheckLanguageBlacklistPolicyHandler blocked_languages_handler;
-  blocked_languages_handler.ApplyPolicySettings(policy, &prefs);
-
-  // Check if forced languages preferences are as expected.
-  CheckPrefs(prefs, spellcheck::prefs::kSpellCheckForcedDictionaries,
-             GetParam().expected_forced_languages);
-
-  // Check if blocked languages preferences are as expected.
-  CheckPrefs(prefs, spellcheck::prefs::kSpellCheckBlacklistedDictionaries,
-             GetParam().expected_blocked_languages);
-}
-
-INSTANTIATE_TEST_SUITE_P(
-    TestCases,
-    SpellcheckLanguagePolicyHandlersTest,
-    testing::Values(
-#if BUILDFLAG(USE_WIN_HYBRID_SPELLCHECKER)
-        // Test cases for Windows spellchecker (policy languages not restricted
-        // to Hunspell).
-        TestCase({"ar-SA", "es-MX", "fi", "fr",
-                  "not-a-language"} /* blocked languages */,
-                 {"fi", "fr", "not-another-language"} /* forced languages */,
-                 {"ar", "es-MX"} /* expected blocked languages */,
-                 {"fi", "fr"} /* expected forced languages */,
-                 true /* spellcheck enabled */,
-                 true /* windows spellchecker enabled */),
-        TestCase({"ar-SA", "es-MX", "fi", "fr "} /* blocked languages */,
-                 {"fi", "fr"} /* forced languages */,
-                 {""} /* expected blocked languages */,
-                 {""} /* expected forced languages */,
-                 false /* spellcheck enabled */,
-                 true /* windows spellchecker enabled */),
-#endif  // BUILDFLAG(USE_WIN_HYBRID_SPELLCHECKER)
-        // Test cases for Hunspell only spellchecker. ar-SA and fi are
-        // non-Hunspell languages so are ignored for policy enforcement. If they
-        // ever obtain Hunspell support, the first test case below will fail.
-        TestCase({"ar-SA", "es-MX", "fi", "fr",
-                  "not-a-language"} /* blocked languages */,
-                 {"fi", "fr", "not-another-language"} /* forced languages */,
-                 {"es-MX"} /* expected blocked languages */,
-                 {"fr"} /* expected forced languages */,
-                 true /* spellcheck enabled */,
-                 false /* windows spellchecker enabled */),
-        TestCase({"ar-SA", "es-MX", "fi", "fr",
-                  "not-a-language"} /* blocked languages */,
-                 {"fi", "fr", "not-another-language"} /* forced languages */,
-                 {""} /* expected blocked languages */,
-                 {""} /* expected forced languages */,
-                 false /* spellcheck enabled */,
-                 false /* windows spellchecker enabled */)));
-
-}  // namespace policy

chrome/browser/spellchecker/spellcheck_service.cc

@@ -5,7 +5,6 @@
 #include "chrome/browser/spellchecker/spellcheck_service.h"
 
 #include <algorithm>
-#include <iterator>
 #include <set>
 #include <utility>
 
@@ -39,7 +38,6 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/storage_partition.h"
 #include "mojo/public/cpp/bindings/remote.h"
-#include "ui/base/l10n/l10n_util.h"
 
 #if defined(OS_WIN) && BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 #include "base/task/post_task.h"
@@ -202,84 +200,6 @@ bool SpellcheckService::SignalStatusEvent(
   return true;
 }
 
-// static
-std::string SpellcheckService::GetSupportedAcceptLanguageCode(
-    const std::string& supported_language_full_tag) {
-  // Default to accept language in hardcoded list of Hunspell dictionaries
-  // (kSupportedSpellCheckerLanguages).
-  std::string supported_accept_language =
-      spellcheck::GetCorrespondingSpellCheckLanguage(
-          supported_language_full_tag);
-
-#if defined(OS_WIN)
-  if (!spellcheck::UseWinHybridSpellChecker())
-    return supported_accept_language;
-
-  // Collect the hardcoded list of accept-languages supported by the browser,
-  // that is, languages that can be added as preferred languages in the
-  // languages settings page.
-  std::vector<std::string> accept_languages;
-  l10n_util::GetAcceptLanguages(&accept_languages);
-
-  // First try exact match. Per BCP47, tags are in ASCII and should be treated
-  // as case-insensitive (although there are conventions for the capitalization
-  // of subtags).
-  auto iter =
-      std::find_if(accept_languages.begin(), accept_languages.end(),
-                   [supported_language_full_tag](const auto& accept_language) {
-                     return base::EqualsCaseInsensitiveASCII(
-                         supported_language_full_tag, accept_language);
-                   });
-  if (iter != accept_languages.end())
-    return *iter;
-
-  // Then try matching just the language and (optional) script subtags, but
-  // not the region subtag. For example, Edge supports sr-Cyrl-RS as an accept
-  // language, but not sr-Cyrl-CS. Matching language + script subtags assures
-  // we get the correct script for spellchecking, and not use sr-Latn-RS if
-  // language packs for both scripts are installed on the system.
-  if (!base::Contains(supported_language_full_tag, "-"))
-    return "";
-
-  iter =
-      std::find_if(accept_languages.begin(), accept_languages.end(),
-                   [supported_language_full_tag](const auto& accept_language) {
-                     return base::EqualsCaseInsensitiveASCII(
-                         SpellcheckService::GetLanguageAndScriptTag(
-                             supported_language_full_tag,
-                             /* include_script_tag */ true),
-                         SpellcheckService::GetLanguageAndScriptTag(
-                             accept_language,
-                             /* include_script_tag */ true));
-                   });
-
-  if (iter != accept_languages.end())
-    return *iter;
-
-  // Then try just matching the leading language subtag. E.g. Edge supports
-  // kok as an accept language, but if the Konkani language pack is
-  // installed the Windows spellcheck API reports kok-Deva-IN for the
-  // dictionary name.
-  iter =
-      std::find_if(accept_languages.begin(), accept_languages.end(),
-                   [supported_language_full_tag](const auto& accept_language) {
-                     return base::EqualsCaseInsensitiveASCII(
-                         SpellcheckService::GetLanguageAndScriptTag(
-                             supported_language_full_tag,
-                             /* include_script_tag */ false),
-                         SpellcheckService::GetLanguageAndScriptTag(
-                             accept_language,
-                             /* include_script_tag */ false));
-                   });
-
-  if (iter != accept_languages.end())
-    return *iter;
-
-#endif  // defined(OS_WIN)
-
-  return supported_accept_language;
-}
-
 void SpellcheckService::StartRecordingMetrics(bool spellcheck_enabled) {
   metrics_ = std::make_unique<SpellCheckHostMetrics>();
   metrics_->RecordEnabledStats(spellcheck_enabled);
@@ -440,33 +360,6 @@ void SpellcheckService::OverrideBinderForTesting(SpellCheckerBinder binder) {
   GetSpellCheckerBinderOverride() = std::move(binder);
 }
 
-// static
-std::string SpellcheckService::GetLanguageAndScriptTag(
-    const std::string& full_tag,
-    bool include_script_tag) {
-  std::string language_and_script_tag;
-
-  std::vector<std::string> subtags = base::SplitString(
-      full_tag, "-", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
-  // Language subtag is required, all others optional.
-  DCHECK_GE(subtags.size(), 1ULL);
-  std::vector<std::string> subtag_tokens_to_pass;
-  subtag_tokens_to_pass.push_back(subtags.front());
-  subtags.erase(subtags.begin());
-
-  // The optional script subtag always follows the language subtag, and is 4
-  // characters in length.
-  if (include_script_tag) {
-    if (!subtags.empty() && subtags.front().length() == 4) {
-      subtag_tokens_to_pass.push_back(subtags.front());
-    }
-  }
-
-  language_and_script_tag = base::JoinString(subtag_tokens_to_pass, "-");
-
-  return language_and_script_tag;
-}
-
 // static
 void SpellcheckService::AttachStatusEvent(base::WaitableEvent* status_event) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);

chrome/browser/spellchecker/spellcheck_service.h

@@ -94,13 +94,6 @@ class SpellcheckService : public KeyedService,
   // when we do not set an event to |status_event_|.
   static bool SignalStatusEvent(EventType type);
 
-  // Get the best match of a supported accept language code for the provided
-  // language tag. Returns an empty string if no match is found. Method cannot
-  // be defined in spellcheck_common.h as it depends on l10n_util, and code
-  // under components cannot depend on ui/base.
-  static std::string GetSupportedAcceptLanguageCode(
-      const std::string& supported_language_full_tag);
-
   // Instantiates SpellCheckHostMetrics object and makes it ready for recording
   // metrics. This should be called only if the metrics recording is active.
   void StartRecordingMetrics(bool spellcheck_enabled);
@@ -168,11 +161,6 @@ class SpellcheckService : public KeyedService,
  private:
   FRIEND_TEST_ALL_PREFIXES(SpellcheckServiceBrowserTest, DeleteCorruptedBDICT);
 
-  // Parses a full BCP47 language tag to return just the language subtag,
-  // optionally with a hyphen and script subtag appended.
-  static std::string GetLanguageAndScriptTag(const std::string& full_tag,
-                                             bool include_script_tag);
-
   // Attaches an event so browser tests can listen the status events.
   static void AttachStatusEvent(base::WaitableEvent* status_event);
 

chrome/test/BUILD.gn

@@ -4473,7 +4473,6 @@ test("unit_tests") {
     sources += [
       "../browser/spellchecker/spell_check_host_chrome_impl_mac_unittest.cc",
       "../browser/spellchecker/spellcheck_custom_dictionary_unittest.cc",
-      "../browser/spellchecker/spellcheck_language_policy_handlers_unittest.cc",
       "../browser/spellchecker/spellcheck_service_unittest.cc",
       "../tools/convert_dict/convert_dict_unittest.cc",
     ]

ui/base/l10n/l10n_util.cc

@@ -888,12 +888,6 @@ void GetAcceptLanguagesForLocale(const std::string& display_locale,
   }
 }
 
-void GetAcceptLanguages(std::vector<std::string>* locale_codes) {
-  for (const char* accept_language : kAcceptLanguageList) {
-    locale_codes->push_back(accept_language);
-  }
-}
-
 bool IsLanguageAccepted(const std::string& display_locale,
                         const std::string& locale) {
   for (const char* accept_language : kAcceptLanguageList) {

ui/base/l10n/l10n_util.h

@@ -212,9 +212,6 @@ UI_BASE_EXPORT void GetAcceptLanguagesForLocale(
     const std::string& display_locale,
     std::vector<std::string>* locale_codes);
 
-// Returns a vector of untranslated locale codes usable for accept-languages.
-UI_BASE_EXPORT void GetAcceptLanguages(std::vector<std::string>* locale_codes);
-
 // Returns true if |locale| is in a predefined AcceptLanguageList and
 // a display name for the |locale| is available in the locale |display_locale|.
 UI_BASE_EXPORT bool IsLanguageAccepted(const std::string& display_locale,