Move NaCl related switches to their own file.

Move NaCl related switches to their own file.

This is in preparation for making NaCl files a browser component.
TBR=cpu@chromium.org
BUG=244791

[yael.aharon1, 2013-05-29 15:56:41]

Could you please review?
thanks!

[Mark Seaborn, 2013-05-29 16:03:13]

https://codereview.chromium.org/15957008/diff/1/chrome/nacl/nacl_switches.cc
File chrome/nacl/nacl_switches.cc (right):

https://codereview.chromium.org/15957008/diff/1/chrome/nacl/nacl_switches.cc#...
chrome/nacl/nacl_switches.cc:13: const char kDisableBundledPpapiFlash[]      =
"disable-bundled-ppapi-flash";
This has nothing to do with NaCl.  Flash doesn't run under NaCl.

Same for the other "ppapi-flash" options below.

https://codereview.chromium.org/15957008/diff/1/chrome/nacl/nacl_switches.cc#...
chrome/nacl/nacl_switches.cc:71: }  // namespace switches
Nit: Add empty line before this for consistency with 'namespace switches {'
above

[yael.aharon1, 2013-05-29 16:10:18]

On 2013/05/29 16:03:13, Mark Seaborn wrote:
> https://codereview.chromium.org/15957008/diff/1/chrome/nacl/nacl_switches.cc
> File chrome/nacl/nacl_switches.cc (right):
> 
>
https://codereview.chromium.org/15957008/diff/1/chrome/nacl/nacl_switches.cc#...
> chrome/nacl/nacl_switches.cc:13: const char kDisableBundledPpapiFlash[]      =
> "disable-bundled-ppapi-flash";
> This has nothing to do with NaCl.  Flash doesn't run under NaCl.
> 
> Same for the other "ppapi-flash" options below.
> 
>
https://codereview.chromium.org/15957008/diff/1/chrome/nacl/nacl_switches.cc#...
> chrome/nacl/nacl_switches.cc:71: }  // namespace switches
> Nit: Add empty line before this for consistency with 'namespace switches {'
> above

The title might be misleading but I think both pepper and NaCl code could be
moved into a component. There are files such as ppb_nacl_private_impl.cc/h that
seem to me to belong in both. Does that make sense to you?

[yael.aharon1, 2013-05-29 16:13:37]

Added the empty missing line

https://codereview.chromium.org/15957008/diff/1/chrome/nacl/nacl_switches.cc
File chrome/nacl/nacl_switches.cc (right):

https://codereview.chromium.org/15957008/diff/1/chrome/nacl/nacl_switches.cc#...
chrome/nacl/nacl_switches.cc:71: }  // namespace switches
On 2013/05/29 16:03:13, Mark Seaborn wrote:
> Nit: Add empty line before this for consistency with 'namespace switches {'
> above

Done.

[yael.aharon1, 2013-05-29 17:33:07]

I took out the changes I did in the ppapi switches. Those can be handled
separately if needed.

[dmichael (off chromium), 2013-05-30 05:00:44]

https://codereview.chromium.org/15957008/diff/8001/chrome/nacl/nacl_switches.h
File chrome/nacl/nacl_switches.h (right):

https://codereview.chromium.org/15957008/diff/8001/chrome/nacl/nacl_switches....
chrome/nacl/nacl_switches.h:5: // Defines all the command-line switches used by
Chrome.
Please update this comment

https://codereview.chromium.org/15957008/diff/8001/chrome/nacl/nacl_switches....
chrome/nacl/nacl_switches.h:13: #include
"content/public/common/content_switches.h"
Is this include necessary?

[yael.aharon1, 2013-05-30 13:50:32]

Updated based on your comments. Thanks.

https://codereview.chromium.org/15957008/diff/8001/chrome/nacl/nacl_switches.h
File chrome/nacl/nacl_switches.h (right):

https://codereview.chromium.org/15957008/diff/8001/chrome/nacl/nacl_switches....
chrome/nacl/nacl_switches.h:5: // Defines all the command-line switches used by
Chrome.
On 2013/05/30 05:00:44, dmichael wrote:
> Please update this comment

Done.

