Split pnacl and nacl mime types

Split pnacl and nacl mime types

Manifest parsing now knows which mime type was used, and does not allow
pnacl to be used from nacl manifests or vice-versa.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3440
R=bradnelson@chromium.org, jam@chromium.org, jvoung@chromium.org

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

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

[sehr, 2013-06-04 19:33:32]

PTAL.

[jvoung (off chromium), 2013-06-04 22:27:25]

https://codereview.chromium.org/16296005/diff/2001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/16296005/diff/2001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:562: // If the params somehow
contain '@dev', remove it.
Where do the @dev params come from?

Should we add a check for the kEnabledPnacl commandline switch for now, and show
a BlockedPlugin if not turned on?  I guess it could be a bit more complicated if
we wanted to enable for the webstore right now, even without --enable-pnacl?

https://codereview.chromium.org/16296005/diff/2001/chrome/test/data/nacl/exit...
File chrome/test/data/nacl/exit_status/pm_exit_status_test.html (right):

https://codereview.chromium.org/16296005/diff/2001/chrome/test/data/nacl/exit...
chrome/test/data/nacl/exit_status/pm_exit_status_test.html:18: <!--
could remove the commented out html

https://codereview.chromium.org/16296005/diff/2001/chrome/test/data/nacl/pnac...
File chrome/test/data/nacl/pnacl_nmf_options/pnacl_options.html (left):

https://codereview.chromium.org/16296005/diff/2001/chrome/test/data/nacl/pnac...
chrome/test/data/nacl/pnacl_nmf_options/pnacl_options.html:8: <title>NaCl Load
Test</title>
Oops, if you don't mind, could you change the title and the <h2> to "PNaCl Load
Options Test", or something like that?

https://codereview.chromium.org/16296005/diff/2001/ppapi/native_client/tests/...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.js (right):

