Move parsing of NaCl modules out of Extension.

Move parsing of NaCl modules out of Extension.

Unify the NaClModuleInfos.

BUG=159265
TBR=mirandac@chromium.org

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

[not at google - send to devlin, 2013-05-29 16:29:21]

lg just that one comment about moving it out of extension service. it may make
more sense to do in a separate CL if at all, though.

https://codereview.chromium.org/16171011/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/16171011/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1091: bool
plugins_or_nacl_changed = false;
Without picking through the source code of all of this: is it possible/feasible
to move the nacl module stuff out of ExtensionService entirely and into its own
service that hangs of ExtensionSystem? At brief perusal it looks like it
basically listens to LOAD and UNLOAD of extensions. It would be a really nice
cleanup to move that stuff out.

https://codereview.chromium.org/16171011/diff/2001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handlers/nacl_modules_handler.cc (right):

https://codereview.chromium.org/16171011/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handlers/nacl_modules_handler.cc:33:
extension->GetManifestData(keys::kNaClModules));
I feel like GetManifestData should be called like

extension->GetManifestData<NaclModuleData>(keys::kNcalModules)

but that's entirely unrelated to this patch.

[not at google - send to devlin, 2013-05-29 16:32:09]

https://codereview.chromium.org/16171011/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/16171011/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1091: bool
plugins_or_nacl_changed = false;
On 2013/05/29 16:29:21, kalman wrote:
> Without picking through the source code of all of this: is it
possible/feasible
> to move the nacl module stuff out of ExtensionService entirely and into its
own
> service that hangs of ExtensionSystem? At brief perusal it looks like it
> basically listens to LOAD and UNLOAD of extensions. It would be a really nice
> cleanup to move that stuff out.

s/ExtensionSystem/a PKS/g

[Yoyo Zhou, 2013-05-29 22:27:47]

https://codereview.chromium.org/16171011/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/16171011/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1091: bool
plugins_or_nacl_changed = false;
On 2013/05/29 16:29:21, kalman wrote:
> Without picking through the source code of all of this: is it
possible/feasible
> to move the nacl module stuff out of ExtensionService entirely and into its
own
> service that hangs of ExtensionSystem? At brief perusal it looks like it
> basically listens to LOAD and UNLOAD of extensions. It would be a really nice
> cleanup to move that stuff out.

Yeah, it's definitely possible; it was all dumped into here 2 years ago. I moved
it all out to PluginManager. How's it look?

https://codereview.chromium.org/16171011/diff/2001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handlers/nacl_modules_handler.cc (right):

https://codereview.chromium.org/16171011/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handlers/nacl_modules_handler.cc:33:
extension->GetManifestData(keys::kNaClModules));
On 2013/05/29 16:29:21, kalman wrote:
> I feel like GetManifestData should be called like
> 
> extension->GetManifestData<NaclModuleData>(keys::kNcalModules)
> 
> but that's entirely unrelated to this patch.

Sounds reasonable. We could have exactly the same amount of type-unsafety we
have now. Not in this patch, though.

[not at google - send to devlin, 2013-05-29 23:04:06]

sweet, and got rid of #ifdefs too!

https://codereview.chromium.org/16171011/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/plugin_manager.cc (right):

https://codereview.chromium.org/16171011/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/plugin_manager.cc:125: }
it's upsetting how similar this code is to itself, but at least it's not in
ExtensionService anymore.

https://codereview.chromium.org/16171011/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/plugin_manager.h (right):

https://codereview.chromium.org/16171011/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/plugin_manager.h:48:
extensions::NaClModuleInfo::List::iterator FindNaClModule(const GURL& url);
purely from an interface perspective, returning an iterator is odd. can you
return a NaclModuleInfo* instead?

[not at google - send to devlin, 2013-05-29 23:04:09]

lgtm

[Yoyo Zhou, 2013-05-29 23:14:36]

The ifdef just got moved out to ChromeBrowserMainExtraPartsProfiles (now there's
a name for you).

https://codereview.chromium.org/16171011/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/plugin_manager.h (right):

https://codereview.chromium.org/16171011/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/plugin_manager.h:48:
extensions::NaClModuleInfo::List::iterator FindNaClModule(const GURL& url);
On 2013/05/29 23:04:06, kalman wrote:
> purely from an interface perspective, returning an iterator is odd. can you
> return a NaclModuleInfo* instead?

Not really - list::erase takes an iterator.

[commit-bot: I haz the power, 2013-05-29 23:15:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/16171011/10001

[commit-bot: I haz the power, 2013-05-29 23:26:20]

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

[Yoyo Zhou, 2013-05-29 23:28:49]

TBR=mirandac for chrome_browser_main_extra_parts_profiles

[commit-bot: I haz the power, 2013-05-29 23:29:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/16171011/10001

[commit-bot: I haz the power, 2013-05-30 01:32:49]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Miranda Callahan, 2013-05-30 11:28:52]

On 2013/05/30 01:32:49, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) content_browsertests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

LGTM for that small addition.

[commit-bot: I haz the power, 2013-05-30 15:22:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/16171011/10001

[commit-bot: I haz the power, 2013-05-30 19:42:32]

Change committed as 203199