https://codereview.chromium.org/15957008/diff/8001/chrome/nacl/nacl_switches....
chrome/nacl/nacl_switches.h:13: #include
"content/public/common/content_switches.h"
On 2013/05/30 05:00:44, dmichael wrote:
> Is this include necessary?

Removed

[yael.aharon1, 2013-05-30 13:50:32]

Updated based on your comments. Thanks.

https://codereview.chromium.org/15957008/diff/8001/chrome/nacl/nacl_switches.h
File chrome/nacl/nacl_switches.h (right):

https://codereview.chromium.org/15957008/diff/8001/chrome/nacl/nacl_switches....
chrome/nacl/nacl_switches.h:5: // Defines all the command-line switches used by
Chrome.
On 2013/05/30 05:00:44, dmichael wrote:
> Please update this comment

Done.

https://codereview.chromium.org/15957008/diff/8001/chrome/nacl/nacl_switches....
chrome/nacl/nacl_switches.h:13: #include
"content/public/common/content_switches.h"
On 2013/05/30 05:00:44, dmichael wrote:
> Is this include necessary?

Removed

[dmichael (off chromium), 2013-05-30 15:35:19]

one more nit, o/w lgtm

https://codereview.chromium.org/15957008/diff/16001/chrome/nacl/nacl_switches.h
File chrome/nacl/nacl_switches.h (right):

https://codereview.chromium.org/15957008/diff/16001/chrome/nacl/nacl_switches...
chrome/nacl/nacl_switches.h:12: #include "base/base_switches.h"
Actually, I don't see why this is necessary...

[yael.aharon1, 2013-05-30 16:08:27]

https://codereview.chromium.org/15957008/diff/16001/chrome/nacl/nacl_switches.h
File chrome/nacl/nacl_switches.h (right):

https://codereview.chromium.org/15957008/diff/16001/chrome/nacl/nacl_switches...
chrome/nacl/nacl_switches.h:12: #include "base/base_switches.h"
On 2013/05/30 15:35:19, dmichael wrote:
> Actually, I don't see why this is necessary...
You are right, this is not necessary. I copied this from chrome_switches.h, but
I doubt that chrome_switches.h needs it either.
Removed this line.

