Enable pnacl by default

Enable pnacl by default

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3560
R=binji@chromium.org, darin@chromium.org, dmichael@chromium.org, jvoung@chromium.org, mseaborn@chromium.org

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

[sehr, 2013-07-12 00:13:39]

PTAL.

[dmichael (off chromium), 2013-07-12 16:43:46]

Per
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/HN2e4sGbVus...
... I haven't seen a PSA on chromium-dev for PNaCl yet. It would also be nice to
add a little more detail to:
http://www.chromestatus.com/features
In particular, "Non-spec documentation for web developers" (I think we have
some, so just a link would help). I think PNaCl team has also done some work
around defining the PNaCl bitcode? That seems like good information to
reference.

FTR, I'm supportive of PNaCl. But we should follow the process we've defined and
try to be as open as we can about our reasons for proceeding without a spec and
in the face of skepticism from other browser vendors.

[Mark Seaborn, 2013-07-18 01:25:52]

https://codereview.chromium.org/19079002/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/19079002/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:6326: +      <message
name="IDS_FLAGS_DISABLE_PNACL_DESCRIPTION" desc="Description of the 'Enable
Portable Native Client' lab.">
Should be "Disable" here

https://codereview.chromium.org/19079002/diff/1/chrome/common/chrome_content_...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/19079002/diff/1/chrome/common/chrome_content_...
chrome/common/chrome_content_client.cc:177: switches::kDisablePnacl)) {
Indent by 4 more chars.  Should be indented as:

// good
if (SomeFunc(
        arg1, arg2)) {

// bad
if (SomeFunc(
    arg1, arg2)) {

https://codereview.chromium.org/19079002/diff/1/chrome/test/data/nacl/pnacl_m...
File chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html (left):

https://codereview.chromium.org/19079002/diff/1/chrome/test/data/nacl/pnacl_m...
chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html:38: var
nacl_mime_type = "application/x-nacl";
I'm not sure why you're removing this.  This needs an explanation in the commit
message at least.

https://codereview.chromium.org/19079002/diff/1/chrome/test/data/nacl/pnacl_m...
File chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html (right):

https://codereview.chromium.org/19079002/diff/1/chrome/test/data/nacl/pnacl_m...
chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html:33:
(navigator.mimeTypes[pnacl_mime_type] !== undefined)) {
Indent to line up with '('

https://codereview.chromium.org/19079002/diff/1/chrome/test/nacl/nacl_browser...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/19079002/diff/1/chrome/test/nacl/nacl_browser...
chrome/test/nacl/nacl_browsertest_util.h:128: class NaClBrowserTestPnaclDisabled
: public NaClBrowserTestBase {
Can you add a comment about what this is for?  It looks like you're just using
it to run a single Javascript-only test.

https://codereview.chromium.org/19079002/diff/1/chrome/test/nacl/nacl_browser...
chrome/test/nacl/nacl_browsertest_util.h:180: body \
Don't need to add a '\' here

https://codereview.chromium.org/19079002/diff/1/chrome/test/ppapi/ppapi_test.cc
File chrome/test/ppapi/ppapi_test.cc (right):

https://codereview.chromium.org/19079002/diff/1/chrome/test/ppapi/ppapi_test....
chrome/test/ppapi/ppapi_test.cc:330: // Enable running NaCl outside of the
store.
Update to "Enable running non-PNaCl NaCl"?  Or some better wording.  Do we have
a preferred term for non-PNaCl NaCl?

https://codereview.chromium.org/19079002/diff/1/chrome/test/ppapi/ppapi_test....
chrome/test/ppapi/ppapi_test.cc:366: // Enable running NaCl outside of the
store.
Ditto

[jvoung (off chromium), 2013-07-18 17:00:59]

https://codereview.chromium.org/19079002/diff/1/chrome/browser/ui/webui/nacl_...
File chrome/browser/ui/webui/nacl_ui.cc (right):

https://codereview.chromium.org/19079002/diff/1/chrome/browser/ui/webui/nacl_...
chrome/browser/ui/webui/nacl_ui.cc:341: ListFlagStatus(list.get(), "Flag
'--disable-pnacl'", switches::kDisablePnacl);
Might be more readable if this directly told the developer if PNaCl is enabled
or not, instead of saying if the "disable" flag was on or off. 

However, I suspect that if you want to say if PNaCl itself is enabled or not,
you also have to check if it's enabled in about:plugins also (not just this flag
from about:flags / the commandline).

You can customize this if you code it manually, instead of using ListFlagStatus.
We could get rid of that helper since it's only used in two places.

Actually, re: about:plugins -- the "Disable" link doesn't seem to work for
Native Client anymore.  Only works for other plugins.

https://codereview.chromium.org/19079002/diff/1/chrome/renderer/pepper/ppb_na...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/19079002/diff/1/chrome/renderer/pepper/ppb_na...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:325:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisablePnacl));
Couple of JS console debug messages get printed:

$ git grep "enable-pnacl" ppapi/native_client/src/trusted/plugin/

ppapi/native_client/src/trusted/plugin/plugin.cc:                            
"the --enable-pnacl flag).");
ppapi/native_client/src/trusted/plugin/plugin_error.h:  // remove the
--enable-pnacl flag).
ppapi/native_client/src/trusted/plugin/service_runtime.cc:                      
        "enabled with --enable-pnacl.");

[sehr, 2013-07-22 23:06:50]

Thanks for the reviews.  New version uploaded.  PTAL.

https://codereview.chromium.org/19079002/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/19079002/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:6326: +      <message
name="IDS_FLAGS_DISABLE_PNACL_DESCRIPTION" desc="Description of the 'Enable
Portable Native Client' lab.">
On 2013/07/18 01:25:52, Mark Seaborn wrote:
> Should be "Disable" here

Done.

https://codereview.chromium.org/19079002/diff/1/chrome/browser/ui/webui/nacl_...
File chrome/browser/ui/webui/nacl_ui.cc (right):

https://codereview.chromium.org/19079002/diff/1/chrome/browser/ui/webui/nacl_...
chrome/browser/ui/webui/nacl_ui.cc:341: ListFlagStatus(list.get(), "Flag
'--disable-pnacl'", switches::kDisablePnacl);
On 2013/07/18 17:01:00, jvoung (cr) wrote:
> Might be more readable if this directly told the developer if PNaCl is enabled
> or not, instead of saying if the "disable" flag was on or off. 
> 
> However, I suspect that if you want to say if PNaCl itself is enabled or not,
> you also have to check if it's enabled in about:plugins also (not just this
flag
> from about:flags / the commandline).
> 
> You can customize this if you code it manually, instead of using
ListFlagStatus.
> We could get rid of that helper since it's only used in two places.
> 
> Actually, re: about:plugins -- the "Disable" link doesn't seem to work for
> Native Client anymore.  Only works for other plugins.

Done.

https://codereview.chromium.org/19079002/diff/1/chrome/common/chrome_content_...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/19079002/diff/1/chrome/common/chrome_content_...
chrome/common/chrome_content_client.cc:177: switches::kDisablePnacl)) {
vim's auto indent disagrees with you, but done.

On 2013/07/18 01:25:52, Mark Seaborn wrote:
> Indent by 4 more chars.  Should be indented as:
> 
> // good
> if (SomeFunc(
>         arg1, arg2)) {
> 
> // bad
> if (SomeFunc(
>     arg1, arg2)) {

https://codereview.chromium.org/19079002/diff/1/chrome/renderer/pepper/ppb_na...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/19079002/diff/1/chrome/renderer/pepper/ppb_na...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:325:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisablePnacl));
Thanks.  I didn't do an expansive enough search.  Done.

On 2013/07/18 17:01:00, jvoung (cr) wrote:
> Couple of JS console debug messages get printed:
> 
> $ git grep "enable-pnacl" ppapi/native_client/src/trusted/plugin/
> 
> ppapi/native_client/src/trusted/plugin/plugin.cc:                            
> "the --enable-pnacl flag).");
> ppapi/native_client/src/trusted/plugin/plugin_error.h:  // remove the
> --enable-pnacl flag).
> ppapi/native_client/src/trusted/plugin/service_runtime.cc:                    
 
>         "enabled with --enable-pnacl.");

