Allow recommended locales to be set for public sessions

chrome/browser/chromeos/policy/device_local_account_browsertest.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be

[pneubeck (no reviews), 2014/07/30 17:55:44]

How about breaking this into several files according to categories of what's
tested and a base test implementation? (if so, should be in a follow up)

[bartfab (slow), 2014/08/05 17:16:23]

Good idea. I filed crbug.com/400702 against myself to keep track of this.

 // found in the LICENSE file.
 
 #include <map>
 #include <string>
+#include <vector>
 
 #include "apps/app_window_registry.h"
 #include "apps/ui/native_app_window.h"
 #include "ash/shell.h"
 #include "ash/system/chromeos/session/logout_confirmation_controller.h"
 #include "ash/system/chromeos/session/logout_confirmation_dialog.h"
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
@@ skipping 15 matching lines @@
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.h"
 #include "chrome/browser/chromeos/extensions/external_cache.h"
+#include "chrome/browser/chromeos/input_method/input_method_util.h"
 #include "chrome/browser/chromeos/login/existing_user_controller.h"
 #include "chrome/browser/chromeos/login/screens/wizard_screen.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host_impl.h"
 #include "chrome/browser/chromeos/login/ui/webui_login_view.h"
 #include "chrome/browser/chromeos/login/users/avatar/user_image_manager.h"
 #include "chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.h"
 #include "chrome/browser/chromeos/login/users/avatar/user_image_manager_test_util.h"
 #include "chrome/browser/chromeos/login/users/user_manager.h"
 #include "chrome/browser/chromeos/login/users/user_manager_impl.h"
@@ skipping 40 matching lines @@
 #include "components/policy/core/common/cloud/cloud_policy_store.h"
 #include "components/policy/core/common/cloud/policy_builder.h"
 #include "components/policy/core/common/external_data_fetcher.h"
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/core/common/policy_namespace.h"
 #include "components/policy/core/common/policy_service.h"
 #include "components/policy/core/common/policy_switches.h"
 #include "components/signin/core/common/signin_pref_names.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_type.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 #include "crypto/rsa_private_key.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/management_policy.h"