[commit-bot: I haz the power, 2013-05-30 16:10:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/15957008/27001

[commit-bot: I haz the power, 2013-05-30 16:24:36]

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

[yael.aharon1, 2013-05-30 17:45:20]

Could you please review this? thanks!

[James Hawkins, 2013-05-30 17:47:54]

On 2013/05/30 17:45:20, yael.aharon1 wrote:
> Could you please review this? thanks!

I'm not sure at which point you added 20 reviewers to this CL, but you should
note that is not an acceptable practice.  You should find the smallest set of
reviewers from OWNERS files that satisfy the OWNERS requirements.  Then make
sure each OWNER knows which files they are to review.

[Nico, 2013-05-30 17:47:57]

Who? For most people on the review, this email looks like a mail with 30
people in the To: line and a message body of "Could you please review this?
thanks!"


On Thu, May 30, 2013 at 10:45 AM, <yael.aharon@chromium.org> wrote:

> Could you please review this? thanks!
>
>
https://codereview.chromium.**org/15957008/<https://codereview.chromium.org/1...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[Nico, 2013-05-30 17:47:57]

Who? For most people on the review, this email looks like a mail with 30
people in the To: line and a message body of "Could you please review this?
thanks!"


On Thu, May 30, 2013 at 10:45 AM, <yael.aharon@chromium.org> wrote:

> Could you please review this? thanks!
>
>
https://codereview.chromium.**org/15957008/<https://codereview.chromium.org/1...
>

[yael.aharon1, 2013-05-30 18:13:43]

On 2013/05/30 17:47:54, James Hawkins wrote:
> On 2013/05/30 17:45:20, yael.aharon1 wrote:
> > Could you please review this? thanks!
> 
> I'm not sure at which point you added 20 reviewers to this CL, but you should
> note that is not an acceptable practice.  You should find the smallest set of
> reviewers from OWNERS files that satisfy the OWNERS requirements.  Then make
> sure each OWNER knows which files they are to review.

Sorry for adding so many people. I am having difficulty understanding who are
the OWNERS of chrome/browser. Once I understand how that works, I will follow
your suggestion.

[yael.aharon1, 2013-05-30 21:17:28]

Some background for this CL can be found in
https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/pepper-dev/...

As suggested by jhawkins, I divided the files to a small subset of the owners:
cpu@chromium.org
    chrome/app/DEPS
    chrome/app/chrome_main_delegate.cc
    chrome/app/nacl_fork_delegate_linux.cc

jhawkins@chromium.org
    chrome/browser/DEPS
    chrome/browser/chrome_browser_main.cc
    chrome/common/DEPS
    chrome/common/nacl_cmd_line.cc
    chrome/renderer/DEPS
    chrome/renderer/chrome_content_renderer_client.cc
    chrome/browser/ui/webui/nacl_ui.cc

jvoung@chromium.org
    chrome/browser/component_updater/pnacl/pnacl_component_installer.cc

bauerb@chromium.org
    chrome/browser/content_settings/content_settings_browsertest.cc
    chrome/browser/plugins/plugin_prefs.cc

mseaborn@chromium.org
    chrome/browser/nacl_host/nacl_process_host.cc
    chrome/browser/nacl_host/test/gdb_debug_stub_browsertest.cc
    chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc
    chrome/nacl/nacl_switches.cc
    chrome/nacl/nacl_switches.h
    chrome/test/nacl/nacl_browsertest_util.cc

cbentzel@chromium.org
    chrome/browser/prerender/prerender_browsertest.cc

Thank you for your cooperation.

[Bernhard Bauer, 2013-05-30 22:43:38]

chrome/browser/plugins and chrome/browser/content_settings LGTM.

I didn't look at the other files, but if it's just changing includes, in general
feel free to TBR the OWNERS.

[yael.aharon1, 2013-05-30 23:17:27]

On 2013/05/30 22:43:38, Bernhard Bauer wrote:
> chrome/browser/plugins and chrome/browser/content_settings LGTM.
> 
> I didn't look at the other files, but if it's just changing includes, in
general
> feel free to TBR the OWNERS.

TBR? Sorry for the newbie question :)

[jvoung (off chromium), 2013-05-30 23:42:41]

LGTM for chrome/browser/component_updater/pnacl/pnacl_component_installer.cc

[Bernhard Bauer, 2013-05-30 23:51:46]

TBR = to be reviewed (by). It means that a change can be reviewed after it
has been landed instead of before, so the tools will allow you to land it.

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[Bernhard Bauer, 2013-05-30 23:51:47]

TBR = to be reviewed (by). It means that a change can be reviewed after it
has been landed instead of before, so the tools will allow you to land it.

[yael.aharon1, 2013-05-30 23:59:49]

On 2013/05/30 23:51:47, Bernhard Bauer wrote:
> TBR = to be reviewed (by). It means that a change can be reviewed after it
> has been landed instead of before, so the tools will allow you to land it.

Thanks!

[yael.aharon1, 2013-05-30 23:59:49]

On 2013/05/30 23:51:47, Bernhard Bauer wrote:
> TBR = to be reviewed (by). It means that a change can be reviewed after it
> has been landed instead of before, so the tools will allow you to land it.

Thanks!

[James Hawkins, 2013-05-31 00:21:00]

lgtm

[Mark Seaborn, 2013-05-31 01:41:44]

On 30 May 2013 14:17, <yael.aharon@chromium.org> wrote:

