Test for --nacl-gdb functionality.

Test for --nacl-gdb functionality.
I expanded access rights for NaCl loader (launched under nacl-gdb) process
handle from QueryLimitedInformation to QueryInformation because Windows XP
doesn't support QueryLimitedInformation.
However, this test is switched off on Windows 32-bit due to problems with
allocating 1Gb of address space. Future nacl-gdb must have special launching
code to solve this problem.

BUG=117010
TEST= browser_tests.exe --gtest_filter=NaClGdbTest.* (on Windows)

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

[halyavin, 2012-03-12 15:24:59]

mseaborn - full review.
brettw - need LGTM due to OWNERS file. I added empty test so that I can check
just that NaCl is working.

[Mark Seaborn, 2012-03-13 01:05:19]

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
File chrome/browser/nacl_host/mock_nacl_gdb.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
I think this file belongs in a directory with "test" in its name. 
"nacl_host/test/*" perhaps?

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.cc:18: return 1;
Having different numbers for each failure is weird.  Can you just use CHECK()
for each check instead, assuming it works OK in this context?

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
File chrome/browser/nacl_host/mock_nacl_gdb.gyp (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.gyp:6: ['disable_nacl==0 and OS=="win"',
{
Why Windows-only?  I was under the impression that your change to
nacl_process_host.cc worked for Linux too, and your nacl64-gdb works on Linux.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_gdb_browsertest.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_gdb_browsertest.cc:11: static const
FilePath::CharType kMockNaClGdb[] =
You could move this line outside of the #if.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_gdb_browsertest.cc:26:
command_line->AppendSwitch(switches::kNoSandbox);
I thought this gets added automatically when using --nacl-gdb?

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_gdb_browsertest.cc:30:
command_line->AppendSwitchPath(switches::kNaClGdb, mock_nacl_gdb);
What happens if Chromium ignores the --nacl-gdb argument?  How does this test
assert that nacl-gdb gets invokved?

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:282: // If nacl-gdb switch is not
empty, NaCl loader have been launched
'have' -> 'has'

Aha, --nacl-gdb already got broken by my bug fix!  Not intentional, I assure
you. :-)  But it demonstrates the need for this test.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:285: switches::kNaClGdb).empty())
{
This line needs indenting

http://codereview.chromium.org/9662055/diff/2023/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:2964: ['OS=="win"', {
Again, why Windows-only?

http://codereview.chromium.org/9662055/diff/2023/ppapi/tests/test_empty.h
File ppapi/tests/test_empty.h (right):

http://codereview.chromium.org/9662055/diff/2023/ppapi/tests/test_empty.h#new...
ppapi/tests/test_empty.h:4: #ifndef PPAPI_TESTS_TEST_EMPTY_H_
Add empty line before this

[brettw, 2012-03-13 05:57:49]

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
File chrome/browser/nacl_host/mock_nacl_gdb.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.cc:32: char* irtFileName =
&argv[2][strlen(kNaClIrt)];
style: irt_file_name (no caps in var names)

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.cc:33: FILE* irtFile = fopen(irtFileName,
"r");
Ditto

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:5: #include "chrome/test/ui/ppapi_uitest.h"
Unless I missed something, I don't see any new tests in this file.

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:308: BuildQuery(const std::string& base, const
std::string& test_case) {
BuildQuery should be on the previous line, then the args on the following one
indented 4 spaces.

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest.h
File chrome/test/ui/ppapi_uitest.h (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.h:76: #endif  // CHROME_TEST_UI_PPAPI_UITEST_H_
Blank like before this.

http://codereview.chromium.org/9662055/diff/2023/ppapi/tests/test_empty.cc
File ppapi/tests/test_empty.cc (right):

http://codereview.chromium.org/9662055/diff/2023/ppapi/tests/test_empty.cc#ne...
ppapi/tests/test_empty.cc:21: std::string TestEmpty::TestNaClLoad() {
Can you add a quick comment here about why this exists?

[halyavin, 2012-03-13 12:48:27]

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
File chrome/browser/nacl_host/mock_nacl_gdb.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/03/13 01:05:20, Mark Seaborn wrote:
> I think this file belongs in a directory with "test" in its name. 
> "nacl_host/test/*" perhaps?

Done.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.cc:18: return 1;
On 2012/03/13 01:05:20, Mark Seaborn wrote:
> Having different numbers for each failure is weird.  Can you just use CHECK()
> for each check instead, assuming it works OK in this context?

Done.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.cc:32: char* irtFileName =
&argv[2][strlen(kNaClIrt)];
On 2012/03/13 05:57:49, brettw wrote:
> style: irt_file_name (no caps in var names)

Done.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.cc:33: FILE* irtFile = fopen(irtFileName,
"r");
On 2012/03/13 05:57:49, brettw wrote:
> Ditto

Done.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
File chrome/browser/nacl_host/mock_nacl_gdb.gyp (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.gyp:6: ['disable_nacl==0 and OS=="win"',
{
On 2012/03/13 01:05:20, Mark Seaborn wrote:
> Why Windows-only?  I was under the impression that your change to
> nacl_process_host.cc worked for Linux too, and your nacl64-gdb works on Linux.
The change to nacl_process_host.cc doesn't work under Linux. I am investigating
this problem.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_gdb_browsertest.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_gdb_browsertest.cc:11: static const
FilePath::CharType kMockNaClGdb[] =
On 2012/03/13 01:05:20, Mark Seaborn wrote:
> You could move this line outside of the #if.

Done.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_gdb_browsertest.cc:26:
command_line->AppendSwitch(switches::kNoSandbox);
On 2012/03/13 01:05:20, Mark Seaborn wrote:
> I thought this gets added automatically when using --nacl-gdb?
Sorry, I knew that this should be removed but forgot to do it.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:282: // If nacl-gdb switch is not
empty, NaCl loader have been launched
On 2012/03/13 01:05:20, Mark Seaborn wrote:
> 'have' -> 'has'
> 
> Aha, --nacl-gdb already got broken by my bug fix!  Not intentional, I assure
> you. :-)  But it demonstrates the need for this test.

Done.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:285: switches::kNaClGdb).empty())
{
On 2012/03/13 01:05:20, Mark Seaborn wrote:
> This line needs indenting

Done.

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:5: #include "chrome/test/ui/ppapi_uitest.h"
On 2012/03/13 05:57:49, brettw wrote:
> Unless I missed something, I don't see any new tests in this file.
There is no new tests here. But I use this code in a new test located in a
separate file (since this new test is not a PPAPI test). So I have to separate
declaration and implementation in this file.

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:308: BuildQuery(const std::string& base, const
std::string& test_case) {
On 2012/03/13 05:57:49, brettw wrote:
> BuildQuery should be on the previous line, then the args on the following one
> indented 4 spaces.

Done.

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest.h
File chrome/test/ui/ppapi_uitest.h (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.h:76: #endif  // CHROME_TEST_UI_PPAPI_UITEST_H_
On 2012/03/13 05:57:49, brettw wrote:
> Blank like before this.

Done.

http://codereview.chromium.org/9662055/diff/2023/ppapi/tests/test_empty.cc
File ppapi/tests/test_empty.cc (right):

http://codereview.chromium.org/9662055/diff/2023/ppapi/tests/test_empty.cc#ne...
ppapi/tests/test_empty.cc:21: std::string TestEmpty::TestNaClLoad() {
On 2012/03/13 05:57:49, brettw wrote:
> Can you add a quick comment here about why this exists?

Done.

http://codereview.chromium.org/9662055/diff/2023/ppapi/tests/test_empty.h
File ppapi/tests/test_empty.h (right):

http://codereview.chromium.org/9662055/diff/2023/ppapi/tests/test_empty.h#new...
ppapi/tests/test_empty.h:4: #ifndef PPAPI_TESTS_TEST_EMPTY_H_
On 2012/03/13 01:05:20, Mark Seaborn wrote:
> Add empty line before this

Done.

[brettw, 2012-03-13 16:15:40]

Oh dud, I missed the other test file. LGTM when Mark is happy.

[Mark Seaborn, 2012-03-13 17:06:26]

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
File chrome/browser/nacl_host/mock_nacl_gdb.gyp (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/moc...
chrome/browser/nacl_host/mock_nacl_gdb.gyp:6: ['disable_nacl==0 and OS=="win"',
{
On 2012/03/13 12:48:27, halyavin wrote:
> On 2012/03/13 01:05:20, Mark Seaborn wrote:
> > Why Windows-only?  I was under the impression that your change to
> > nacl_process_host.cc worked for Linux too, and your nacl64-gdb works on
Linux.
>
> The change to nacl_process_host.cc doesn't work under Linux. I am
> investigating this problem.

OK, in the mean time please add TODOs next to the 'OS=="win"' cases.

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_gdb_browsertest.cc (right):

http://codereview.chromium.org/9662055/diff/2023/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_gdb_browsertest.cc:30:
command_line->AppendSwitchPath(switches::kNaClGdb, mock_nacl_gdb);
On 2012/03/13 01:05:20, Mark Seaborn wrote:
> What happens if Chromium ignores the --nacl-gdb argument?
> How does this test assert that nacl-gdb gets invokved?

Andrey, you didn't answer this question.  This is the main thing I don't
understand about this change.

http://codereview.chromium.org/9662055/diff/11005/ppapi/tests/test_empty.cc
File ppapi/tests/test_empty.cc (right):

http://codereview.chromium.org/9662055/diff/11005/ppapi/tests/test_empty.cc#n...
ppapi/tests/test_empty.cc:9: // This test checks only that NaCl module loads
successfully.
'the NaCl module'

http://codereview.chromium.org/9662055/diff/11005/ppapi/tests/test_empty.cc#n...
ppapi/tests/test_empty.cc:10: // This is needed when NaCl module is launched
under debugger and so we want
'the debugger'

http://codereview.chromium.org/9662055/diff/11005/ppapi/tests/test_empty.cc#n...
ppapi/tests/test_empty.cc:11: // check only the launching sequence.
'to check'

[halyavin, 2012-03-13 18:54:22]

http://codereview.chromium.org/9662055/diff/11005/ppapi/tests/test_empty.cc
File ppapi/tests/test_empty.cc (right):

http://codereview.chromium.org/9662055/diff/11005/ppapi/tests/test_empty.cc#n...
ppapi/tests/test_empty.cc:9: // This test checks only that NaCl module loads
successfully.
On 2012/03/13 17:06:27, Mark Seaborn wrote:
> 'the NaCl module'

Done.

http://codereview.chromium.org/9662055/diff/11005/ppapi/tests/test_empty.cc#n...
ppapi/tests/test_empty.cc:10: // This is needed when NaCl module is launched
under debugger and so we want
On 2012/03/13 17:06:27, Mark Seaborn wrote:
> 'the debugger'

Done.

http://codereview.chromium.org/9662055/diff/11005/ppapi/tests/test_empty.cc#n...
ppapi/tests/test_empty.cc:11: // check only the launching sequence.
On 2012/03/13 17:06:27, Mark Seaborn wrote:
> 'to check'

Done.

[Mark Seaborn, 2012-03-14 23:33:13]

You're getting there, but you need to fix the trybot failure first.

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/na...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:282: // If nacl-gdb switch is not
empty, NaCl loader has been launched
"the nacl-gdb switch"

"the NaCl loader"

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:283: // without broker and so we
shouldn't inform broker about its termination.
"the broker"

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:607:
base::kProcessAccessQueryInformation |
Please document this change in the commit message.

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
File chrome/browser/nacl_host/test/mock_nacl_gdb.cc (right):

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/mock_nacl_gdb.cc:20:
file_util::WriteFile(kMockNaClGdbFile, NULL, 0);
Shouldn't this go just before the LaunchProcess() call?  You should also comment
that this marks the gdb test as passed.

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
File chrome/browser/nacl_host/test/mock_nacl_gdb.gyp (right):

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/mock_nacl_gdb.gyp:6: ['disable_nacl==0 and
OS=="win"', {
Again: Please add TODOs next to the 'OS=="win"' cases.

However, your try job failed with:
LaunchProcess: failed to execvp:
/b/build/slave/mac/build/src/out/Release/Chromium.app/Contents/MacOS/mock_nacl_gdb
- from
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/8338/s...

So the test does seem to be being run on Mac.

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

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:40:
EXPECT_TRUE(file_util::PathExists(mock_nacl_gdb_file));
This file seems to be going into the current directory, which is presumably the
Chromium source dir.  I don't like that.

How about this:
 * Create an empty temp file (using an interface in 'base').
 * Set an env var with the temp file's pathname.
 * Have the mock gdb write a string to the temp file.
 * Assert that the temp file contains the string.
 * Delete the file; unset the env var.

http://codereview.chromium.org/9662055/diff/15001/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9662055/diff/15001/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:4: 
I'd do the change to ppapi_uitest.cc in a separate commit, given that this
change is taking a while and this might be getting out of sync.

[halyavin, 2012-03-15 15:31:36]

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/na...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:282: // If nacl-gdb switch is not
empty, NaCl loader has been launched
On 2012/03/14 23:33:14, Mark Seaborn wrote:
> "the nacl-gdb switch"
> 
> "the NaCl loader"

Done.

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:283: // without broker and so we
shouldn't inform broker about its termination.
On 2012/03/14 23:33:14, Mark Seaborn wrote:
> "the broker"

Done.

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:607:
base::kProcessAccessQueryInformation |
On 2012/03/14 23:33:14, Mark Seaborn wrote:
> Please document this change in the commit message.

Done.

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
File chrome/browser/nacl_host/test/mock_nacl_gdb.cc (right):

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/mock_nacl_gdb.cc:20:
file_util::WriteFile(kMockNaClGdbFile, NULL, 0);
On 2012/03/14 23:33:14, Mark Seaborn wrote:
> Shouldn't this go just before the LaunchProcess() call?  You should also
comment
> that this marks the gdb test as passed.
No. The file marks that mock_nacl_gdb is launched. The rest is checked by
waiting PASS from NaCl loader. If mock_nacl_gdb will not be launched, we will
see timeout and assertion failure in the test. If something will go wrong, we
will see only timeout.

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
File chrome/browser/nacl_host/test/mock_nacl_gdb.gyp (right):

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/mock_nacl_gdb.gyp:6: ['disable_nacl==0 and
OS=="win"', {
On 2012/03/14 23:33:14, Mark Seaborn wrote:
> Again: Please add TODOs next to the 'OS=="win"' cases.
> 
> However, your try job failed with:
> LaunchProcess: failed to execvp:
>
/b/build/slave/mac/build/src/out/Release/Chromium.app/Contents/MacOS/mock_nacl_gdb
> - from
>
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/8338/s...
> 
> So the test does seem to be being run on Mac.

Done.

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

http://codereview.chromium.org/9662055/diff/15001/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:40:
EXPECT_TRUE(file_util::PathExists(mock_nacl_gdb_file));
On 2012/03/14 23:33:14, Mark Seaborn wrote:
> This file seems to be going into the current directory, which is presumably
the
> Chromium source dir. 
No, it is src/build/Debug directory (src/out/Debug or linux). It doesn't have
any source files. All executables and test files like (test.html) go there.

[Mark Seaborn, 2012-03-16 17:28:59]

http://codereview.chromium.org/9662055/diff/17007/chrome/browser/nacl_host/te...
File chrome/browser/nacl_host/test/mock_nacl_gdb.gyp (right):

http://codereview.chromium.org/9662055/diff/17007/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/mock_nacl_gdb.gyp:6: #TODO(halyavin): Implement
this test for linux.
Spacing: "# TODO..."  Capitalise "Linux"

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

http://codereview.chromium.org/9662055/diff/17007/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:49:
env->SetVar("MOCK_NACL_GDB", mock_nacl_gdb_file.AsUTF8Unsafe());
Can you unset the env var afterwards so that it doesn't leak into subsequent
tests?

http://codereview.chromium.org/9662055/diff/17007/chrome/browser/nacl_host/te...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:51:
EXPECT_TRUE(file_util::PathExists(mock_nacl_gdb_file));
Surely the file will exist regardless of whether the mock nacl-gdb got run,
because CreateTemporaryFile() creates it?  As I suggested before, you should
have the mock nacl-gdb write a string to the file, and check for that string
here.

[Mark Seaborn, 2012-03-16 17:47:11]

LGTM if you fix the issues from this message and the previous one.  Thanks for
persevering!

http://codereview.chromium.org/9662055/diff/17007/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9662055/diff/17007/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:160: // TestServer expects a path relative to
source. So we must first
You've changed the whitespace with your move here.  There was a newline before
this line before.

[halyavin, 2012-03-16 17:57:42]

https://chromiumcodereview.appspot.com/9662055/diff/17007/chrome/browser/nacl...
File chrome/browser/nacl_host/test/mock_nacl_gdb.gyp (right):

https://chromiumcodereview.appspot.com/9662055/diff/17007/chrome/browser/nacl...
chrome/browser/nacl_host/test/mock_nacl_gdb.gyp:6: #TODO(halyavin): Implement
this test for linux.
On 2012/03/16 17:28:59, Mark Seaborn wrote:
> Spacing: "# TODO..."  Capitalise "Linux"

Done.

https://chromiumcodereview.appspot.com/9662055/diff/17007/chrome/browser/nacl...
File chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9662055/diff/17007/chrome/browser/nacl...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:49:
env->SetVar("MOCK_NACL_GDB", mock_nacl_gdb_file.AsUTF8Unsafe());
On 2012/03/16 17:28:59, Mark Seaborn wrote:
> Can you unset the env var afterwards so that it doesn't leak into subsequent
> tests?

Done.

https://chromiumcodereview.appspot.com/9662055/diff/17007/chrome/browser/nacl...
chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc:51:
EXPECT_TRUE(file_util::PathExists(mock_nacl_gdb_file));
On 2012/03/16 17:28:59, Mark Seaborn wrote:
> Surely the file will exist regardless of whether the mock nacl-gdb got run,
> because CreateTemporaryFile() creates it?  As I suggested before, you should
> have the mock nacl-gdb write a string to the file, and check for that string
> here.

Done.

https://chromiumcodereview.appspot.com/9662055/diff/17007/chrome/test/ui/ppap...
File chrome/test/ui/ppapi_uitest.cc (right):

https://chromiumcodereview.appspot.com/9662055/diff/17007/chrome/test/ui/ppap...
chrome/test/ui/ppapi_uitest.cc:160: // TestServer expects a path relative to
source. So we must first
On 2012/03/16 17:47:11, Mark Seaborn wrote:
> You've changed the whitespace with your move here.  There was a newline before
> this line before.

Done.

[commit-bot: I haz the power, 2012-03-19 07:25:12]

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

[commit-bot: I haz the power, 2012-03-19 07:25:16]

Presubmit check for 9662055-26009 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  ppapi/tests/test_empty.cc

Presubmit checks took 1.1s to calculate.

[commit-bot: I haz the power, 2012-03-19 08:48:01]

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

[commit-bot: I haz the power, 2012-03-19 11:19:46]

Try job failure for 9662055-31007 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-19 11:26:02]

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

[commit-bot: I haz the power, 2012-03-19 13:09:01]

Change committed as 127453