Add an installType property to the management API

Add an installType property to the management API

Mapping from extension.location():

INTERNAL -> 'normal'
LOAD -> 'development'
EXTERNAL_POLICY_DOWNLOAD -> 'admin'
EXTERNAL* -> 'sideload'
(all others) -> 'other'

BUG=135564
TBR=jam@chromium.org

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

[Aaron Boodman, 2012-07-10 22:19:14]

Seems good to me. Antony should do this review.

http://codereview.chromium.org/10750010/diff/1/chrome/common/extensions/api/m...
File chrome/common/extensions/api/management.json (right):

http://codereview.chromium.org/10750010/diff/1/chrome/common/extensions/api/m...
chrome/common/extensions/api/management.json:103: "description": "How the
extension was installed (\"normal\", \"development\", \"sideload\"). \"normal\"
means the extension was installed normally via a crx file. \"development\" means
the extension was loaded unpacked in developer mode. \"sideload\" means the
extension was installed externally.",
"sideload" means the extension was installed by other software on the machine.

[mitchellwrosen, 2012-07-11 18:41:23]

Ping, because I'm not sure that Antony Sargent got an email about this one.
Thanks Aaron

[asargent_no_longer_on_chrome, 2012-07-11 20:49:55]

Can you add tests of the other install types besides just development?

Also, do we want to try to draw a distinction between external extensions
installed because 3rd party software (eg antivirus programs) requested it, and
ones installed because of enterprise policy rules? To me the name "sideloaded"
has a connotation that doesn't quite fit with "because my IT guys configured it
to be installed".

[mitchellwrosen, 2012-07-23 18:26:25]

On 2012/07/11 20:49:55, Antony Sargent wrote:
> Can you add tests of the other install types besides just development?
> 
> Also, do we want to try to draw a distinction between external extensions
> installed because 3rd party software (eg antivirus programs) requested it, and
> ones installed because of enterprise policy rules? To me the name "sideloaded"
> has a connotation that doesn't quite fit with "because my IT guys configured
it
> to be installed".

'normal' and 'development' are simple enough, but I'm having trouble writing
clean, concise code that installs an extension via an external_extensions file
(to confirm that installType == 'sideload').

I've been looking at extension_service_unittest:1266 to get an idea of how it
may done, but it looks like there's a lot of infrastructure (initialize
extension service, etc.).

Sorry it's taken me so long to get back to you, been working on other things =)

[Aaron Boodman, 2012-07-23 23:47:09]

On 2012/07/11 20:49:55, Antony Sargent wrote:
> Also, do we want to try to draw a distinction between external extensions
> installed because 3rd party software (eg antivirus programs) requested it, and
> ones installed because of enterprise policy rules? To me the name "sideloaded"
> has a connotation that doesn't quite fit with "because my IT guys configured
it
> to be installed".

Seems sensible. So:

external_policy_download -> admin
external_* -> sideload

?

[mitchellwrosen, 2012-07-31 18:05:41]

Sorry this took so long. A combination of working on a million things at once,
and trying to come up with a simple way to install an external extension without
a bunch of extension service infrastructure. I decided that just directly
modifying the manifest would be appropriate, but unfortunately requires a
FRIEND_TEST. Let me know if this isn't ideal!

http://codereview.chromium.org/10750010/diff/1/chrome/common/extensions/api/m...
File chrome/common/extensions/api/management.json (right):