https://codereview.chromium.org/16296005/diff/2001/ppapi/native_client/tests/...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.js:26: function
badLoadTest(tester, id, src, type, error_string) {
Can you look for other uses of badLoadTest under the ppapi/native_client/tests/
directory?  I think there are still some old-style nacl-integration tests
running for the non-PNaCl case:

ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html

Running those would be done with something like:

python chrome/test/nacl_test_injection/buildbot_nacl_integration.py --mode
Release

You might need some arch flag also, and may want to try newlib vs glibc.  That
will also be tested with try-jobs against the naclmore bots (see the try more
bots links in the codereview page).

I was hoping to port that to the browser_tests framework so that it's easier to
run, but haven't gotten around to it.

[sehr, 2013-06-06 22:03:45]

Thanks for all the help with the decisions, btw.  PTAL.

https://codereview.chromium.org/16296005/diff/2001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/16296005/diff/2001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:562: // If the params somehow
contain '@dev', remove it.
On 2013/06/04 22:27:25, jvoung (cr) wrote:
> Where do the @dev params come from?
> 
> Should we add a check for the kEnabledPnacl commandline switch for now, and
show
> a BlockedPlugin if not turned on?  I guess it could be a bit more complicated
if
> we wanted to enable for the webstore right now, even without --enable-pnacl?

The @dev parameters are added for nacl content handlers in the whitelist, and so
don't believe they are ever set for pnacl.  I have removed the filter.  And
added the blocked plugin UI as you suggest.

https://codereview.chromium.org/16296005/diff/2001/chrome/test/data/nacl/exit...
File chrome/test/data/nacl/exit_status/pm_exit_status_test.html (right):

https://codereview.chromium.org/16296005/diff/2001/chrome/test/data/nacl/exit...
chrome/test/data/nacl/exit_status/pm_exit_status_test.html:18: <!--
On 2013/06/04 22:27:25, jvoung (cr) wrote:
> could remove the commented out html

Done.

https://codereview.chromium.org/16296005/diff/2001/chrome/test/data/nacl/pnac...
File chrome/test/data/nacl/pnacl_nmf_options/pnacl_options.html (left):

https://codereview.chromium.org/16296005/diff/2001/chrome/test/data/nacl/pnac...
chrome/test/data/nacl/pnacl_nmf_options/pnacl_options.html:8: <title>NaCl Load
Test</title>
On 2013/06/04 22:27:25, jvoung (cr) wrote:
> Oops, if you don't mind, could you change the title and the <h2> to "PNaCl
Load
> Options Test", or something like that?

Done.

https://codereview.chromium.org/16296005/diff/2001/ppapi/native_client/tests/...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.js (right):

https://codereview.chromium.org/16296005/diff/2001/ppapi/native_client/tests/...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.js:26: function
badLoadTest(tester, id, src, type, error_string) {
On 2013/06/04 22:27:25, jvoung (cr) wrote:
> Can you look for other uses of badLoadTest under the
ppapi/native_client/tests/
> directory?  I think there are still some old-style nacl-integration tests
> running for the non-PNaCl case:
> 
> ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html
> 
> Running those would be done with something like:
> 
> python chrome/test/nacl_test_injection/buildbot_nacl_integration.py --mode
> Release
> 
> You might need some arch flag also, and may want to try newlib vs glibc.  That
> will also be tested with try-jobs against the naclmore bots (see the try more
> bots links in the codereview page).
> 
> I was hoping to port that to the browser_tests framework so that it's easier
to
> run, but haven't gotten around to it.

Done.  Sorry I missed these.

[jvoung (off chromium), 2013-06-06 22:41:25]

LGTM -- could clean up the ppapi_bad a little bit more (can delete one test
case).

https://codereview.chromium.org/16296005/diff/10001/chrome/common/chrome_cont...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/16296005/diff/10001/chrome/common/chrome_cont...
chrome/common/chrome_content_client.cc:73: const char kPnaclPluginMimeType[] =
"application/x-pnacl";
At some point we might want some kind of javascript test to check that PNaCl
shows up under navigator.mimeTypes when --enable-pnacl is set.

I'm not sure if there is a more direct way to unittest that.

https://codereview.chromium.org/16296005/diff/10001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/16296005/diff/10001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:572:
l10n_util::GetStringUTF16(IDS_NACL_PLUGIN_BLOCKED));
Hmm, I guess the message is pretty generic (doesn't mention NaCl at all), so we
can use it for both NaCl and PNaCl...

https://codereview.chromium.org/16296005/diff/10001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/16296005/diff/10001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:77: // given both a
nexe and a portable program to choose from.
Can update comment for this test, since we took out the "portable" entry.

Actually, we probably only need the "ppapi_bad_doesnotexist_nexe_only.nmf" and
don't need the "ppapi_bad_doesnotexist.nmf" anymore?

[jvoung (off chromium), 2013-06-07 16:14:46]

https://codereview.chromium.org/16296005/diff/2001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/16296005/diff/2001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:565: for (size_t i = 0; i <
attribute_count; ++i) {
Also, if you want to allow PNaCl without --enable-nacl for the time being, there
is chrome/browser/extensions/extension_nacl_browsertest.cc that helps test that
NaCl isn't blocked for webstore apps and could be extended to check that PNaCl
isn't blocked?

[sehr, 2013-06-07 23:34:47]

As discussed --enable-pnacl enables it without regard for URL or web store.

PTAL.

https://codereview.chromium.org/16296005/diff/10001/chrome/common/chrome_cont...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/16296005/diff/10001/chrome/common/chrome_cont...
chrome/common/chrome_content_client.cc:73: const char kPnaclPluginMimeType[] =
"application/x-pnacl";
On 2013/06/06 22:41:25, jvoung (cr) wrote:
> At some point we might want some kind of javascript test to check that PNaCl
> shows up under navigator.mimeTypes when --enable-pnacl is set.
> 
> I'm not sure if there is a more direct way to unittest that.

As discussed, I have added a JS test to catch this.

https://codereview.chromium.org/16296005/diff/10001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/16296005/diff/10001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:572:
l10n_util::GetStringUTF16(IDS_NACL_PLUGIN_BLOCKED));
On 2013/06/06 22:41:25, jvoung (cr) wrote:
> Hmm, I guess the message is pretty generic (doesn't mention NaCl at all), so
we
> can use it for both NaCl and PNaCl...

I was hoping you'd say that :-).  I will change it if needed.

https://codereview.chromium.org/16296005/diff/10001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/16296005/diff/10001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:77: // given both a
nexe and a portable program to choose from.
On 2013/06/06 22:41:25, jvoung (cr) wrote:
> Can update comment for this test, since we took out the "portable" entry.
> 
> Actually, we probably only need the "ppapi_bad_doesnotexist_nexe_only.nmf" and
> don't need the "ppapi_bad_doesnotexist.nmf" anymore?

I retained the first, since the nexe_only part is sort of implicit.  The other
is removed.

[jvoung (off chromium), 2013-06-07 23:55:24]

otherwise LGTM

https://codereview.chromium.org/16296005/diff/27001/chrome/test/nacl/nacl_bro...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/16296005/diff/27001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest.cc:61: NACL_BROWSER_TEST_F(NaClBrowserTest,
MAYBE_PNACL(PnaclMimeType), {
I think for the NACL_BROWSER_TEST_F it already does "MAYBE_PNACL".  However you
should do MAYBE_testname for skipping asan under newlib and glibc like the other
tests, unless you want to check if asan works under newlib ;-)

[sehr, 2013-06-08 00:16:03]

On 2013/06/07 23:55:24, jvoung (cr) wrote:
> otherwise LGTM
> 
>
https://codereview.chromium.org/16296005/diff/27001/chrome/test/nacl/nacl_bro...
> File chrome/test/nacl/nacl_browsertest.cc (right):
> 
>
https://codereview.chromium.org/16296005/diff/27001/chrome/test/nacl/nacl_bro...
> chrome/test/nacl/nacl_browsertest.cc:61: NACL_BROWSER_TEST_F(NaClBrowserTest,
> MAYBE_PNACL(PnaclMimeType), {
> I think for the NACL_BROWSER_TEST_F it already does "MAYBE_PNACL".  However
you
> should do MAYBE_testname for skipping asan under newlib and glibc like the
other
> tests, unless you want to check if asan works under newlib ;-)

Done.

Thanks, only trybots to go.

[jvoung (off chromium), 2013-06-10 16:23:14]

https://codereview.chromium.org/16296005/diff/27001/chrome/test/nacl/nacl_bro...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/16296005/diff/27001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest.cc:61: NACL_BROWSER_TEST_F(NaClBrowserTest,
MAYBE_PNACL(PnaclMimeType), {
On 2013/06/07 23:55:24, jvoung (cr) wrote:
> I think for the NACL_BROWSER_TEST_F it already does "MAYBE_PNACL".  However
you
> should do MAYBE_testname for skipping asan under newlib and glibc like the
other
> tests, unless you want to check if asan works under newlib ;-)

Oh the macro has to be defined between line 10 and 32?

[sehr, 2013-06-10 20:34:18]

Darin, Brad,

I need OWNERS reviews for this change.

Thanks,

David

[jam, 2013-06-10 21:12:48]

lgtm

[bradnelson, 2013-06-10 21:17:18]

lgtm

[sehr, 2013-06-10 22:15:37]

Committed patchset #6 manually as r205329 (presubmit successful).

[sehr, 2013-06-12 17:41:12]

Committed patchset #8 manually as r205855 (presubmit successful).