Add permission request creator class.

Add permission request creator class.

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

[Bernhard Bauer, 2014-05-15 13:56:39]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/managed_user_service.cc:480: output,
Nit: Indent (4 spaces)

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/managed_user_service.cc:595:
->GetAuthenticatedAccountId();
This is going to be empty for a supervised user (because that user is not
considered signed in). The pseudo account ID that we use in
ProfileOAuth2TokenService is managed_users::kManagedUserPseudoEmail.

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.cc:47: const std::string&
account_id,
Nit: alignment

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.cc:181: if (!value.get()
|| !value->GetAsDictionary(&dict)) {
scoped_ptr has an implicit conversion to boolean, so .get() is unnecessary.

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.cc:209: const
std::string& account_id,
Nit: alignment

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.h:34: virtual void
CreatePermissionRequest(
Are you planning on adding a mock implementation for tests? Otherwise it's IMO
not worth adding an abstract interface.

https://codereview.chromium.org/288913003/diff/1/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:996: const char kPermissionRequestAPI[] =
"permission-request-api";
I would maybe call this ...URL?

[Adrian Kuegel, 2014-05-15 14:35:31]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/managed_user_service.cc:480: output,
On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> Nit: Indent (4 spaces)

Done.

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/managed_user_service.cc:595:
->GetAuthenticatedAccountId();
On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> This is going to be empty for a supervised user (because that user is not
> considered signed in). The pseudo account ID that we use in
> ProfileOAuth2TokenService is managed_users::kManagedUserPseudoEmail.

Right. This explains why fetching the access token didn't work. It works now,
thanks :)

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.cc:47: const std::string&
account_id,
On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> Nit: alignment

Done.

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.cc:181: if (!value.get()
|| !value->GetAsDictionary(&dict)) {
On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> scoped_ptr has an implicit conversion to boolean, so .get() is unnecessary.

Done.

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.cc:209: const
std::string& account_id,
On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> Nit: alignment

Done.

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.h:34: virtual void
CreatePermissionRequest(
On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> Are you planning on adding a mock implementation for tests? Otherwise it's IMO
> not worth adding an abstract interface.

I assume we will need a mock implementation for tests, or otherwise mock out the
call to the API.

https://codereview.chromium.org/288913003/diff/1/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:996: const char kPermissionRequestAPI[] =
"permission-request-api";
On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> I would maybe call this ...URL? 

Are you suggesting the suffix APIURL or just URL? I assumed just URL, because
APIURL looks weird to me.

[Bernhard Bauer, 2014-05-15 15:04:54]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/managed_user_service.cc:595:
->GetAuthenticatedAccountId();
On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > This is going to be empty for a supervised user (because that user is not
> > considered signed in). The pseudo account ID that we use in
> > ProfileOAuth2TokenService is managed_users::kManagedUserPseudoEmail.
> 
> Right. This explains why fetching the access token didn't work. It works now,
> thanks :)

An alternative would be to use
ManagedUserSigninManagerWrapper::GetAccountIdToUse(), which will return the same
thing, but doesn't require clients of PermissionRequestCreator to know which
exact account ID to use.

https://codereview.chromium.org/288913003/diff/1/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:996: const char kPermissionRequestAPI[] =
"permission-request-api";
On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > I would maybe call this ...URL? 
> 
> Are you suggesting the suffix APIURL or just URL? I assumed just URL, because
> APIURL looks weird to me.

Meh, ApiUrl maybe? :) My point is more that it should end with URL, independent
of what comes before, because that makes it clearer what the value means. But
maybe I'm overthinking this.

[Adrian Kuegel, 2014-05-15 15:42:16]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/managed_user_service.cc:595:
->GetAuthenticatedAccountId();
On 2014/05/15 15:04:54, Bernhard Bauer wrote:
> On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> > On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > > This is going to be empty for a supervised user (because that user is not
> > > considered signed in). The pseudo account ID that we use in
> > > ProfileOAuth2TokenService is managed_users::kManagedUserPseudoEmail.
> > 
> > Right. This explains why fetching the access token didn't work. It works
now,
> > thanks :)
> 
> An alternative would be to use
> ManagedUserSigninManagerWrapper::GetAccountIdToUse(), which will return the
same
> thing, but doesn't require clients of PermissionRequestCreator to know which
> exact account ID to use.

