Handle ListAccount fetches from within the GaiaCookieManagerService.

Handle ListAccount fetches and watching the GAIA cookies from within the GaiaCookieManagerService.

This CL only implements non-ChromeOS features; AboutSigninInterals and the AccountReconcilor. ChromeOS features will come later, as they require more support. This CL also removes the ReconcilorSource as a parameter to ListAccounts. Changes the observed cookie from the LSID cookie to the APISID cookie at nickk@'s suggestion.

Design doc: https://docs.google.com/document/d/1FfmSS7M87L_2tkVuHO3NuruborFm6qHXe2VdktZg-HQ/

BUG=466799, 463611, 471210
Committed: https://crrev.com/2933d01b2d36bf278a7bf1cb9996dfbeea0d0ef0
Cr-Commit-Position: refs/heads/master@{#326846}

[Mike Lerman, 2015-04-13 20:50:03]

mlerman@chromium.org changed reviewers:
+ rogerta@chromium.org

[Mike Lerman, 2015-04-13 20:50:04]

Hi Roger,

This is CL #6 in my chain :) PTAL!

Thanks,
Mike

[Roger Tawa OOO till Jul 10th, 2015-04-15 18:51:06]

Great cleanup Mike!

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
chrome/browser/signin/account_reconcilor_unittest.cc:360: // This will be the
first call it ListAccounts.
it --> to

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/fake_account_reconcilor.cc (left):

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
chrome/browser/signin/fake_account_reconcilor.cc:38: }
Maybe we should keep this code?  I like the idea of having fake PKSs so that
people don't need to write their own when needed.

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/fake_gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
chrome/browser/signin/fake_gaia_cookie_manager_service.cc:25:
net::URLRequestStatus::SUCCESS);
I think you can wrap lines 20-25 in a static helper function and call it from
the unit test where this Fake is used.  If you added an email argument to the
helper function, you could probably remove a lot of code from the account
reconcilor unit tests.

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/fake_gaia_cookie_manager_service.h (right):

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
chrome/browser/signin/fake_gaia_cookie_manager_service.h:26:
net::FakeURLFetcherFactory url_fetcher_factory_;
I think it would be better to keep this member outside the Fake.  Will it work
if I write a test that uses FakeGaiaCookieManagerService but also uses its own
instance of FakeURLFetcherFactory to mock out other fetches?

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
File components/signin/core/browser/account_reconcilor.cc (left):

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/account_reconcilor.cc:213: }
I don't see the logic from lines 209-213 in OnGaiaAccountsInCookieUpdated().  Is
it no longer needed?

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/account_reconcilor.cc:265: gaia_accounts_,
GoogleServiceAuthError(GoogleServiceAuthError::NONE));
Nit: need { and }

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
File components/signin/core/browser/gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.cc:300:
GURL("http://.google.com"),
A constant very similar to this is also used on chrome_signin_client.cc.  Maybe
this should be moved to GaiaUrls.  Should it be https instead, or in addition
to?

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.cc:328:
accounts->assign(listed_accounts_.begin(), listed_accounts_.end());
Should do this only if the return value is true. Should clean |accounts| if the
return value is false.

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.cc:414:
std::string::npos);
nit: use DCHECK_NE ?

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.cc:529:
GaiaCookieRequestType::LIST_ACCOUNTS);
If |requests_.size() > 1| then maybe we should just ignore this result.  We are
about to change the cookie anyway, so might as well wait.  Maybe queue up
another /ListAccounts at the very end if one is not already there?

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
File components/signin/core/browser/gaia_cookie_manager_service.h (right):

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.h:204: // then we
call ListAccounts and update the UI element.
nit: comment should probably not say UI element but say it fires the appropriate
callback.

[Mike Lerman, 2015-04-16 13:13:46]

Thanks for going over this, Roger.

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
chrome/browser/signin/account_reconcilor_unittest.cc:360: // This will be the
first call it ListAccounts.
On 2015/04/15 18:51:05, Roger Tawa wrote:
> it --> to

