Enables the user to select multiple languages for spellchecking (UI)

chrome/browser/spellchecker/spellcheck_service_browsertest.cc

Index: chrome/browser/spellchecker/spellcheck_service_browsertest.cc
diff --git a/chrome/browser/spellchecker/spellcheck_service_browsertest.cc b/chrome/browser/spellchecker/spellcheck_service_browsertest.cc
index 7c6eed772fc9386dc9219534dee1912c70344bdc..adc93039d9b60fa5c0c47d4fd53393264a411a5d 100644
--- a/chrome/browser/spellchecker/spellcheck_service_browsertest.cc
+++ b/chrome/browser/spellchecker/spellcheck_service_browsertest.cc
@@ -2,17 +2,26 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/spellchecker/spellcheck_service.h"
+
+#include <string>
+#include <vector>
+
+#include "base/command_line.h"
 #include "base/path_service.h"
+#include "base/prefs/pref_service.h"
+#include "base/strings/string_piece.h"
 #include "base/synchronization/waitable_event.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/spellchecker/spellcheck_factory.h"
-#include "chrome/browser/spellchecker/spellcheck_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/spellcheck_common.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "components/user_prefs/user_prefs.h"
 #include "content/public/test/test_utils.h"
-#include "url/gurl.h"
 
 using content::BrowserContext;
 
@@ -21,26 +30,103 @@ namespace {
 // A corrupted BDICT data used in DeleteCorruptedBDICT. Please do not use this
 // BDICT data for other tests.
 const uint8 kCorruptedBDICT[] = {
-  0x42, 0x44, 0x69, 0x63, 0x02, 0x00, 0x01, 0x00,
-  0x20, 0x00, 0x00, 0x00, 0x3b, 0x00, 0x00, 0x00,
-  0x65, 0x72, 0xe0, 0xac, 0x27, 0xc7, 0xda, 0x66,
-  0x6d, 0x1e, 0xa6, 0x35, 0xd1, 0xf6, 0xb7, 0x35,
-  0x32, 0x00, 0x00, 0x00, 0x38, 0x00, 0x00, 0x00,
-  0x39, 0x00, 0x00, 0x00, 0x3a, 0x00, 0x00, 0x00,
-  0x0a, 0x0a, 0x41, 0x46, 0x20, 0x30, 0x00, 0x00,
-  0x00, 0x00, 0x00, 0xe6, 0x49, 0x00, 0x68, 0x02,
-  0x73, 0x06, 0x74, 0x0b, 0x77, 0x11, 0x79, 0x15,
+    0x42, 0x44, 0x69, 0x63, 0x02, 0x00, 0x01, 0x00, 0x20, 0x00, 0x00, 0x00,

[groby-ooo-7-16, 2015/06/18 00:44:29]

Please don't reformat this when none of the data has changed.

[Julius, 2015/06/24 21:17:58]

clang-format changed it. I've reverted it.

+    0x3b, 0x00, 0x00, 0x00, 0x65, 0x72, 0xe0, 0xac, 0x27, 0xc7, 0xda, 0x66,
+    0x6d, 0x1e, 0xa6, 0x35, 0xd1, 0xf6, 0xb7, 0x35, 0x32, 0x00, 0x00, 0x00,
+    0x38, 0x00, 0x00, 0x00, 0x39, 0x00, 0x00, 0x00, 0x3a, 0x00, 0x00, 0x00,
+    0x0a, 0x0a, 0x41, 0x46, 0x20, 0x30, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe6,
+    0x49, 0x00, 0x68, 0x02, 0x73, 0x06, 0x74, 0x0b, 0x77, 0x11, 0x79, 0x15,
 };
 
 }  // namespace
 
-class SpellcheckServiceBrowserTest : public InProcessBrowserTest {
+struct SpellcheckLanguageTestCase {
+  SpellcheckLanguageTestCase(const std::string& spellcheck_dictionary,
+                             const std::string& accept_languages,
+                             size_t expected_enabled_spellcheck_languages,
+                             base::StringPiece first,

[groby-ooo-7-16, 2015/06/18 00:44:29]

StringPiece is nice, and very performant, and overkill here. 

As somebody who reads the test, all I care about is that a string is passed here
- there's no need for StringPiece.

Also, just pass in a list of comma-separated values and split them in here -
it's much easier to read that than a long parameter list. (Plus, it's already
done implicitly for accept_languages, so you have a mix of styles in the API)

[Julius, 2015/06/24 21:17:58]

I've implemented the csv approach instead. Done.

+                             base::StringPiece second,
+                             base::StringPiece third)
+      : spellcheck_dictionary(spellcheck_dictionary),
+        accept_languages(accept_languages),
+        expected_enabled_spellcheck_languages(
+            expected_enabled_spellcheck_languages) {
+    if (!first.empty())
+      expected_spellcheck_languages.push_back(first.as_string());
+
+    if (!second.empty())
+      expected_spellcheck_languages.push_back(second.as_string());
+
+    if (!third.empty())
+      expected_spellcheck_languages.push_back(third.as_string());
+  }
+  ~SpellcheckLanguageTestCase() {}
+
+  const std::string spellcheck_dictionary;
+  const std::string accept_languages;
+  size_t expected_enabled_spellcheck_languages;
+  std::vector<std::string> expected_spellcheck_languages;
+};
+
+class SpellcheckServiceBrowserTest
+    : public InProcessBrowserTest,
+      public testing::WithParamInterface<SpellcheckLanguageTestCase> {

[groby-ooo-7-16, 2015/06/18 00:44:29]

While WithParamInterface is clever - is the error output actually meaningful
here if one of the tests fails?

[Julius, 2015/06/24 21:17:57]

Yes, it tells you which test and the number of the case it failed on (of the
form GetSpellcheckLanguages/#) and it tells you which condition failed and what
the values were that caused it to fail.

  public:
-  Profile* GetProfile() {
-    return browser()->profile();
+  SpellcheckServiceBrowserTest() {}
+  ~SpellcheckServiceBrowserTest() override {}
+
+  BrowserContext* GetContext() {
+    return static_cast<BrowserContext*>(browser()->profile());
   }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SpellcheckServiceBrowserTest);
 };
 
+INSTANTIATE_TEST_CASE_P(
+    SpellcheckLanguageTestCases,
+    SpellcheckServiceBrowserTest,
+    testing::Values(SpellcheckLanguageTestCase("en-US",

[groby-ooo-7-16, 2015/06/18 00:44:30]

It seems to me these tests all test different facets of the language system. If
a test fails, will somebody not familiar with the code be able to reasonably
fast figure out why it failed?

[Julius, 2015/06/24 21:17:57]

I think these different cases of the GetSpellCheckLanguages function and the
fact that the parameterized tests show expected and actual output will make it
easy for people to see the error.

+                                               "en,en-US",
+                                               1UL,
+                                               "en-US",
+                                               nullptr,
+                                               nullptr),
+                    SpellcheckLanguageTestCase("en-US",
+                                               "en-US,en",
+                                               1UL,
+                                               "en-US",
+                                               nullptr,

[groby-ooo-7-16, 2015/06/18 00:44:29]

Don't use a nullptr here. Yes, StringPiece handles it like an empty string, but
an empty string is a bit more readable.

[Julius, 2015/06/24 21:17:58]

Not using StringPiece anymore.

+                                               nullptr),
+                    SpellcheckLanguageTestCase("en-US",
+                                               "en,fr,en-US,en-AU",
+                                               1UL,
+                                               "en-US",
+                                               "fr",
+                                               "en-AU"),
+                    SpellcheckLanguageTestCase("fr",
+                                               "en,en-JP,fr,zz,en-US",
+                                               1UL,
+                                               "fr",
+                                               "en-US",
+                                               nullptr)));
+
+IN_PROC_BROWSER_TEST_P(SpellcheckServiceBrowserTest, GetSpellcheckLanguages) {
+  BrowserContext* context = GetContext();
+  PrefService* prefs = user_prefs::UserPrefs::Get(context);

[groby-ooo-7-16, 2015/06/18 00:44:29]

This is pure setup for the test case - can this move into the fixture's SetUp()?

[Julius, 2015/06/24 21:17:57]

Not doing it this way anymore now that it's a unit test but I've moved some
things into a SetUp method.

+  prefs->SetString(prefs::kSpellCheckDictionary,
+                   GetParam().spellcheck_dictionary);
+  prefs->SetString(prefs::kAcceptLanguages, GetParam().accept_languages);
+
+  std::vector<std::string> spellcheck_languages;
+  size_t enabled_spellcheck_languages =
+      SpellcheckService::GetSpellCheckLanguages(context, &spellcheck_languages);
+
+  EXPECT_EQ(GetParam().expected_enabled_spellcheck_languages,
+            enabled_spellcheck_languages);
+  EXPECT_EQ(GetParam().expected_spellcheck_languages, spellcheck_languages);
+}
+
 // Tests that we can delete a corrupted BDICT file used by hunspell. We do not
 // run this test on Mac because Mac does not use hunspell by default.
 IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, DeleteCorruptedBDICT) {
@@ -50,8 +136,8 @@ IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, DeleteCorruptedBDICT) {
   base::FilePath bdict_path =
       chrome::spellcheck_common::GetVersionedFileName("en-US", dict_dir);
 
-  size_t actual = base::WriteFile(bdict_path,
-      reinterpret_cast<const char*>(kCorruptedBDICT),
+  size_t actual = base::WriteFile(
+      bdict_path, reinterpret_cast<const char*>(kCorruptedBDICT),
       arraysize(kCorruptedBDICT));
   EXPECT_EQ(arraysize(kCorruptedBDICT), actual);
 
@@ -60,15 +146,14 @@ IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, DeleteCorruptedBDICT) {
   base::WaitableEvent event(true, false);
   SpellcheckService::AttachStatusEvent(&event);
 
-  BrowserContext * context = static_cast<BrowserContext*>(GetProfile());
+  BrowserContext* context = GetContext();
 
   // Ensure that the SpellcheckService object does not already exist. Otherwise
   // the next line will not force creation of the SpellcheckService and the
   // test will fail.
   SpellcheckService* service = static_cast<SpellcheckService*>(
       SpellcheckServiceFactory::GetInstance()->GetServiceForBrowserContext(
-          context,
-          false));
+          context, false));
   ASSERT_EQ(NULL, service);
 
   // Getting the spellcheck_service will initialize the SpellcheckService