@@ skipping 53 matching lines @@
 const char kHostedAppCRXPath[] = "extensions/hosted_app.crx";
 const char kHostedAppVersion[] = "1.0.0.0";
 const char kGoodExtensionID[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
 const char kGoodExtensionCRXPath[] = "extensions/good.crx";
 const char kGoodExtensionVersion[] = "1.0";
 const char kPackagedAppCRXPath[] = "extensions/platform_apps/app_window_2.crx";
 
 const char kExternalData[] = "External data";
 const char kExternalDataURL[] = "http://localhost/external_data";
 
+const char* kRecommendedLocales1[] = {
+  "pl",
+  "et",
+  "en-US",
+};
+const char* kRecommendedLocales2[] = {
+  "fr",
+  "nl",
+};
 const char kPublicSessionLocale[] = "de";
 const char kPublicSessionInputMethodIDTemplate[] = "_comp_ime_%sxkb:de:neo:ger";
 
 // Helper that serves extension update manifests to Chrome.
 class TestingUpdateManifestProvider {
  public:
   // Update manifests will be served at |relative_update_url|.
   explicit TestingUpdateManifestProvider(
       const std::string& relative_update_url);
   ~TestingUpdateManifestProvider();
@@ skipping 299 matching lines @@
     em::DeviceLocalAccountInfoProto* account =
         proto.mutable_device_local_accounts()->add_account();
     account->set_account_id(username);
     account->set_type(
         em::DeviceLocalAccountInfoProto::ACCOUNT_TYPE_PUBLIC_SESSION);
     RefreshDevicePolicy();
     test_server_.UpdatePolicy(dm_protocol::kChromeDevicePolicyType,
                               std::string(), proto.SerializeAsString());
   }
 
+  void EnableAutoLogin() {
+    em::ChromeDeviceSettingsProto& proto(device_policy()->payload());
+    em::DeviceLocalAccountsProto* device_local_accounts =
+        proto.mutable_device_local_accounts();
+    device_local_accounts->set_auto_login_id(kAccountId1);
+    device_local_accounts->set_auto_login_delay(0);
+    RefreshDevicePolicy();
+    test_server_.UpdatePolicy(dm_protocol::kChromeDevicePolicyType,
+                              std::string(), proto.SerializeAsString());
+  }
+
   void CheckPublicSessionPresent(const std::string& id) {
     const user_manager::User* user = chromeos::UserManager::Get()->FindUser(id);
     ASSERT_TRUE(user);
     EXPECT_EQ(id, user->email());
     EXPECT_EQ(user_manager::USER_TYPE_PUBLIC_ACCOUNT, user->GetType());
   }
 
   base::FilePath GetExtensionCacheDirectoryForAccountID(
       const std::string& account_id) {
     base::FilePath extension_cache_root_dir;
@@ skipping 11 matching lines @@
     return GetExtensionCacheDirectoryForAccountID(account_id)
         .Append(base::StringPrintf("%s-%s.crx", id.c_str(), version.c_str()));
   }
 
   // Returns a profile which can be used for testing.
   Profile* GetProfileForTest() {
     // Any profile can be used here since this test does not test multi profile.
     return ProfileManager::GetActiveUserProfile();
   }
 
+  content::WebContents* GetWebContents() {
+    chromeos::LoginDisplayHostImpl* host =
+        reinterpret_cast<chromeos::LoginDisplayHostImpl*>(
+            chromeos::LoginDisplayHostImpl::default_host());
+    if (!host)
+      return NULL;
+    chromeos::WebUILoginView* web_ui_login_view = host->GetWebUILoginView();
+    if (!web_ui_login_view)
+      return NULL;
+    content::WebUI* web_ui = web_ui_login_view->GetWebUI();
+    if (!web_ui)
+      return NULL;
+    return web_ui->GetWebContents();
+  }
+
   void WaitForDisplayName(const std::string& user_id,
                           const std::string& expected_display_name) {
     DictionaryPrefValueWaiter("UserDisplayName",
                               user_id,
                               expected_display_name).Wait();
   }
 
   void WaitForPolicy() {
     // Wait for the display name becoming available as that indicates
     // device-local account policy is fully loaded, which is a prerequisite for
@@ skipping 34 matching lines @@
     controller->LoginAsPublicSession(user_context);
   }
 
   void WaitForSessionStart() {
     if (IsSessionStarted())
       return;
     content::WindowedNotificationObserver(chrome::NOTIFICATION_SESSION_STARTED,
                                           base::Bind(IsSessionStarted)).Wait();
   }
 
+  void SkipToLoginScreen() {
+    chromeos::WizardController* wizard_controller =
+        chromeos::WizardController::default_controller();
+    ASSERT_TRUE(wizard_controller);

[pneubeck (no reviews), 2014/07/30 15:23:19]

just FYI: ASSERTs in functions only skip the rest of the function. They don't
abort the whole test.

[bartfab (slow), 2014/08/05 17:16:23]

That's why I use ASSERT_NO_FATAL_FAILURE().

+    wizard_controller->SkipToLoginForTesting(LoginScreenContext());
+    content::WindowedNotificationObserver(
+        chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
+        content::NotificationService::AllSources()).Wait();
+  }
+
+  void VerifyKeyboardLayoutMatchesLocale() {
+    chromeos::input_method::InputMethodManager* input_method_manager =
+        chromeos::input_method::InputMethodManager::Get();
+    std::vector<std::string> layouts_from_locale;
+    input_method_manager->GetInputMethodUtil()->

[pneubeck (no reviews), 2014/07/30 17:55:44]

I don't know how the logic for selecting the keyboard layout looks like after
your latest changes.
But is this function applying the exact same logic/algorithm?

You might consider calling to the same production function, which should have
it's own unit tests,
or to replace this logic by concrete test values.

[bartfab (slow), 2014/08/05 17:16:24]

This is exactly the production function that the UI code calls. You feed it the
locale and you get back a list of input methods matching the locale, in priority
order. I could hard-code the result but there are differences between branded
and non-branded builds that I would have to hack around. I think it is better to
verify that the UI's behavior matches this function's output. And yes, as you
say, making sure that this function returns sensible values is the job of a
separate unit test (which actually does exist! :).

+        GetInputMethodIdsFromLanguageCode(
+            g_browser_process->GetApplicationLocale(),
+            chromeos::input_method::kKeyboardLayoutsOnly,
+            &layouts_from_locale);
+    ASSERT_FALSE(layouts_from_locale.empty());
+    EXPECT_EQ(layouts_from_locale.front(),
+              input_method_manager->GetCurrentInputMethod().id());
+  }
+
   const std::string user_id_1_;
   const std::string user_id_2_;
   const std::string public_session_input_method_id_;
 
   scoped_ptr<base::RunLoop> run_loop_;
 
   UserPolicyBuilder device_local_account_policy_;
   LocalPolicyTestServer test_server_;
 
  private:
@@ skipping 15 matching lines @@
 
   CheckPublicSessionPresent(user_id_1_);
   CheckPublicSessionPresent(user_id_2_);
 }
 
 IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, DisplayName) {
   UploadAndInstallDeviceLocalAccountPolicy();
   AddPublicSessionToDevicePolicy(kAccountId1);
 
   WaitForPolicy();
-
-  // Skip to the login screen.
-  chromeos::WizardController* wizard_controller =
-      chromeos::WizardController::default_controller();
-  ASSERT_TRUE(wizard_controller);
-  wizard_controller->SkipToLoginForTesting(LoginScreenContext());
-  content::WindowedNotificationObserver(
-      chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
-      content::NotificationService::AllSources()).Wait();
+  ASSERT_NO_FATAL_FAILURE(SkipToLoginScreen());
 
   // Verify that the display name is shown in the UI.
-  chromeos::LoginDisplayHostImpl* host =
-      reinterpret_cast<chromeos::LoginDisplayHostImpl*>(
-          chromeos::LoginDisplayHostImpl::default_host());
-  ASSERT_TRUE(host);
-  chromeos::WebUILoginView* web_ui_login_view = host->GetWebUILoginView();
-  ASSERT_TRUE(web_ui_login_view);
-  content::WebUI* web_ui = web_ui_login_view->GetWebUI();
-  ASSERT_TRUE(web_ui);
-  content::WebContents* contents = web_ui->GetWebContents();
+  content::WebContents* contents = GetWebContents();
   ASSERT_TRUE(contents);
   const std::string get_compact_pod_display_name = base::StringPrintf(
       "domAutomationController.send(document.getElementById('pod-row')"
       "    .getPodWithUsername_('%s').nameElement.textContent);",
       user_id_1_.c_str());
   std::string display_name;
   ASSERT_TRUE(content::ExecuteScriptAndExtractString(
       contents,
       get_compact_pod_display_name,
       &display_name));
@@ skipping 644 matching lines @@
   ASSERT_TRUE(dialog);
 
   // Deny the logout.
   dialog->GetWidget()->Close();
   dialog = NULL;
   base::RunLoop().RunUntilIdle();
 
   app_window_registry->RemoveObserver(this);
 };
 
-IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, DoNotSelectLanguageAndKeyboard) {
-  chromeos::input_method::InputMethodManager* input_method_manager =
-      chromeos::input_method::InputMethodManager::Get();
+IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, NoRecommendedLocale) {

[pneubeck (no reviews), 2014/07/30 15:39:23]

is this case different from no locale policy being set at all?

i didn't check the whole file, but is there a test that switches the locale if
the policy is not set?

[bartfab (slow), 2014/08/05 17:16:24]

There was no test verifying that scenario. I added one now,
NoRecommendedLocaleSwitch.

   const std::string initial_locale = g_browser_process->GetApplicationLocale();
-  const std::string initial_input_method =
-      input_method_manager->GetCurrentInputMethod().id();
 
   UploadAndInstallDeviceLocalAccountPolicy();
   AddPublicSessionToDevicePolicy(kAccountId1);
 
   WaitForPolicy();
-  ASSERT_NO_FATAL_FAILURE(WaitForLoginUI());
-  ASSERT_NO_FATAL_FAILURE(StartLogin(std::string(), std::string()));
+  ASSERT_NO_FATAL_FAILURE(SkipToLoginScreen());
+
+  // Click on the pod to expand it. Verify that the pod expands to its basic
+  // form as there are no recommended locales.
+  content::WebContents* contents = GetWebContents();
+  ASSERT_TRUE(contents);
+  bool advanced = false;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      contents,
+      base::StringPrintf(
+          "var pod ="
+          "    document.getElementById('pod-row').getPodWithUsername_('%s');"
+          "pod.click();"
+          "domAutomationController.send(pod.classList.contains('advanced'));",
+          user_id_1_.c_str()),
+      &advanced));
+  EXPECT_FALSE(advanced);
+
+  // Click the enter button to start the session.
+  ASSERT_TRUE(content::ExecuteScript(
+      contents,
+      base::StringPrintf(
+          "document.getElementById('pod-row').getPodWithUsername_('%s')"
+          "    .querySelector('.enter-button').click();",
+          user_id_1_.c_str())));
+
   WaitForSessionStart();
 
+  // 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_input_method,
-            input_method_manager->GetCurrentInputMethod().id());
+  VerifyKeyboardLayoutMatchesLocale();
 }
 
-IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, SelectLanguageAndKeyboard) {
-  // Specify startup pages.
-  device_local_account_policy_.payload().mutable_restoreonstartup()->set_value(
-      SessionStartupPref::kPrefValueURLs);
-  em::StringListPolicyProto* startup_urls_proto =
-      device_local_account_policy_.payload().mutable_restoreonstartupurls();
-  for (size_t i = 0; i < arraysize(kStartupURLs); ++i)
-    startup_urls_proto->mutable_value()->add_entries(kStartupURLs[i]);
+IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, OneRecommendedLocale) {
+  // Specify a recommended locale.
+  em::StringListPolicyProto* session_locales_proto =
+      device_local_account_policy_.payload().mutable_sessionlocales();
+  session_locales_proto->mutable_policy_options()->set_mode(
+      em::PolicyOptions_PolicyMode_RECOMMENDED);
+  session_locales_proto->mutable_value()->add_entries(kRecommendedLocales1[0]);

[pneubeck (no reviews), 2014/07/30 17:55:44]

instead of using the unrelated kRecommendedLocales1 array, better define a
separate constant local to this test and reuse it in line 1426

[bartfab (slow), 2014/08/05 17:16:24]

Done.

   UploadAndInstallDeviceLocalAccountPolicy();
   AddPublicSessionToDevicePolicy(kAccountId1);
 
-  // Log in to the device-local account with a specific locale and keyboard
-  // layout.
   WaitForPolicy();
-  ASSERT_NO_FATAL_FAILURE(WaitForLoginUI());
-  ASSERT_NO_FATAL_FAILURE(
-      StartLogin(kPublicSessionLocale, public_session_input_method_id_));
+  ASSERT_NO_FATAL_FAILURE(SkipToLoginScreen());
+
+  // Click on the pod to expand it. Verify that the pod expands to its basic
+  // form as there is only one recommended locale.
+  content::WebContents* contents = GetWebContents();
+  ASSERT_TRUE(contents);
+  bool advanced = false;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      contents,
+      base::StringPrintf(
+          "var pod ="
+          "    document.getElementById('pod-row').getPodWithUsername_('%s');"
+          "pod.click();"
+          "domAutomationController.send(pod.classList.contains('advanced'));",
+          user_id_1_.c_str()),
+      &advanced));
+  EXPECT_FALSE(advanced);
+
+  // Click the enter button to start the session.
+  ASSERT_TRUE(content::ExecuteScript(
+      contents,
+      base::StringPrintf(
+          "document.getElementById('pod-row').getPodWithUsername_('%s')"
+          "    .querySelector('.enter-button').click();",
+          user_id_1_.c_str())));
+
   WaitForSessionStart();
 
+  // Verify that the recommended locale has been applied and the first keyboard
+  // layout applicable to the locale was chosen.
+  EXPECT_EQ(kRecommendedLocales1[0], g_browser_process->GetApplicationLocale());
+  VerifyKeyboardLayoutMatchesLocale();
+}
+
+IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, MultipleRecommendedLocales) {

[pneubeck (no reviews), 2014/07/30 17:55:44]

can the user also minify the pod after expanding? what should happen to the
locale selection?
just asking, might not make sense to test.

[bartfab (slow), 2014/08/05 17:16:23]

Yes, the pod will contract if it loses focus. The pod should remember its state
if the user focuses it again. I extended the test to verify this.

+  // Specify recommended locales.
+  em::StringListPolicyProto* session_locales_proto =

[pneubeck (no reviews), 2014/07/30 17:55:44]

the setting of the locales could be in helper functions.
taking an array+size. where you set a single locale or empty list, you could
define an according array constant.

[bartfab (slow), 2014/08/05 17:16:24]

Done.

+      device_local_account_policy_.payload().mutable_sessionlocales();
+  session_locales_proto->mutable_policy_options()->set_mode(
+      em::PolicyOptions_PolicyMode_RECOMMENDED);
+  for (size_t i = 0; i < arraysize(kRecommendedLocales1); ++i) {
+    session_locales_proto->mutable_value()->add_entries(
+        kRecommendedLocales1[i]);
+  }
+  UploadAndInstallDeviceLocalAccountPolicy();
+  AddPublicSessionToDevicePolicy(kAccountId1);
+
+  WaitForPolicy();
+  ASSERT_NO_FATAL_FAILURE(SkipToLoginScreen());
+
+  // Verify that the pod shows a list of locales beginning with the recommended
+  // ones, followed by others.
+  content::WebContents* contents = GetWebContents();
+  ASSERT_TRUE(contents);
+  const std::string get_locale_list = base::StringPrintf(
+      "var languageSelect = document.getElementById('pod-row')"
+      "    .getPodWithUsername_('%s').querySelector('.language-select');"
+      "var locales = [];"
+      "for (var i = 0; i < languageSelect.length; ++i)"
+      "  locales.push(languageSelect.options[i].value);"
+      "domAutomationController.send(JSON.stringify(locales));",
+      user_id_1_.c_str());
+  std::string json;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(contents,
+                                                     get_locale_list,
+                                                     &json));
+  scoped_ptr<base::Value> value_ptr(base::JSONReader::Read(json));
+  const base::ListValue* locales = NULL;
+  ASSERT_TRUE(value_ptr);
+  ASSERT_TRUE(value_ptr->GetAsList(&locales));
+  EXPECT_LT(arraysize(kRecommendedLocales1), locales->GetSize());
+  for (size_t i = 0; i < arraysize(kRecommendedLocales1); ++i) {
+    std::string locale;
+    EXPECT_TRUE(locales->GetString(i, &locale));
+    EXPECT_EQ(kRecommendedLocales1[i], locale);
+  }
+
+  // Verify that the first recommended locale is selected.

