[Chromoting] Add a helper class to map an OAuth access token to an email address.

[Chromoting] Add a helper class to map an OAuth access token to an email address.

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

[simonmorris, 2012-10-02 19:58:18]

ptal

[Jamie, 2012-10-02 20:38:35]

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
File remoting/host/gaia_user_email_fetcher.cc (right):

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:18:
"https://www.googleapis.com/oauth2/v1/userinfo";
This is a different URL to the one we use in the Javascript. Is this a JSON
version of the same thing?

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:38: friend class
base::RefCountedThreadSafe<Core>;
Why is the friend declaration needed?

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:54: DCHECK(!request_.get()) << "Tried
to fetch two things at once!";
This feels like is needs more than just a DCHECK. How about notifying either the
new or the old delegate via a callback?

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:79: request_.reset();
It seems more natural for the reset to happen directly in OnURLFetchComplete,
since that's the callback function.

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
File remoting/host/gaia_user_email_fetcher.h (right):

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.h:30: virtual void OnOAuthError() = 0;
This function doesn't appear ever to be called. Can it be removed?

[simonmorris, 2012-10-02 21:50:52]

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
File remoting/host/gaia_user_email_fetcher.cc (right):

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:18:
"https://www.googleapis.com/oauth2/v1/userinfo";
On 2012/10/02 20:38:35, Jamie wrote:
> This is a different URL to the one we use in the Javascript. Is this a JSON
> version of the same thing?

Yes. This URL is used in google_apis/gaia.

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:38: friend class
base::RefCountedThreadSafe<Core>;
On 2012/10/02 20:38:35, Jamie wrote:
> Why is the friend declaration needed?

To let the ref counter delete the ref-counted object.

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:54: DCHECK(!request_.get()) << "Tried
to fetch two things at once!";
On 2012/10/02 20:38:35, Jamie wrote:
> This feels like is needs more than just a DCHECK. How about notifying either
the
> new or the old delegate via a callback?

It's done this way in GaiaOAuthClient, and the similar routines in
google_apis/gaia.

Run-time notifications don't feel right for a programming error. What should the
new/old delegates do if they get such a notification?

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:79: request_.reset();
On 2012/10/02 20:38:35, Jamie wrote:
> It seems more natural for the reset to happen directly in OnURLFetchComplete,
> since that's the callback function.

Unfortunately, moving the reset to before the OnUserInfoFetchComplete() means
that |source| is mutated, which means the parameters for
OnUserInfoFetchComplete() can't be found.

Again, this is consistent with GaiaOAuthClient.

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
File remoting/host/gaia_user_email_fetcher.h (right):

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.h:30: virtual void OnOAuthError() = 0;
On 2012/10/02 20:38:35, Jamie wrote:
> This function doesn't appear ever to be called. Can it be removed?

Done.

[Jamie, 2012-10-02 22:45:27]

Assuming that the changes to remoting.gyp since patchset 1 are due to a rebase,
lgtm.

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
File remoting/host/gaia_user_email_fetcher.cc (right):

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:54: DCHECK(!request_.get()) << "Tried
to fetch two things at once!";
On 2012/10/02 21:50:52, simonmorris wrote:
> On 2012/10/02 20:38:35, Jamie wrote:
> > This feels like is needs more than just a DCHECK. How about notifying either
> the
> > new or the old delegate via a callback?
> 
> It's done this way in GaiaOAuthClient, and the similar routines in
> google_apis/gaia.
> 
> Run-time notifications don't feel right for a programming error. What should
the
> new/old delegates do if they get such a notification?

It just ensures that the GetUserEmail/callback pairs line up as the caller
expects. It feels like the right thing to do is throw an exception, but that's
not allowed :) Since existing similar classes work this way, I'm happy to leave
it as-is.

http://codereview.chromium.org/11049006/diff/1/remoting/host/gaia_user_email_...
remoting/host/gaia_user_email_fetcher.cc:79: request_.reset();
On 2012/10/02 21:50:52, simonmorris wrote:
> On 2012/10/02 20:38:35, Jamie wrote:
> > It seems more natural for the reset to happen directly in
OnURLFetchComplete,
> > since that's the callback function.
> 
> Unfortunately, moving the reset to before the OnUserInfoFetchComplete() means
> that |source| is mutated, which means the parameters for
> OnUserInfoFetchComplete() can't be found.
> 
> Again, this is consistent with GaiaOAuthClient.

Well, you could save the status and response code before calling the
sub-function, or just eliminate the sub-function altogether. But consistency is
good, so we can agree to disagree here.

[commit-bot: I haz the power, 2012-10-02 23:00:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/11049006/2003

[Sergey Ulanov, 2012-10-02 23:03:05]

Maybe it's better to extend GaiaOAuthClient to allow getting e-mail without
refresh token

[simonmorris, 2012-10-02 23:18:01]

On 2012/10/02 23:03:05, sergeyu wrote:
> Maybe it's better to extend GaiaOAuthClient to allow getting e-mail without
> refresh token

Maybe. There is a TODO to consider moving this class to google_apis/gaia. We can
always refactor later, separately. Refactoring isn't subject to the same time
pressure as adding functionality.

[commit-bot: I haz the power, 2012-10-03 01:11:37]

List of reviewers changed. sergeyu@chromium.org did a drive-by without LGTM'ing!

[commit-bot: I haz the power, 2012-10-03 15:31:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/11049006/2003

[commit-bot: I haz the power, 2012-10-03 15:45:05]

Change committed as 159900