> mseaborn@chromium.org
>     chrome/browser/nacl_host/nacl_**process_host.cc
>     chrome/browser/nacl_host/test/**gdb_debug_stub_browsertest.cc
>     chrome/browser/nacl_host/test/**nacl_gdb_browsertest.cc
>     chrome/nacl/nacl_switches.cc
>     chrome/nacl/nacl_switches.h
>     chrome/test/nacl/nacl_browsertest_util.cc
>

LGTM for those files.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[Mark Seaborn, 2013-05-31 01:41:46]

On 30 May 2013 14:17, <yael.aharon@chromium.org> wrote:

> mseaborn@chromium.org
>     chrome/browser/nacl_host/nacl_**process_host.cc
>     chrome/browser/nacl_host/test/**gdb_debug_stub_browsertest.cc
>     chrome/browser/nacl_host/test/**nacl_gdb_browsertest.cc
>     chrome/nacl/nacl_switches.cc
>     chrome/nacl/nacl_switches.h
>     chrome/test/nacl/nacl_browsertest_util.cc
>

LGTM for those files.

Mark

[cbentzel, 2013-05-31 02:06:21]

prerender_browsertest LGTM

https://codereview.chromium.org/15957008/diff/27001/chrome/nacl/nacl_switches.h
File chrome/nacl/nacl_switches.h (right):

https://codereview.chromium.org/15957008/diff/27001/chrome/nacl/nacl_switches...
chrome/nacl/nacl_switches.h:10: #include "build/build_config.h"
Why is this include needed?

[yael.aharon1, 2013-05-31 13:45:33]

On 2013/05/31 02:06:21, cbentzel wrote:
> prerender_browsertest LGTM
> 
>
https://codereview.chromium.org/15957008/diff/27001/chrome/nacl/nacl_switches.h
> File chrome/nacl/nacl_switches.h (right):
> 
>
https://codereview.chromium.org/15957008/diff/27001/chrome/nacl/nacl_switches...
> chrome/nacl/nacl_switches.h:10: #include "build/build_config.h"
> Why is this include needed?

Took it out since it is not strictly needed

[jam, 2013-06-03 17:31:29]

https://codereview.chromium.org/15957008/diff/47001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/15957008/diff/47001/chrome/renderer/DEPS#newc...
chrome/renderer/DEPS:3: "+chrome/nacl",
not lgtm

sub directories of chrome correspond to different processes. they can only
include code from chrome/common. they can't include code from each other's
directories.

[jam, 2013-06-03 17:32:48]

https://codereview.chromium.org/15957008/diff/47001/chrome/common/DEPS
File chrome/common/DEPS (right):

https://codereview.chromium.org/15957008/diff/47001/chrome/common/DEPS#newcode4
chrome/common/DEPS:4: "+chrome/nacl",
ditto

[yael.aharon1, 2013-06-03 17:39:48]

On 2013/06/03 17:32:48, jam wrote:
> https://codereview.chromium.org/15957008/diff/47001/chrome/common/DEPS
> File chrome/common/DEPS (right):
> 
>
https://codereview.chromium.org/15957008/diff/47001/chrome/common/DEPS#newcode4
> chrome/common/DEPS:4: "+chrome/nacl",
> ditto

This CL is the first step towards moving all the nacl code out of src/chrome.
Once I break all the dependencies, I will move chrome/nacl to components/nacl. 
This was discussed in
https://groups.google.com/a/chromium.org/forum/?fromgroups#%21topic/pepper-de...
.

I thought it would be better to do this in many small steps rather than one huge
step. Are you suggesting that I should do it in one big step?

[jam, 2013-06-03 17:43:29]

On 2013/06/03 17:39:48, yael.aharon1 wrote:
> On 2013/06/03 17:32:48, jam wrote:
> > https://codereview.chromium.org/15957008/diff/47001/chrome/common/DEPS
> > File chrome/common/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/15957008/diff/47001/chrome/common/DEPS#newcode4
> > chrome/common/DEPS:4: "+chrome/nacl",
> > ditto
> 
> This CL is the first step towards moving all the nacl code out of src/chrome.
> Once I break all the dependencies, I will move chrome/nacl to components/nacl.

> This was discussed in
>
https://groups.google.com/a/chromium.org/forum/?fromgroups#%2521topic/pepper-...
> .
> 
> I thought it would be better to do this in many small steps rather than one
huge
> step. Are you suggesting that I should do it in one big step?

I see, I wasn't aware that this is temporary. If it is, that's fine. Please add
comments to the two DEPS files that you edited that this is temporary, and link
to the bug.

lgtm

[commit-bot: I haz the power, 2013-06-03 18:10:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/15957008/57001

[cpu_(ooo_6.6-7.5), 2013-06-20 22:47:05]

lgtm