[pneubeck (no reviews), 2014/07/30 15:39:23]

It's irritating that you're testing this before the pod is expanded.
Better move it to after expanding it.

[bartfab (slow), 2014/08/05 17:16:23]

Done.

+  const std::string get_selected_locale =
+      base::StringPrintf(

[pneubeck (no reviews), 2014/07/30 15:39:23]

This block and the next together should test:
 - first locale in the list is selected (no matter what the content of the list)
 - the prefix of the list is matching the policy (in a certain order)
 - the rest of the list is containing other locales (and maybe that it doesn't
repeat the recommended locales)

not sure whether only the comments don't mention this or whether you're indeed
not testing it.

Your call, what of the above is worth testing (depends on how you implemented
it).

[bartfab (slow), 2014/08/05 17:16:23]

My test verifies this but the focus is slightly different: First, I verify that
the list starts with the recommended locales in the right order. Then, I verify
that the selected locale is the first recommended one. As a corollary of the
two, I can be sure that the first list entry is selected.
Yes, this is tested.
Yes, this is tested - though I did not verify that none of the recommended
locales repeat in that list. I added that now.

+          "domAutomationController.send(document.getElementById('pod-row')"
+          "    .getPodWithUsername_('%s').querySelector('.language-select')"
+          "        .value);",
+          user_id_1_.c_str());
+  std::string selected_locale;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(contents,
+                                                     get_selected_locale,
+                                                     &selected_locale));
+  EXPECT_EQ(kRecommendedLocales1[0], selected_locale);
+
+  // Click on the pod to expand it. Verify that the pod expands to its advanced
+  // form directly as there are two or more recommended locales.

[pneubeck (no reviews), 2014/07/30 15:39:23]

nit: "as there are more than two"

[bartfab (slow), 2014/08/05 17:16:24]

No, "two or more" is correct. The advanced pod is shown for 2, 3, 4, etc.
locales, not "more than two," which would be 3, 4, etc.

+  bool advanced = false;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      contents,
+      base::StringPrintf(
+          "var pod ="
+          "    document.getElementById('pod-row').getPodWithUsername_('%s');"
+          "pod.click();"
+          "domAutomationController.send(pod.classList.contains('advanced'));",
+          user_id_1_.c_str()),
+      &advanced));
+  EXPECT_TRUE(advanced);
+
+  // Change the list of recommended locales.
+  session_locales_proto->mutable_value()->Clear();
+  for (size_t i = 0; i < arraysize(kRecommendedLocales2); ++i) {
+    session_locales_proto->mutable_value()->add_entries(
+        kRecommendedLocales2[i]);
+  }

[pneubeck (no reviews), 2014/07/30 15:39:23]

nit: empty line after this one.

[bartfab (slow), 2014/08/05 17:16:23]

Done.

+  // Also change the display name as it is easy to ensure that policy has been
+  // updated by waiting for a display name change.
+  device_local_account_policy_.payload().mutable_userdisplayname()->set_value(
+      kDisplayName2);
+  UploadAndInstallDeviceLocalAccountPolicy();
+  policy::BrowserPolicyConnectorChromeOS* connector =
+      g_browser_process->platform_part()->browser_policy_connector_chromeos();
+  DeviceLocalAccountPolicyBroker* broker =
+      connector->GetDeviceLocalAccountPolicyService()->GetBrokerForUser(
+          user_id_1_);
+  ASSERT_TRUE(broker);
+  broker->core()->store()->Load();
+  WaitForDisplayName(user_id_1_, kDisplayName2);
+
+  // Verify that the new list of locales is shown in the UI.
+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(contents,
+                                                     get_locale_list,
+                                                     &json));
+  value_ptr.reset(base::JSONReader::Read(json));

[pneubeck (no reviews), 2014/07/30 17:55:44]

if this code part (JSONReader -> cast to concrete value type) is used more often
than the two times that you added, it could be moved to a helper (very likely,
beyond this browser test, this is done a hundred times...)

regarding error logging, the different error cases could be LOG(ERROR)'ed and
the result ASSERTed here.

[bartfab (slow), 2014/08/05 17:16:23]

I do such a conversion three times in this file. I agree that if there was a
general helper, it would be nice to use it. But I am reluctant to start adding
one as that would be a large undertaking with rather little ROI.

