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

Issue 713934 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Make ash::Shell::pref_service() useable in both cash and mash

Project Member Reported by afakhry@chromium.org, Apr 20 2017

Issue description

I encountered today the need to use the pref_service inside ash, but without mash, this Shell::pref_service_ instance is null.

Xiyuan and I discussed the need to make this available in both cash and mash. One proposal was to do the following:
- Instead of a simple accessor pref_service(), we can have a more involved function like:
   PrefService* GetActiveUserPrefService();

- In the case of mash, this can make use of the pref_service_ instance.
- In the case of cash, it goes through the ShellDelegate to get the active user pref service.

Need to consult with sky and james about this.
 
Cc: jonr...@chromium.org
+jonross. My understanding is that there is work underway to make a real pref service available to ash always. Your approach seems OK as a temporary measure.

Cc: tibell@chromium.org
+tibell@ for update on the timing of having pref service available by default.

There were plans to analyze the performance of the pref service before turning it on by default.

I would support #1 as a temporary work around for classic ash.
Description: Show this description
Summary: Make ash::Shell::pref_service() useable in both cash and mash (was: Make ash::Shell::perf_service() useable in both cash and mash)

Comment 5 by sky@chromium.org, Apr 21 2017

Can we not assume there is always a pref_service? ash should still function without one. If it has to assume defaults, that's fine. No pref_service is useful when running in a config similar to "ash_shell_with_content". This is basically what you get when running mash_session with no connection to chrome.
I have a proposed CL at https://codereview.chromium.org/2827193004. sky@ Can you please take a look and suggest alternatives?

Comment 7 by tibell@chromium.org, Apr 24 2017

Cc: sa...@chromium.org
We're currently running a Finch experiment (PrefService) to make sure performance is good enough before we turn the service on (for all of Chrome). We hope that once https://codereview.chromium.org/2812863002/ lands and we get some data for that change we should be good to go. At that point any other service should be able to use it.

Comment 8 by tibell@chromium.org, Apr 24 2017

That CL should have been https://chromium-review.googlesource.com/c/476370/ (it was moved to Gerrit).
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 27 2017

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

commit 15eb928d886a49c90111f25af454be9df4fba3a7
Author: afakhry <afakhry@chromium.org>
Date: Thu Apr 27 04:03:31 2017

Make PrefService available in *ash

This CL adds a temporary solution to make the active user's pref
service available in *ash.

BUG= 713934 

Review-Url: https://codereview.chromium.org/2827193004
Cr-Commit-Position: refs/heads/master@{#467576}

[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/mus/shell_delegate_mus.cc
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/mus/shell_delegate_mus.h
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/shell.cc
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/shell.h
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/shell/shell_delegate_impl.cc
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/shell/shell_delegate_impl.h
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/shell_delegate.h
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/test/BUILD.gn
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/test/test_shell_delegate.cc
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/ash/test/test_shell_delegate.h
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/15eb928d886a49c90111f25af454be9df4fba3a7/chrome/browser/ui/ash/chrome_shell_delegate.h

Status: Fixed (was: Assigned)
Project Member

Comment 11 by bugdroid1@chromium.org, May 4 2017

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

commit 47aabc548f0762b7f8ada3a20ac16d07a8b0e35c
Author: afakhry <afakhry@chromium.org>
Date: Thu May 04 18:20:20 2017

The user's profile might not be ready even if we have an active user

Fix a potential crash when we have an active user, but its profile
is not ready yet.

BUG= 713934 

Review-Url: https://codereview.chromium.org/2864453003
Cr-Commit-Position: refs/heads/master@{#469386}

[modify] https://crrev.com/47aabc548f0762b7f8ada3a20ac16d07a8b0e35c/chrome/browser/ui/ash/chrome_shell_delegate.cc

Labels: VerifyIn-61

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

Status: Archived (was: Fixed)

Sign in to add a comment