Do not switch the ICU locale when interacting with public session pods

chrome/browser/chromeos/policy/device_local_account_browsertest.cc

Index: chrome/browser/chromeos/policy/device_local_account_browsertest.cc
diff --git a/chrome/browser/chromeos/policy/device_local_account_browsertest.cc b/chrome/browser/chromeos/policy/device_local_account_browsertest.cc
index 17071d6ee28fe368b10e27633e937aaec4f08ad9..82bdf883145c09551334560e1b9b2f488a17c301 100644
--- a/chrome/browser/chromeos/policy/device_local_account_browsertest.cc
+++ b/chrome/browser/chromeos/policy/device_local_account_browsertest.cc
@@ -128,12 +128,12 @@
 #include "net/url_request/url_request_status.h"
 #include "policy/policy_constants.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "third_party/icu/source/common/unicode/locid.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/window_open_disposition.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/views/widget/widget.h"
 #include "url/gurl.h"
-//#include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace em = enterprise_management;
 
@@ -460,6 +460,8 @@ class DeviceLocalAccountTest : public DevicePolicyCrosBrowserTest,
   virtual void SetUpOnMainThread() OVERRIDE {
     DevicePolicyCrosBrowserTest::SetUpOnMainThread();
 
+    initial_locale_ = g_browser_process->GetApplicationLocale();
+
     content::WindowedNotificationObserver(
         chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
         content::NotificationService::AllSources()).Wait();
@@ -701,6 +703,8 @@ class DeviceLocalAccountTest : public DevicePolicyCrosBrowserTest,
   const std::string user_id_2_;
   const std::string public_session_input_method_id_;
 
+  std::string initial_locale_;
+
   scoped_ptr<base::RunLoop> run_loop_;
 
   UserPolicyBuilder device_local_account_policy_;
@@ -1397,8 +1401,6 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, LastWindowClosedLogoutReminder) {
 };
 
 IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, NoRecommendedLocaleNoSwitch) {
-  const std::string initial_locale = g_browser_process->GetApplicationLocale();
-
   UploadAndInstallDeviceLocalAccountPolicy();
   AddPublicSessionToDevicePolicy(kAccountId1);
 
@@ -1417,6 +1419,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, NoRecommendedLocaleNoSwitch) {
           user_id_1_.c_str()),
       &advanced));
   EXPECT_FALSE(advanced);
+  EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),

[pneubeck (no reviews), 2014/08/19 15:09:30]

from reading this file and the function declaration of
BrowserProcess::GetApplicationLocale(), it's unclear why you would even mention
icu::Locale::getDefault() in this file.

Could you at least wrap the comparison like here (compare only initial_locale_
to Locale::getDefault)
and like in lines 1437-1439 into helper functions, where you document the reason
for the comparison to icu::Locale::getDefault() ?

[bartfab (slow), 2014/08/19 17:03:01]

I added comments and made this code a bit more concise. I specifically did not
want to move the comparisons to helper functions. This is because a failure in
the main test body gives you a useful line number while a failure in a helper
function only points you at the helper, making it impossible to tell which
invocation of the helper failed.

[pneubeck (no reviews), 2014/08/20 09:03:40]

FROM_HERE ? :-)
I suggested that to keep the redundancy low, now you have a 10x duplication of a
two-line comment. And if you have to ever change this...

Anyways it's not dramatic, so your choice.

[bartfab (slow), 2014/08/20 17:52:28]

OK, you got me convinced. I moved the locale checks into helper methods that
existed already anyway and one additional helper that I could have extracted a
while ago. Things are cleaner and shorter that way.

+            icu::Locale::getDefault().getLanguage());
 
   // Click the enter button to start the session.
   ASSERT_TRUE(content::ExecuteScript(
@@ -1430,7 +1434,9 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, NoRecommendedLocaleNoSwitch) {
 
   // Verify that the locale has not changed and the first keyboard layout
   // applicable to the locale was chosen.
-  EXPECT_EQ(initial_locale, g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(initial_locale_, g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
+            icu::Locale::getDefault().getLanguage());
   VerifyKeyboardLayoutMatchesLocale();
 }
 
@@ -1453,6 +1459,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, NoRecommendedLocaleSwitch) {
           user_id_1_.c_str()),
       &advanced));
   EXPECT_FALSE(advanced);
+  EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
+            icu::Locale::getDefault().getLanguage());
 
   // Click the link that switches the pod to its advanced form. Verify that the
   // pod switches from basic to advanced.
@@ -1483,6 +1491,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, NoRecommendedLocaleSwitch) {
   // The UI will have requested an updated list of keyboard layouts at this
   // point. Wait for the constructions of this list to finish.
   WaitForGetKeyboardLayoutsForLocaleToFinish();
+  EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
+            icu::Locale::getDefault().getLanguage());
 
   // Manually select a different keyboard layout and click the enter button to
   // start the session.
