Remove disable-pnacl flag.

Remove disable-pnacl flag.

This was used as part of the PNaCl rollout but is no longer necessary. I came
across this while investigating what code in chrome needs to be updated once
the file for the trusted plugin library is gone.

Usage statistics say that this flag has been used 1850 times in the past 7
days by 197 users.

TBR=ncbray
BUG=354867
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284997

[jvoung (off chromium), 2014-07-15 22:04:16]

There's also:

chrome/browser/component_updater/pnacl/pnacl_component_installer.cc: 
updates_disabled_ = command_line.HasSwitch(switches::kDisablePnaclInstall);


nacl_browsertest.cc:

IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclDisabled, PnaclMimeType) {           
           
  RunLoadTest(FILE_PATH_LITERAL("pnacl_mime_type.html"));
}

https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
File chrome/common/chrome_content_client.cc (left):

https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
chrome/common/chrome_content_client.cc:172: // Specifically, Portable Native
Client is only enabled by the command line
Can remove this old comment about PNaCl being only enabled by the command line
switch.

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

https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
chrome/common/chrome_content_client.cc:176: if (skip_nacl_file_check ||
base::PathExists(path)) {
It seems like we need to be careful about registering the mime_type once the
trusted plugin is gone.

The PathExists() check seems to serve as detecting if the gyp_defines did/did
not compile in NaCl support.

https://codereview.chromium.org/392023002/diff/1/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/392023002/diff/1/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:683: if (is_nacl_mime_type ||
is_pnacl_mime_type) {
I don't think this preserves the original's meaning.

Only one of the mime types should be set at a time. If is_pnacl_mime_type is
true, then is_nacl_unrestricted is just plain true (it's not dependent on the
kEnableNaCl).

Let me know if I'm reading this wrong, since this seems pretty important to get
right!

[jvoung (off chromium), 2014-07-15 22:04:22]

And thanks!

[jvoung (off chromium), 2014-07-15 22:04:56]

On 2014/07/15 22:04:16, jvoung wrote:
> There's also:
> 
> chrome/browser/component_updater/pnacl/pnacl_component_installer.cc: 
> updates_disabled_ = command_line.HasSwitch(switches::kDisablePnaclInstall);

Err... n/m about that one above. That's a separate flag that I need to remove...


> 
> 
> nacl_browsertest.cc:
> 
> IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclDisabled, PnaclMimeType) {         
 
>            
>   RunLoadTest(FILE_PATH_LITERAL("pnacl_mime_type.html"));
> }
> 
>
https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
> File chrome/common/chrome_content_client.cc (left):
> 
>
https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
> chrome/common/chrome_content_client.cc:172: // Specifically, Portable Native
> Client is only enabled by the command line
> Can remove this old comment about PNaCl being only enabled by the command line
> switch.
> 
>
https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
> File chrome/common/chrome_content_client.cc (right):
> 
>
https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
> chrome/common/chrome_content_client.cc:176: if (skip_nacl_file_check ||
> base::PathExists(path)) {
> It seems like we need to be careful about registering the mime_type once the
> trusted plugin is gone.
> 
> The PathExists() check seems to serve as detecting if the gyp_defines did/did
> not compile in NaCl support.
> 
>
https://codereview.chromium.org/392023002/diff/1/chrome/renderer/chrome_conte...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/392023002/diff/1/chrome/renderer/chrome_conte...
> chrome/renderer/chrome_content_renderer_client.cc:683: if (is_nacl_mime_type
||
> is_pnacl_mime_type) {
> I don't think this preserves the original's meaning.
> 
> Only one of the mime types should be set at a time. If is_pnacl_mime_type is
> true, then is_nacl_unrestricted is just plain true (it's not dependent on the
> kEnableNaCl).
> 
> Let me know if I'm reading this wrong, since this seems pretty important to
get
> right!

[teravest, 2014-07-15 23:08:51]

https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
File chrome/common/chrome_content_client.cc (left):

https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
chrome/common/chrome_content_client.cc:172: // Specifically, Portable Native
Client is only enabled by the command line
On 2014/07/15 22:04:16, jvoung wrote:
> Can remove this old comment about PNaCl being only enabled by the command line
> switch.

Done.

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

https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
chrome/common/chrome_content_client.cc:176: if (skip_nacl_file_check ||
base::PathExists(path)) {
On 2014/07/15 22:04:16, jvoung wrote:
> It seems like we need to be careful about registering the mime_type once the
> trusted plugin is gone.
> 
> The PathExists() check seems to serve as detecting if the gyp_defines did/did
> not compile in NaCl support.

I'd expect the behavior for registering the nacl mime type to be the same as the
pnacl mime type. Is there something I'm misunderstanding?

https://codereview.chromium.org/392023002/diff/1/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/392023002/diff/1/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:683: if (is_nacl_mime_type ||
is_pnacl_mime_type) {
On 2014/07/15 22:04:16, jvoung wrote:
> I don't think this preserves the original's meaning.
> 
> Only one of the mime types should be set at a time. If is_pnacl_mime_type is
> true, then is_nacl_unrestricted is just plain true (it's not dependent on the
> kEnableNaCl).
> 
> Let me know if I'm reading this wrong, since this seems pretty important to
get
> right!

You're right, thanks for catching that.

[jvoung (off chromium), 2014-07-15 23:24:16]

Otherwise LGTM

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

https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
chrome/common/chrome_content_client.cc:176: if (skip_nacl_file_check ||
base::PathExists(path)) {
On 2014/07/15 23:08:51, teravest wrote:
> On 2014/07/15 22:04:16, jvoung wrote:
> > It seems like we need to be careful about registering the mime_type once the
> > trusted plugin is gone.
> > 
> > The PathExists() check seems to serve as detecting if the gyp_defines
did/did
> > not compile in NaCl support.
> 
> I'd expect the behavior for registering the nacl mime type to be the same as
the
> pnacl mime type. Is there something I'm misunderstanding?


You're right that registering the nacl mime type should be the same as
registering the pnacl mime type. I think this code is fine as is.

I'm just saying that when there isn't a "FILE_NACL_PLUGIN", the code might have
be more careful and check #ifdef DISABLE_NACL or something.

https://codereview.chromium.org/392023002/diff/20001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/392023002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:683: "disable-pnacl",  // FLAGS:RECORD_UMA
Might also have to mark the tools/metrics/actions/actions.xml entry as obsolete?

https://codereview.chromium.org/392023002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:684: IDS_FLAGS_DISABLE_PNACL_NAME,
I think you also want to remove IDS_FLAGS_DISABLE_PNACL_NAME, and the
DESCRIPTION

https://codereview.chromium.org/392023002/diff/20001/chrome/test/nacl/nacl_br...
File chrome/test/nacl/nacl_browsertest_util.cc (left):

https://codereview.chromium.org/392023002/diff/20001/chrome/test/nacl/nacl_br...
chrome/test/nacl/nacl_browsertest_util.cc:191: static void
AddPnaclDisabledParm(const base::FilePath::StringType& url,
Can remove this now.

https://codereview.chromium.org/392023002/diff/20001/chrome/test/nacl/nacl_br...
chrome/test/nacl/nacl_browsertest_util.cc:257:
AddPnaclDisabledParm(test_file_with_pnacl, &test_file_with_both);
Could potentially simplify 
chrome/test/data//nacl/pnacl_mime_type/pnacl_mime_type.html

which was testing for this url param.

[teravest, 2014-07-16 01:38:47]

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

https://codereview.chromium.org/392023002/diff/1/chrome/common/chrome_content...
chrome/common/chrome_content_client.cc:176: if (skip_nacl_file_check ||
base::PathExists(path)) {
On 2014/07/15 23:24:15, jvoung wrote:
> On 2014/07/15 23:08:51, teravest wrote:
> > On 2014/07/15 22:04:16, jvoung wrote:
> > > It seems like we need to be careful about registering the mime_type once
the
> > > trusted plugin is gone.
> > > 
> > > The PathExists() check seems to serve as detecting if the gyp_defines
> did/did
> > > not compile in NaCl support.
> > 
> > I'd expect the behavior for registering the nacl mime type to be the same as
> the
> > pnacl mime type. Is there something I'm misunderstanding?
> 
> 
> You're right that registering the nacl mime type should be the same as
> registering the pnacl mime type. I think this code is fine as is.
> 
> I'm just saying that when there isn't a "FILE_NACL_PLUGIN", the code might
have
> be more careful and check #ifdef DISABLE_NACL or something.

Ah, okay.

https://codereview.chromium.org/392023002/diff/20001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/392023002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:683: "disable-pnacl",  // FLAGS:RECORD_UMA
On 2014/07/15 23:24:15, jvoung wrote:
> Might also have to mark the tools/metrics/actions/actions.xml entry as
obsolete?

Done.

https://codereview.chromium.org/392023002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:684: IDS_FLAGS_DISABLE_PNACL_NAME,
On 2014/07/15 23:24:15, jvoung wrote:
> I think you also want to remove IDS_FLAGS_DISABLE_PNACL_NAME, and the
> DESCRIPTION

Done.

https://codereview.chromium.org/392023002/diff/20001/chrome/test/nacl/nacl_br...
File chrome/test/nacl/nacl_browsertest_util.cc (left):

https://codereview.chromium.org/392023002/diff/20001/chrome/test/nacl/nacl_br...
chrome/test/nacl/nacl_browsertest_util.cc:191: static void
AddPnaclDisabledParm(const base::FilePath::StringType& url,
On 2014/07/15 23:24:15, jvoung wrote:
> Can remove this now.

Done.

https://codereview.chromium.org/392023002/diff/20001/chrome/test/nacl/nacl_br...
chrome/test/nacl/nacl_browsertest_util.cc:257:
AddPnaclDisabledParm(test_file_with_pnacl, &test_file_with_both);
On 2014/07/15 23:24:15, jvoung wrote:
> Could potentially simplify 
> chrome/test/data//nacl/pnacl_mime_type/pnacl_mime_type.html
> 
> which was testing for this url param.

Done.

[teravest, 2014-07-16 16:31:57]

The CQ bit was checked by teravest@chromium.org

[teravest, 2014-07-16 16:31:59]

The CQ bit was unchecked by teravest@chromium.org

[teravest, 2014-07-16 16:56:35]

+ncbray for chrome/test/nacl
+jochen for the rest of chrome

[jochen (gone - plz use gerrit), 2014-07-17 07:19:00]

lgtm

[teravest, 2014-07-21 15:47:23]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-07-21 15:48:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/392023002/100001

[commit-bot: I haz the power, 2014-07-21 17:45:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-21 17:51:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-21 17:51:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[teravest, 2014-07-21 17:59:41]

+asvitkine for tools/metrics/actions/actions.xml

[teravest, 2014-07-21 18:00:07]

Nick,

Mind taking a look? I still need owners approval for
	chrome/test/data/nacl/pnacl_mime_type/pnacl_mime_type.html

[Alexei Svitkine (slow), 2014-07-21 18:50:00]

histograms lgtm

https://codereview.chromium.org/392023002/diff/100001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/392023002/diff/100001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:82: <obsolete>This flag no longer
exists.</obsolete>
Nit: Mention when the change took place (i.e. a date or Chrome version).

[teravest, 2014-07-23 16:11:30]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-07-23 16:12:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/392023002/100001

[commit-bot: I haz the power, 2014-07-23 18:39:37]

Change committed as 284997