Add a new webstorePrivate API to show a permission prompt for delegated installs

Add a new webstorePrivate API to show a permission prompt for delegated installs
(i.e. remote installs for another user).

Also add a new permission prompt type for this case.

Design doc (slightly outdated wrt naming): https://docs.google.com/document/d/1gXvIt0TMMGVMWlIdaLZAGB_V2CbHO24-UL5WHL5siOc/edit#heading=h.bawdzzep787b

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

[Marc Treib, 2015-02-04 12:34:25]

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

[Marc Treib, 2015-02-04 12:34:25]

Hi Benjamin, can you PTAL?
I'll add the other required OWNERS later if you think this looks okay :)

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/webstore_app/manifest.json (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
chrome/browser/resources/webstore_app/manifest.json:16:
"https://www.google.com/settings/chrome/manage",
We will need to call the new function from our dashboard (hosted on
accountcentral) as well as from the web store.

[Marc Treib, 2015-02-04 12:39:00]

[Semi-random note: I'm aware that the permissions strings aren't appropriate for
this usage right now, e.g. they say "Access your location", but should say
"Access *their* location". I'll fix that later, after the permission messages
refactoring (crbug.com/398257) is done. I'd still like to land this now, to
unblock Tereza and Todd who are working on the corresponding server-side parts.]

[not at google - send to devlin, 2015-02-04 21:28:29]

https://codereview.chromium.org/850283003/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:4252: +        Confirm Installation for <ph
name="USER_NAME">$1<ex>Wallace</ex></ph>
Is there any reason not to be consistent and say "Confirm New Extension for
..."?

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.h
(right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.h:174:
ERROR_NONE = 0,
We should just formalise this as an enum in the schema, then reference it
directly when you need to use it.

launchEphemeralApp basically did it already, you just need to pull that enum out
into a common place. hopefully defining it as a type will work.
beginInstallWithManifestVersion3 should use the same enum.

Because, amusingly, it's already out of sync :)

This will clean up a bunch of code.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.h:197: //
WebstoreInstallHelper::Delegate:
All these delegate methods can be private, right?

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.h:213: bool
RunAsync() override;
Override Run() not RunAsync().

This will also let you clean up a bunch of code.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.h:231:
scoped_refptr<extensions::Extension> dummy_extension_;
This file is already in the extensions:: namespace

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:73:
IDS_EXTENSION_DELEGATED_INSTALL_PROMPT_TITLE,
This relates to the resource - the way this code is written, and the resource is
written, the only wording you'll ever see is "this extension". You might want to
install Apps though, right? Or Themes?

The way this resource mapping is written is pretty confusing, sorry bout that :(
these should all just have a substitution of the type rather than needing to
define 2-3 versions of the same string. Ah well.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:345: resource_id,
base::UTF8ToUTF16(username_));
Why is there both username_ and delegated_username_?

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/webstore_app/manifest.json (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
chrome/browser/resources/webstore_app/manifest.json:16:
"https://www.google.com/settings/chrome/manage",
On 2015/02/04 12:34:25, Marc Treib wrote:
> We will need to call the new function from our dashboard (hosted on
> accountcentral) as well as from the web store.

So the problem is that |urls| actually gives scope to the entire domain. So this
will make the scope of the webstorePrivate API... www.google.com. That can't
happen. Can you at least make this settings.google.com?

If not - and perhaps, even if so - I need to think about this. We may need to
pull this all into another namespace (like managedInstallPrivate or something)
to avoid leaking the entire webstorePrivate API to anything other than
webstore.google.com.

[Marc Treib, 2015-02-05 10:33:37]

https://codereview.chromium.org/850283003/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:4252: +        Confirm Installation for <ph
name="USER_NAME">$1<ex>Wallace</ex></ph>
On 2015/02/04 21:28:28, kalman wrote:
> Is there any reason not to be consistent and say "Confirm New Extension for
> ..."?

Ah, I was looking at IDS_EXTENSION_INSTALL_PROMPT_TITLE (which is "Confirm
installation"), but apparently that's only used for bundle installs.
So, done.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.h
(right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.h:174:
ERROR_NONE = 0,
On 2015/02/04 21:28:29, kalman wrote:
> We should just formalise this as an enum in the schema, then reference it
> directly when you need to use it.
> 
> launchEphemeralApp basically did it already, you just need to pull that enum
out
> into a common place. hopefully defining it as a type will work.
> beginInstallWithManifestVersion3 should use the same enum.
> 
> Because, amusingly, it's already out of sync :)
> 
> This will clean up a bunch of code.

Okay, that makes sense. I'll do that in a separate cleanup CL though, and then
rebase this one on top of it.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.h:197: //
WebstoreInstallHelper::Delegate:
On 2015/02/04 21:28:29, kalman wrote:
> All these delegate methods can be private, right?

Indeed they can; I just mimicked what all the other WebStore private functions
do.
I've made mine private; for all the other functions, I'll do it in the separate
cleanup CL ^^.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.h:213: bool
RunAsync() override;
On 2015/02/04 21:28:29, kalman wrote:
> Override Run() not RunAsync().
> 
> This will also let you clean up a bunch of code.

This also applies to all the other WebstorePrivate functions, right? Oh well...
-> cleanup CL.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.h:231:
scoped_refptr<extensions::Extension> dummy_extension_;
On 2015/02/04 21:28:29, kalman wrote:
> This file is already in the extensions:: namespace

Done.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:73:
IDS_EXTENSION_DELEGATED_INSTALL_PROMPT_TITLE,
On 2015/02/04 21:28:29, kalman wrote:
> This relates to the resource - the way this code is written, and the resource
is
> written, the only wording you'll ever see is "this extension". You might want
to
> install Apps though, right? Or Themes?
> 
> The way this resource mapping is written is pretty confusing, sorry bout that
:(
> these should all just have a substitution of the type rather than needing to
> define 2-3 versions of the same string. Ah well.

Done.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:345: resource_id,
base::UTF8ToUTF16(username_));
On 2015/02/04 21:28:29, kalman wrote:
> Why is there both username_ and delegated_username_?

delegated_username_ lives in ExtensionInstallPrompt; username_ lives in
ExtensionInstallPrompt::Prompt.
There's no reason for them to have different names though; now they're both
called delegated_username_.

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/webstore_app/manifest.json (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
chrome/browser/resources/webstore_app/manifest.json:16:
"https://www.google.com/settings/chrome/manage",
On 2015/02/04 21:28:29, kalman wrote:
> On 2015/02/04 12:34:25, Marc Treib wrote:
> > We will need to call the new function from our dashboard (hosted on
> > accountcentral) as well as from the web store.
> 
> So the problem is that |urls| actually gives scope to the entire domain. So
this
> will make the scope of the webstorePrivate API... http://www.google.com. That
can't
> happen. Can you at least make this settings.google.com?

Woah, that's quite unexpected! Why can I even specify a full URL if the whole
domain will be whitelisted anyway?
Anyway: I agree allowing all of www.google.com looks like a bad idea.
Unfortunately, our dashboard doesn't have its own subdomain yet. We're planning
to move to chrome.google.com, but that's probably still a ways out. I'll
investigate and see if I can accelerate that.
But then, we wouldn't need any extra entries here, since all of
chrome.google.com is whitelisted anyway, correct?

> If not - and perhaps, even if so - I need to think about this. We may need to
> pull this all into another namespace (like managedInstallPrivate or something)
> to avoid leaking the entire webstorePrivate API to anything other than
> http://webstore.google.com.

It's actually chrome.google.com, but afaik only the webstore is hosted there so
far.

[not at google - send to devlin, 2015-02-05 17:34:20]

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/webstore_app/manifest.json (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
chrome/browser/resources/webstore_app/manifest.json:16:
"https://www.google.com/settings/chrome/manage",
On 2015/02/05 10:33:37, Marc Treib wrote:
> On 2015/02/04 21:28:29, kalman wrote:
> > On 2015/02/04 12:34:25, Marc Treib wrote:
> > > We will need to call the new function from our dashboard (hosted on
> > > accountcentral) as well as from the web store.
> > 
> > So the problem is that |urls| actually gives scope to the entire domain. So
> this
> > will make the scope of the webstorePrivate API... http://www.google.com.
That
> can't
> > happen. Can you at least make this settings.google.com?
> 
> Woah, that's quite unexpected! Why can I even specify a full URL if the whole
> domain will be whitelisted anyway?
> Anyway: I agree allowing all of http://www.google.com looks like a bad idea.
> Unfortunately, our dashboard doesn't have its own subdomain yet. We're
planning
> to move to http://chrome.google.com, but that's probably still a ways out.
I'll
> investigate and see if I can accelerate that.
> But then, we wouldn't need any extra entries here, since all of
> http://chrome.google.com is whitelisted anyway, correct?

That's a good question. One difference is that the UI behaves differently. If
you were to go to www.google.com then www.google.com would have these
bindings[*], but the search app's icon in the taskbar wouldn't be lit up. It's
only lit up on www.google.com. The webstore's icon would only be lit up on
www.google.com/settings/chrome/family.

Which reminds me - I don't think that the scope of the webstore app should
include anything other than chrome.google.com/webstore. There are places in the
code where the webstore domain is treated specially, for example, extensions
aren't allow to run content scripts on webstore.google.com. You'd need to apply
the same logic to here as well.

Which is why - despite the refactoring you did - I think this does need to be in
its own API, one that is carefully controlled. Even that... not fantastic.

Best situation IMO would be to continue to have the installation served off
chrome.google.com/webstore, but set up some sneaky redirects so that
installation can be triggered from the family console? Is that possible?

> 
> > If not - and perhaps, even if so - I need to think about this. We may need
to
> > pull this all into another namespace (like managedInstallPrivate or
something)
> > to avoid leaking the entire webstorePrivate API to anything other than
> > http://webstore.google.com.
> 
> It's actually http://chrome.google.com, but afaik only the webstore is hosted
there so
> far.

Yeah, you wouldn't need any additional rules here if you were to host on
chrome.google.com.

[Marc Treib, 2015-02-06 13:58:17]

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/webstore_app/manifest.json (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
chrome/browser/resources/webstore_app/manifest.json:16:
"https://www.google.com/settings/chrome/manage",
On 2015/02/05 17:34:20, kalman wrote:
> On 2015/02/05 10:33:37, Marc Treib wrote:
> > On 2015/02/04 21:28:29, kalman wrote:
> > > On 2015/02/04 12:34:25, Marc Treib wrote:
> > > > We will need to call the new function from our dashboard (hosted on
> > > > accountcentral) as well as from the web store.
> > > 
> > > So the problem is that |urls| actually gives scope to the entire domain.
So
> > this
> > > will make the scope of the webstorePrivate API... http://www.google.com.
> That
> > can't
> > > happen. Can you at least make this settings.google.com?
> > 
> > Woah, that's quite unexpected! Why can I even specify a full URL if the
whole
> > domain will be whitelisted anyway?
> > Anyway: I agree allowing all of http://www.google.com looks like a bad idea.
> > Unfortunately, our dashboard doesn't have its own subdomain yet. We're
> planning
> > to move to http://chrome.google.com, but that's probably still a ways out.
> I'll
> > investigate and see if I can accelerate that.
> > But then, we wouldn't need any extra entries here, since all of
> > http://chrome.google.com is whitelisted anyway, correct?
> 
> That's a good question. One difference is that the UI behaves differently. If
> you were to go to http://www.google.com then http://www.google.com would have
these
> bindings[*], but the search app's icon in the taskbar wouldn't be lit up. It's
> only lit up on http://www.google.com. The webstore's icon would only be lit up
on
> http://www.google.com/settings/chrome/family.

I see, thanks. Still somewhat surprising that the whole host will be
whitelisted, but oh well.
I've removed the extra URLs again. We hope to be moving our dashboard to
chrome.google.com by the end of the quarter, which will make all this moot. For
testing before that, we can easily work around the restriction locally.
[I'll upload a new patchset after the refactoring/cleanup CLs.]

> Which reminds me - I don't think that the scope of the webstore app should
> include anything other than chrome.google.com/webstore. There are places in
the
> code where the webstore domain is treated specially, for example, extensions
> aren't allow to run content scripts on http://webstore.google.com. You'd need
to apply
> the same logic to here as well.

Well, the scope of the webstore app already is all of chrome.google.com :)

> Which is why - despite the refactoring you did - I think this does need to be
in
> its own API, one that is carefully controlled. Even that... not fantastic.
> 
> Best situation IMO would be to continue to have the installation served off
> chrome.google.com/webstore, but set up some sneaky redirects so that
> installation can be triggered from the family console? Is that possible?

Note that the installation itself is not triggered by this API at all; all it
does is show a permission prompt. The actual installation is handled
server-side.

> > 
> > > If not - and perhaps, even if so - I need to think about this. We may need
> to
> > > pull this all into another namespace (like managedInstallPrivate or
> something)
> > > to avoid leaking the entire webstorePrivate API to anything other than
> > > http://webstore.google.com.
> > 
> > It's actually http://chrome.google.com, but afaik only the webstore is
hosted
> there so
> > far.
> 
> Yeah, you wouldn't need any additional rules here if you were to host on
> http://chrome.google.com.

[not at google - send to devlin, 2015-02-06 18:25:36]

I'll wait until this is rebased on the other CL to review again.

[not at google - send to devlin, 2015-02-06 18:27:55]

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/webstore_app/manifest.json (right):

https://codereview.chromium.org/850283003/diff/60001/chrome/browser/resources...
chrome/browser/resources/webstore_app/manifest.json:16:
"https://www.google.com/settings/chrome/manage",
> I see, thanks. Still somewhat surprising that the whole host will be
> whitelisted, but oh well.

I agree. There are reasons, which are strictly correct, but IMO impractical for
a couple of reasons.

> I've removed the extra URLs again. We hope to be moving our dashboard to
> http://chrome.google.com by the end of the quarter, which will make all this
moot. For

SGTM.

> 
> Note that the installation itself is not triggered by this API at all; all it
> does is show a permission prompt. The actual installation is handled
> server-side.

Oh, right - good point. Definitely makes this less of an issue.

[Marc Treib, 2015-02-24 11:52:06]

On 2015/02/06 18:25:36, kalman wrote:
> I'll wait until this is rebased on the other CL to review again.

This CL is now updated/rebased on top of the cleanup. PTAL again, thanks!

[not at google - send to devlin, 2015-02-24 18:30:01]

https://codereview.chromium.org/850283003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/850283003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:530:
WebstorePrivateShowPermissionPromptForDelegatedInstallFunction::Run() {
Is there a nice way to factor this that shares the bulk of its code with
WebstorePrivateBeginInstallWithManifest3Function? They're very similar.

For example, they could extend a common abstract function implementation
WebPrivateWithPermissionPromptFunction. Better would compose something but that
might be more effort than it's worth.

https://codereview.chromium.org/850283003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:546:
std::string tmp_url;
Unused? (also unused above)

[Marc Treib, 2015-02-25 13:38:47]

https://codereview.chromium.org/850283003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/850283003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:530:
WebstorePrivateShowPermissionPromptForDelegatedInstallFunction::Run() {
On 2015/02/24 18:30:00, kalman wrote:
> Is there a nice way to factor this that shares the bulk of its code with
> WebstorePrivateBeginInstallWithManifest3Function? They're very similar.
> 
> For example, they could extend a common abstract function implementation
> WebPrivateWithPermissionPromptFunction. Better would compose something but
that
> might be more effort than it's worth.

Done (not sure on the "nice" part, though).
I've extracted a common base class. It's a bit ugly that it needs to be a
template (because the Params types are different for the different subclasses).
I guess the alternative would be to introduce a new Params struct for the base
class, which would be populated by the subclasses, but I'm not convinced that's
any prettier - WDYT?

https://codereview.chromium.org/850283003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:546:
std::string tmp_url;
On 2015/02/24 18:30:00, kalman wrote:
> Unused? (also unused above)

Done, thanks! (copy&paste ftl)

[not at google - send to devlin, 2015-02-25 17:23:00]

Sorry for my lack of understanding, but I think... basically done here, though
it would be nice to test this. You should be able to adapt an
beginInstallWithManifest3 test?

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:258: if
(RunImpl(&result, &error))
RunImpl isn't a very helpful name. How about a method which returns a
scoped_ptr<ResponseValue>, which if non-empty indicates responding now? Call
it... RunExtra maybe? Something better?

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:341: void
WebstorePrivateFunctionWithPermissionPrompt<Params>::InstallUIProceed() {
It would be nice to avoid needing these extra InstallUIProceed methods - and the
behavior here is confusing for me, why is this calling into the parent? It's an
override.

If necessary provide some other override, even a method like, err,
DeferInstallUIResponse which only returns true for the manifestV3 function. If
this makes sense. Some comments might help me understand it better.

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:593:
ShowPermissionPromptForDelegatedInstall::Results::Create(result));
For better or worse, I think showPermissionPromptForDelegateInstall should
behave the same as beginInstallWithManifestVersion3 - empty string should imply
success. This would have the happy sideeffect of being able to make
BuildResponse non-virtual.

[not at google - send to devlin, 2015-02-25 17:24:41]

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:258: if
(RunImpl(&result, &error))
On 2015/02/25 17:23:00, kalman wrote:
> RunImpl isn't a very helpful name. How about a method which returns a
> scoped_ptr<ResponseValue>, which if non-empty indicates responding now? Call
> it... RunExtra maybe? Something better?

RunExtraForResponse? RunHookForResponse?

[Marc Treib, 2015-02-26 11:49:58]

One more try :)

I thought about adding tests, but all the failure cases (invalid manifest etc)
are already covered by the existing beginInstall tests since the code is now
shared, and the success case doesn't do anything - the dialog would be
auto-confirmed/canceled in the test. So there isn't really anything to test
here.

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:258: if
(RunImpl(&result, &error))
On 2015/02/25 17:23:00, kalman wrote:
> RunImpl isn't a very helpful name. How about a method which returns a
> scoped_ptr<ResponseValue>, which if non-empty indicates responding now? Call
> it... RunExtra maybe? Something better?

Done.

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:341: void
WebstorePrivateFunctionWithPermissionPrompt<Params>::InstallUIProceed() {
On 2015/02/25 17:23:00, kalman wrote:
> It would be nice to avoid needing these extra InstallUIProceed methods - and
the
> behavior here is confusing for me, why is this calling into the parent? It's
an
> override.

I'm not sure I understand the question. These are the common implementations,
and they're calling into the subclass. That's necessary because
BeginInstallWithManifest needs custom logic here.

Maybe the issue was just the confusing naming?
InstallUIProceed (overridden from ExtensionInstallPrompt::Delegate)
vs.
OnInstallUIProceed (my custom hook defined in the common base class, and
overridden by subclasses)

I've renamed the latter to InstallUIProceedHook, I hope that makes things
clearer. I've also added a comment in the header.

> If necessary provide some other override, even a method like, err,
> DeferInstallUIResponse which only returns true for the manifestV3 function. If
> this makes sense. Some comments might help me understand it better.

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:593:
ShowPermissionPromptForDelegatedInstall::Results::Create(result));
On 2015/02/25 17:23:00, kalman wrote:
> For better or worse, I think showPermissionPromptForDelegateInstall should
> behave the same as beginInstallWithManifestVersion3 - empty string should
imply
> success. This would have the happy sideeffect of being able to make
> BuildResponse non-virtual.

Hrm... alright, consistency over correctness, I guess. Done.
I still need an extra virtual function though, to create the Results ListValue
:-/

[not at google - send to devlin, 2015-02-26 17:25:08]

lgtm

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/850283003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:593:
ShowPermissionPromptForDelegatedInstall::Results::Create(result));
On 2015/02/26 11:49:58, Marc Treib wrote:
> On 2015/02/25 17:23:00, kalman wrote:
> > For better or worse, I think showPermissionPromptForDelegateInstall should
> > behave the same as beginInstallWithManifestVersion3 - empty string should
> imply
> > success. This would have the happy sideeffect of being able to make
> > BuildResponse non-virtual.
> 
> Hrm... alright, consistency over correctness, I guess. Done.
> I still need an extra virtual function though, to create the Results ListValue
> :-/

See last comment I left.

https://codereview.chromium.org/850283003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/850283003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:364:
InstallUIAbortHook(user_initiated);
Yep, this is what I meant. Thanks.

https://codereview.chromium.org/850283003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:452:
WebstorePrivateBeginInstallWithManifest3Function::CreateResults(
You can share the implementation here by templatising over the function, not its
params.

E.g. if in the header file you have:

template<typename Function>
WebstorePrivateFunctionWithPermissionPrompt

rather than:

template<typename Params>
WebstorePrivateFunctionWithPermissionPrompt

and then everywhere you have, for example:

const Function::Params& params() const { return *params_; }

rather than:

const Params& params() const { return *params_; }

Then you'd be able to just automatically implement CreateResults:

scoped_ptr<base::ListValue> CreateResults(
    api::webstore_private::Result result) const {
  return Function::Result::Create(result);
}

[Marc Treib, 2015-02-27 09:33:19]

https://codereview.chromium.org/850283003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/850283003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:452:
WebstorePrivateBeginInstallWithManifest3Function::CreateResults(
On 2015/02/26 17:25:08, kalman wrote:
> You can share the implementation here by templatising over the function, not
its
> params.

Unfortunatly I can't (unless I'm missing something).

> E.g. if in the header file you have:
> 
> template<typename Function>
> WebstorePrivateFunctionWithPermissionPrompt
> 
> rather than:
> 
> template<typename Params>
> WebstorePrivateFunctionWithPermissionPrompt
> 
> and then everywhere you have, for example:
> 
> const Function::Params& params() const { return *params_; }
> 
> rather than:
> 
> const Params& params() const { return *params_; }

Params isn't defined in the Function class; it comes from the generated code. To
make this work, I'd still need to do something like
using Params = api::webstore_private::BeginInstallWithManifest3::Params;
in each child class. But this still doesn't help with Results, since that's a
namespace, not a type. So I don't see a way around the separate CreateResults
implementations. :-/
Please tell me I'm missing something!

> Then you'd be able to just automatically implement CreateResults:
> 
> scoped_ptr<base::ListValue> CreateResults(
>     api::webstore_private::Result result) const {
>   return Function::Result::Create(result);
> }

[Marc Treib, 2015-02-27 16:20:04]

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

[Marc Treib, 2015-02-27 16:20:04]

+mpearson: Can I get an OWNERS review for histograms, please? Thanks!

[not at google - send to devlin, 2015-02-27 16:24:03]

https://codereview.chromium.org/850283003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/850283003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:452:
WebstorePrivateBeginInstallWithManifest3Function::CreateResults(
On 2015/02/27 09:33:19, Marc Treib wrote:
> On 2015/02/26 17:25:08, kalman wrote:
> > You can share the implementation here by templatising over the function, not
> its
> > params.
> 
> Unfortunatly I can't (unless I'm missing something).
> 
> > E.g. if in the header file you have:
> > 
> > template<typename Function>
> > WebstorePrivateFunctionWithPermissionPrompt
> > 
> > rather than:
> > 
> > template<typename Params>
> > WebstorePrivateFunctionWithPermissionPrompt
> > 
> > and then everywhere you have, for example:
> > 
> > const Function::Params& params() const { return *params_; }
> > 
> > rather than:
> > 
> > const Params& params() const { return *params_; }
> 
> Params isn't defined in the Function class; it comes from the generated code.
To
> make this work, I'd still need to do something like
> using Params = api::webstore_private::BeginInstallWithManifest3::Params;
> in each child class. But this still doesn't help with Results, since that's a
> namespace, not a type. So I don't see a way around the separate CreateResults
> implementations. :-/
> Please tell me I'm missing something!
> 
> > Then you'd be able to just automatically implement CreateResults:
> > 
> > scoped_ptr<base::ListValue> CreateResults(
> >     api::webstore_private::Result result) const {
> >   return Function::Result::Create(result);
> > }
> 

Ah, bummer, these are namespaces. Ugh well that was an unfortunate decision, I
presume you can't have templates of namespaces. Ok.

[Mark P, 2015-02-27 16:52:18]

histograms.xml lgtm

[Marc Treib, 2015-02-28 17:18:58]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-02-28 17:18:59]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/850283003/#ps200001
(title: "histograms")

[commit-bot: I haz the power, 2015-02-28 17:19:25]

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

[commit-bot: I haz the power, 2015-02-28 17:39:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-28 17:39:34]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2015-03-01 11:26:18]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-03-01 11:27:26]

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

[commit-bot: I haz the power, 2015-03-01 12:21:01]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-01 12:21:53]

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