PermissionRequestCreatorApiary: Support for multiple simultaneous requests.

PermissionRequestCreatorApiary: Support for multiple simultaneous requests.

BUG=412402
Committed: https://crrev.com/950c6e878db5bfc6fa7e70a2f2134f4c43a43dc8
Cr-Commit-Position: refs/heads/master@{#296173}

[Bernhard Bauer, 2014-09-23 10:17:00]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2014-09-23 10:17:01]

https://codereview.chromium.org/592353002/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/permission_request_creator_apiary.h (right):

https://codereview.chromium.org/592353002/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/permission_request_creator_apiary.h:54:
std::string access_token;
It's a bit weird that we store the access token per request. Do we actually
expect to get different access tokens?

[Marc Treib, 2014-09-23 10:31:03]

https://codereview.chromium.org/592353002/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/permission_request_creator_apiary.h (right):

https://codereview.chromium.org/592353002/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/permission_request_creator_apiary.h:54:
std::string access_token;
On 2014/09/23 10:17:01, Bernhard Bauer wrote:
> It's a bit weird that we store the access token per request. Do we actually
> expect to get different access tokens?

It's my understanding that access tokens are usually quite short-lived, and so
we wouldn't necessarily expect to get the same one across multiple requests. At
least it seems to be common practice to just request a new one every time you
need one.

[Bernhard Bauer, 2014-09-23 11:01:59]

lgtm

https://codereview.chromium.org/592353002/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/permission_request_creator_apiary.h (right):

https://codereview.chromium.org/592353002/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/permission_request_creator_apiary.h:54:
std::string access_token;
On 2014/09/23 10:31:02, Marc Treib wrote:
> On 2014/09/23 10:17:01, Bernhard Bauer wrote:
> > It's a bit weird that we store the access token per request. Do we actually
> > expect to get different access tokens?
> 
> It's my understanding that access tokens are usually quite short-lived, and so
> we wouldn't necessarily expect to get the same one across multiple requests.
At
> least it seems to be common practice to just request a new one every time you
> need one.

Well, short-lived means half an hour or so, I think, and we have support for
fetching a new token if the old one expired, but I suppose there is no reason to
implement our own token caching if PO2TS already does that anyway.

[Marc Treib, 2014-09-23 11:08:43]

General concern: PermissionRequestCreatorApiary doesn't have any tests atm,
though it probably should. But I guess that can go into another CL.

https://codereview.chromium.org/592353002/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/permission_request_creator_apiary.h (right):

https://codereview.chromium.org/592353002/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/permission_request_creator_apiary.h:54:
std::string access_token;
On 2014/09/23 11:01:59, Bernhard Bauer wrote:
> On 2014/09/23 10:31:02, Marc Treib wrote:
> > On 2014/09/23 10:17:01, Bernhard Bauer wrote:
> > > It's a bit weird that we store the access token per request. Do we
actually
> > > expect to get different access tokens?
> > 
> > It's my understanding that access tokens are usually quite short-lived, and
so
> > we wouldn't necessarily expect to get the same one across multiple requests.
> At
> > least it seems to be common practice to just request a new one every time
you
> > need one.
> 
> Well, short-lived means half an hour or so, I think, and we have support for
> fetching a new token if the old one expired, but I suppose there is no reason
to
> implement our own token caching if PO2TS already does that anyway.

Yeah, we could probably avoid explicitly storing the token for each request, but
imo that would only complicate things here.

https://codereview.chromium.org/592353002/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/592353002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:548:
ProfileSyncService* service =
Semi-related: This doesn't make sense if we're using the Apiary creator. Can be
cleaned up in a separate CL though.

[Bernhard Bauer, 2014-09-23 11:56:11]

https://codereview.chromium.org/592353002/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/592353002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:548:
ProfileSyncService* service =
On 2014/09/23 11:08:43, Marc Treib wrote:
> Semi-related: This doesn't make sense if we're using the Apiary creator. Can
be
> cleaned up in a separate CL though.

Yes, and yes. I have a CL that moves this into the PermissionRequestCreator
interface.

[Marc Treib, 2014-09-23 11:59:21]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-09-23 11:59:47]

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

[commit-bot: I haz the power, 2014-09-23 13:07:36]

Committed patchset #2 (id:20001) as 5ec913f68ecfe36de742cde1a4a8cc0dcabdf940

[commit-bot: I haz the power, 2014-09-23 13:08:07]

Patchset 2 (id:??) landed as
https://crrev.com/950c6e878db5bfc6fa7e70a2f2134f4c43a43dc8
Cr-Commit-Position: refs/heads/master@{#296173}