Notify sync observers whenever preferred types change

Notify sync observers whenever preferred types change

Changed ProfileSyncService so any call to OnUserChoseDatatypes() will result in observers being notified even if configuration is aborted due to passphrase-required errors.

Also updated sync config UI so the "invalid passphrase" error is generated even for blank passphrases.

BUG=122734
TEST=Turn on explicit passphrase, setup sync, turn off passwords, click OK. Then go into settings and turn passwords on again, click OK, then cancel out of the dialog - wrench menu should have "passphrase error" badge.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136934

[Andrew T Wilson (Slow), 2012-05-10 03:40:11]

PTAL.

[Nicolas Zea, 2012-05-10 20:35:07]

LGTM

http://codereview.chromium.org/10382071/diff/7001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10382071/diff/7001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1138: NotifyObservers();
was the consensus to put this in OnUserChoseDataTypes or in
ChangePreferredTypes?

http://codereview.chromium.org/10382071/diff/7001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1408: // TODO(sync): Maybe toast??
?

http://codereview.chromium.org/10382071/diff/7001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1411: DLOG(ERROR) << "DONE";
remove

[Andrew T Wilson (Slow), 2012-05-11 23:58:39]

jhawkins: Please take a look at sync_setup_handler.cc.

http://codereview.chromium.org/10382071/diff/7001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10382071/diff/7001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1138: NotifyObservers();
I think this is the right place for it - I can conceive of at least one use case
where we'd want it to be called after AcknowledgeSyncedTypes().

[James Hawkins, 2012-05-14 05:19:12]

LGTM. Is it just me or is this beyond hairy?

[Andrew T Wilson (Slow), 2012-05-14 17:05:25]

On 2012/05/14 05:19:12, James Hawkins wrote:
> LGTM. Is it just me or is this beyond hairy?

It's a bit hairy, but it's because the UX is complex/subtle, and I'm trying not
to keep more state in sync_setup_handler if I can avoid it (I'm trying to derive
the state of the UI from the current sync state, rather than trying to keep
around a flag in SyncSetupHandler that says something like "the user has already
tried configuring sync once, so it's OK to display passphrase errors").

[Andrew T Wilson (Slow), 2012-05-14 17:05:30]

On 2012/05/14 05:19:12, James Hawkins wrote:
> LGTM. Is it just me or is this beyond hairy?

It's a bit hairy, but it's because the UX is complex/subtle, and I'm trying not
to keep more state in sync_setup_handler if I can avoid it (I'm trying to derive
the state of the UI from the current sync state, rather than trying to keep
around a flag in SyncSetupHandler that says something like "the user has already
tried configuring sync once, so it's OK to display passphrase errors").

[commit-bot: I haz the power, 2012-05-14 17:05:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/10382071/13001

[commit-bot: I haz the power, 2012-05-14 19:23:19]

Change committed as 136934