Enable account reconcilor when --new-profile-management is used.

Enable account reconcilor when --new-profile-management is used.
Move mergesession from SigninManager to reconcilor.
Implement ListAccounts.
Start implementing reconcile action, but still a noop for now.

This CL depends on https://codereview.chromium.org/57103002/

Alan: here is an implementation of the ListAccounts.  I also added lots of
DVLOG() to the code.  Let me know if you'd rather I took those out and put
in a separate CL.

Hui: please review.  This CL adds more functionality to the reconcilor but
its still not complete.  However, it enables AC behind the flag for merge
session so you don't need to do that manually.

BUG=305249
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233424

[acleung1, 2013-11-04 21:18:59]

LOG's fine, I just had some high level questions first.

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:134: DVLOG(1) <<
"AccountReconcilor::Observe: cookies changed";
This might be too verbose. We should probably LOG only when it is a
accounts.google.com cookie

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:216: scoped_ptr<base::Value>
value(base::JSONReader::Read(data));
What are the cases where we get no data? Should there be re-try logics if that's
the case?

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:272: for (size_t i = 0; i <
chrome_accounts_.size(); ++i) {
I am not entirely following this step.

Wouldn't we normally get a onRefreshToken_____ if the O2TS's account changed?
Wouldn't this be redundant?

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.h (right):

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.h:99: std::vector<std::string>
gaia_accounts_;
My understanding is that we are going to eventually post PerformReconcilation to
worker thread.

Would we run into race conditions if we have member variables like this?

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.h:100: // These members are used to
validate the tokens in OAuth2TokenService.
May be a new line before this for better readability?

[guohui, 2013-11-04 22:44:15]

lgtm

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:162: GoogleAutoLoginHelper* helper =
new GoogleAutoLoginHelper(profile_);
another helper? we have a bunch of helpers to clean up after the demo =)

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:211: std::vector<std::string>
AccountReconcilor::ParseListAccountsData(
is it better to move the parsing code to GaiaAuthFetcher so that it could be
reused by others?

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:263: DCHECK(chrome_accounts_.size()
> 0);
why is this a DCHECK? shouldn't we handle this just as another error case and
show reauth UI?

[guohui, 2013-11-04 22:44:16]

lgtm

[Roger Tawa OOO till Jul 10th, 2013-11-04 23:38:42]

Thanks Alan, Hui.  Changes uploaded.

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:134: DVLOG(1) <<
"AccountReconcilor::Observe: cookies changed";
On 2013/11/04 21:19:00, acleung1 wrote:
> This might be too verbose. We should probably LOG only when it is a
> http://accounts.google.com cookie

Agreed. Removed for now.

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:162: GoogleAutoLoginHelper* helper =
new GoogleAutoLoginHelper(profile_);
On 2013/11/04 22:44:16, guohui wrote:
> another helper? we have a bunch of helpers to clean up after the demo =)

:-)  Yes lets do a pass on all our helpers and see how to refactor.

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:211: std::vector<std::string>
AccountReconcilor::ParseListAccountsData(
On 2013/11/04 22:44:16, guohui wrote:
> is it better to move the parsing code to GaiaAuthFetcher so that it could be
> reused by others? 

I'm not sure.  Maybe leave it here for now until some other code needs it?

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:216: scoped_ptr<base::Value>
value(base::JSONReader::Read(data));
On 2013/11/04 21:19:00, acleung1 wrote:
> What are the cases where we get no data? Should there be re-try logics if
that's
> the case?

This would be an error.  The gaia auth fetcher already has retry login built in,
so not sure we need to do anything more.

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:263: DCHECK(chrome_accounts_.size()
> 0);
On 2013/11/04 22:44:16, guohui wrote:
> why is this a DCHECK? shouldn't we handle this just as another error case and
> show reauth UI?

The AC is only enabled for connected profiles.  Therefore if we get here, there
must be at least one token in O2TS.  This is not a count of validated accounts,
this is a count of the accounts known to chrome.

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:272: for (size_t i = 0; i <
chrome_accounts_.size(); ++i) {
On 2013/11/04 21:19:00, acleung1 wrote:
> I am not entirely following this step.
> 
> Wouldn't we normally get a onRefreshToken_____ if the O2TS's account changed?
> Wouldn't this be redundant?

The O2TS may have an list of tokens, but they could be invalid.  For example,
they could have been revoked on the server.  Chrome does not know this until the
next request for an access token.  This loop force a request for access tokens,
therefore acts as a way to check if the tokens are still valid.

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.h (right):

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.h:99: std::vector<std::string>
gaia_accounts_;
On 2013/11/04 21:19:00, acleung1 wrote:
> My understanding is that we are going to eventually post PerformReconcilation
to
> worker thread.
> 
> Would we run into race conditions if we have member variables like this?

These members would only be used by one thread, even when we use a worker
thread, so should be good.

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.h:100: // These members are used to
validate the tokens in OAuth2TokenService.
On 2013/11/04 21:19:00, acleung1 wrote:
> May be a new line before this for better readability?

Done.

[Roger Tawa OOO till Jul 10th, 2013-11-04 23:43:14]

Hi Rachel,

Can you please do an owner review for profile_manager?  Thanks.

[acleung1, 2013-11-05 00:04:27]

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/57363003/diff/430001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:162: GoogleAutoLoginHelper* helper =
new GoogleAutoLoginHelper(profile_);
On 2013/11/04 23:38:42, Roger Tawa wrote:
> On 2013/11/04 22:44:16, guohui wrote:
> > another helper? we have a bunch of helpers to clean up after the demo =)
> 
> :-)  Yes lets do a pass on all our helpers and see how to refactor.