+  locales = NULL;
+  ASSERT_TRUE(value_ptr);
+  ASSERT_TRUE(value_ptr->GetAsList(&locales));
+  EXPECT_LT(arraysize(kRecommendedLocales2), locales->GetSize());
+  for (size_t i = 0; i < arraysize(kRecommendedLocales2); ++i) {
+    std::string locale;
+    EXPECT_TRUE(locales->GetString(i, &locale));
+    EXPECT_EQ(kRecommendedLocales2[i], locale);
+  }
+
+  // Verify that the first new recommended locale is selected.
+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(contents,
+                                                     get_selected_locale,
+                                                     &selected_locale));
+  EXPECT_EQ(kRecommendedLocales2[0], selected_locale);
+
+  // Manually select a different locale.
+  ASSERT_TRUE(content::ExecuteScript(
+      contents,
+      base::StringPrintf(
+          "var languageSelect = document.getElementById('pod-row')"
+          "    .getPodWithUsername_('%s').querySelector('.language-select');"
+          "languageSelect.value = '%s';"
+          "var event = document.createEvent('HTMLEvents');"
+          "event.initEvent('change', false, true);"
+          "languageSelect.dispatchEvent(event);",
+          user_id_1_.c_str(),
+          kPublicSessionLocale)));
+
+  // Change the list of recommended locales.
+  session_locales_proto->mutable_value()->Clear();
+  for (size_t i = 0; i < arraysize(kRecommendedLocales1); ++i) {
+    session_locales_proto->mutable_value()->add_entries(
+        kRecommendedLocales1[i]);
+  }
+  device_local_account_policy_.payload().mutable_userdisplayname()->set_value(
+      kDisplayName1);
+  UploadAndInstallDeviceLocalAccountPolicy();
+  broker->core()->store()->Load();
+  WaitForDisplayName(user_id_1_, kDisplayName1);
+
+  // Verify that the first manually selected locale is still selected.

[pneubeck (no reviews), 2014/07/30 17:55:44]

what means 'the first manually selected' ?
Can the user select more than one? Maybe change to 'Verify that the manually
selected locale...'

[bartfab (slow), 2014/08/05 17:16:24]

Oops, typo. Removed the "first".

+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(contents,
+                                                     get_selected_locale,
+                                                     &selected_locale));
+  EXPECT_EQ(kPublicSessionLocale, selected_locale);
+
+  // The UI will have requested an updated list of keyboard layouts at this

[pneubeck (no reviews), 2014/07/30 17:55:44]

nit: move to a helper "FlushFileThread"

[bartfab (slow), 2014/08/05 17:16:23]

Done.

+  // point. Flush the FILE and UI thread loops to allow the process that
+  // constructs this list to complete.
+  base::RunLoop run_loop;
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&base::DoNothing),
+      run_loop.QuitClosure());
+  run_loop.Run();
+  base::RunLoop().RunUntilIdle();
+
+  // Manually select a different keyboard layout and click the enter button to
+  // start the session.
+  ASSERT_TRUE(content::ExecuteScript(
+      contents,
+      base::StringPrintf(
+          "var pod ="
+          "    document.getElementById('pod-row').getPodWithUsername_('%s');"
+          "pod.querySelector('.keyboard-select').value = '%s';"
+          "pod.querySelector('.enter-button').click();",
+          user_id_1_.c_str(),
+          public_session_input_method_id_.c_str())));
+
+  WaitForSessionStart();
+
   // Verify that the locale and keyboard layout have been applied.

[pneubeck (no reviews), 2014/07/30 17:55:44]

'Verify that the manually selected ...'

Hm. Is this locale / keyboard layout independent of the settings _on the login
screen_ ?
Is there yet-another menu to change the layout on the login screen?
Shouldn't they be coupled? If not, this could be rather confusing to the user.

[bartfab (slow), 2014/08/05 17:16:24]

There are several places where you can pick the locale and/or keyboard layout:

1) During OOBE, you can pick the initial locale and keyboard layout for the
device. You only do this once. It happens before you ever reach the login
screen. Changes apply immediately.
2) On the login screen, you can switch the currently active keyboard layout via
keyboard shortcuts or the ash tray bubble. Changes apply immediately.
3) During a session, you can switch the currently active keyboard layout via
keyboard shortcuts or the settings page. Changes apply immediately.
4) During a session, you can switch the locale via the settings page. Changes
apply after the next logout/login.

What I am adding is:
5) On the login screen, you can switch the keyboard layout and locale that will
be used for the public session you are about to start. Changes apply when the
session actually starts.

So technically, yes, you have access to 2) and 5) on the login screen. You could
use 2) to change the currently active keyboard layout to A, select layout B via
5) and then wonder whether you get A or B. This is unavoidable and is by design:

* Choices made for a public session (5) should never apply outside the session.
So we cannot switch to the selected locale and keyboard before the start of the
session.
* On the other hand, you should be able to switch the currently active keyboard
layout (2) because you may want to log in to another account that requires a
password and you want to use a particular layout to type in this password.

