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

Issue 774221 link

Starred by 1 user
Status: Fixed
Owner:
sa...@chromium.org
Closed: Oct 2017
Cc:
█ bauerb@chromium.org
jonr...@chromium.org
jamescook@chromium.org
█ tibell@chromium.org
mastiz@chromium.org
mustash-bugs@google.com
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Foreign prefs do not sync as expected.

Project Member Reported by msw@chromium.org, Oct 12 2017 
Foreign prefs do not sync as expected.

The only two foreign SYNCABLE_PREF prefs (afaict) are for the Chrome OS shelf; ash::prefs::kShelfAlignment and ash::prefs::kShelfAutoHideBehavior are registered here:
https://cs.chromium.org/chromium/src/ash/shelf/shelf_controller.cc?rcl=fde4a1c817c9b03dec8a0ceb5f46b589874d774a&l=125

Changing the primary display's shelf properties from the shelf/desktop context menu sets these prefs:
https://cs.chromium.org/chromium/src/ash/public/cpp/shelf_prefs.cc?rcl=fc622083527efd731c4d6f89bacd0dae3f9b508c&l=229

When I do that, I expect about:sync-internals to show a Commit Request containing the new alignment value.
(at least, that's the behavior I get when I change another synced pref, like toggling the bookmarks bar)

This seems like a serious issue, but thankfully has limited impact at the moment.

Regression on ToT/Stable (your first login to use a new device should use your most recently synced shelf prefs):
(1) Sign in to a chrome device
(2) Set the shelf on the primary display to left-alignment (via context menu).
(3) Sign in to another device with the same account *for the first time* (or wipe and re-signin on same device).
Expected: The shelf is left-aligned.
Actual: The shelf is bottom-aligned.
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 13 2017 

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

commit 450271fac784bf2ef465450164e271ea0f68cff9
Author: Sam McNally <sammc@chromium.org>
Date: Fri Oct 13 11:47:34 2017

Move OnPrefRegistered from PrefRegistrySimple to PrefRegistry.

PrefRegistrySyncable uses the PrefRegistrySimple::OnPrefRegistered()
hook to register syncable prefs with the PrefServiceSyncable.
However, foreign-owned prefs are registered directly with the base class
PrefRegistry, skipping this hook, resulting in synced prefs owned by a
service other than chrome (in this case ash) not being registered with
the PrefServiceSyncable and thus are not synced.

Move OnPrefRegistered to PrefRegistry and ensure it is called for all
pref registrations with PrefRegistry so synced pref registrations from
all services are correctly registered with the PrefServiceSyncable.

Bug:  774221 
Change-Id: Ic2ffcac651d94b592b263df7f523acf3f9e05f8d
Reviewed-on: https://chromium-review.googlesource.com/717576
Commit-Queue: Sam McNally <sammc@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508670}
[modify] https://crrev.com/450271fac784bf2ef465450164e271ea0f68cff9/components/prefs/pref_registry.cc
[modify] https://crrev.com/450271fac784bf2ef465450164e271ea0f68cff9/components/prefs/pref_registry.h
[modify] https://crrev.com/450271fac784bf2ef465450164e271ea0f68cff9/components/prefs/pref_registry_simple.cc
[modify] https://crrev.com/450271fac784bf2ef465450164e271ea0f68cff9/components/prefs/pref_registry_simple.h

Comment 2 by msw@chromium.org, Oct 13 2017 

Nice, this landed before the M63 branch point: #508578
It might be nice to merge back to M-62, but it's up to you.

Comment 3 by sa...@chromium.org, Oct 17 2017

Status: Fixed (was: Assigned)

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