OOBE localization browser test

chrome/browser/chromeos/login/oobe_localization_browsertest.cc

Index: chrome/browser/chromeos/login/oobe_localization_browsertest.cc
diff --git a/chrome/browser/chromeos/login/oobe_localization_browsertest.cc b/chrome/browser/chromeos/login/oobe_localization_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d4c3f7e2d4bf55f77f7fcc0e3f7bc3fcb7765f2a
--- /dev/null
+++ b/chrome/browser/chromeos/login/oobe_localization_browsertest.cc
@@ -0,0 +1,193 @@
+// Copyright 2014 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.
+
+#include "base/message_loop/message_loop.h"
+#include "base/prefs/pref_service.h"
+#include "base/strings/stringprintf.h"
+#include "base/task_runner.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/chromeos/customization_document.h"
+#include "chrome/browser/chromeos/login/login_display_host_impl.h"
+#include "chrome/browser/chromeos/login/login_wizard.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chromeos/system/statistics_provider.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_utils.h"
+
+namespace base {
+class TaskRunner;
+}
+
+namespace chromeos {
+namespace system {
+
+// Custom StatisticsProvider that will return each set of region settings.
+class FakeStatisticsProvider : public StatisticsProvider {

[achuithb, 2014/02/10 22:19:07]

Do you mind not inlining the functions of this class (the ones that are not
empty)? There's something in the style guide to this effect and there was a
discussion on chromium-dev about how this is not desirable. I can dig up the
link if you're interested.

[michaelpg, 2014/02/10 23:38:26]

This is what I see in the majority of tests. There's some discussion about
relaxing inline guidelines in .cc files, especially test files:
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

and this thread:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/inlini...

Did you read something more recently? I tried moving the function definitions
out and it seems less readable to me. Given the discussions in those threads, do
you still prefer to see this not inlined?

[achuithb, 2014/02/11 00:02:19]

Yup, that's the thread. I guess it kind of went into the weeds after the first
few posts, but I basically agree with PK that test code should be held to the
same standards of normal code, and I also find long inlined function bodies make
it difficult to figure out where the class begins and ends. 

I'll leave it up to you, but I prefer de-inlined, at least for longer functions.

+ public:

[achuithb, 2014/02/10 22:19:07]

protected?

[michaelpg, 2014/02/10 23:38:26]

why? the browser test and the customization document classes need to access
these functions.

[achuithb, 2014/02/11 00:02:19]

I didn't realize the base class had a protected dtor.

+  virtual ~FakeStatisticsProvider() {}
+
+  virtual void StartLoadingMachineStatistics(

[achuithb, 2014/02/10 22:19:07]

Shouldn't the rest of these be private?

[michaelpg, 2014/02/10 23:38:26]

The point of this fake StatisticsProvider is to be passed around and used --
this is outside of the OobeLocalizationTest class.

[achuithb, 2014/02/11 00:02:19]

It's passed around and used via the base class pointer, which exposes these
methods publicly.

[michaelpg, 2014/02/11 00:45:31]

Oh right, my mistake, private would work. It would break the Liskov substitution
principle though -- is there a particular reason these methods should be
private?

On 2014/02/11 00:02:19, achuith.bhandarkar wrote:

+      const scoped_refptr<base::TaskRunner>& file_task_runner,
+      bool load_oem_manifest) OVERRIDE {
+  }
+
+  // Populates the named machine statistic.

[achuithb, 2014/02/10 22:19:07]

Let's make this comment more verbose and say that we only populate
initial_locale and keyboard_layout.

[michaelpg, 2014/02/10 23:38:26]

Done.

+  virtual bool GetMachineStatistic(const std::string& name,
+                                   std::string* result) OVERRIDE {
+    if (name == "initial_locale")
+      *result = initial_locale_;
+    else if (name == "keyboard_layout")
+      *result = keyboard_layout_;
+    else
+      return false;
+
+    return true;
+  }
+
+  virtual bool GetMachineFlag(const std::string& name, bool* result) OVERRIDE {
+    return false;
+  }
+
+  virtual void Shutdown() OVERRIDE {
+  }
+
+  void set_locale(const std::string& locale) {
+    initial_locale_ = locale;
+  }
+
+  void set_keyboard_layout(const std::string& keyboard_layout) {
+    keyboard_layout_ = keyboard_layout;
+  }
+
+ private:
+  std::string initial_locale_;
+  std::string keyboard_layout_;
+};
+
+}  // namespace system
+
+namespace {

[achuithb, 2014/02/10 22:19:07]

newline after this I believe? I think it's better to have this anonymous
namespace before the system namespace.

[michaelpg, 2014/02/10 23:38:26]

Done.

+// OOBE constants.
+const char* kLocaleSelect = "language-select";
+const char* kKeyboardSelect = "keyboard-select";
+const char* kUSLayout = "xkb:us::eng";

[achuithb, 2014/02/10 22:19:07]

newline after this I believe?

[michaelpg, 2014/02/10 23:38:26]

Done.

+}
+
+class OobeLocalizationTest : public InProcessBrowserTest {

[achuithb, 2014/02/10 22:19:07]

Please de-inline this class too.

[michaelpg, 2014/02/10 23:38:26]

Done.

+ public:
+  OobeLocalizationTest() {
+    statistics_provider_.reset(new system::FakeStatisticsProvider());
+    // Set the instance returned by GetInstance() for testing.
+    system::StatisticsProvider::SetTestProvider(statistics_provider_.get());
+  }
+
+  void JsExpect(const std::string& expression) {

[achuithb, 2014/02/10 22:19:07]

I believe it's better for the caller of this function to do the actual assert
(so you can see which function is erroring out without having to look at the
stack).

[michaelpg, 2014/02/10 23:38:26]

Done.

+    bool result;
+    chromeos::LoginDisplayHostImpl* display_host =
+        static_cast<chromeos::LoginDisplayHostImpl*>(
+            chromeos::LoginDisplayHostImpl::default_host());
+    ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+        display_host->GetOobeUI()->web_ui()->GetWebContents(),
+        "window.domAutomationController.send(!!(" + expression + "));",
+        &result));
+    ASSERT_TRUE(result) << expression;
+  }
+
+  void VerifySelectedOption(const char* select_id, const char* value) {
+    JsExpect(base::StringPrintf(
+        "(function () {\n"
+        "  var select = document.querySelector('#%s');\n"
+        "  if (!select)\n"
+        "    return false;\n"
+        "  return select.options[select.selectedIndex].value == '%s';\n"
+        "})()", select_id, value));
+  }
+
+  void VerifyOptionExists(const char* select_id, const char* value) {
+    JsExpect(base::StringPrintf(
+        "(function () {\n"
+        "  var select = document.querySelector('#%s');\n"
+        "  if (!select)\n"
+        "    return false;\n"
+        "  for (var i = 0; i < select.options.length; i++) {\n"
+        "    if (select.options[i].value == '%s')\n"
+        "      return true;\n"
+        "  }\n"
+        "  return false;\n"
+        "})()", select_id, value));
+  }
+
+ protected:
+  // Runs the test for the given locale and keyboard layout.
+  void RunLocalizationTest(const std::string& initial_locale,
+                           const std::string& keyboard_layout,
+                           const std::string& expected_locale,
+                           const std::string& expected_keyboard_layout) {
+    statistics_provider()->set_locale(initial_locale);
+    statistics_provider()->set_keyboard_layout(keyboard_layout);
+
+    // Initialize StartupCustomizationDocument with fake statistics provider
+    StartupCustomizationDocument::GetInstance()->Init(statistics_provider());
+
+    // Bring up the OOBE network screen
+    chromeos::ShowLoginWizard(chromeos::WizardController::kNetworkScreenName);
+    content::WindowedNotificationObserver(
+        chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
+        content::NotificationService::AllSources()).Wait();
+
+    VerifySelectedOption(kLocaleSelect, expected_locale.c_str());
+    VerifySelectedOption(kKeyboardSelect, expected_keyboard_layout.c_str());
+
+    // Make sure we have a fallback keyboard.
+    VerifyOptionExists(kKeyboardSelect, kUSLayout);
+
+    // Shut down the display host.
+    chromeos::LoginDisplayHostImpl::default_host()->Finalize();
+    base::MessageLoopForUI::current()->RunUntilIdle();
+
+    // Clear the locale pref so the statistics provider is pinged next time.
+    g_browser_process->local_state()->SetString(prefs::kApplicationLocale,
+                                                std::string());
+  }
+
+  system::FakeStatisticsProvider* statistics_provider() {

[achuithb, 2014/02/10 22:19:07]

This function seems unnecessary - why not just use statistics_provider_.get()
instead?

[michaelpg, 2014/02/10 23:38:26]

Done (this was vestigial, thanks for catching)

+    return statistics_provider_.get();
+  }
+
+ private:
+  scoped_ptr<system::FakeStatisticsProvider> statistics_provider_;

