New issue
Advanced search Search tips

Issue 753149 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
Proj-Servicification

Blocking:
issue 753012



Sign in to add a comment

mash: Refine AshTestBase's TestPrefService support

Project Member Reported by msw@chromium.org, Aug 7 2017

Issue description

mash: Refine AshTestBase's TestPrefService support

Shell::GetActiveUserPrefService() differs for classic ash and mash:
-classic ash returns a raw pointer from the ShellDelegate.
-mash returns the value of its locally-owned unique_ptr (null in tests).
 (the unique_ptr is set via connection to the preferences service)

AshTestBase/TestShellDelegate doesn't provide a default/test PrefService.

It seems impossible to use Shell::GetActiveUserPrefService in Mash tests.
(OnProfilePrefServiceInitialized() is private and there's no Test Api)

We should make it easier for AshTestBase subclasses to test prefs.
Maybe add a ShellTestApi::Set[Profile|ActiveUser]PrefService...
 (similar to https://chromium-review.googlesource.com/c/604478)
Maybe refine Shell[Delegate] or AshTestBase themselves?

This came up as a null ptr deref in my attempted Ash test code:
https://chromium-review.googlesource.com/c/602737/7/ash/shelf/shelf_controller_unittest.cc#188
 
Owner: sa...@chromium.org
Status: Assigned (was: Available)
It sounds like Sam is working on a similar solution to this.

Sam, assign back to me if that's not the case.

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f4dab61e56b780f1ee1c73daa643fe2a918c5e69

commit f4dab61e56b780f1ee1c73daa643fe2a918c5e69
Author: Sam McNally <sammc@chromium.org>
Date: Wed Aug 16 03:06:33 2017

Maintain a connection to each profile's PrefService from ash.

Move responsibility for managing per-user PrefService instances into
ash::SessionController. When a new user is added, SessionController uses
its Connector to connect to the "ash_pref_connector" service to obtain a
prefs::mojom::PrefStoreConnector for that user, which it then uses to
construct a client PrefService.

Provide access to per-user prefs via GetUserPrefServiceForUser() which
returns the PrefService for a particular AccountId or null if the
connection has not completed successfully, and
GetLastActiveUserPrefService(), which returns the PrefService for the
last active user that had a PrefService.

Consolidate the mash and non-mash code-paths for per-user prefs to
always access per-user PrefService over mojo.

Add a test-only method to SessionController so unit tests can inject a
PrefService for an individual user without mocking out service manager.
Change TestSessionControllerClient to default to injecting a
PrefServicefPrefService for each user session it adds. Update ash
unit tests to no longer inject their own PrefServices now the test
helper handles it.

Replace the preferences_forwarder service with ash_preferences_connector
service. Instead of forwarding connections to the pref service for the
active user, it provides access to a prefs::mojom::PrefStoreConnector
for the AccountId request by SessionController.

Add a new overload to prefs::ConnectToPrefService that takes a
user-provided PrefStoreConnectorPtr for SessionController to use.

Change prefs::ConnectToPrefService to wait until all PrefStores are
initialized instead of just the user prefs pref store and add plumbing
to PrefService and PrefValueStore to expose this information.

Fix PrefRegistrySyncable::ForkForIncognito() to copy all fields from
PrefRegistry.

Bug:  752993 , 753149 
Change-Id: Id36a18988b1e713199f3029cb04a72dd63bf1b34
Reviewed-on: https://chromium-review.googlesource.com/605027
Commit-Queue: Sam McNally <sammc@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494684}
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/BUILD.gn
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/mus/manifest.json
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/mus/shell_delegate_mus.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/mus/shell_delegate_mus.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/public/cpp/shelf_prefs.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/public/cpp/shelf_prefs.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/public/interfaces/BUILD.gn
[add] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/public/interfaces/pref_connector.mojom
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/session/session_controller.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/session/session_controller.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/session/session_controller_unittest.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/session/session_observer.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/session/test_session_controller_client.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/session/test_session_controller_client.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shelf/app_list_button_unittest.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shelf/shelf_alignment_menu.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shelf/shelf_controller.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shelf/shelf_controller.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shelf/shelf_controller_unittest.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shell.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shell.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shell/shell_delegate_impl.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shell/shell_delegate_impl.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shell_delegate.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shell_observer.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/shell_unittest.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/system/night_light/night_light_controller.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/system/night_light/night_light_controller.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/system/night_light/night_light_controller_unittest.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/system/night_light/tray_night_light_unittest.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/system/session/logout_button_tray.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/system/session/logout_button_tray.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/system/session/logout_button_tray_unittest.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/system/tray_caps_lock.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/system/tray_caps_lock_unittest.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/test_shell_delegate.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/ash/test_shell_delegate.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/app/BUILD.gn
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/BUILD.gn
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/browser_process_platform_part_chromeos.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/chrome_content_browser_manifest_overlay.json
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/chromeos/preferences.cc
[add] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/chromeos/prefs/ash_pref_connector_manifest.json
[add] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/chromeos/prefs/pref_connector_service.cc
[add] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/chromeos/prefs/pref_connector_service.h
[delete] https://crrev.com/e3684bdd09ae1a352d1696bc574f3e6f81b3abb8/chrome/browser/prefs/active_profile_pref_service.cc
[delete] https://crrev.com/e3684bdd09ae1a352d1696bc574f3e6f81b3abb8/chrome/browser/prefs/active_profile_pref_service.h
[delete] https://crrev.com/e3684bdd09ae1a352d1696bc574f3e6f81b3abb8/chrome/browser/prefs/forwarder_manifest.json
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/ui/ash/chrome_shell_delegate.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/components/pref_registry/pref_registry_syncable.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/components/prefs/pref_service.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/components/prefs/pref_service.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/components/prefs/pref_value_store.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/components/prefs/pref_value_store.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/components/user_manager/fake_user_manager.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/components/user_manager/user.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/components/user_manager/user.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/services/preferences/public/cpp/pref_service_factory.cc
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/services/preferences/public/cpp/pref_service_factory.h
[modify] https://crrev.com/f4dab61e56b780f1ee1c73daa643fe2a918c5e69/services/preferences/public/interfaces/preferences.mojom

Status: Fixed (was: Assigned)
I think this is sufficiently done for now. AshTestBase::SimulateUserLogin() and anything that calls TestSessionControllerClient::AddUserSession() will get a user pref service by default. Access to user prefs is now via SessionController, not ash::Shell, so it should be obvious that's the place to look for prefs.

You can also manually inject pref services into SessionController via test methods.

Comment 4 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 5 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment