Add NaCl support to app_shell

Add NaCl support to app_shell

Perform NaCl initialization during app_shell's startup path similar to
how Chrome initializes it.
* Refactor some of the lazy background page impulse code into ProcessManager so it can be shared with Chrome

BUG=400577
TEST=manual tests of app_shell, existing ProcessManager unit tests and browser tests

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

[James Cook, 2014-08-11 21:51:48]

teravest@, PTAL

Let me know what I should refactor/split into other changes. Thanks!

[teravest, 2014-08-12 15:48:32]

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/app_sh...
File extensions/shell/app_shell.gyp (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/app_sh...
extensions/shell/app_shell.gyp:127: 'browser/shell_nacl_browser_delegate.cc',
It looks like these were accidentally pasted twice.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:168: void
ShellBrowserMainParts::InitExtensionsModule() {
Why does this code have to be moved into InitExtensionsModule() as part of this
change?

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_pepper_host_factory.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_browser_pepper_host_factory.cc:53: // app_shell
always allows access to private APIs.
I don't understand this comment.

PpapiHostMsg_Broker_IsAllowed is sent to determine if a plugin can connect to
the broker to perform some operations. 

The Pepper Broker is for supporting unsandboxed plugins (like flash)-- I don't
think you should need it to support NaCl plugins, and you should consider
splitting any Broker stuff out to another change if you need it.

If you don't have to deal with the pepper broker, I don't think you need this
message filter at all.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_content_browser_client.cc:201: //
TODO(jamescook): Does this need a PepperHostFactory filter?
I don't think you need to do anything for DidCreatePpapiPlugin(); it's only used
in chrome/ to add support for some ResourceHosts not already provided in
content/.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_content_browser_client.cc:238: //
TODO(jamescook): What about kAllowNaClFileHandleAPI?
You'd only need kAllowNaClFileHandleAPI for RequestOSFileHandle, I think, which
is used by ARC, but I can't think of anything else.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_nacl_browser_delegate.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_nacl_browser_delegate.cc:90: // On Posix plugins
are in the module directory.
nit: It's more readable to have a comma: "On posix, ..."

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_nacl_browser_delegate.cc:125: //JAMES - this does
not appear necessary. Why not?
I don't think this is necessary (since you don't really need what's provided in
ShellBrowserPepperHostFactory anyway). This is a place to allow chrome to
register additional hosts, but I don't think that's necessary for app_shell.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_nacl_browser_delegate.cc:187: bool
ShellNaClBrowserDelegate::IsNonSfiModeAllowed(
Are you intending to support non-SFI mode in app_shell? I saw elsewhere that the
flag was being passed through.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/common...
File extensions/shell/common/shell_content_client.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/common...
extensions/shell/common/shell_content_client.cc:28: // module. Perhaps they
should live in components/nacl.
components/nacl is probably a better place for this stuff.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/render...
File extensions/shell/renderer/shell_content_renderer_client.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/render...
extensions/shell/renderer/shell_content_renderer_client.cc:106: // It doesn't
seem necessary for either Pepper or NaCl.
Hmm.

It looks like PepperHelper sets up PepperSharedMemoryMessageFilter, which allows
for shared memory to be created from the plugin for faster ArrayBuffer
transmission through the messaging API.

I don't see why that should have to be in chrome/ (and set up through
PepperHelper) instead of content/, though.

I'll file a bug and CC you to get that moved.

Otherwise, I don't think you can use PepperHelper here anyway since it's in
chrome/

[James Cook, 2014-08-12 18:17:06]

teravest, please take another look

I upload the move of the constants in a separate step if you want to see just
that part of the change.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/app_sh...
File extensions/shell/app_shell.gyp (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/app_sh...
extensions/shell/app_shell.gyp:127: 'browser/shell_nacl_browser_delegate.cc',
On 2014/08/12 15:48:31, teravest wrote:
> It looks like these were accidentally pasted twice.

Yep. Fixed.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:168: void
ShellBrowserMainParts::InitExtensionsModule() {
On 2014/08/12 15:48:31, teravest wrote:
> Why does this code have to be moved into InitExtensionsModule() as part of
this
> change?

It doesn't have to be. I thought PreMainMessageLoopRun() was getting a little
too long, but I can clean that up in another CL.  Moved it back.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_pepper_host_factory.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_browser_pepper_host_factory.cc:53: // app_shell
always allows access to private APIs.
On 2014/08/12 15:48:31, teravest wrote:
> I don't understand this comment.
> 
> PpapiHostMsg_Broker_IsAllowed is sent to determine if a plugin can connect to
> the broker to perform some operations. 
> 
> The Pepper Broker is for supporting unsandboxed plugins (like flash)-- I don't
> think you should need it to support NaCl plugins, and you should consider
> splitting any Broker stuff out to another change if you need it.
> 
> If you don't have to deal with the pepper broker, I don't think you need this
> message filter at all.

Yeah, I don't need this at all.  Removed.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_content_browser_client.cc:201: //
TODO(jamescook): Does this need a PepperHostFactory filter?
On 2014/08/12 15:48:31, teravest wrote:
> I don't think you need to do anything for DidCreatePpapiPlugin(); it's only
used
> in chrome/ to add support for some ResourceHosts not already provided in
> content/.

Removed.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_content_browser_client.cc:238: //
TODO(jamescook): What about kAllowNaClFileHandleAPI?
On 2014/08/12 15:48:31, teravest wrote:
> You'd only need kAllowNaClFileHandleAPI for RequestOSFileHandle, I think,
which
> is used by ARC, but I can't think of anything else.

Removed.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_nacl_browser_delegate.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_nacl_browser_delegate.cc:90: // On Posix plugins
are in the module directory.
On 2014/08/12 15:48:31, teravest wrote:
> nit: It's more readable to have a comma: "On posix, ..."

Done.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_nacl_browser_delegate.cc:125: //JAMES - this does
not appear necessary. Why not?
On 2014/08/12 15:48:31, teravest wrote:
> I don't think this is necessary (since you don't really need what's provided
in
> ShellBrowserPepperHostFactory anyway). This is a place to allow chrome to
> register additional hosts, but I don't think that's necessary for app_shell.

Switched to return NULL.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_nacl_browser_delegate.cc:187: bool
ShellNaClBrowserDelegate::IsNonSfiModeAllowed(
On 2014/08/12 15:48:31, teravest wrote:
> Are you intending to support non-SFI mode in app_shell? I saw elsewhere that
the
> flag was being passed through.

No, I'm not intending to support non-SFI mode, at least for the time being. 
Removed the other flags.

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/common...
File extensions/shell/common/shell_content_client.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/common...
extensions/shell/common/shell_content_client.cc:28: // module. Perhaps they
should live in components/nacl.
On 2014/08/12 15:48:32, teravest wrote:
> components/nacl is probably a better place for this stuff.

Moved into components/nacl/common and updated all the src/chrome references to
these constants (and copies of the same data).

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/render...
File extensions/shell/renderer/shell_content_renderer_client.cc (right):

https://codereview.chromium.org/437503004/diff/140001/extensions/shell/render...
extensions/shell/renderer/shell_content_renderer_client.cc:106: // It doesn't
seem necessary for either Pepper or NaCl.
On 2014/08/12 15:48:32, teravest wrote:
> Hmm.
> 
> It looks like PepperHelper sets up PepperSharedMemoryMessageFilter, which
allows
> for shared memory to be created from the plugin for faster ArrayBuffer
> transmission through the messaging API.
> 
> I don't see why that should have to be in chrome/ (and set up through
> PepperHelper) instead of content/, though.
> 
> I'll file a bug and CC you to get that moved.
> 
> Otherwise, I don't think you can use PepperHelper here anyway since it's in
> chrome/

Yep, can't use it here. I'll leave the comment and wait for the message filter
to move, then look at it again.

https://codereview.chromium.org/437503004/diff/200001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/437503004/diff/200001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:188: nacl.permissions =
ppapi::PERMISSION_PRIVATE | ppapi::PERMISSION_DEV;
I inlined this to avoid a src/components/nacl/common -> src/ppapi dependency.

https://codereview.chromium.org/437503004/diff/200001/chrome/common/chrome_co...
File chrome/common/chrome_content_client_constants.cc (left):

https://codereview.chromium.org/437503004/diff/200001/chrome/common/chrome_co...
chrome/common/chrome_content_client_constants.cc:9: const char* const
ChromeContentClient::kNaClOldPluginName = "Chrome NaCl";
This is unused.

[teravest, 2014-08-12 19:57:12]

lgtm

https://codereview.chromium.org/437503004/diff/200001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/437503004/diff/200001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:188: nacl.permissions =
ppapi::PERMISSION_PRIVATE | ppapi::PERMISSION_DEV;
On 2014/08/12 18:17:06, James Cook wrote:
> I inlined this to avoid a src/components/nacl/common -> src/ppapi dependency.

Acknowledged.

https://codereview.chromium.org/437503004/diff/200001/chrome/common/chrome_co...
File chrome/common/chrome_content_client_constants.cc (left):

https://codereview.chromium.org/437503004/diff/200001/chrome/common/chrome_co...
chrome/common/chrome_content_client_constants.cc:9: const char* const
ChromeContentClient::kNaClOldPluginName = "Chrome NaCl";
On 2014/08/12 18:17:06, James Cook wrote:
> This is unused.

Acknowledged.

https://codereview.chromium.org/437503004/diff/200001/extensions/shell/browse...
File extensions/shell/browser/shell_nacl_browser_delegate.cc (right):

https://codereview.chromium.org/437503004/diff/200001/extensions/shell/browse...
extensions/shell/browser/shell_nacl_browser_delegate.cc:139: bool
ShellNaClBrowserDelegate::MapUrlToLocalFilePath(
It'd be nice if this code could be shared with
NaClBrowserDelegateImpl::MapUrlToLocalFilePath.

I don't see much nacl-specific stuff in there; perhaps it could all be moved to
extensions/.

Feel free to leave a TODO behind instead of doing that as part of this change,
though.

[James Cook, 2014-08-12 20:20:45]

rockot, PTAL at:

chrome/browser/extensions/plugin_manager.cc
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc
extensions/browser/process_manager.h
extensions/browser/process_manager.cc

https://codereview.chromium.org/437503004/diff/200001/extensions/shell/browse...
File extensions/shell/browser/shell_nacl_browser_delegate.cc (right):

https://codereview.chromium.org/437503004/diff/200001/extensions/shell/browse...
extensions/shell/browser/shell_nacl_browser_delegate.cc:139: bool
ShellNaClBrowserDelegate::MapUrlToLocalFilePath(
On 2014/08/12 19:57:12, teravest wrote:
> It'd be nice if this code could be shared with
> NaClBrowserDelegateImpl::MapUrlToLocalFilePath.
> 
> I don't see much nacl-specific stuff in there; perhaps it could all be moved
to
> extensions/.
> 
> Feel free to leave a TODO behind instead of doing that as part of this change,
> though.

I'll file a bug for it, leave a TODO, and do it in another CL.

[Ken Rockot(use gerrit already), 2014-08-13 00:37:46]

On 2014/08/12 20:20:45, James Cook wrote:
> rockot, PTAL at:
> 
> chrome/browser/extensions/plugin_manager.cc
> chrome/browser/nacl_host/nacl_browser_delegate_impl.cc
> extensions/browser/process_manager.h
> extensions/browser/process_manager.cc
> 

^ That stuff LGTM

[James Cook, 2014-08-13 16:27:33]

sehr, can I get OWNERS for adding dependencies from src/extensions/shell to
src/components/nacl? This is the NaCl support in app_shell that we talked about
in the meeting last week.

sky, can I get OWNERS for moving some constants out of:
chrome/common/chrome_content_client.h/cc
chrome/renderer/chrome_content_renderer_client.cc

[James Cook, 2014-08-13 16:41:22]

derat, can I get OWNERS for adding dependency from src/extensions/shell/browser
to cros_system_api? This dep isn't part of this change (it comes from
shell_network_controller_chromeos.cc) but it apparently never got added and
causes checkdeps failures.

[Daniel Erat, 2014-08-13 16:45:47]

sure, gltm

[Daniel Erat, 2014-08-13 16:45:59]

lgtm </yoda>

[sky, 2014-08-13 16:52:08]

Said files LGTM

[James Cook, 2014-08-14 20:23:12]

mseaborn, can I get OWNERS for adding dependencies from src/extensions/shell to
src/components/nacl?  extensions/shell contains the implementation of app_shell
(go/appshell) that we're trying to bring up on Chrome OS.

(sehr@ appears to be some in some combination of OOO/committee meetings and
derat@ has a change blocked on this one.)

[Mark Seaborn, 2014-08-14 21:00:35]

On 2014/08/14 20:23:12, James Cook wrote:
> mseaborn, can I get OWNERS for adding dependencies from src/extensions/shell
to
> src/components/nacl?  extensions/shell contains the implementation of
app_shell
> (go/appshell) that we're trying to bring up on Chrome OS.

Looks OK, but one question is: what is your testing plan for the new code in
extensions/shell/?  Is it possible to make some subset of NaCl tests from
browser_tests run in app_shell_browsertests?  I'm not saying you need to do that
in this change, but it would be sensible to do that later otherwise this code is
liable to get broken.

https://codereview.chromium.org/437503004/diff/240001/extensions/shell/browse...
File extensions/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/437503004/diff/240001/extensions/shell/browse...
extensions/shell/browser/shell_content_browser_client.cc:228: // NOTE: app_shell
does not support ARC or non-SFI mode, so does not pass
Nit: You don't need to mention "ARC" here, just non-SFI mode.  ARC is not
something Chromium supports as such anyway (except for whitelisting some
extensions that are allowed to use non-SFI mode).

[James Cook, 2014-08-14 21:53:40]

mseaborn, PTAL

Yes, I plan to test the NaCl startup code via app_shell_browsertests -- I'll
take a look at the existing NaCl tests in Chrome browser_tests. Filed
crbug.com/403950 to track that work.

https://codereview.chromium.org/437503004/diff/240001/extensions/shell/browse...
File extensions/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/437503004/diff/240001/extensions/shell/browse...
extensions/shell/browser/shell_content_browser_client.cc:228: // NOTE: app_shell
does not support ARC or non-SFI mode, so does not pass
On 2014/08/14 21:00:35, Mark Seaborn wrote:
> Nit: You don't need to mention "ARC" here, just non-SFI mode.  ARC is not
> something Chromium supports as such anyway (except for whitelisting some
> extensions that are allowed to use non-SFI mode).

Done.

[James Cook, 2014-08-15 20:21:19]

mseaborn, friendly ping?

[Mark Seaborn, 2014-08-15 20:44:23]

On 2014/08/14 21:53:40, James Cook wrote:
> mseaborn, PTAL
> 
> Yes, I plan to test the NaCl startup code via app_shell_browsertests -- I'll
> take a look at the existing NaCl tests in Chrome browser_tests. Filed
> crbug.com/403950 to track that work.

OK, LGTM, thanks.

[James Cook, 2014-08-15 21:53:13]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-08-15 21:55:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamescook@chromium.org/437503004/320001

[commit-bot: I haz the power, 2014-08-16 01:38:17]

Committed patchset #13 (320001) as 290082