@@ -1500,6 +1510,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, NoRecommendedLocaleSwitch) {
 
   // Verify that the locale and keyboard layout have been applied.
   EXPECT_EQ(kPublicSessionLocale, g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(l10n_util::GetLanguage(kPublicSessionLocale),
+            icu::Locale::getDefault().getLanguage());
   EXPECT_EQ(public_session_input_method_id_,
             chromeos::input_method::InputMethodManager::Get()
                 ->GetActiveIMEState()
@@ -1529,6 +1541,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, OneRecommendedLocale) {
           user_id_1_.c_str()),
       &advanced));
   EXPECT_FALSE(advanced);
+  EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
+            icu::Locale::getDefault().getLanguage());
 
   // Click the enter button to start the session.
   ASSERT_TRUE(content::ExecuteScript(
@@ -1544,6 +1558,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, OneRecommendedLocale) {
   // layout applicable to the locale was chosen.
   EXPECT_EQ(kSingleRecommendedLocale[0],
             g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(l10n_util::GetLanguage(kSingleRecommendedLocale[0]),
+            icu::Locale::getDefault().getLanguage());
   VerifyKeyboardLayoutMatchesLocale();
 }
 
@@ -1569,6 +1585,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, MultipleRecommendedLocales) {
           user_id_1_.c_str()),
       &advanced));
   EXPECT_TRUE(advanced);
+  EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
+            icu::Locale::getDefault().getLanguage());
 
   // Verify that the pod shows a list of locales beginning with the recommended
   // ones, followed by others.
@@ -1690,6 +1708,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, MultipleRecommendedLocales) {
   // The UI will request an updated list of keyboard layouts at this point. Wait
   // for the constructions of this list to finish.
   WaitForGetKeyboardLayoutsForLocaleToFinish();
+  EXPECT_EQ(initial_locale_,
+            g_browser_process->GetApplicationLocale());
 
   // Manually select a different keyboard layout.
   ASSERT_TRUE(content::ExecuteScript(
@@ -1751,6 +1771,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, MultipleRecommendedLocales) {
 
   // Verify that the locale and keyboard layout have been applied.
   EXPECT_EQ(kPublicSessionLocale, g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(l10n_util::GetLanguage(kPublicSessionLocale),
+            icu::Locale::getDefault().getLanguage());
   EXPECT_EQ(public_session_input_method_id_,
             chromeos::input_method::InputMethodManager::Get()
                 ->GetActiveIMEState()
@@ -1760,8 +1782,6 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, MultipleRecommendedLocales) {
 
 IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest,
                        AutoLoginWithoutRecommendedLocales) {
-  const std::string initial_locale = g_browser_process->GetApplicationLocale();
-
   UploadAndInstallDeviceLocalAccountPolicy();
   AddPublicSessionToDevicePolicy(kAccountId1);
   EnableAutoLogin();
@@ -1772,7 +1792,9 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest,
 
   // Verify that the locale has not changed and the first keyboard layout
   // applicable to the locale was chosen.
-  EXPECT_EQ(initial_locale, g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(initial_locale_, g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
+            icu::Locale::getDefault().getLanguage());
   VerifyKeyboardLayoutMatchesLocale();
 }
 
@@ -1791,6 +1813,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest,
   // Verify that the first recommended locale has been applied and the first
   // keyboard layout applicable to the locale was chosen.
   EXPECT_EQ(kRecommendedLocales1[0], g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(l10n_util::GetLanguage(kRecommendedLocales1[0]),
+            icu::Locale::getDefault().getLanguage());
   VerifyKeyboardLayoutMatchesLocale();
 }
 
@@ -1821,6 +1845,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, TermsOfServiceWithLocaleSwitch) {
   // The UI will have requested an updated list of keyboard layouts at this
   // point. Wait for the constructions of this list to finish.
   WaitForGetKeyboardLayoutsForLocaleToFinish();
+  EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
+            icu::Locale::getDefault().getLanguage());
 
   // Set up an observer that will quit the message loop when login has succeeded
   // and the first wizard screen, if any, is being shown.
@@ -1878,6 +1904,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, TermsOfServiceWithLocaleSwitch) {
 
   // Verify that the locale and keyboard layout have been applied.
   EXPECT_EQ(kPublicSessionLocale, g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(l10n_util::GetLanguage(kPublicSessionLocale),
+            icu::Locale::getDefault().getLanguage());
   EXPECT_EQ(public_session_input_method_id_,
             chromeos::input_method::InputMethodManager::Get()
                 ->GetActiveIMEState()
@@ -1892,6 +1920,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, TermsOfServiceWithLocaleSwitch) {
 
   // Verify that the locale and keyboard layout are still in force.
   EXPECT_EQ(kPublicSessionLocale, g_browser_process->GetApplicationLocale());
+  EXPECT_EQ(l10n_util::GetLanguage(kPublicSessionLocale),
+            icu::Locale::getDefault().getLanguage());
   EXPECT_EQ(public_session_input_method_id_,
             chromeos::input_method::InputMethodManager::Get()
                 ->GetActiveIMEState()