Use new people.get api instead of oauth2/v1/userinfo.

Use new people.get api instead of oauth2/v1/userinfo.
Consolidate all uses into main helper class.

Details about new api: https://developers.google.com/+/api/latest/people/get

Format of returned response: https://developers.google.com/+/api/latest/people#resource

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

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

[Roger Tawa OOO till Jul 10th, 2014-04-25 15:51:56]

Hi Drew, Hui, Monica,

Drew: please look at components/policy
Monica: please look at profile downloader
Hui: please do general review

Thanks.

[Roger Tawa OOO till Jul 10th, 2014-04-25 15:54:15]

Drew: please also look at the cros policy code in chrome/browser/chromeos/policy

Thanks.

[noms (inactive), 2014-04-25 17:07:40]

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_downloader.cc (left):

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_downloader.cc:285:
base::StringPrintf(kAuthorizationHeader, auth_token_.c_str()));
So here we check whether the token is empty, and add the extra headers, but in
GaiaOauthClient::PeopleGet I see we add the headers regardless of whether the
token is empty. Is this ok?

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_downloader_unittest.cc (right):

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_downloader_unittest.cc:16:
scoped_ptr<base::DictionaryValue> GetJSonData(const std::string& full_name,
Can you do something here like in the ProfileDownloader, where you pass the
dictionary as a pointer?

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:265:
SetFakeResponse(GaiaUrls::GetInstance()->people_get_url().spec(),
Should this function be made into a helper? I see that it's called multiple
times with the same parameters

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
File google_apis/gaia/gaia_oauth_client.cc (right):

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
google_apis/gaia/gaia_oauth_client.cc:79: USER_INFO
nit: I think the last enum should end in a trailing comma

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
google_apis/gaia/gaia_oauth_client.cc:299: if (response_dict->GetList("emails",
&emails_list)) {
nit: please add a comment. this looks scary :)

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
google_apis/gaia/gaia_oauth_client.cc:407: int max_retries,
nit: wrong indentation

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
File google_apis/gaia/gaia_oauth_client.h (right):

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
google_apis/gaia/gaia_oauth_client.h:116: int max_retries,
nit: wrong indentation

[Andrew T Wilson (Slow), 2014-04-27 22:58:15]

LGTM with one comment which you can ignore if you like.

https://codereview.chromium.org/257773002/diff/110001/components/policy/core/...
File components/policy/core/common/cloud/user_info_fetcher.h (right):

https://codereview.chromium.org/257773002/diff/110001/components/policy/core/...
components/policy/core/common/cloud/user_info_fetcher.h:53: class GaiaDelegate :
public gaia::GaiaOAuthClient::Delegate {
This is fine as-is. I have a slight preference for the pattern where this .h
file just has a forward declaration/scoped_ptr:

class GaiaDelegate;
scoped_ptr<GaiaDelegate> delegate_;

...and we define + instantiate the class in the .cc file, hiding the internals.
But it's OK if you keep it like this too.

[Joao da Silva, 2014-04-28 17:27:57]

https://codereview.chromium.org/257773002/diff/150001/components/policy/core/...
File components/policy/core/common/cloud/user_info_fetcher.cc (left):

https://codereview.chromium.org/257773002/diff/150001/components/policy/core/...
components/policy/core/common/cloud/user_info_fetcher.cc:45: 0,
GaiaUrls::GetInstance()->oauth_user_info_url(),
This is the problem: the original code used a URLFetcher with ID 0 here, but
GaiaOAuthClient uses an internal fetcher with ID 17109006. The test code must be
updated to expect this ID instead, where appropriate (note that there are more
fetchers involved in those tests that still expect ID 0).

I guess the easiest would be to check one or the other :-)

[Roger Tawa OOO till Jul 10th, 2014-04-28 20:44:12]

Thanks Monica, Drew.  Comments addressed, changes uploaded.  Also fixed failing
user policy unit tests.

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_downloader.cc (left):

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_downloader.cc:285:
base::StringPrintf(kAuthorizationHeader, auth_token_.c_str()));
On 2014/04/25 17:07:40, Monica Dinculescu wrote:
> So here we check whether the token is empty, and add the extra headers, but in
> GaiaOauthClient::PeopleGet I see we add the headers regardless of whether the
> token is empty. Is this ok?

Yup.  In fact, this can't work with an empty token so not sure what the old code
was trying to do.  This function is only called from OnGetTokenSuccess() below
which always returns a non-empty token anyway.

I'll add a DCHECK and also remove the kUserEntryURL constant which is not used.

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_downloader_unittest.cc (right):

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_downloader_unittest.cc:16:
scoped_ptr<base::DictionaryValue> GetJSonData(const std::string& full_name,
On 2014/04/25 17:07:40, Monica Dinculescu wrote:
> Can you do something here like in the ProfileDownloader, where you pass the
> dictionary as a pointer?

Done.

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/257773002/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:265:
SetFakeResponse(GaiaUrls::GetInstance()->people_get_url().spec(),
On 2014/04/25 17:07:40, Monica Dinculescu wrote:
> Should this function be made into a helper? I see that it's called multiple
> times with the same parameters

I think its clearer and more explicit like this for the tests.

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
File google_apis/gaia/gaia_oauth_client.cc (right):

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
google_apis/gaia/gaia_oauth_client.cc:79: USER_INFO
On 2014/04/25 17:07:40, Monica Dinculescu wrote:
> nit: I think the last enum should end in a trailing comma

Done.

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
google_apis/gaia/gaia_oauth_client.cc:299: if (response_dict->GetList("emails",
&emails_list)) {
On 2014/04/25 17:07:40, Monica Dinculescu wrote:
> nit: please add a comment. this looks scary :)

Added comment, but also forgotten error case.

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
google_apis/gaia/gaia_oauth_client.cc:407: int max_retries,
On 2014/04/25 17:07:40, Monica Dinculescu wrote:
> nit: wrong indentation

Done.

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
File google_apis/gaia/gaia_oauth_client.h (right):

https://codereview.chromium.org/257773002/diff/60001/google_apis/gaia/gaia_oa...
google_apis/gaia/gaia_oauth_client.h:116: int max_retries,
On 2014/04/25 17:07:40, Monica Dinculescu wrote:
> nit: wrong indentation

Done.

[Roger Tawa OOO till Jul 10th, 2014-04-28 20:46:00]

Hi Gene,

Can you please do owner review for
chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc?

Thanks.

[noms (inactive), 2014-04-28 20:46:52]

profiles lgtm

[gene, 2014-04-28 21:48:12]

lgtm for
chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc

[Roger Tawa OOO till Jul 10th, 2014-04-29 12:10:13]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:10:16]

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

[commit-bot: I haz the power, 2014-04-29 12:12:10]

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

[commit-bot: I haz the power, 2014-04-29 12:12:10]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium

[guohui, 2014-04-29 13:33:58]

lgtm

[Roger Tawa OOO till Jul 10th, 2014-04-29 16:57:48]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-04-29 16:58:51]

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

[commit-bot: I haz the power, 2014-04-29 22:53:30]

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

[commit-bot: I haz the power, 2014-04-29 22:53:31]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Roger Tawa OOO till Jul 10th, 2014-04-30 00:01:08]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-04-30 00:04:24]

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

