New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 804138 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 789285



Sign in to add a comment

Add mojom::PrefServiceObserver for OnProfilePrefServiceInitialized

Project Member Reported by warx@chromium.org, Jan 21 2018

Issue description

In browser tests, we could have the following case (crrev.com/c/858938):

Start new user session, which will make ash side SessionController call AddUserSession and connect to pref service. Since ash's ConnectToPrefService is a async across process call, we need a reliable way to wait ConnectToPrefService is done before performing tests that need ash's pref change communicated to chrome side.

Propose:
interface PrefServiceObserver {
  // Called when the pref service of |account_id| is initialized.
  OnProfilePrefServiceInitialized(signin.mojom.AccountId account_id); 
};

Questions:
(1) observer interface location:
ash/public/interfaces/session_controller.mojom or services/preferences/public/interfaces/preferences.mojom?

(2) how about OnConnectedToPrefService?

(3) Currently it will be used in tests only. Any suggestion because this, location or naming?

An example usage: https://chromium-review.googlesource.com/c/chromium/src/+/877109 (diff between ps1 and ps4).
 

Comment 1 by warx@chromium.org, Jan 21 2018

Btw, in ps11 in crrev.com/c/858938, xiyuan@ suggested we do ConnectToPrefService for the same |account_id| with a empty pref service registration in chrome side. It turns out shared_pref_registry would compare the prefs registered for the same service name: see is_initial_connection DCHECKs in shared_pref_registry.cc

Comment 2 by xiy...@chromium.org, Jan 22 2018

Adding this observer interface should only be our last resort. 

Can we wait for a pref change in test like [1]? This would make test depends on impl details but...

[1]: https://cs.chromium.org/chromium/src/services/preferences/pref_service_factory_unittest.cc?rcl=983d2e755d9c1602ffd1af620bd6dfa484f0dd5e&l=201
I agree it would be nice to avoid adding this interface. I'm OK with relying on impl details.

Comment 4 by warx@chromium.org, Jan 30 2018

Status: WontFix (was: Assigned)
Re #2, it is a StringPrefMember which doesn't go though PrefChangeRegistrar. I am not able to fix it through this way right now.

But For #1, I made a change in shared_pref_registry.cc: if pref_registry is empty, create separate connection for it:

See the new patch in: https://chromium-review.googlesource.com/c/chromium/src/+/858938/9..13

I am going to close this issue as this method is not encouraged.

Sign in to add a comment