[achuithb, 2014/02/10 22:19:07]

Should this be StatisticsProvider instead of FakeStatisticsProvider?

[michaelpg, 2014/02/10 23:38:26]

It can't be -- StatisticsProvider::~StatisticsProvider() is protected.

[achuithb, 2014/02/11 00:02:19]

Ok

+
+  DISALLOW_COPY_AND_ASSIGN(OobeLocalizationTest);
+};
+
+IN_PROC_BROWSER_TEST_F(OobeLocalizationTest, NetworkScreen) {
+  // For a non-Latin keyboard layout like Russian, we expect to see the US
+  // keyboard.
+  RunLocalizationTest("ru", "xkb:ru::rus",

[achuithb, 2014/02/10 22:19:07]

If there's a danger of running into the time limit, you can break this up into 5
browser tests (or 2, or 3 by grouping a few tests). I would err on the side of
making this as flake free as possible.

+                      "ru", kUSLayout);
+
+  // IMEs do not load at OOBE, so we just expect to see the (Latin) Japanese
+  // keyboard.
+  RunLocalizationTest("ja", "xkb:jp::jpn",
+                      "ja", "xkb:jp::jpn");
+
+  // We don't use the Icelandic locale but the Icelandic keyboard layout
+  // should still be selected when specified as the default.
+  RunLocalizationTest("en-US", "xkb:is::ice",
+                      "en-US", "xkb:is::ice");
+
+  // French Swiss keyboard.
+  RunLocalizationTest("fr", "xkb:ch:fr:fra",
+                      "fr", "xkb:ch:fr:fra");
+
+  // German Swiss keyboard.
+  RunLocalizationTest("de", "xkb:ch::ger",
+                      "de", "xkb:ch::ger");
+}
+
+}  // namespace chromeos