Basic multi-module support

Basic multi-module support

Allows resource export from one extension that can
be imported into another extension's namespace.


BUG=236044
TEST=browser_tests ExtensionApiTest.SharedModule
TEST=unit_tests SharedModuleManifestTest.*

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

[elijahtaylor1, 2013-04-24 01:03:46]

There are a few things missing from this CL so far, but I would like early
feedback to make sure I'm not way off base here, so let me have it.

The things that are missing are:
1) Check on install that imported modules are valid, installed, and the correct
version
2) tests
3) potentially moving the _modules redirection code to Extension::GetResource if
I can find a good way to give knowledge of exported modules base paths to the
importing module.

[Matt Perry, 2013-04-24 21:22:11]

The code itself looks good. I think it should live outside the Extension class,
though. See below.

https://codereview.chromium.org/13971005/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/13971005/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension.cc:1620: if
(!imports_.back().min_version->IsValid()) {*/
rm this dead code

https://codereview.chromium.org/13971005/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/13971005/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension.h:552: bool
LoadSharedModuleFeatures(string16* error);
All the shared module stuff should move to its own file and ManifestHandler.
Take a look at BackgroundInfo for an example of how that's done:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

[elijahtaylor1, 2013-04-25 19:42:57]

Thanks for the feedback, I've updated to move the code like you suggested.  

I also made two other changes.

1) Made the exporting module a new "shared module" extension type to avoid
accidental privilege escalation in case we get something wrong and the shared
module somehow gets running.  

2) Added checks to crx_installer to make sure you can't install a dependent
extension if the dependencies aren't met.  I tried adding these to the unpacked
loader as well but because of timing issues between loading extensions from the
command line and being available in the extension service, I was unable to do
so.

I'm working on adding tests for this code now.

PTAL

[Matt Perry, 2013-04-25 20:16:31]

