Fix the sync sign in dialog issues for app notifications:

Fix the sync sign in dialog issues for app notifications:
- Call PSS::ShowLoginDialog directly from app notification code so that the dialog comes up even if the user is logged in.
- Small fix to PSS to avoid infinite spinner in cases where the sync backend does not really change anything (ahd hence does not call back PSS).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112840

[asargent_no_longer_on_chrome, 2011-12-02 21:41:37]

lgtm

[lipalani1, 2011-12-02 21:46:39]

LGTM from my side. Here are some issues that you want to close before check in:
1. Can you add a comment of why you are calling OnAuthError.
2. Can you find out from Kevin the legitimate cases for which the call to
UpdateCredentials is needed(one of them is sync being disabled at the domain
level).
3. Based on the feedback for 2 can you add a todo to remove it.

[Andrew T Wilson (Slow), 2011-12-02 21:55:41]

http://codereview.chromium.org/8783001/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notify_channel_ui.cc (right):

http://codereview.chromium.org/8783001/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_ui.cc:119:
service->ShowLoginDialog();
Do we need to worry about non-desktop platforms? Should we be prompting the user
for credentials on CrOS?

http://codereview.chromium.org/8783001/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/8783001/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service.cc:1479: const GoogleServiceAuthError&
last_error = GetAuthError();
Add a comment to explain why this needs to happen.

[Munjal (Google), 2011-12-03 01:25:33]

http://codereview.chromium.org/8783001/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notify_channel_ui.cc (right):

http://codereview.chromium.org/8783001/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_ui.cc:119:
service->ShowLoginDialog();
On 2011/12/02 21:55:41, Andrew T Wilson wrote:
> Do we need to worry about non-desktop platforms? Should we be prompting the
user
> for credentials on CrOS?

On CrOS, the user will be logged in after Chrome updates and at that time the
OAuth2 login token will be generated and hence this method will never execute.

Having said that, I want to think about this more and there is anyway anotehr
fix to this code that is needed. So let me address that in a separate patch in
which I do that fix.

http://codereview.chromium.org/8783001/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/8783001/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service.cc:1479: const GoogleServiceAuthError&
last_error = GetAuthError();
On 2011/12/02 21:55:41, Andrew T Wilson wrote:
> Add a comment to explain why this needs to happen.

Done.

[Andrew T Wilson (Slow), 2011-12-03 01:39:00]

lgtm

[tim (not reviewing), 2011-12-06 23:39:31]

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

http://codereview.chromium.org/8783001/diff/7001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1491: OnAuthError();
Why did we do this this way?  From my email, I suggested "...making it so you
can refresh credentials in the SyncManager, and be notified asynchronously of
that finishing right up to the top of the stack that originated the
UpdateCredentials call in the first place (in ProfileSyncService). "

This seems like more of a hack, so I'm curious what obstacles there were with
the other route (changing it in sync manager).

[Munjal (Google), 2011-12-07 00:05:14]

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

http://codereview.chromium.org/8783001/diff/7001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1491: OnAuthError();
On 2011/12/06 23:39:31, timsteele wrote:
> Why did we do this this way?  From my email, I suggested "...making it so you
> can refresh credentials in the SyncManager, and be notified asynchronously of
> that finishing right up to the top of the stack that originated the
> UpdateCredentials call in the first place (in ProfileSyncService). "
> 
> This seems like more of a hack, so I'm curious what obstacles there were with
> the other route (changing it in sync manager).

The main reason is that this seemed the safest (lowest risk) thing to do in
various discussions with Lingesh and Drew. In fact, Lingesh and Drew
independently mentioned the same approach.

FWIW, our analyses of the code concluded that actually PSS does not need to wait
for a callback from SBH::UpdateCredentials since that only ends up calling
gettime, which is not an authenticated call anyway. Lingesh and Kevin and I
concluded that.

So, if we are right, the right change to do would be to fix that PSS <-> SBH
communication when a user is authenticated.