It follows that the correct behavior is:

* Honor layout A as long as you are on the login screen.
* Switch to layout B when you actually start the public session.

And this is exactly how things work.

   EXPECT_EQ(kPublicSessionLocale, g_browser_process->GetApplicationLocale());
   EXPECT_EQ(public_session_input_method_id_,
             chromeos::input_method::InputMethodManager::Get()->
                 GetCurrentInputMethod().id());
 }
 

[pneubeck (no reviews), 2014/07/30 15:39:23]

will continue review after this part later.

[bartfab (slow), 2014/08/05 17:16:24]

Acknowledged.

 IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest,
-                       SelectLanguageAndKeyboardWithTermsOfService) {
+                       AutoLoginWithoutRecommendedLocales) {

[pneubeck (no reviews), 2014/07/30 17:55:44]

can you compare this to whatever other test maybe already exists for the auto
login?

[bartfab (slow), 2014/08/05 17:16:23]

The only other auto-login tests I am aware of are in
ExistingUserControllerPublicSessionTest. The tests there verify that different
auto-login delays work as they should (zero delay, short delay, long delay). The
focus of the test here is different. It just uses a zero delay but verifies that
language and keyboard layout work as expected. I think both parts have their
merits.

+  const std::string initial_locale = g_browser_process->GetApplicationLocale();
+
+  UploadAndInstallDeviceLocalAccountPolicy();
+  AddPublicSessionToDevicePolicy(kAccountId1);
+  EnableAutoLogin();
+
+  WaitForPolicy();
+  ASSERT_NO_FATAL_FAILURE(SkipToLoginScreen());
+
+  WaitForSessionStart();
+
+  // 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());
+  VerifyKeyboardLayoutMatchesLocale();
+}
+
+IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest,
+                       AutoLoginWithRecommendedLocales) {
+  // Specify recommended locales.
+  em::StringListPolicyProto* session_locales_proto =
+      device_local_account_policy_.payload().mutable_sessionlocales();
+  session_locales_proto->mutable_policy_options()->set_mode(
+      em::PolicyOptions_PolicyMode_RECOMMENDED);
+  for (size_t i = 0; i < arraysize(kRecommendedLocales1); ++i) {
+    session_locales_proto->mutable_value()->add_entries(
+        kRecommendedLocales1[i]);
+  }
+  UploadAndInstallDeviceLocalAccountPolicy();
+  AddPublicSessionToDevicePolicy(kAccountId1);
+  EnableAutoLogin();
+
+  WaitForPolicy();
+  ASSERT_NO_FATAL_FAILURE(SkipToLoginScreen());
+
+  WaitForSessionStart();
+
+  // 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());
+  VerifyKeyboardLayoutMatchesLocale();
+}
+
+IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, TermsOfServiceWithLocaleSwitch) {
   // Specify Terms of Service URL.
   ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
   device_local_account_policy_.payload().mutable_termsofserviceurl()->set_value(
       embedded_test_server()->GetURL(
           std::string("/") + kExistentTermsOfServicePath).spec());
   UploadAndInstallDeviceLocalAccountPolicy();
   AddPublicSessionToDevicePolicy(kAccountId1);
 
-  // Log in to the device-local account with a specific locale and keyboard
-  // layout.
   WaitForPolicy();
-  ASSERT_NO_FATAL_FAILURE(WaitForLoginUI());
-  ASSERT_NO_FATAL_FAILURE(
-      StartLogin(kPublicSessionLocale, public_session_input_method_id_));
+  ASSERT_NO_FATAL_FAILURE(SkipToLoginScreen());
+
+  // Select a different locale.
+  content::WebContents* contents = GetWebContents();
+  ASSERT_TRUE(contents);
+  ASSERT_TRUE(content::ExecuteScript(
+      contents,
+      base::StringPrintf(
+          "var languageSelect = document.getElementById('pod-row')"
+          "    .getPodWithUsername_('%s').querySelector('.language-select');"
+          "languageSelect.value = '%s';"
+          "var event = document.createEvent('HTMLEvents');"
+          "event.initEvent('change', false, true);"
+          "languageSelect.dispatchEvent(event);",
+          user_id_1_.c_str(),
+          kPublicSessionLocale)));
+
+  // The UI will have requested an updated list of keyboard layouts at this
+  // point. Flush the FILE and UI thread loops to allow the process that
+  // constructs this list to complete.
+  base::RunLoop run_loop;
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&base::DoNothing),
+      run_loop.QuitClosure());
+  run_loop.Run();
+  base::RunLoop().RunUntilIdle();
 
   // Set up an observer that will quit the message loop when login has succeeded
   // and the first wizard screen, if any, is being shown.
   base::RunLoop login_wait_run_loop;
   chromeos::MockAuthStatusConsumer login_status_consumer;
   EXPECT_CALL(login_status_consumer, OnAuthSuccess(_)).Times(1).WillOnce(
       InvokeWithoutArgs(&login_wait_run_loop, &base::RunLoop::Quit));