Done.

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/fake_account_reconcilor.cc (left):

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
chrome/browser/signin/fake_account_reconcilor.cc:38: }
On 2015/04/15 18:51:05, Roger Tawa wrote:
> Maybe we should keep this code?  I like the idea of having fake PKSs so that
> people don't need to write their own when needed.

With the Getting of Accounts now being done by the [Fake]GCMS, this Fake would
be identical to the real one, there'd be no meaningful overrides.

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/fake_gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
chrome/browser/signin/fake_gaia_cookie_manager_service.cc:25:
net::URLRequestStatus::SUCCESS);
On 2015/04/15 18:51:05, Roger Tawa wrote:
> I think you can wrap lines 20-25 in a static helper function and call it from
> the unit test where this Fake is used.  If you added an email argument to the
> helper function, you could probably remove a lot of code from the account
> reconcilor unit tests.

Discussed offline - adding helper methods to prepare a variety of responses
(List Accounts should return none, 1, 2 accounts or error).

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/fake_gaia_cookie_manager_service.h (right):

https://codereview.chromium.org/1075273002/diff/140001/chrome/browser/signin/...
chrome/browser/signin/fake_gaia_cookie_manager_service.h:26:
net::FakeURLFetcherFactory url_fetcher_factory_;
On 2015/04/15 18:51:05, Roger Tawa wrote:
> I think it would be better to keep this member outside the Fake.  Will it work
> if I write a test that uses FakeGaiaCookieManagerService but also uses its own
> instance of FakeURLFetcherFactory to mock out other fetches?

Good point. I'll accept a FakeURLFetcherFactory as a parameter.

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
File components/signin/core/browser/account_reconcilor.cc (left):

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/account_reconcilor.cc:213: }
On 2015/04/15 18:51:06, Roger Tawa wrote:
> I don't see the logic from lines 209-213 in OnGaiaAccountsInCookieUpdated(). 
Is
> it no longer needed?

It is still - thanks. Good catch.

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/account_reconcilor.cc:265: gaia_accounts_,
GoogleServiceAuthError(GoogleServiceAuthError::NONE));
On 2015/04/15 18:51:06, Roger Tawa wrote:
> Nit: need { and }

Done.

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
File components/signin/core/browser/gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.cc:300:
GURL("http://.google.com"),
On 2015/04/15 18:51:06, Roger Tawa wrote:
> A constant very similar to this is also used on chrome_signin_client.cc. 
Maybe
> this should be moved to GaiaUrls.  Should it be https instead, or in addition
> to?

Done. Also, it's supposed to be http not https - the APISID token isn't secure.

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.cc:328:
accounts->assign(listed_accounts_.begin(), listed_accounts_.end());
On 2015/04/15 18:51:06, Roger Tawa wrote:
> Should do this only if the return value is true. Should clean |accounts| if
the
> return value is false.

Done.

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.cc:414:
std::string::npos);
On 2015/04/15 18:51:06, Roger Tawa wrote:
> nit: use DCHECK_NE ?

Now that I have a GaiaURL for google.com, this is much cleaner.

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.cc:529:
GaiaCookieRequestType::LIST_ACCOUNTS);
On 2015/04/15 18:51:06, Roger Tawa wrote:
> If |requests_.size() > 1| then maybe we should just ignore this result.  We
are
> about to change the cookie anyway, so might as well wait.  Maybe queue up
> another /ListAccounts at the very end if one is not already there?

I'm hesitant if that's the best thing to do. We've already executed the
expensive network request - why not use the result? Also, we may be about to
execute a request which will fail, or is invalid, or otherwise won't change the
cookie. If the cookie ends up changing, then we'll do another ListAccounts to
update the world with The New Truth.

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
File components/signin/core/browser/gaia_cookie_manager_service.h (right):

https://codereview.chromium.org/1075273002/diff/140001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.h:204: // then we
call ListAccounts and update the UI element.
On 2015/04/15 18:51:06, Roger Tawa wrote:
> nit: comment should probably not say UI element but say it fires the
appropriate
> callback.

copy paste error. Done.

