Add nacl-gdb-script switch.

Add nacl-gdb-script switch. 
It points to a script that will be executed by nacl-gdb at startup.

BUG=117010
TEST= browser_tests --gtest_filter=NaClGdbScript*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139524

[Mark Seaborn, 2012-05-24 17:25:57]

You could set BUG= to http://code.google.com/p/chromium/issues/detail?id=117010.

Thanks for adding a test, and sorry to be picky about it. :-)

http://codereview.chromium.org/10447012/diff/1/chrome/app/generated_resources...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10447012/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5106: +	  <message
name="IDS_FLAGS_NACL_GDB_SCRIPT_NAME" desc="Name of the 'Script executed by
Native Client application debugger at startup' lab.">
The indentation looks wrong here.  (But for some reason, Rietveld is not showing
the side-by-side view.)

http://codereview.chromium.org/10447012/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5107: +	    Script executed by Native Client
application debugger at startup.
"Script to be executed by nacl-gdb at startup"?  Since this is a GDB-specific
script you should mention GDB.

I don't understand how this string differs in use from the next one though.

http://codereview.chromium.org/10447012/diff/1/chrome/browser/nacl_host/nacl_...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10447012/diff/1/chrome/browser/nacl_host/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:315: const FilePath script =
CommandLine::ForCurrentProcess()->GetSwitchValuePath(
Nit: 'const' seems unnecessary for a simple, non-reference variable and doesn't
match manifest_path above.

http://codereview.chromium.org/10447012/diff/1/chrome/browser/nacl_host/test/...
File chrome/browser/nacl_host/test/mock_nacl_gdb.cc (right):

http://codereview.chromium.org/10447012/diff/1/chrome/browser/nacl_host/test/...
chrome/browser/nacl_host/test/mock_nacl_gdb.cc:68: if (strcmp(argv[i], kCommand)
== 0) {
It looks to me that if no --command arg is provided, the test will still pass. 
There's no equivalent of has_attach_cmd in this case.

http://codereview.chromium.org/10447012/diff/1/chrome/browser/nacl_host/test/...
File chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc (right):

http://codereview.chromium.org/10447012/diff/1/chrome/browser/nacl_host/test/...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:38: // TODO(halyavin):
Make this test to work on Windows 32-bit. Currently this
"Make this test work"

http://codereview.chromium.org/10447012/diff/1/chrome/browser/nacl_host/test/...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:84:
NaClGdbTest::RunWithNaClGdb("Empty");
I don't like this use of inheritance.  It's kind of confusing and would make
extending this test further difficult.

Having a separate test case to cover the new arg seems unnecessary.  Maybe just
add the script arg test into the existing test case?

http://codereview.chromium.org/10447012/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10447012/diff/1/chrome/common/chrome_switches....
chrome/common/chrome_switches.cc:850: // GDB script that will be executed by
nacl-gdb debugger at startup.
Nit: "by the nacl-gdb debugger"

[halyavin, 2012-05-25 09:37:45]

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/app/generated_r...
chrome/app/generated_resources.grd:5106: +	  <message
name="IDS_FLAGS_NACL_GDB_SCRIPT_NAME" desc="Name of the 'Script executed by
Native Client application debugger at startup' lab.">
On 2012/05/24 17:25:58, Mark Seaborn wrote:
> The indentation looks wrong here.  (But for some reason, Rietveld is not
showing
> the side-by-side view.)
Replaced tab with spaces.

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/app/generated_r...
chrome/app/generated_resources.grd:5107: +	    Script executed by Native Client
application debugger at startup.
On 2012/05/24 17:25:58, Mark Seaborn wrote:
> "Script to be executed by nacl-gdb at startup"?  Since this is a GDB-specific
> script you should mention GDB.
> 
> I don't understand how this string differs in use from the next one though.
DESCRIPTION is supposed to be more detailed.

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/browser/nacl_ho...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/browser/nacl_ho...
chrome/browser/nacl_host/nacl_process_host.cc:315: const FilePath script =
CommandLine::ForCurrentProcess()->GetSwitchValuePath(
On 2012/05/24 17:25:58, Mark Seaborn wrote:
> Nit: 'const' seems unnecessary for a simple, non-reference variable and
doesn't
> match manifest_path above.

Done.

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/browser/nacl_ho...
File chrome/browser/nacl_host/test/mock_nacl_gdb.cc (right):

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/browser/nacl_ho...
chrome/browser/nacl_host/test/mock_nacl_gdb.cc:68: if (strcmp(argv[i], kCommand)
== 0) {
On 2012/05/24 17:25:58, Mark Seaborn wrote:
> It looks to me that if no --command arg is provided, the test will still pass.

> There's no equivalent of has_attach_cmd in this case.

Done.

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/browser/nacl_ho...
File chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/browser/nacl_ho...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:38: // TODO(halyavin):
Make this test to work on Windows 32-bit. Currently this
On 2012/05/24 17:25:58, Mark Seaborn wrote:
> "Make this test work"

Done.

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/browser/nacl_ho...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:84:
NaClGdbTest::RunWithNaClGdb("Empty");
On 2012/05/24 17:25:58, Mark Seaborn wrote:
> I don't like this use of inheritance.  It's kind of confusing and would make
> extending this test further difficult.
It will make extending it easier. All tests could extend NaClGdbTest. But we can
always separate the main part of test later if we need more that 2 tests.
> 
> Having a separate test case to cover the new arg seems unnecessary.  Maybe
just
> add the script arg test into the existing test case?

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10447012/diff/1/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:850: // GDB script that will be executed by
nacl-gdb debugger at startup.
On 2012/05/24 17:25:58, Mark Seaborn wrote:
> Nit: "by the nacl-gdb debugger"

Done.

[halyavin, 2012-05-28 09:21:23]

PTAL

[Mark Seaborn, 2012-05-29 18:19:44]

LGTM

http://codereview.chromium.org/10447012/diff/3005/chrome/browser/nacl_host/te...
File chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc (right):

http://codereview.chromium.org/10447012/diff/3005/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:33:
command_line->AppendSwitchASCII("nacl-gdb-script", script_.AsUTF8Unsafe());
You use AppendSwitchPath() above.  Can't it be the same here?  Also, you use
switches::kNaClGdb above; can you use switches::kNaClGdbScript here?

http://codereview.chromium.org/10447012/diff/3005/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10447012/diff/3005/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:850: // GDB script that will be executed by the
nacl-gdb debugger at startup.
Nit: maybe "GDB script to pass to the nacl-gdb debugger at startup"?  Otherwise
"that will be" suggests that this codebase is implementing the scripting.

[halyavin, 2012-05-30 08:12:04]

http://codereview.chromium.org/10447012/diff/3005/chrome/browser/nacl_host/te...
File chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc (right):

http://codereview.chromium.org/10447012/diff/3005/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:33:
command_line->AppendSwitchASCII("nacl-gdb-script", script_.AsUTF8Unsafe());
On 2012/05/29 18:19:44, Mark Seaborn wrote:
> You use AppendSwitchPath() above.  Can't it be the same here?  
I tried to support non-latin TMP directories. But looks like it doesn't work.
> Also, you use
> switches::kNaClGdb above; can you use switches::kNaClGdbScript here?
Done

http://codereview.chromium.org/10447012/diff/3005/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10447012/diff/3005/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:850: // GDB script that will be executed by the
nacl-gdb debugger at startup.
On 2012/05/29 18:19:44, Mark Seaborn wrote:
> Nit: maybe "GDB script to pass to the nacl-gdb debugger at startup"? 
Otherwise
> "that will be" suggests that this codebase is implementing the scripting.

Done.

[commit-bot: I haz the power, 2012-05-30 10:32:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/halyavin@google.com/10447012/12003

[commit-bot: I haz the power, 2012-05-30 11:41:16]

Change committed as 139524