Use Google Plus API to get profile information

Use Google OAuth userinfo API to get profile information

BUG=91241
TEST=

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

[sail, 2011-12-02 21:28:01]

Originally I tried to use the API from this page:
https://developers.google.com/+/api/latest/people/get

I set kUserEntryURL to "https://www.googleapis.com/plus/v1/people/me" and
kAPIScope to "https://www.googleapis.com/auth/plus.me"

Unfortunately I couldn't get this to work. I kept getting this error:

 "error": {
  "errors": [
   {
    "domain": "usageLimits",
    "reason": "accessNotConfigured",
    "message": "Access Not Configured",
    "debugInfo": "QuotaState: BLOCKED"
   }
  ],
  "code": 403,
  "message": "Access Not Configured"
 }
}

I'm still investigating but the
"https://www.googleapis.com/oauth2/v1/userinfo?alt=json" API seems to work fine
for now.

[Munjal (Google), 2011-12-03 06:02:32]

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_downloader.cc (right):

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:82: bool GetImageURLWithSize(const
GURL& old_url, int size, GURL* new_url) {
Would be nice to add a comment about what this function does? May be with an
example old url and new url.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:89: std::string old_spec =
old_url.spec();
Nit: const std::string& old_spec

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:101: old_spec.substr(end,
old_spec.size() - end);
Nit: You can skip the second parameter to substr since it defaults to the end of
the string.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:108: std::string file_name =
old_url.ExtractFileName();
Nit: const std::string& file_name

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:110: if (pos != std::string::npos)
{
Would it ever happen that pos is npos given that file_name was extracted from
old_url so it has to exist right?

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:112: old_spec.substr(pos - 1,
old_spec.size() - pos + 1);
Nit: Same comment about skipping second arg to substr here.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:115: }
Looking at the two code paths here, it seems like you can just compute prefix
length and suffix start in two code paths and then just have a common code path.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:120: }
It will be nice to add some unit tests for this method since the code is
non-trivial.

[sail, 2011-12-04 01:48:53]

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_downloader.cc (right):

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:82: bool GetImageURLWithSize(const
GURL& old_url, int size, GURL* new_url) {
On 2011/12/03 06:02:33, munjal wrote:
> Would be nice to add a comment about what this function does? May be with an
> example old url and new url.

Done.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:89: std::string old_spec =
old_url.spec();
On 2011/12/03 06:02:33, munjal wrote:
> Nit: const std::string& old_spec

Done.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:101: old_spec.substr(end,
old_spec.size() - end);
On 2011/12/03 06:02:33, munjal wrote:
> Nit: You can skip the second parameter to substr since it defaults to the end
of
> the string.

Done.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:108: std::string file_name =
old_url.ExtractFileName();
On 2011/12/03 06:02:33, munjal wrote:
> Nit: const std::string& file_name

Done.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:110: if (pos != std::string::npos)
{
On 2011/12/03 06:02:33, munjal wrote:
> Would it ever happen that pos is npos given that file_name was extracted from
> old_url so it has to exist right?

Good point. The file name can be empty so I replace this with a check for empty
instead.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:112: old_spec.substr(pos - 1,
old_spec.size() - pos + 1);
On 2011/12/03 06:02:33, munjal wrote:
> Nit: Same comment about skipping second arg to substr here.

Done.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:115: }
On 2011/12/03 06:02:33, munjal wrote:
> Looking at the two code paths here, it seems like you can just compute prefix
> length and suffix start in two code paths and then just have a common code
path.

Done.

http://codereview.chromium.org/8772055/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:120: }
On 2011/12/03 06:02:33, munjal wrote:
> It will be nice to add some unit tests for this method since the code is
> non-trivial.

Done.

[Ivan Korotkov, 2011-12-04 15:08:52]

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_downloader.cc (right):

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:41: // URL requesting user info.
URL requesting G+ API for user info

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:62: const char kPicasaPhotoId[] =
"AAAAAAAAAAA";
We probably don't need this one since we're not using PWA API anymore.

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_downloader.h (right):

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.h:66: static bool
GetProfileNameAndImageURL(const std::string& data,
Please document what |data| is (entry response is vague without context).

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.h:69: int image_size);
if this is public for testing only, consider declaring class
ProfileDownloaderTest as friend instead (a common practice if you search for
"friend\ class\ .*Test").

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_downloader_unittest.cc (right):

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader_unittest.cc:22: 
Please add tests for IsDefaultImageURL as well.

[Munjal (Google), 2011-12-05 09:09:23]

LGTM for changes to use a different endpoint.

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_downloader.cc (right):

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:86: //  
https://example.com/--Abc/AAAAAAAAAAI/AAAAAAAAACQ/Efg/s256-c/photo.jpg
It would be nice to mention the other case also where the old URL has size and
the function replaces that.

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:116: if (pos != std::string::npos)
{
NIt: this can be a DCHECK or a CHECk instead of if since given the code above
pos should never be npos.

[sail, 2011-12-05 19:26:12]

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_downloader.cc (right):

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:41: // URL requesting user info.
On 2011/12/04 15:08:52, Ivan Korotkov wrote:
> URL requesting G+ API for user info

It turns out that using the G+ API won't work. It returns a 404 error for users
who haven't setup G+. Using the OAuth end point works much better.

I'd like to keep the comment general. This way there's less chance that it'll
get out of sync.

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:62: const char kPicasaPhotoId[] =
"AAAAAAAAAAA";
On 2011/12/04 15:08:52, Ivan Korotkov wrote:
> We probably don't need this one since we're not using PWA API anymore.

Since we're using the G+ API it seems safer to leave this.

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:86: //  
https://example.com/--Abc/AAAAAAAAAAI/AAAAAAAAACQ/Efg/s256-c/photo.jpg
On 2011/12/05 09:09:24, munjal wrote:
> It would be nice to mention the other case also where the old URL has size and
> the function replaces that.

Done.

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.cc:116: if (pos != std::string::npos)
{
On 2011/12/05 09:09:24, munjal wrote:
> NIt: this can be a DCHECK or a CHECk instead of if since given the code above
> pos should never be npos.

pos can be std::string::npos if the URL doesn't have a default size component
AND it doesn't have a file name. For example, if the URL Is "http://example.com"
then pos will be std::string::npos.

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_downloader.h (right):

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.h:66: static bool
GetProfileNameAndImageURL(const std::string& data,
On 2011/12/04 15:08:52, Ivan Korotkov wrote:
> Please document what |data| is (entry response is vague without context).

Done.

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader.h:69: int image_size);
On 2011/12/04 15:08:52, Ivan Korotkov wrote:
> if this is public for testing only, consider declaring class
> ProfileDownloaderTest as friend instead (a common practice if you search for
> "friend\ class\ .*Test").

Done.

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_downloader_unittest.cc (right):

http://codereview.chromium.org/8772055/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_downloader_unittest.cc:22: 
On 2011/12/04 15:08:52, Ivan Korotkov wrote:
> Please add tests for IsDefaultImageURL as well.

Done.

[commit-bot: I haz the power, 2011-12-06 02:39:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8772055/17001

[commit-bot: I haz the power, 2011-12-06 03:16:45]

Try job failure for 8772055-17001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-06 04:25:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8772055/21001

[commit-bot: I haz the power, 2011-12-06 05:14:22]

Try job failure for 8772055-21001 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...