Allow sync with 2-factor StrongAuth accounts in ChromeOS.

Allow sync with 2-factor StrongAuth accounts in ChromeOS.
1. Detect login with 2-factor error and direct new accounts to chrome://settings/personal
2. Open BorwserSignin from 'Set Up Sync' button in sync settings
3. Enable constrained html dialog in BrowserSigning (was implemented for ChromeOS in a previous CL)
4. Attempt to log into sync when exiting screen locker if sync becomes disabled

BUG=chromium-os:8752
TEST=see issue

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

[Chris Masone, 2010-11-17 01:11:08]

http://codereview.chromium.org/4980005/diff/7001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_status_consumer.h (right):

http://codereview.chromium.org/4980005/diff/7001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_status_consumer.h:101: virtual void
SetLoginFailure(const LoginFailure& error) = 0;
This mechanism is kind of ugly.  Why not just create some kind of flag
(sync_disabled?) in GaiaAuthConsumer::ClientLoginResult that carries this
information for you, since the only time we care is in the case of a TWO_FACTOR
"failure"

http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/google_authenticator.cc (right):

http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/google_authenticator.cc:247:
OnClientLoginSuccess(GaiaAuthConsumer::ClientLoginResult());
And then, here, you would create an appropriately configured CLientLoginResult
object, obviating the need for exposing this notion of "last error"

http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/google_authenticator.cc:287: if
(last_error_.state() != GoogleServiceAuthError::NONE) {
You could toss this too.

http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/google_authenticator.h (right):

http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/google_authenticator.h:103: const
GoogleServiceAuthError& last_error() { return last_error_; }
You can get rid of this if you take the approach I suggest in
login_status_consumer.h

[Nikita (slow), 2010-11-17 12:07:43]

http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:63: DCHECK(!pending_requests);
In R10 we're moving to the case when offline auth is performed first and then
browser is launched, parallel to that online auth is performed.

So this path should be used to open a tab in the currently launching browser
with the URL you're adding in this CL.

See http://codereview.chromium.org/3583013/

[stevenjb, 2010-11-17 23:48:41]

Changes based on Chris's feedback. Improved CL with less fat is now available.

http://codereview.chromium.org/4980005/diff/7001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_status_consumer.h (right):

http://codereview.chromium.org/4980005/diff/7001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_status_consumer.h:101: virtual void
SetLoginFailure(const LoginFailure& error) = 0;
On 2010/11/17 01:11:09, Chris Masone wrote:
> This mechanism is kind of ugly.  Why not just create some kind of flag
> (sync_disabled?) in GaiaAuthConsumer::ClientLoginResult that carries this
> information for you, since the only time we care is in the case of a
TWO_FACTOR
> "failure"

I agree that it was an awkward solution. I failed to notice that
ClientLoginResult was a struct that I could pass this information in. Made
changes to do so which simplify the CL significantly.

http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:63: DCHECK(!pending_requests);
On 2010/11/17 12:07:45, Nikita Kostylev wrote:
> In R10 we're moving to the case when offline auth is performed first and then
> browser is launched, parallel to that online auth is performed.
> 
> So this path should be used to open a tab in the currently launching browser
> with the URL you're adding in this CL.
> 
> See http://codereview.chromium.org/3583013/

The refactoring based on chris's feedback eliminates the need for this change,
so should at least reduce any conflicts with 3583013, but we will want to test
the new logic with 2-factor strong auth and ensure that the new state is
successfully propagated.

[Chris Masone, 2010-11-17 23:51:06]

The google_authenticator.cc change LGTM, though I can't speak to the rest of the
code.

On 2010/11/17 23:48:41, Steven Bennetts wrote:
> Changes based on Chris's feedback. Improved CL with less fat is now available.
> 
>
http://codereview.chromium.org/4980005/diff/7001/chrome/browser/chromeos/logi...
> File chrome/browser/chromeos/login/login_status_consumer.h (right):
> 
>
http://codereview.chromium.org/4980005/diff/7001/chrome/browser/chromeos/logi...
> chrome/browser/chromeos/login/login_status_consumer.h:101: virtual void
> SetLoginFailure(const LoginFailure& error) = 0;
> On 2010/11/17 01:11:09, Chris Masone wrote:
> > This mechanism is kind of ugly.  Why not just create some kind of flag
> > (sync_disabled?) in GaiaAuthConsumer::ClientLoginResult that carries this
> > information for you, since the only time we care is in the case of a
> TWO_FACTOR
> > "failure"
> 
> I agree that it was an awkward solution. I failed to notice that
> ClientLoginResult was a struct that I could pass this information in. Made
> changes to do so which simplify the CL significantly.
> 
>
http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/login_performer.cc (right):
> 
>
http://codereview.chromium.org/4980005/diff/11001/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/login_performer.cc:63:
DCHECK(!pending_requests);
> On 2010/11/17 12:07:45, Nikita Kostylev wrote:
> > In R10 we're moving to the case when offline auth is performed first and
then
> > browser is launched, parallel to that online auth is performed.
> > 
> > So this path should be used to open a tab in the currently launching browser
> > with the URL you're adding in this CL.
> > 
> > See http://codereview.chromium.org/3583013/
> 
> The refactoring based on chris's feedback eliminates the need for this change,
> so should at least reduce any conflicts with 3583013, but we will want to test
> the new logic with 2-factor strong auth and ensure that the new state is
> successfully propagated.

[John Gregg, 2010-11-18 00:05:29]

http://codereview.chromium.org/4980005/diff/19001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/4980005/diff/19001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:358: SigninManager* signin() { return
signin_.get(); }
why are you making signin() public?  Would rather not if not necessary, and I
don't see a call site in your patch.

[stevenjb, 2010-11-18 00:11:09]

http://codereview.chromium.org/4980005/diff/19001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/4980005/diff/19001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:358: SigninManager* signin() { return
signin_.get(); }
On 2010/11/18 00:05:30, John Gregg wrote:
> why are you making signin() public?  Would rather not if not necessary, and I
> don't see a call site in your patch.

I didn't add signin(), just cros_user() (I eliminated a newline). signin() is
called in profile_sync_service_harness.cc.

[John Gregg, 2010-11-18 00:22:50]

http://codereview.chromium.org/4980005/diff/19001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/4980005/diff/19001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:358: SigninManager* signin() { return
signin_.get(); }
On 2010/11/18 00:11:09, Steven Bennetts wrote:
> On 2010/11/18 00:05:30, John Gregg wrote:
> > why are you making signin() public?  Would rather not if not necessary, and
I
> > don't see a call site in your patch.
> 
> I didn't add signin(), just cros_user() (I eliminated a newline). signin() is
> called in profile_sync_service_harness.cc.
> 

Oh, never mind, read it incorrectly.  The sync/browser_signin parts LGTM.

[tim (not reviewing), 2010-11-18 18:37:28]

On 2010/11/18 00:22:50, John Gregg wrote:
>
http://codereview.chromium.org/4980005/diff/19001/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service.h (right):
> 
>
http://codereview.chromium.org/4980005/diff/19001/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service.h:358: SigninManager* signin() {
return
> signin_.get(); }
> On 2010/11/18 00:11:09, Steven Bennetts wrote:
> > On 2010/11/18 00:05:30, John Gregg wrote:
> > > why are you making signin() public?  Would rather not if not necessary,
and
> I
> > > don't see a call site in your patch.
> > 
> > I didn't add signin(), just cros_user() (I eliminated a newline). signin()
is
> > called in profile_sync_service_harness.cc.
> > 
> 
> Oh, never mind, read it incorrectly.  The sync/browser_signin parts LGTM.

If it LGTJohn it LGTM