https://codereview.chromium.org/19079002/diff/1/chrome/test/data/nacl/pnacl_m...
File chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html (left):

https://codereview.chromium.org/19079002/diff/1/chrome/test/data/nacl/pnacl_m...
chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html:38: var
nacl_mime_type = "application/x-nacl";
I can't remember either, and the test passes with this code re-inserted, so I am
leaving it in.

On 2013/07/18 01:25:52, Mark Seaborn wrote:
> I'm not sure why you're removing this.  This needs an explanation in the
commit
> message at least.

https://codereview.chromium.org/19079002/diff/1/chrome/test/data/nacl/pnacl_m...
File chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html (right):

https://codereview.chromium.org/19079002/diff/1/chrome/test/data/nacl/pnacl_m...
chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html:33:
(navigator.mimeTypes[pnacl_mime_type] !== undefined)) {
On 2013/07/18 01:25:52, Mark Seaborn wrote:
> Indent to line up with '('

Done.

https://codereview.chromium.org/19079002/diff/1/chrome/test/nacl/nacl_browser...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/19079002/diff/1/chrome/test/nacl/nacl_browser...
chrome/test/nacl/nacl_browsertest_util.h:128: class NaClBrowserTestPnaclDisabled
: public NaClBrowserTestBase {
On 2013/07/18 01:25:52, Mark Seaborn wrote:
> Can you add a comment about what this is for?  It looks like you're just using
> it to run a single Javascript-only test.

Done.

https://codereview.chromium.org/19079002/diff/1/chrome/test/nacl/nacl_browser...
chrome/test/nacl/nacl_browsertest_util.h:180: body \
On 2013/07/18 01:25:52, Mark Seaborn wrote:
> Don't need to add a '\' here

Done.

https://codereview.chromium.org/19079002/diff/1/chrome/test/ppapi/ppapi_test.cc
File chrome/test/ppapi/ppapi_test.cc (right):

https://codereview.chromium.org/19079002/diff/1/chrome/test/ppapi/ppapi_test....
chrome/test/ppapi/ppapi_test.cc:330: // Enable running NaCl outside of the
store.
Done, but by adding (non-portable), as we don't have a better name.

On 2013/07/18 01:25:52, Mark Seaborn wrote:
> Update to "Enable running non-PNaCl NaCl"?  Or some better wording.  Do we
have
> a preferred term for non-PNaCl NaCl?

https://codereview.chromium.org/19079002/diff/1/chrome/test/ppapi/ppapi_test....
chrome/test/ppapi/ppapi_test.cc:366: // Enable running NaCl outside of the
store.
Ditto.
On 2013/07/18 01:25:52, Mark Seaborn wrote:
> Ditto

[sehr, 2013-07-22 23:08:08]

James, I am requesting an owners review for 
    chrome/browser/ui/webui/nacl_ui.cc
    chrome/common/chrome_content_client.cc
    chrome/renderer/chrome_content_renderer_client.cc

Thanks in advance.

[Mark Seaborn, 2013-07-22 23:28:10]

https://codereview.chromium.org/19079002/diff/13001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/19079002/diff/13001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6329: +      <message
name="IDS_FLAGS_DISABLE_PNACL_DESCRIPTION" desc="Description of the 'Enable
Portable Native Client' lab.">
Same comment again: Should be "Disable" here

https://codereview.chromium.org/19079002/diff/13001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/nacl_ui.cc (right):

https://codereview.chromium.org/19079002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:306: plugin_enabled = false;
I don't understand this.  If there are multiple plugins handling
"application/x-nacl" (when would that happen?), and one of them is disabled, we
say "plugin_enabled = false"?  That doesn't seem right.

What's the intent behind the changes to nacl_ui.cc?  Can you comment this in the
commit message?

https://codereview.chromium.org/19079002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:328: else if
(CommandLine::ForCurrentProcess()->HasSwitch(
Should go on previous line as "} else if"

https://codereview.chromium.org/19079002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:329: switches::kDisablePnacl)) {
Fix indentation here.

// good
if (SomeFunc(
        arg1, arg2)) {

// bad
if (SomeFunc(
    arg1, arg2)) {

https://codereview.chromium.org/19079002/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://codereview.chromium.org/19079002/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin_error.h:96: ERROR_PNACL_DISABLED =
68,
Should this have a different UMA code from ERROR_PNACL_NOT_ENABLED? 
ERROR_PNACL_DISABLED is the result of user action, whereas
ERROR_PNACL_NOT_ENABLED is the result of user inaction.

[sehr, 2013-07-23 22:03:26]

I've decided to continue with the UI cleanups at the same time.  PTAL.

https://codereview.chromium.org/19079002/diff/13001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/19079002/diff/13001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6329: +      <message
name="IDS_FLAGS_DISABLE_PNACL_DESCRIPTION" desc="Description of the 'Enable
Portable Native Client' lab.">
On 2013/07/22 23:28:10, Mark Seaborn wrote:
> Same comment again: Should be "Disable" here

Done.

https://codereview.chromium.org/19079002/diff/13001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/nacl_ui.cc (right):

https://codereview.chromium.org/19079002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:306: plugin_enabled = false;
On 2013/07/22 23:28:10, Mark Seaborn wrote:
> I don't understand this.  If there are multiple plugins handling
> "application/x-nacl" (when would that happen?), and one of them is disabled,
we
> say "plugin_enabled = false"?  That doesn't seem right.
> 
> What's the intent behind the changes to nacl_ui.cc?  Can you comment this in
the
> commit message?

There weren't multiple plugins handling the mime type, but we had a print
artifact, which I have now removed, along with the flag setting.

https://codereview.chromium.org/19079002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:328: else if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2013/07/22 23:28:10, Mark Seaborn wrote:
> Should go on previous line as "} else if"

Done.

https://codereview.chromium.org/19079002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:329: switches::kDisablePnacl)) {
On 2013/07/22 23:28:10, Mark Seaborn wrote:
> Fix indentation here.
> 
> // good
> if (SomeFunc(
>         arg1, arg2)) {
> 
> // bad
> if (SomeFunc(
>     arg1, arg2)) {

Done.

https://codereview.chromium.org/19079002/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://codereview.chromium.org/19079002/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin_error.h:96: ERROR_PNACL_DISABLED =
68,
On 2013/07/22 23:28:10, Mark Seaborn wrote:
> Should this have a different UMA code from ERROR_PNACL_NOT_ENABLED? 
> ERROR_PNACL_DISABLED is the result of user action, whereas
> ERROR_PNACL_NOT_ENABLED is the result of user inaction.

I have reverted this change.

[jvoung (off chromium), 2013-07-23 23:16:00]

Also have to change native_client_sdk, I think.

$ git grep "enable-pnacl" native_client_sdk/

native_client_sdk/src/build_tools/test_projects.py:   
args.extend(['--browser_flag', '--enable-pnacl'])
native_client_sdk/src/tools/common.mk:CHROME_ARGS += --enable-nacl
--enable-pnacl --no-first-run

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest_util.cc:307: void
NaClBrowserTestPnacl::SetUpCommandLine(CommandLine* command_line) {
no need to override anymore

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest_util.h:121: virtual void
SetUpCommandLine(CommandLine* command_line) OVERRIDE;
no need to override anymore

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest_util.h:175: // Otherwise, we have Glibc,
Newlib, Pnacl, and PnaclDisabled tests
PnaclDisabled shouldn't be in this "test-all" list

[sehr, 2013-07-24 00:39:14]

Thanks, Jan.  PTAL.

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest_util.cc:307: void
NaClBrowserTestPnacl::SetUpCommandLine(CommandLine* command_line) {
On 2013/07/23 23:16:00, jvoung (cr) wrote:
> no need to override anymore

Done.

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest_util.h:121: virtual void
SetUpCommandLine(CommandLine* command_line) OVERRIDE;
On 2013/07/23 23:16:00, jvoung (cr) wrote:
> no need to override anymore

Done.

https://codereview.chromium.org/19079002/diff/35001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest_util.h:175: // Otherwise, we have Glibc,
Newlib, Pnacl, and PnaclDisabled tests
On 2013/07/23 23:16:00, jvoung (cr) wrote:
> PnaclDisabled shouldn't be in this "test-all" list

I left this as a vestige of an earlier attempt.  Removed.

[jvoung (off chromium), 2013-07-24 15:46:58]

A couple more things, otherwise the code LGTM

https://codereview.chromium.org/19079002/diff/42001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/19079002/diff/42001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6329: +      <message
name="IDS_FLAGS_DISABLE_PNACL_DESCRIPTION" desc="Description of the 'Disable
Portable Native Client' lab.">
"Description of the 'Portable Native ...'"

to match the *_NAME entry.  Could also be IDS_FLAGS_PNACL_NAME and
IDS_FLAGS_PNACL_DESCRIPTION, now that the about:flags entry is more neutral.

Then again, the only on-the-commandline switch is --disable-pnacl, not sure of
the convention for that.

https://codereview.chromium.org/19079002/diff/42001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/19079002/diff/42001/chrome/common/chrome_swit...
chrome/common/chrome_switches.h:182: extern const char kDisablePnacl[];
Oops when I added kDisablePnaclInstall, should have moved it (sort).

https://codereview.chromium.org/19079002/diff/42001/native_client_sdk/src/bui...
File native_client_sdk/src/build_tools/test_projects.py (right):

https://codereview.chromium.org/19079002/diff/42001/native_client_sdk/src/bui...
native_client_sdk/src/build_tools/test_projects.py:106:
args.extend(['--browser_flag'])
I don't think you need --browser_flag either.  My ssh is bricked, so I can't
look at the code, but I think it's just a way to pass through browser flags from
the "browser_tester_py" to the actual browser.

[sehr, 2013-07-24 16:57:01]

One more time, and I'm running trybots now.  The PSA is going out shortly also.

https://codereview.chromium.org/19079002/diff/42001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/19079002/diff/42001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6329: +      <message
name="IDS_FLAGS_DISABLE_PNACL_DESCRIPTION" desc="Description of the 'Disable
Portable Native Client' lab.">
On 2013/07/24 15:46:58, jvoung (cr) wrote:
> "Description of the 'Portable Native ...'"
> 
> to match the *_NAME entry.  Could also be IDS_FLAGS_PNACL_NAME and
> IDS_FLAGS_PNACL_DESCRIPTION, now that the about:flags entry is more neutral.
> 
> Then again, the only on-the-commandline switch is --disable-pnacl, not sure of
> the convention for that.

I changed the name of the IDS_FLAGS... but will leave the flag name as-is.

https://codereview.chromium.org/19079002/diff/42001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/19079002/diff/42001/chrome/common/chrome_swit...
chrome/common/chrome_switches.h:182: extern const char kDisablePnacl[];
On 2013/07/24 15:46:58, jvoung (cr) wrote:
> Oops when I added kDisablePnaclInstall, should have moved it (sort).

You weren't the only one.  I moved all the kDisable* flags to their respective
positions.

https://codereview.chromium.org/19079002/diff/42001/native_client_sdk/src/bui...
File native_client_sdk/src/build_tools/test_projects.py (right):

https://codereview.chromium.org/19079002/diff/42001/native_client_sdk/src/bui...
native_client_sdk/src/build_tools/test_projects.py:106:
args.extend(['--browser_flag'])
On 2013/07/24 15:46:58, jvoung (cr) wrote:
> I don't think you need --browser_flag either.  My ssh is bricked, so I can't
> look at the code, but I think it's just a way to pass through browser flags
from
> the "browser_tester_py" to the actual browser.

Yes, what I had would never have worked.  Sorry.

[jvoung (off chromium), 2013-07-24 17:32:09]

https://codereview.chromium.org/19079002/diff/54001/build/ios/grit_whitelist.txt
File build/ios/grit_whitelist.txt (right):

https://codereview.chromium.org/19079002/diff/54001/build/ios/grit_whitelist....
build/ios/grit_whitelist.txt:478: IDS_FLAGS_DISABLE_PNACL_DESCRIPTION
Doesn't these need to be changed too?

[James Hawkins, 2013-07-24 18:27:22]

https://codereview.chromium.org/19079002/diff/54001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/nacl_ui.cc (right):

https://codereview.chromium.org/19079002/diff/54001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:235: void
NaClDomHandler::PopulatePageInformation(DictionaryValue* naclInfo) {
This method is now 122 lines long (which is too long); please break it up.

https://codereview.chromium.org/19079002/diff/54001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:281: bool plugin_enabled = true;
This var being set in so many places is not very readable.  I suggest pulling
out a helper function that would basically be:

plugin_enabled = isPluginEnabled();

bool isPluginEnabled() {
  return !info_array.empty() && plugin_prefs->IsPluginEnabled(info_array[0]);
}

[sehr, 2013-07-24 22:30:52]

I am backing off the change to refactor the about://nacl page for now.  PTAL.

https://codereview.chromium.org/19079002/diff/54001/build/ios/grit_whitelist.txt
File build/ios/grit_whitelist.txt (right):

https://codereview.chromium.org/19079002/diff/54001/build/ios/grit_whitelist....
build/ios/grit_whitelist.txt:478: IDS_FLAGS_DISABLE_PNACL_DESCRIPTION
On 2013/07/24 17:32:09, jvoung (cr) wrote:
> Doesn't these need to be changed too?

Done.

https://codereview.chromium.org/19079002/diff/54001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/nacl_ui.cc (right):

https://codereview.chromium.org/19079002/diff/54001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:235: void
NaClDomHandler::PopulatePageInformation(DictionaryValue* naclInfo) {
On 2013/07/24 18:27:22, James Hawkins wrote:
> This method is now 122 lines long (which is too long); please break it up.

I am reverting the UI refactoring for now, and have only changed the flag name. 
I will do the refactoring next.

https://codereview.chromium.org/19079002/diff/54001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/nacl_ui.cc:281: bool plugin_enabled = true;
On 2013/07/24 18:27:22, James Hawkins wrote:
> This var being set in so many places is not very readable.  I suggest pulling
> out a helper function that would basically be:
> 
> plugin_enabled = isPluginEnabled();
> 
> bool isPluginEnabled() {
>   return !info_array.empty() && plugin_prefs->IsPluginEnabled(info_array[0]);
> }

The refactoring CL will address this issue.

[sehr, 2013-07-24 22:32:52]

+binji, for native_client_sdk changes.  PTAL.

[binji, 2013-07-24 23:36:58]

native_client_sdk lgtm

[sehr, 2013-07-25 23:38:22]

On 2013/07/24 23:36:58, binji wrote:
> native_client_sdk lgtm

jhawkins, dmichael, I'm still looking for OWNERs of other parts of this.  The
PSA will go out before the check in, but once I have the LGTMs.

David

[dmichael (off chromium), 2013-07-26 22:06:03]

content/renderer/pepper and ppapi OWNERS lgtm

[sehr, 2013-07-26 22:49:15]

Nico, I need an owner to approve this CL.  Your review is humbly requested.

David

[Nico, 2013-07-26 22:52:44]

This should probably be approved by a top-level owner :-) +ben/darin

[darin (slow to review), 2013-07-26 23:03:24]

LGTM

This seems like a hefty patch for flipping this switch, but it all seems
reasonable to me.  It makes me wish for a better flags system :)

[Mark Seaborn, 2013-07-26 23:10:39]

LGTM for chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html

[sehr, 2013-07-26 23:19:27]

Committed patchset #7 manually as r213999 (presubmit successful).