Load shelf preferences from ChromeLauncherController::AttachProfile.

Load shelf preferences from ChromeLauncherController::AttachProfile.

Load prefs on AttachProfile for initialization and user changes.
( the previous ActiveUserChanged call only addressed user changes )

BUG=679208
TEST=Shelf preferences are respected on login/startup.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2640023005
Cr-Commit-Position: refs/heads/master@{#444625}
Committed: https://chromium.googlesource.com/chromium/src/+/185ea388249bc19676a7f44a12d6fe0881fb0957

[msw, 2017-01-19 00:20:06]

Description was changed from

==========
Load shelf preferences from ChromeLauncherController::AttachProfile.

BUG=679208
==========

to

==========
Load shelf preferences from ChromeLauncherController::AttachProfile.

Load prefs on AttachProfile for initialization and user changes.
( the previous ActiveUserChanged call only addressed user changes )

Also avoid DictionaryPrefUpdate's notifications for unchanged prefs.
( may relate to infinite SetAutoHideBehavior updates http://crbug.com/665093 )

BUG=679208
TEST=Shelf preferences are respected on login/startup.
R=jamescook@chromium.org
==========

[msw, 2017-01-19 00:20:07]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org

[msw, 2017-01-19 00:20:14]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[msw, 2017-01-19 00:20:27]

Description was changed from

==========
Load shelf preferences from ChromeLauncherController::AttachProfile.

Load prefs on AttachProfile for initialization and user changes.
( the previous ActiveUserChanged call only addressed user changes )

Also avoid DictionaryPrefUpdate's notifications for unchanged prefs.
( may relate to infinite SetAutoHideBehavior updates http://crbug.com/665093 )

BUG=679208
TEST=Shelf preferences are respected on login/startup.
R=jamescook@chromium.org
==========

to

==========
Load shelf preferences from ChromeLauncherController::AttachProfile.

Load prefs on AttachProfile for initialization and user changes.
( the previous ActiveUserChanged call only addressed user changes )

Also avoid DictionaryPrefUpdate's notifications for unchanged prefs.
( may relate to infinite SetAutoHideBehavior http://crbug.com/665093 )

BUG=679208
TEST=Shelf preferences are respected on login/startup.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2017-01-19 00:20:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2017-01-19 00:59:55]

Hey James, please take a look; thanks!
I'm working on a test, but that may come in a followup.

[commit-bot: I haz the power, 2017-01-19 01:27:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 01:27:51]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-01-19 01:54:25]

Do you have any idea why this regressed? I'm a little surprised the old code
worked (just updating on active user change).

https://codereview.chromium.org/2640023005/diff/20001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2640023005/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:127: shelf_prefs->Set(key,
pref_dictionary);
Do we rely on this side-effect to create the new prefs dictionary for a new
display?

It looks like maybe if there's a pref with the same name on the primary display,
if you early-exit on GetPerDisplayPref then it might not create the new
dictionary.

[msw, 2017-01-19 02:19:15]

On 2017/01/19 01:54:25, James Cook wrote:
> Do you have any idea why this regressed? I'm a little surprised the old code
> worked (just updating on active user change).

I don't know for sure, but this might have regressed with my change three months
ago! https://codereview.chromium.org/2391253004
I'll have to do some investigation before merging the fix...

https://codereview.chromium.org/2640023005/diff/20001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2640023005/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:127: shelf_prefs->Set(key,
pref_dictionary);
On 2017/01/19 01:54:25, James Cook wrote:
> Do we rely on this side-effect to create the new prefs dictionary for a new
> display?
> 
> It looks like maybe if there's a pref with the same name on the primary
display,
> if you early-exit on GetPerDisplayPref then it might not create the new
> dictionary.

Hmm, good question... it would probably be more prudent to only bail if the
specific per-display preference is already set to the supplied value... I'll try
to work on this tonight or tomorrow, but I'll be on the road to Tahoe and
probably taking off a day or two.

[James Cook, 2017-01-19 02:42:44]

Friendly reminder: M57 branch is tomorrow and this seems like a fix we
definitely want in M57. You might want to keep things small so you can backport
it.

[msw, 2017-01-19 02:57:07]

I reverted the SetPerDisplayPref change; please take another look.

[msw, 2017-01-19 02:57:12]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 02:57:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2017-01-19 03:10:30]

LGTM. You might want to update the CL description. Thanks for jumping on this!

[msw, 2017-01-19 03:10:48]

Description was changed from

==========
Load shelf preferences from ChromeLauncherController::AttachProfile.

Load prefs on AttachProfile for initialization and user changes.
( the previous ActiveUserChanged call only addressed user changes )

Also avoid DictionaryPrefUpdate's notifications for unchanged prefs.
( may relate to infinite SetAutoHideBehavior http://crbug.com/665093 )

BUG=679208
TEST=Shelf preferences are respected on login/startup.
R=jamescook@chromium.org
==========

to

==========
Load shelf preferences from ChromeLauncherController::AttachProfile.

Load prefs on AttachProfile for initialization and user changes.
( the previous ActiveUserChanged call only addressed user changes )

BUG=679208
TEST=Shelf preferences are respected on login/startup.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2017-01-19 03:26:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 03:26:30]

Dry run: This issue passed the CQ dry run.

[msw, 2017-01-19 03:32:48]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2017-01-19 03:33:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 03:37:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484796768259610,
"parent_rev": "453979e27c501fba94dc3156b753235f1ba8c57e", "commit_rev":
"185ea388249bc19676a7f44a12d6fe0881fb0957"}

[commit-bot: I haz the power, 2017-01-19 03:38:10]

Description was changed from

==========
Load shelf preferences from ChromeLauncherController::AttachProfile.

Load prefs on AttachProfile for initialization and user changes.
( the previous ActiveUserChanged call only addressed user changes )

BUG=679208
TEST=Shelf preferences are respected on login/startup.
R=jamescook@chromium.org
==========

to

==========
Load shelf preferences from ChromeLauncherController::AttachProfile.

Load prefs on AttachProfile for initialization and user changes.
( the previous ActiveUserChanged call only addressed user changes )

BUG=679208
TEST=Shelf preferences are respected on login/startup.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2640023005
Cr-Commit-Position: refs/heads/master@{#444625}
Committed:
https://chromium.googlesource.com/chromium/src/+/185ea388249bc19676a7f44a12d6...
==========

[commit-bot: I haz the power, 2017-01-19 03:38:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/185ea388249bc19676a7f44a12d6...