[Roger Tawa OOO till Jul 10th, 2015-04-21 13:46:36]

lgtm

Looks great Mike.  Couple of nits below.

https://codereview.chromium.org/1075273002/diff/200001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1075273002/diff/200001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client.cc:67:
cookie_settings->IsSettingCookieAllowed(google_url, google_url);
With this change, |google_url| now has the protocol "http://", where before it
did not.  Just checking that is ok.

https://codereview.chromium.org/1075273002/diff/200001/google_apis/gaia/gaia_...
File google_apis/gaia/gaia_switches.cc (right):

https://codereview.chromium.org/1075273002/diff/200001/google_apis/gaia/gaia_...
google_apis/gaia/gaia_switches.cc:9: const char kGoogleUrl[]                    
= "google_apis-url";
Is this string what you intended?

[Mike Lerman, 2015-04-21 15:16:15]

mlerman@chromium.org changed reviewers:
+ courage@chromium.org, estade@chromium.org, jochen@chromium.org

[Mike Lerman, 2015-04-21 15:16:16]

Thanks for the review, Roger.

courage@ can you look at c/b/extensions/api/identity/identity_apitest.cc?

estade@ can you please look at c/b/ui/webui/signin_internals_ui.cc?

jochen@ can you please look at c/b/BUILD.gn?

Thanks!

https://codereview.chromium.org/1075273002/diff/200001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1075273002/diff/200001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client.cc:67:
cookie_settings->IsSettingCookieAllowed(google_url, google_url);
On 2015/04/21 13:46:36, Roger Tawa wrote:
> With this change, |google_url| now has the protocol "http://", where before it
> did not.  Just checking that is ok.

Yes - the APISID cookie is http:// not https://.

https://codereview.chromium.org/1075273002/diff/200001/google_apis/gaia/gaia_...
File google_apis/gaia/gaia_switches.cc (right):

https://codereview.chromium.org/1075273002/diff/200001/google_apis/gaia/gaia_...
google_apis/gaia/gaia_switches.cc:9: const char kGoogleUrl[]                    
= "google_apis-url";
On 2015/04/21 13:46:36, Roger Tawa wrote:
> Is this string what you intended?

Ah, no! Thanks for catching that.

[Michael Courage, 2015-04-21 16:45:31]

c/b/extensions/api/identity/identity_apitest.cc lgtm

[jochen (gone - plz use gerrit), 2015-04-22 14:46:29]

I can't access the design doc linked in the CL, can you please make it world
readable?

can you send an intent to implement to chromium-dev@ please?

with that, lgtm

[Mike Lerman, 2015-04-22 14:59:55]

World readable.

Intent posted on chromium-dev:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/An7O13Yp2io

Thanks!

[Evan Stade, 2015-04-24 15:22:27]

lgtm

https://codereview.chromium.org/1075273002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin_internals_ui.cc (right):

https://codereview.chromium.org/1075273002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin_internals_ui.cc:76:
std::vector<std::pair<std::string, bool> > cookie_accounts;
you don't need the space between the >s any more

[Mike Lerman, 2015-04-24 15:26:38]

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

[Mike Lerman, 2015-04-24 15:26:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, jochen@chromium.org, courage@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1075273002/#ps260001
(title: "Angle brackets don't need a space")

[commit-bot: I haz the power, 2015-04-24 15:27:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1075273002/260001

[commit-bot: I haz the power, 2015-04-24 15:49:40]

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

[commit-bot: I haz the power, 2015-04-24 15:49:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike Lerman, 2015-04-24 17:31:53]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2015-04-24 17:31:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, jochen@chromium.org, courage@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1075273002/#ps280001
(title: "Fix a test post rebase")

[commit-bot: I haz the power, 2015-04-24 17:32:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1075273002/280001

[commit-bot: I haz the power, 2015-04-24 18:34:37]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-04-24 18:36:52]

Patchset 15 (id:??) landed as
https://crrev.com/2933d01b2d36bf278a7bf1cb9996dfbeea0d0ef0
Cr-Commit-Position: refs/heads/master@{#326846}