Right. I refactored this a bit, and pass now only the profile to
PermissionRequestCreator::Create, and use the ManagedUserSigninManagerWrapper
there. What do you think?

https://codereview.chromium.org/288913003/diff/1/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:996: const char kPermissionRequestAPI[] =
"permission-request-api";
On 2014/05/15 15:04:54, Bernhard Bauer wrote:
> On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> > On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > > I would maybe call this ...URL? 
> > 
> > Are you suggesting the suffix APIURL or just URL? I assumed just URL,
because
> > APIURL looks weird to me.
> 
> Meh, ApiUrl maybe? :) My point is more that it should end with URL,
independent
> of what comes before, because that makes it clearer what the value means. But
> maybe I'm overthinking this.

Done.

[Bernhard Bauer, 2014-05-15 15:58:17]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/managed_user_service.cc:595:
->GetAuthenticatedAccountId();
On 2014/05/15 15:42:16, Adrian Kuegel wrote:
> On 2014/05/15 15:04:54, Bernhard Bauer wrote:
> > On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> > > On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > > > This is going to be empty for a supervised user (because that user is
not
> > > > considered signed in). The pseudo account ID that we use in
> > > > ProfileOAuth2TokenService is managed_users::kManagedUserPseudoEmail.
> > > 
> > > Right. This explains why fetching the access token didn't work. It works
> now,
> > > thanks :)
> > 
> > An alternative would be to use
> > ManagedUserSigninManagerWrapper::GetAccountIdToUse(), which will return the
> same
> > thing, but doesn't require clients of PermissionRequestCreator to know which
> > exact account ID to use.
> 
> Right. I refactored this a bit, and pass now only the profile to
> PermissionRequestCreator::Create, and use the ManagedUserSigninManagerWrapper
> there. What do you think?

Passing only the profile to Create() is okay, but personally, I would pass in
the ManagedUserSigninManagerWrapper, ProfileOAuth2TokenService and
RequestContextGetter to the constructor, because dependency injection.

Also, if you have a ManagedUserSigninManagerWrapper, you could use that to get
the OAuth2 scope to use, which removes more dependencies on Supervised User
implementation details.

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.h:34: virtual void
CreatePermissionRequest(
On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > Are you planning on adding a mock implementation for tests? Otherwise it's
IMO
> > not worth adding an abstract interface.
> 
> I assume we will need a mock implementation for tests, or otherwise mock out
the
> call to the API.

OK. Another thing that we could do actually is to use this interface as well for
the code that currently calls ManagedUserSettingsService::UploadItem. Then we'd
just instantiate a different implementation depending on the command line flag.

[Bernhard Bauer, 2014-05-15 16:23:34]

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
File chrome/browser/managed_mode/permission_request_creator.cc (right):

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
chrome/browser/managed_mode/permission_request_creator.cc:97: :
OAuth2TokenService::Consumer("managed_user"),
Should we use a different ID here?

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
chrome/browser/managed_mode/permission_request_creator.cc:125: access_token_ =
access_token;
We don't really use the |access_token_| outside of this method.

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
chrome/browser/managed_mode/permission_request_creator.cc:206: const
std::string& token) {
This parameter is unnecessary.

[Adrian Kuegel, 2014-05-16 13:21:57]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/managed_user_service.cc:595:
->GetAuthenticatedAccountId();
On 2014/05/15 15:58:18, Bernhard Bauer wrote:
> On 2014/05/15 15:42:16, Adrian Kuegel wrote:
> > On 2014/05/15 15:04:54, Bernhard Bauer wrote:
> > > On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> > > > On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > > > > This is going to be empty for a supervised user (because that user is
> not
> > > > > considered signed in). The pseudo account ID that we use in
> > > > > ProfileOAuth2TokenService is managed_users::kManagedUserPseudoEmail.
> > > > 
> > > > Right. This explains why fetching the access token didn't work. It works
> > now,
> > > > thanks :)
> > > 
> > > An alternative would be to use
> > > ManagedUserSigninManagerWrapper::GetAccountIdToUse(), which will return
the
> > same
> > > thing, but doesn't require clients of PermissionRequestCreator to know
which
> > > exact account ID to use.
> > 
> > Right. I refactored this a bit, and pass now only the profile to
> > PermissionRequestCreator::Create, and use the
ManagedUserSigninManagerWrapper
> > there. What do you think?
> 
> Passing only the profile to Create() is okay, but personally, I would pass in
> the ManagedUserSigninManagerWrapper, ProfileOAuth2TokenService and
> RequestContextGetter to the constructor, because dependency injection.
> 
> Also, if you have a ManagedUserSigninManagerWrapper, you could use that to get
> the OAuth2 scope to use, which removes more dependencies on Supervised User
> implementation details.