http://codereview.chromium.org/10750010/diff/1/chrome/common/extensions/api/m...
chrome/common/extensions/api/management.json:103: "description": "How the
extension was installed (\"normal\", \"development\", \"sideload\"). \"normal\"
means the extension was installed normally via a crx file. \"development\" means
the extension was loaded unpacked in developer mode. \"sideload\" means the
extension was installed externally.",
On 2012/07/10 22:19:14, Aaron Boodman wrote:
> "sideload" means the extension was installed by other software on the machine.

Done.

[asargent_no_longer_on_chrome, 2012-07-31 19:55:52]

LGTM assuming you fix the 2 issues mentioned in comments

http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/management/management_apitest.cc (right):

http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/management/management_apitest.cc:82:
//ASSERT_TRUE(extension);
nit: you should leave this check in, but you can change it to:

if (!extension) {
  ADD_FAILURE("extension is null");
  return NULL;
}

http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/management/management_apitest.cc:97:
manifest->location_ = Extension::INTERNAL;
Instead of removing the const and relying on friending to be able to modify this
private data member, can you instead just do this?

const base::DictionaryValue* source = extension->manifest()->value();
scoped_ptr<Manifest> manifest(
    new Manifest(Extension::INTERNAL, source->DeepCopy());

...

manifest.reset(
  new Manifest(Extension::EXTERNAL_PREF, source->DeepCopy());


etc.

[mitchellwrosen, 2012-07-31 21:48:48]

http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/management/management_apitest.cc (right):

http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/management/management_apitest.cc:82:
//ASSERT_TRUE(extension);
On 2012/07/31 19:55:53, Antony Sargent wrote:
> nit: you should leave this check in, but you can change it to:
> 
> if (!extension) {
>   ADD_FAILURE("extension is null");
>   return NULL;
> }

Sorry, forgot to un-comment this line. I was getting a weird compiler error that
I meant to investigate: void value ought to be ignored.

http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/management/management_apitest.cc:97:
manifest->location_ = Extension::INTERNAL;
On 2012/07/31 19:55:53, Antony Sargent wrote:
> Instead of removing the const and relying on friending to be able to modify
this
> private data member, can you instead just do this?
> 
> const base::DictionaryValue* source = extension->manifest()->value();
> scoped_ptr<Manifest> manifest(
>     new Manifest(Extension::INTERNAL, source->DeepCopy());
> 
> ...
> 
> manifest.reset(
>   new Manifest(Extension::EXTERNAL_PREF, source->DeepCopy());
> 
> 
> etc.

Sure, but I'm not seeing any way to modify the extension's manifest with this
scoped ptr. Could you elaborate?

[asargent_no_longer_on_chrome, 2012-07-31 22:33:00]

http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/management/management_apitest.cc (right):

http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/management/management_apitest.cc:82:
//ASSERT_TRUE(extension);
On 2012/07/31 21:48:48, mitchellwrosen wrote:
> On 2012/07/31 19:55:53, Antony Sargent wrote:
> > nit: you should leave this check in, but you can change it to:
> > 
> > if (!extension) {
> >   ADD_FAILURE("extension is null");
> >   return NULL;
> > }
> 
> Sorry, forgot to un-comment this line. I was getting a weird compiler error
that
> I meant to investigate: void value ought to be ignored.

yeah, ASSERT_TRUE(expr) compiles to something like:

{ if (expr) { ADD_FAILURE(); return; } }

so it's a common problem that when you are using it in a helper function which
returns void, but change to start returning something else, that you get the
compiler warning you about trying to return without a value of the appropriate
type.

http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/management/management_apitest.cc:97:
manifest->location_ = Extension::INTERNAL;
> Sure, but I'm not seeing any way to modify the extension's manifest with this
> scoped ptr. Could you elaborate?

Sorry, I didn't think this all the way through. I guess you need a couple
different extensions installed with various locations before we jump over to
finish the test by running the js test code in the extension's renderer process.


I think the right way to do this is to add the ability to specify an
Extension::Location to ExtensionBrowserTest::LoadExtensionWithOptions, and then
have a few different test extensions that you load with the various location
values we want to test.

[mitchellwrosen, 2012-07-31 22:34:51]

On 2012/07/31 22:33:00, Antony Sargent wrote:
>
http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/management/management_apitest.cc (right):
> 
>
http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
> chrome/browser/extensions/api/management/management_apitest.cc:82:
> //ASSERT_TRUE(extension);
> On 2012/07/31 21:48:48, mitchellwrosen wrote:
> > On 2012/07/31 19:55:53, Antony Sargent wrote:
> > > nit: you should leave this check in, but you can change it to:
> > > 
> > > if (!extension) {
> > >   ADD_FAILURE("extension is null");
> > >   return NULL;
> > > }
> > 
> > Sorry, forgot to un-comment this line. I was getting a weird compiler error
> that
> > I meant to investigate: void value ought to be ignored.
> 
> yeah, ASSERT_TRUE(expr) compiles to something like:
> 
> { if (expr) { ADD_FAILURE(); return; } }
> 
> so it's a common problem that when you are using it in a helper function which
> returns void, but change to start returning something else, that you get the
> compiler warning you about trying to return without a value of the appropriate
> type.
> 
>
http://codereview.chromium.org/10750010/diff/7001/chrome/browser/extensions/a...
> chrome/browser/extensions/api/management/management_apitest.cc:97:
> manifest->location_ = Extension::INTERNAL;
> > Sure, but I'm not seeing any way to modify the extension's manifest with
this
> > scoped ptr. Could you elaborate?
> 
> Sorry, I didn't think this all the way through. I guess you need a couple
> different extensions installed with various locations before we jump over to
> finish the test by running the js test code in the extension's renderer
process.
> 
> 
> I think the right way to do this is to add the ability to specify an
> Extension::Location to ExtensionBrowserTest::LoadExtensionWithOptions, and
then
> have a few different test extensions that you load with the various location
> values we want to test.

Yes sir.

[mitchellwrosen, 2012-08-01 18:05:44]

> > I think the right way to do this is to add the ability to specify an
> > Extension::Location to ExtensionBrowserTest::LoadExtensionWithOptions, and
> then
> > have a few different test extensions that you load with the various location
> > values we want to test.

Minor hiccup: the DCHECK on line 374 of extension_prefs.cc is giving me trouble.
Only unpacked extensions are allowed to have absolute paths. I'm simply mocking
the install location a la LoadExtensionWithOptions("path/to/extension",
Extension::INTERNAL);

What are your thoughts on this?

[asargent_no_longer_on_chrome, 2012-08-01 21:41:31]

Hmm, interesting conundrum. Would it work better if you instead added the
ability to specify an Extension::Location to
ExtensionBrowserTest::InstallExtension (or added a new method alongside it)?

[asargent_no_longer_on_chrome, 2012-08-02 21:28:07]

LGTM, and thanks for taking the time to find the right approach. Sorry it was a
bit of a hassle!

https://chromiumcodereview.appspot.com/10750010/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/extension_browsertest.h (right):

https://chromiumcodereview.appspot.com/10750010/diff/13002/chrome/browser/ext...
chrome/browser/extensions/extension_browsertest.h:70: // specified. Intended to
be used only in tests.
nit: You probably don't need to mention that it's only to be used in tests,
since this whole class is only compiled into the browser_tests binary. =)

[mitchellwrosen, 2012-08-02 21:31:04]

https://chromiumcodereview.appspot.com/10750010/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/extension_browsertest.h (right):

https://chromiumcodereview.appspot.com/10750010/diff/13002/chrome/browser/ext...
chrome/browser/extensions/extension_browsertest.h:70: // specified. Intended to
be used only in tests.
On 2012/08/02 21:28:07, Antony Sargent wrote:
> nit: You probably don't need to mention that it's only to be used in tests,
> since this whole class is only compiled into the browser_tests binary. =)

Sure thing.

[commit-bot: I haz the power, 2012-08-02 21:31:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10750010/1...

[commit-bot: I haz the power, 2012-08-02 21:31:35]

Failed to apply patch for
chrome/browser/extensions/extension_function_registry.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_function_registry.cc
Hunk #1 succeeded at 22 with fuzz 2 (offset 3 lines).
Hunk #2 FAILED at 39.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_registry.cc.rej

[commit-bot: I haz the power, 2012-08-02 22:12:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10750010/1...

[commit-bot: I haz the power, 2012-08-02 22:45:35]

Try job failure for 10750010-11038 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-02 23:27:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10750010/1...

[commit-bot: I haz the power, 2012-08-03 01:13:59]

Change committed as 149767