-
-  // Spin the loop until the observer fires. Then, unregister the observer.
   chromeos::ExistingUserController* controller =
       chromeos::ExistingUserController::current_controller();
   ASSERT_TRUE(controller);
   controller->set_login_status_consumer(&login_status_consumer);
+
+  // Manually select a different keyboard layout and click the enter button to
+  // start the session.
+  ASSERT_TRUE(content::ExecuteScript(
+      contents,
+      base::StringPrintf(
+          "var pod ="
+          "    document.getElementById('pod-row').getPodWithUsername_('%s');"
+          "pod.querySelector('.keyboard-select').value = '%s';"
+          "pod.querySelector('.enter-button').click();",
+          user_id_1_.c_str(),
+          public_session_input_method_id_.c_str())));
+
+  // Spin the loop until the login observer fires. Then, unregister the
+  // observer.
   login_wait_run_loop.Run();
   controller->set_login_status_consumer(NULL);
 
   // Verify that the Terms of Service screen is being shown.
   chromeos::WizardController* wizard_controller =
         chromeos::WizardController::default_controller();
   ASSERT_TRUE(wizard_controller);
   ASSERT_TRUE(wizard_controller->current_screen());
   EXPECT_EQ(chromeos::WizardController::kTermsOfServiceScreenName,
             wizard_controller->current_screen()->GetName());
 
   // Wait for the Terms of Service to finish downloading.
-  chromeos::LoginDisplayHostImpl* host =
-      reinterpret_cast<chromeos::LoginDisplayHostImpl*>(
-          chromeos::LoginDisplayHostImpl::default_host());
-  ASSERT_TRUE(host);
-  chromeos::WebUILoginView* web_ui_login_view = host->GetWebUILoginView();
-  ASSERT_TRUE(web_ui_login_view);
-  content::WebUI* web_ui = web_ui_login_view->GetWebUI();
-  ASSERT_TRUE(web_ui);
-  content::WebContents* contents = web_ui->GetWebContents();
-  ASSERT_TRUE(contents);
   bool done = false;
   ASSERT_TRUE(content::ExecuteScriptAndExtractBool(contents,
       "var screenElement = document.getElementById('terms-of-service');"
       "function SendReplyIfDownloadDone() {"
       "  if (screenElement.classList.contains('tos-loading'))"
       "    return false;"
       "  domAutomationController.send(true);"
       "  observer.disconnect();"
       "  return true;"
       "}"
@@ skipping 60 matching lines @@
   // Verify that the Terms of Service screen is being shown.
   chromeos::WizardController* wizard_controller =
         chromeos::WizardController::default_controller();
   ASSERT_TRUE(wizard_controller);
   ASSERT_TRUE(wizard_controller->current_screen());
   EXPECT_EQ(chromeos::WizardController::kTermsOfServiceScreenName,
             wizard_controller->current_screen()->GetName());
 
   // Wait for the Terms of Service to finish downloading, then get the status of
   // the screen's UI elements.
-  chromeos::LoginDisplayHostImpl* host =
-      reinterpret_cast<chromeos::LoginDisplayHostImpl*>(
-          chromeos::LoginDisplayHostImpl::default_host());
-  ASSERT_TRUE(host);
-  chromeos::WebUILoginView* web_ui_login_view = host->GetWebUILoginView();
-  ASSERT_TRUE(web_ui_login_view);
-  content::WebUI* web_ui = web_ui_login_view->GetWebUI();
-  ASSERT_TRUE(web_ui);
-  content::WebContents* contents = web_ui->GetWebContents();
+  content::WebContents* contents = GetWebContents();
   ASSERT_TRUE(contents);
   std::string json;
   ASSERT_TRUE(content::ExecuteScriptAndExtractString(contents,
       "var screenElement = document.getElementById('terms-of-service');"
       "function SendReplyIfDownloadDone() {"
       "  if (screenElement.classList.contains('tos-loading'))"
       "    return false;"
       "  var status = {};"
       "  status.heading = document.getElementById('tos-heading').textContent;"
       "  status.subheading ="
@@ skipping 69 matching lines @@
   ASSERT_TRUE(content::ExecuteScript(contents,
                                      "$('tos-accept-button').click();"));
 
   WaitForSessionStart();
 }
 
 INSTANTIATE_TEST_CASE_P(TermsOfServiceDownloadTestInstance,
                         TermsOfServiceDownloadTest, testing::Bool());
 
 }  // namespace policy