https://codereview.chromium.org/13971005/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/13971005/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:414: if
(SharedModuleInfo::ImportsModules(extension_)) {
What if you uninstall a shared module that something depends on?

https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
File chrome/common/extensions/manifest.h (right):

https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
chrome/common/extensions/manifest.h:59: TYPE_SHARED_MODULE
I'm wary of adding another extension type. What's wrong with shared modules
being a TYPE_EXTENSION with a special key? You can still use
SharedModuleInfo::IsSharedModule to do the same types of checks.

[elijahtaylor1, 2013-04-25 21:01:28]

https://codereview.chromium.org/13971005/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/13971005/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:414: if
(SharedModuleInfo::ImportsModules(extension_)) {
On 2013/04/25 20:16:31, Matt Perry wrote:
> What if you uninstall a shared module that something depends on?

In the final version we had talked about making shared modules not visible from
the normal chrome://extensions UI and thus uninstallable by the user.  The only
way a shared module would be uninstalled is when it is no longer referenced by
any other installed extension.


It's kind of dependent on the ability to auto-download/install shared modules on
demand to complete the entire seamless experience.  Let me know if you feel
strongly about leaving this dangling piece of functionality out there for now.

https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
File chrome/common/extensions/manifest.h (right):

https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
chrome/common/extensions/manifest.h:59: TYPE_SHARED_MODULE
On 2013/04/25 20:16:31, Matt Perry wrote:
> I'm wary of adding another extension type. What's wrong with shared modules
> being a TYPE_EXTENSION with a special key? You can still use
> SharedModuleInfo::IsSharedModule to do the same types of checks.

It was suggested by erikkay and asargent originally.  I think it is mostly to
avoid running any code in the context of the shared module (background
pages/scripts aren't allowed for instance), as well as avoiding granting
permissions accidentally to the shared module in case the first case is broken.

[Matt Perry, 2013-04-25 21:07:23]

lgtm

https://codereview.chromium.org/13971005/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/13971005/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:414: if
(SharedModuleInfo::ImportsModules(extension_)) {
On 2013/04/25 21:01:28, elijahtaylor1 wrote:
> On 2013/04/25 20:16:31, Matt Perry wrote:
> > What if you uninstall a shared module that something depends on?
> 
> In the final version we had talked about making shared modules not visible
from
> the normal chrome://extensions UI and thus uninstallable by the user.  The
only
> way a shared module would be uninstalled is when it is no longer referenced by
> any other installed extension.
> 
> 
> It's kind of dependent on the ability to auto-download/install shared modules
on
> demand to complete the entire seamless experience.  Let me know if you feel
> strongly about leaving this dangling piece of functionality out there for now.

Oh yeah, that's right. Makes sense. Leave as-is.

https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
File chrome/common/extensions/manifest.h (right):

https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
chrome/common/extensions/manifest.h:59: TYPE_SHARED_MODULE
On 2013/04/25 21:01:28, elijahtaylor1 wrote:
> On 2013/04/25 20:16:31, Matt Perry wrote:
> > I'm wary of adding another extension type. What's wrong with shared modules
> > being a TYPE_EXTENSION with a special key? You can still use
> > SharedModuleInfo::IsSharedModule to do the same types of checks.
> 
> It was suggested by erikkay and asargent originally.  I think it is mostly to
> avoid running any code in the context of the shared module (background
> pages/scripts aren't allowed for instance), as well as avoiding granting
> permissions accidentally to the shared module in case the first case is
broken.

Does a new Type really buy you those things, though? You still have to special
case SHARED_MODULE types and have code that recognizes them. Whether that code
checks the extension type or uses something else to test IsSharedModule seems
irrelevant.

That said, thinking more about it, it does feel like this is separate from an
extension. (An app could depend on a shared module.) I was just worried that
this would lead to fragmentation in our code depending on the package type.

[elijahtaylor1, 2013-04-25 21:26:24]

https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
File chrome/common/extensions/manifest.h (right):

https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
chrome/common/extensions/manifest.h:59: TYPE_SHARED_MODULE
On 2013/04/25 21:07:23, Matt Perry wrote:

> Does a new Type really buy you those things, though? 

I think so, right?  The keys listed in _manifest_features.json have most
features keyed to extension and apps types, very few things are keyed to "all",
and most of them seem appropriate for shared modules too.

> I was just worried that
> this would lead to fragmentation in our code depending on the package type.

Yeah, I don't necessarily like that aspect of it (as I had to fix a few switch
statements that didn't really relate just to fix compiler errors, I could see it
getting worse).

[Matt Perry, 2013-04-25 22:25:53]

On 2013/04/25 21:26:24, elijahtaylor1 wrote:
>
https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
> File chrome/common/extensions/manifest.h (right):
> 
>
https://codereview.chromium.org/13971005/diff/8001/chrome/common/extensions/m...
> chrome/common/extensions/manifest.h:59: TYPE_SHARED_MODULE
> On 2013/04/25 21:07:23, Matt Perry wrote:
> 
> > Does a new Type really buy you those things, though? 
> 
> I think so, right?  The keys listed in _manifest_features.json have most
> features keyed to extension and apps types, very few things are keyed to
"all",
> and most of them seem appropriate for shared modules too.

Oh right, I forgot that Features were keyed off package type. Cool, sounds like
this is what we want then.

> 
> > I was just worried that
> > this would lead to fragmentation in our code depending on the package type.
> 
> Yeah, I don't necessarily like that aspect of it (as I had to fix a few switch
> statements that didn't really relate just to fix compiler errors, I could see
it
> getting worse).

[asargent_no_longer_on_chrome, 2013-04-26 21:10:40]

lgtm

CL description nit: this is a big enough feature that it probably warrants
having a bug entry.

[elijahtaylor1, 2013-04-26 23:20:47]

On 2013/04/26 21:10:40, Antony Sargent wrote:
> lgtm
> 
> CL description nit: this is a big enough feature that it probably warrants
> having a bug entry.

Done, thanks.

The only substantial thing I've done since your LGTMs is add tests.  If you have
time, I'd appreciate a 'once over' of the tests to make sure I'm not doing
things way against convention.  Other than that, with clean trys, I'll try to
submit Monday.

[Matt Perry, 2013-04-26 23:43:34]

Good tests! LGTM

[asargent_no_longer_on_chrome, 2013-04-27 00:13:50]

https://codereview.chromium.org/13971005/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/shared_module_apitest.cc (right):

https://codereview.chromium.org/13971005/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/shared_module_apitest.cc:22:
EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
I think you might be able to slightly simplify this code (drop one of the Load
calls, and not need your own ResultCatcher) if you used one of the helper
methods you inherit like  ExtensionApiTest::RunExtensionTest or
ExtensionApiTest::RunPageTest.

[elijahtaylor1, 2013-04-27 00:26:09]

https://codereview.chromium.org/13971005/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/shared_module_apitest.cc (right):

https://codereview.chromium.org/13971005/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/shared_module_apitest.cc:22:
EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
Good call, Done

[commit-bot: I haz the power, 2013-04-29 16:58:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/13971005/51001

[commit-bot: I haz the power, 2013-04-29 17:07:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[elijahtaylor1, 2013-04-29 17:33:53]

@thakis: PTAL for OWNERS approval for
chrome/browser/renderer_host/chrome_render_view_host_observer.cc

[Nico, 2013-04-29 17:46:32]

lgtm

[commit-bot: I haz the power, 2013-04-29 19:09:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/13971005/69002

[commit-bot: I haz the power, 2013-04-30 01:04:38]

Change committed as 197201