This is slightly misnamed. We used to use that for Android's autologin. 

But we are using it now to add accounts to content area now.

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:207: FinishReconcileAction();
What if I just want a list of accounts for PerformRemoveAction and don't need to
Reconcile?

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.h (right):

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.h:50: 
Should this be a static method?

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.h:51: std::vector<std::string>
ParseListAccountsData(const std::string& data);
Also, seems like it'be very quick to add a unit test for this?

[rpetterson, 2013-11-05 00:16:15]

profiles/ LGTM

[Roger Tawa OOO till Jul 10th, 2013-11-05 21:33:28]

Thanks Alan.  Unit tests always good.  PTAL.

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.cc:207: FinishReconcileAction();
On 2013/11/05 00:04:28, acleung1 wrote:
> What if I just want a list of accounts for PerformRemoveAction and don't need
to
> Reconcile?

Could you use gaia auth fetcher directly and call ParseListAccountsData() (now
static)?  Is there a way for me to refactor this to make it easier?

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor.h (right):

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.h:50: 
On 2013/11/05 00:04:28, acleung1 wrote:
> Should this be a static method?

Done.

https://codereview.chromium.org/57363003/diff/660001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor.h:51: std::vector<std::string>
ParseListAccountsData(const std::string& data);
On 2013/11/05 00:04:28, acleung1 wrote:
> Also, seems like it'be very quick to add a unit test for this?

Done.

[acleung1, 2013-11-06 00:27:01]

lgtm

[commit-bot: I haz the power, 2013-11-06 20:44:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/57363003/1000001

[commit-bot: I haz the power, 2013-11-07 00:03:53]

Change committed as 233424

[acleung1, 2013-11-07 03:46:31]

https://codereview.chromium.org/57363003/diff/1000001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/57363003/diff/1000001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:811:
AccountReconcilorFactory::GetForProfile(profile);
Hmm this works when I patched it in a few days ago but when I sync just now we
stop hitting this path on Android.

I will look into it unless you happen to know why.

[Roger Tawa OOO till Jul 10th, 2013-11-07 03:59:59]

Hi Alan,

Sorry, I don't know why it would stopped working on android. Let me know if
you need anything from me.
On Nov 6, 2013 10:46 PM, <acleung@chromium.org> wrote:

>
> https://codereview.chromium.org/57363003/diff/1000001/
> chrome/browser/profiles/profile_manager.cc
> File chrome/browser/profiles/profile_manager.cc (right):
>
> https://codereview.chromium.org/57363003/diff/1000001/
> chrome/browser/profiles/profile_manager.cc#newcode811
> chrome/browser/profiles/profile_manager.cc:811:
> AccountReconcilorFactory::GetForProfile(profile);
> Hmm this works when I patched it in a few days ago but when I sync just
> now we stop hitting this path on Android.
>
> I will look into it unless you happen to know why.
>
> https://codereview.chromium.org/57363003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.