@@ -89,3 +174,68 @@ IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, DeleteCorruptedBDICT) {
     EXPECT_TRUE(base::DeleteFile(bdict_path, true));
   }
 }
+
+class MultilingualSpellcheckServiceBrowserTest

[groby-ooo-7-16, 2015/06/18 00:44:30]

Can't this just inherit from the fixture above?

[Julius, 2015/06/24 21:17:57]

Done.

+    : public InProcessBrowserTest,
+      public testing::WithParamInterface<SpellcheckLanguageTestCase> {
+ public:
+  MultilingualSpellcheckServiceBrowserTest() {}
+  ~MultilingualSpellcheckServiceBrowserTest() override {}
+
+  BrowserContext* GetContext() {
+    return static_cast<BrowserContext*>(browser()->profile());
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    InProcessBrowserTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kEnableMultilingualSpellChecker);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MultilingualSpellcheckServiceBrowserTest);
+};
+
+INSTANTIATE_TEST_CASE_P(
+    MultilingualSpellcheckLanguageTestCases,
+    MultilingualSpellcheckServiceBrowserTest,
+    testing::Values(SpellcheckLanguageTestCase("en-US",
+                                               "en,en-US",
+                                               1UL,
+                                               "en-US",
+                                               nullptr,
+                                               nullptr),
+                    SpellcheckLanguageTestCase("en-US",
+                                               "en-US,en",
+                                               1UL,
+                                               "en-US",
+                                               nullptr,
+                                               nullptr),
+                    SpellcheckLanguageTestCase("en-US,fr",
+                                               "en,fr,en-US,en-AU",
+                                               2UL,
+                                               "en-US",
+                                               "fr",
+                                               "en-AU"),
+                    SpellcheckLanguageTestCase("fr",
+                                               "en,en-JP,fr,zz,en-US",
+                                               1UL,
+                                               "fr",
+                                               "en-US",
+                                               nullptr)));
+
+IN_PROC_BROWSER_TEST_P(MultilingualSpellcheckServiceBrowserTest,
+                       GetSpellcheckLanguages) {
+  BrowserContext* context = GetContext();
+  PrefService* prefs = user_prefs::UserPrefs::Get(context);
+  prefs->SetString(prefs::kSpellCheckDictionaries,
+                   GetParam().spellcheck_dictionary);
+  prefs->SetString(prefs::kAcceptLanguages, GetParam().accept_languages);
+
+  std::vector<std::string> spellcheck_languages;
+  size_t enabled_spellcheck_languages =
+      SpellcheckService::GetSpellCheckLanguages(context, &spellcheck_languages);
+
+  EXPECT_EQ(GetParam().expected_enabled_spellcheck_languages,
+            enabled_spellcheck_languages);
+  EXPECT_EQ(GetParam().expected_spellcheck_languages, spellcheck_languages);
+}