[commit-bot: I haz the power, 2014-04-30 04:51:57]

Change committed as 267068

[Michael Achenbach, 2014-04-30 07:16:09]

A revert of this CL has been created in
https://codereview.chromium.org/265563002/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff]Speculative revert for breaking CrOS:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

Will reland if it doesn't help..

[Roger Tawa OOO till Jul 10th, 2014-04-30 13:40:44]

Hi Michael,

I don't believe my CL broke this test.  If you look this trybot from patchset
#7:

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...

You will see that these tests broke with and without my patch.  Did the revert
do anything?  There is not enough history on waterfall console for me to see my
commit, the revert and what happened after that.

[Michael Achenbach, 2014-04-30 18:12:49]

I was just speculating. But with the revert the test stopped failing. You can
see the full build history when clicking on the builders:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

Build 23489 contains your CL and the test starts failing. Build 23491 contains
the revert and the test stopped failing. Maybe try locally? And if it really was
your CL, then there is something wrong with the try bot (which is possible).

[Roger Tawa OOO till Jul 10th, 2014-04-30 19:41:46]

Thanks Michael.  So I can confirm that CL does break this test for real (fix
coming).

This does seem to point to a bug with trybots though.  If you look at the trybot
"linux_chromium_chromeos_rel" for patchset #7 and patchet #8, then are both
green.  The tests failed with my patch but also without my patch.  The latter is
not correct, so maybe there is a problem with the trybots undoing patchsets? 
I'll chat with the bot master.

[Roger Tawa OOO till Jul 10th, 2014-04-30 19:54:01]

Hi Nikita,

Can you do an owner review for
chrome/browser/chromeos/login/user_image_manager_browsertest.cc ?

Thanks.

[Nikita (slow), 2014-04-30 20:01:53]

On 2014/04/30 19:54:01, Roger Tawa wrote:
> Hi Nikita,
> 
> Can you do an owner review for
> chrome/browser/chromeos/login/user_image_manager_browsertest.cc ?
> 
> Thanks.

lgtm

[Roger Tawa OOO till Jul 10th, 2014-04-30 22:35:48]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:36:11]

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

[commit-bot: I haz the power, 2014-04-30 23:27:25]

Change committed as 267374