Fix handling of BROWSER_WRAPPER in the test launcher.

Fix handling of BROWSER_WRAPPER in the test launcher.

BUG=254476, 236893
R=sky@chromium.org, zhaoqin@google.com

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

[zhaoqin, 2013-06-26 19:03:28]

build failure

https://codereview.chromium.org/17910005/diff/1/base/test/test_launcher.cc
File base/test/test_launcher.cc (right):

https://codereview.chromium.org/17910005/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:353: const std::string& wrapper,
shall we add comment somewhere to indicate why we must pass wrapper instead of
calling PrependWrapper at caller.

https://codereview.chromium.org/17910005/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:372:
new_command_line.PrependWrapper(ASCIIToWide(wrapper));
src\base\test\test_launcher.cc(372) : error C3861: 'ASCIIToWide': identifier not
found

https://codereview.chromium.org/17910005/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:376: 
shall we move the VLOG here too?

[Paweł Hajdan Jr., 2013-06-26 19:59:49]

https://codereview.chromium.org/17910005/diff/1/base/test/test_launcher.cc
File base/test/test_launcher.cc (right):

https://codereview.chromium.org/17910005/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:353: const std::string& wrapper,
On 2013/06/26 19:03:28, zhaoqin wrote:
> shall we add comment somewhere to indicate why we must pass wrapper instead of
> calling PrependWrapper at caller.

Done.

https://codereview.chromium.org/17910005/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:372:
new_command_line.PrependWrapper(ASCIIToWide(wrapper));
On 2013/06/26 19:03:28, zhaoqin wrote:
> src\base\test\test_launcher.cc(372) : error C3861: 'ASCIIToWide': identifier
not
> found

Done.

https://codereview.chromium.org/17910005/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:376: 
On 2013/06/26 19:03:28, zhaoqin wrote:
> shall we move the VLOG here too?

IMHO not worth it. I'm happy to change though if anybody has a strong opinion.

[sky, 2013-06-26 21:24:20]

LGTM

https://codereview.chromium.org/17910005/diff/9001/base/test/test_launcher.h
File base/test/test_launcher.h (right):

https://codereview.chromium.org/17910005/diff/9001/base/test/test_launcher.h#...
base/test/test_launcher.h:76: const std::string& wrapper,
Please add a description of wrapper.

[zhaoqin, 2013-06-26 21:39:09]

LGTM

tested and works.

https://codereview.chromium.org/17910005/diff/9001/content/public/test/test_l...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/17910005/diff/9001/content/public/test/test_l...
content/public/test/test_launcher.cc:101: browser_wrapper ? browser_wrapper :
std::string(),
why not pass browser_wrapper directly and let the callee decide if it is NULL?
It seems this is because of using reference instead of pointer.

[Paweł Hajdan Jr., 2013-06-27 18:18:35]

https://codereview.chromium.org/17910005/diff/9001/base/test/test_launcher.h
File base/test/test_launcher.h (right):

https://codereview.chromium.org/17910005/diff/9001/base/test/test_launcher.h#...
base/test/test_launcher.h:76: const std::string& wrapper,
On 2013/06/26 21:24:20, sky wrote:
> Please add a description of wrapper.

Done.

https://codereview.chromium.org/17910005/diff/9001/content/public/test/test_l...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/17910005/diff/9001/content/public/test/test_l...
content/public/test/test_launcher.cc:101: browser_wrapper ? browser_wrapper :
std::string(),
On 2013/06/26 21:39:09, zhaoqin wrote:
> why not pass browser_wrapper directly and let the callee decide if it is NULL?
> It seems this is because of using reference instead of pointer.

I strongly prefer const std::string& over char* in interfaces.

I think this is fine. :)

[Paweł Hajdan Jr., 2013-06-27 23:53:51]

Committed patchset #3 manually as r209029 (presubmit successful).