Supervised users: Include extension version in update requests

Supervised users: Include extension version in update requests

BUG=397951
Committed: https://crrev.com/f832a99186ee1be764e806793efdf400c44795ac
Cr-Commit-Position: refs/heads/master@{#322029}

[Marc Treib, 2015-03-23 13:57:22]

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

[Marc Treib, 2015-03-23 13:57:22]

We need to include the extension version so that requests for different versions
of the same extension get distinct request IDs.

[Bernhard Bauer, 2015-03-23 16:18:39]

LGTM, but it might be possible to simplify this code a bit:

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1677: // supervised user itself
can't re-enable the extension.
Nit: "themselves" :)

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/supervis...
File
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc
(right):

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc:124:
std::string object_ref = extension_id + ":" + version.GetString();
BTW, is there a reason why this is part of the PermissionRequestCreator
interface and therefore has to be repeated with every implementation, instead of
moving it to SupervisedUserService?

[Marc Treib, 2015-03-23 16:26:46]

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1677: // supervised user itself
can't re-enable the extension.
On 2015/03/23 16:18:39, Bernhard Bauer wrote:
> Nit: "themselves" :)

Done.

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/supervis...
File
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc
(right):

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc:124:
std::string object_ref = extension_id + ":" + version.GetString();
On 2015/03/23 16:18:39, Bernhard Bauer wrote:
> BTW, is there a reason why this is part of the PermissionRequestCreator
> interface and therefore has to be repeated with every implementation, instead
of
> moving it to SupervisedUserService?

I'd say it's only by coincidence that both PRC implementations handle this the
same way.

[Bernhard Bauer, 2015-03-23 16:51:49]

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/supervis...
File
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc
(right):

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc:124:
std::string object_ref = extension_id + ":" + version.GetString();
On 2015/03/23 16:26:46, Marc Treib wrote:
> On 2015/03/23 16:18:39, Bernhard Bauer wrote:
> > BTW, is there a reason why this is part of the PermissionRequestCreator
> > interface and therefore has to be repeated with every implementation,
instead
> of
> > moving it to SupervisedUserService?
> 
> I'd say it's only by coincidence that both PRC implementations handle this the
> same way.

Really? IMO it's fair to say that what we are abstracting away behind the PRC
interface is *how* we talk to the permission request server, not the server
itself, so all PRC implementations can use a shared format to encode their
requests.

[Marc Treib, 2015-03-24 14:48:03]

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/supervis...
File
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc
(right):

https://codereview.chromium.org/1028123002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc:124:
std::string object_ref = extension_id + ":" + version.GetString();
On 2015/03/23 16:51:48, Bernhard Bauer wrote:
> On 2015/03/23 16:26:46, Marc Treib wrote:
> > On 2015/03/23 16:18:39, Bernhard Bauer wrote:
> > > BTW, is there a reason why this is part of the PermissionRequestCreator
> > > interface and therefore has to be repeated with every implementation,
> instead
> > of
> > > moving it to SupervisedUserService?
> > 
> > I'd say it's only by coincidence that both PRC implementations handle this
the
> > same way.
> 
> Really? IMO it's fair to say that what we are abstracting away behind the PRC
> interface is *how* we talk to the permission request server, not the server
> itself, so all PRC implementations can use a shared format to encode their
> requests. 

Done.

[Marc Treib, 2015-03-24 14:49:52]

treib@chromium.org changed reviewers:
+ mek@chromium.org

[Marc Treib, 2015-03-24 14:49:53]

+mek: Can I get an OWNERS review for c/b/extensions, please?

[Marijn Kruisselbrink, 2015-03-24 16:11:21]

On 2015/03/24 14:49:53, Marc Treib wrote:
> +mek: Can I get an OWNERS review for c/b/extensions, please?

[Marijn Kruisselbrink, 2015-03-24 16:11:35]

c/b/extensions LGTM

[Marc Treib, 2015-03-24 17:14:36]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-03-24 17:14:36]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1028123002/#ps80001
(title: "unit_tests")

[commit-bot: I haz the power, 2015-03-24 17:14:56]

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

[commit-bot: I haz the power, 2015-03-24 18:09:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-24 18:10:54]

Patchset 5 (id:??) landed as
https://crrev.com/f832a99186ee1be764e806793efdf400c44795ac
Cr-Commit-Position: refs/heads/master@{#322029}