Makes sense. Done.

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.h:34: virtual void
CreatePermissionRequest(
On 2014/05/15 15:58:18, Bernhard Bauer wrote:
> On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> > On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > > Are you planning on adding a mock implementation for tests? Otherwise it's
> IMO
> > > not worth adding an abstract interface.
> > 
> > I assume we will need a mock implementation for tests, or otherwise mock out
> the
> > call to the API.
> 
> OK. Another thing that we could do actually is to use this interface as well
for
> the code that currently calls ManagedUserSettingsService::UploadItem. Then
we'd
> just instantiate a different implementation depending on the command line
flag.

Would the implementation for that then be in a separate file, for example
permission_request_creator_sync.cc? Or should I just put another class in the
same file?

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
File chrome/browser/managed_mode/permission_request_creator.cc (right):

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
chrome/browser/managed_mode/permission_request_creator.cc:97: :
OAuth2TokenService::Consumer("managed_user"),
On 2014/05/15 16:23:35, Bernhard Bauer wrote:
> Should we use a different ID here?

What about "permissions_creator"?

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
chrome/browser/managed_mode/permission_request_creator.cc:125: access_token_ =
access_token;
On 2014/05/15 16:23:35, Bernhard Bauer wrote:
> We don't really use the |access_token_| outside of this method.

It is also used in OnURLFetchComplete for invalidating the token if the http
request fails.

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
chrome/browser/managed_mode/permission_request_creator.cc:206: const
std::string& token) {
On 2014/05/15 16:23:35, Bernhard Bauer wrote:
> This parameter is unnecessary.

Done.

[Bernhard Bauer, 2014-05-16 13:52:35]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.h:34: virtual void
CreatePermissionRequest(
On 2014/05/16 13:21:57, Adrian Kuegel wrote:
> On 2014/05/15 15:58:18, Bernhard Bauer wrote:
> > On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> > > On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > > > Are you planning on adding a mock implementation for tests? Otherwise
it's
> > IMO
> > > > not worth adding an abstract interface.
> > > 
> > > I assume we will need a mock implementation for tests, or otherwise mock
out
> > the
> > > call to the API.
> > 
> > OK. Another thing that we could do actually is to use this interface as well
> for
> > the code that currently calls ManagedUserSettingsService::UploadItem. Then
> we'd
> > just instantiate a different implementation depending on the command line
> flag.
> 
> Would the implementation for that then be in a separate file, for example
> permission_request_creator_sync.cc? Or should I just put another class in the
> same file?

No, I think it should be a separate file, because both implementations of the
interface are on a par.

(Which I guess means that the HTTP API implementation should also get its own
file).

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
File chrome/browser/managed_mode/permission_request_creator.cc (right):

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
chrome/browser/managed_mode/permission_request_creator.cc:97: :
OAuth2TokenService::Consumer("managed_user"),
On 2014/05/16 13:21:57, Adrian Kuegel wrote:
> On 2014/05/15 16:23:35, Bernhard Bauer wrote:
> > Should we use a different ID here?
> 
> What about "permissions_creator"?

SG. We can still use "managed user" in there, it just shouldn't be the exact
same string, as that would sort of defeat the purpose.

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
chrome/browser/managed_mode/permission_request_creator.cc:215: Profile* profile)
{
I think having a convenience method that will just take a profile is fine. What
I wanted was just a way to inject other dependencies into the constructor (e.g.
for tests).

[Adrian Kuegel, 2014-05-16 14:57:19]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.h:34: virtual void
CreatePermissionRequest(
On 2014/05/16 13:52:36, Bernhard Bauer wrote:
> On 2014/05/16 13:21:57, Adrian Kuegel wrote:
> > On 2014/05/15 15:58:18, Bernhard Bauer wrote:
> > > On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> > > > On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > > > > Are you planning on adding a mock implementation for tests? Otherwise
> it's
> > > IMO
> > > > > not worth adding an abstract interface.
> > > > 
> > > > I assume we will need a mock implementation for tests, or otherwise mock
> out
> > > the
> > > > call to the API.
> > > 
> > > OK. Another thing that we could do actually is to use this interface as
well
> > for
> > > the code that currently calls ManagedUserSettingsService::UploadItem. Then
> > we'd
> > > just instantiate a different implementation depending on the command line
> > flag.
> > 
> > Would the implementation for that then be in a separate file, for example
> > permission_request_creator_sync.cc? Or should I just put another class in
the
> > same file?
> 
> No, I think it should be a separate file, because both implementations of the
> interface are on a par.
> 
> (Which I guess means that the HTTP API implementation should also get its own
> file).

Done.

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
File chrome/browser/managed_mode/permission_request_creator.cc (right):

https://codereview.chromium.org/288913003/diff/70001/chrome/browser/managed_m...
chrome/browser/managed_mode/permission_request_creator.cc:215: Profile* profile)
{
On 2014/05/16 13:52:36, Bernhard Bauer wrote:
> I think having a convenience method that will just take a profile is fine.
What
> I wanted was just a way to inject other dependencies into the constructor
(e.g.
> for tests).

Done.

[Bernhard Bauer, 2014-05-16 15:22:50]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.h:34: virtual void
CreatePermissionRequest(
On 2014/05/16 14:57:19, Adrian Kuegel wrote:
> On 2014/05/16 13:52:36, Bernhard Bauer wrote:
> > On 2014/05/16 13:21:57, Adrian Kuegel wrote:
> > > On 2014/05/15 15:58:18, Bernhard Bauer wrote:
> > > > On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> > > > > On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > > > > > Are you planning on adding a mock implementation for tests?
Otherwise
> > it's
> > > > IMO
> > > > > > not worth adding an abstract interface.
> > > > > 
> > > > > I assume we will need a mock implementation for tests, or otherwise
mock
> > out
> > > > the
> > > > > call to the API.
> > > > 
> > > > OK. Another thing that we could do actually is to use this interface as
> well
> > > for
> > > > the code that currently calls ManagedUserSettingsService::UploadItem.
Then
> > > we'd
> > > > just instantiate a different implementation depending on the command
line
> > > flag.
> > > 
> > > Would the implementation for that then be in a separate file, for example
> > > permission_request_creator_sync.cc? Or should I just put another class in
> the
> > > same file?
> > 
> > No, I think it should be a separate file, because both implementations of
the
> > interface are on a par.
> > 
> > (Which I guess means that the HTTP API implementation should also get its
own
> > file).

^^^ Note this part.

Sorry if I was not being clear enough here. I would like to see the following:
* A fully abstract interface PermissionRequestCreator
* An implementation PermissionRequestCreatorApiary that uses an API call over
HTTP (declared in the class header, implemented in the .cc file)
* An implementation PermissionRequestCreatorSync that uploads a
ManagedUserSetting (also declared in the class header, implemented in the .cc
file)

The names are just suggestions; I could also imagine calling the Apiary one
...Impl and the other one ...Legacy, although that won't scale if we should ever
add another implementation :D (See also: (New)+ Tab Page).

https://codereview.chromium.org/288913003/diff/110001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/110001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator.h:36: static
scoped_ptr<PermissionRequestCreator> CreateWithProfile(
Let's move these to the corresponding implementation classes. The code that
calling these creator methods knows which implementation they want, so it's not
like we have to hide which class it is from them.

https://codereview.chromium.org/288913003/diff/110001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator_sync.cc (right):

https://codereview.chromium.org/288913003/diff/110001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator_sync.cc:29: class
PermissionRequestCreatorImpl : public PermissionRequestCreator {
Wait, do you have two classes with the same name? I know they're in anonymous
namespaces, but that's just asking for trouble.

[Adrian Kuegel, 2014-05-19 11:00:52]

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/1/chrome/browser/managed_mode/...
chrome/browser/managed_mode/permission_request_creator.h:34: virtual void
CreatePermissionRequest(
On 2014/05/16 15:22:50, Bernhard Bauer wrote:
> On 2014/05/16 14:57:19, Adrian Kuegel wrote:
> > On 2014/05/16 13:52:36, Bernhard Bauer wrote:
> > > On 2014/05/16 13:21:57, Adrian Kuegel wrote:
> > > > On 2014/05/15 15:58:18, Bernhard Bauer wrote:
> > > > > On 2014/05/15 14:35:32, Adrian Kuegel wrote:
> > > > > > On 2014/05/15 13:56:40, Bernhard Bauer wrote:
> > > > > > > Are you planning on adding a mock implementation for tests?
> Otherwise
> > > it's
> > > > > IMO
> > > > > > > not worth adding an abstract interface.
> > > > > > 
> > > > > > I assume we will need a mock implementation for tests, or otherwise
> mock
> > > out
> > > > > the
> > > > > > call to the API.
> > > > > 
> > > > > OK. Another thing that we could do actually is to use this interface
as
> > well
> > > > for
> > > > > the code that currently calls ManagedUserSettingsService::UploadItem.
> Then
> > > > we'd
> > > > > just instantiate a different implementation depending on the command
> line
> > > > flag.
> > > > 
> > > > Would the implementation for that then be in a separate file, for
example
> > > > permission_request_creator_sync.cc? Or should I just put another class
in
> > the
> > > > same file?
> > > 
> > > No, I think it should be a separate file, because both implementations of
> the
> > > interface are on a par.
> > > 
> > > (Which I guess means that the HTTP API implementation should also get its
> own
> > > file).
> 
> ^^^ Note this part.
> 
> Sorry if I was not being clear enough here. I would like to see the following:
> * A fully abstract interface PermissionRequestCreator
> * An implementation PermissionRequestCreatorApiary that uses an API call over
> HTTP (declared in the class header, implemented in the .cc file)
> * An implementation PermissionRequestCreatorSync that uploads a
> ManagedUserSetting (also declared in the class header, implemented in the .cc
> file)
> 
> The names are just suggestions; I could also imagine calling the Apiary one
> ...Impl and the other one ...Legacy, although that won't scale if we should
ever
> add another implementation :D (See also: (New)+ Tab Page).

Done.

https://codereview.chromium.org/288913003/diff/110001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator.h (right):

https://codereview.chromium.org/288913003/diff/110001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator.h:36: static
scoped_ptr<PermissionRequestCreator> CreateWithProfile(
On 2014/05/16 15:22:50, Bernhard Bauer wrote:
> Let's move these to the corresponding implementation classes. The code that
> calling these creator methods knows which implementation they want, so it's
not
> like we have to hide which class it is from them.

Done.

https://codereview.chromium.org/288913003/diff/110001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator_sync.cc (right):

https://codereview.chromium.org/288913003/diff/110001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator_sync.cc:29: class
PermissionRequestCreatorImpl : public PermissionRequestCreator {
On 2014/05/16 15:22:50, Bernhard Bauer wrote:
> Wait, do you have two classes with the same name? I know they're in anonymous
> namespaces, but that's just asking for trouble.

Acknowledged.

[Bernhard Bauer, 2014-05-19 17:11:26]

This is coming together nicely :)

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_service.cc:452: const
GoogleServiceAuthError& error) {
Given that GoogleServiceAuthError leaks the (Apiary) implementation and we don't
really know how to surface the error anyway, I'm wondering whether it would make
more sense to change the callback to a base::Closure for now? We can still
return more information when we have a use for it.

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator_apiary.h (right):

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator_apiary.h:52: virtual void
OnGetTokenSuccess(const OAuth2TokenService::Request* request,
Why is this protected?

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator_sync.h (right):

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator_sync.h:25: static
scoped_ptr<PermissionRequestCreator> Create(
This isn't really necessary anymore if you have a public constructor.

[Adrian Kuegel, 2014-05-20 08:38:05]

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_service.cc:452: const
GoogleServiceAuthError& error) {
On 2014/05/19 17:11:26, Bernhard Bauer wrote:
> Given that GoogleServiceAuthError leaks the (Apiary) implementation and we
don't
> really know how to surface the error anyway, I'm wondering whether it would
make
> more sense to change the callback to a base::Closure for now? We can still
> return more information when we have a use for it.

Done.

Should I keep the basic structure to support returning the proper error in
PermissionRequestApiary, or simplify that also?

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator_apiary.h (right):

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator_apiary.h:52: virtual void
OnGetTokenSuccess(const OAuth2TokenService::Request* request,
On 2014/05/19 17:11:26, Bernhard Bauer wrote:
> Why is this protected?

Copy/paste from ManagedUserRefreshTokenFetcher, and forgot to adjust. Thanks for
catching.

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator_sync.h (right):

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator_sync.h:25: static
scoped_ptr<PermissionRequestCreator> Create(
On 2014/05/19 17:11:26, Bernhard Bauer wrote:
> This isn't really necessary anymore if you have a public constructor.

Right. I removed this, and the other Create method, just kept the convenience
method in PermissionRequestCreatorApiary which takes the profile.

[Bernhard Bauer, 2014-05-20 09:04:04]

LGTM! Just one nit below:

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/288913003/diff/150001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_service.cc:452: const
GoogleServiceAuthError& error) {
On 2014/05/20 08:38:06, Adrian Kuegel wrote:
> On 2014/05/19 17:11:26, Bernhard Bauer wrote:
> > Given that GoogleServiceAuthError leaks the (Apiary) implementation and we
> don't
> > really know how to surface the error anyway, I'm wondering whether it would
> make
> > more sense to change the callback to a base::Closure for now? We can still
> > return more information when we have a use for it.
> 
> Done.
> 
> Should I keep the basic structure to support returning the proper error in
> PermissionRequestApiary, or simplify that also?

I think keeping the structure in PermissionRequestCreatorApiary around is okay,
as that is a legitimate thing for an Apiary client to return, we just don't have
a place for it in the PermissionRequestCreator interface right now.

https://codereview.chromium.org/288913003/diff/170001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator_apiary.cc (right):

https://codereview.chromium.org/288913003/diff/170001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator_apiary.cc:60:
signin_wrapper.Pass(),
Nit: align

[Adrian Kuegel, 2014-05-20 09:15:43]

https://codereview.chromium.org/288913003/diff/170001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator_apiary.cc (right):

https://codereview.chromium.org/288913003/diff/170001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator_apiary.cc:60:
signin_wrapper.Pass(),
On 2014/05/20 09:04:04, Bernhard Bauer wrote:
> Nit: align

Thanks for catching. Just to be sure, I just ran git cl format, and kept almost
all of the changes it suggested (I didn't keep the suggested change to put the
closing brace of an empty method in a new line).

[Adrian Kuegel, 2014-05-20 09:15:54]

The CQ bit was checked by akuegel@chromium.org

[commit-bot: I haz the power, 2014-05-20 09:16:12]

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

[Bernhard Bauer, 2014-05-20 09:21:40]

https://codereview.chromium.org/288913003/diff/170001/chrome/browser/managed_...
File chrome/browser/managed_mode/permission_request_creator_apiary.cc (right):

https://codereview.chromium.org/288913003/diff/170001/chrome/browser/managed_...
chrome/browser/managed_mode/permission_request_creator_apiary.cc:60:
signin_wrapper.Pass(),
On 2014/05/20 09:15:44, Adrian Kuegel wrote:
> On 2014/05/20 09:04:04, Bernhard Bauer wrote:
> > Nit: align
> 
> Thanks for catching. Just to be sure, I just ran git cl format, and kept
almost
> all of the changes it suggested (I didn't keep the suggested change to put the
> closing brace of an empty method in a new line).

I am actively not going to have an opinion on this. :-)

[commit-bot: I haz the power, 2014-05-20 15:46:33]

Change committed as 271665