Add (not yet working) content_browsertests target.

Add (not yet working) content_browsertests target.















This implements ContentBrowserTest and a transparent switch







between content and chrome browser tests.















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

[jam, 2011-09-26 23:45:19]

(I believe Rietveld annoyingly autocompleted jam@ to simonjam@, switching to me.
I'll look into why Rietveld is doing this)

[jam, 2011-09-27 01:07:19]

very cool, thanks for working on this

I'm not that familiar with how browsertests work, so I can't review in depth,
but overall this looks good. so all the deleted code wasn't used?

[sky, 2011-09-27 15:31:52]

http://codereview.chromium.org/8036044/diff/1/content/test/out_of_proc_test_r...
File content/test/out_of_proc_test_runner.cc (right):

http://codereview.chromium.org/8036044/diff/1/content/test/out_of_proc_test_r...
content/test/out_of_proc_test_runner.cc:24: #include
"content/test/browser_test.h"
Are these classes landing separately?

[Paweł Hajdan Jr., 2011-09-27 19:06:30]

Oops no, I just forgot to svn the files. PTAL.

[jam, 2011-09-27 19:11:35]

http://codereview.chromium.org/8036044/diff/6002/content/test/browser_test.h
File content/test/browser_test.h (right):

http://codereview.chromium.org/8036044/diff/6002/content/test/browser_test.h#...
content/test/browser_test.h:17: #if defined(BROWSER_TESTS_USE_CONTENT)
instead of having each test binary having to define a macro, and the code in
content to hardcode a mapping from that macro to a header to include, why not
have the macro be the source to the header?

[Paweł Hajdan Jr., 2011-09-27 20:05:18]

http://codereview.chromium.org/8036044/diff/6002/content/test/browser_test.h
File content/test/browser_test.h (right):

http://codereview.chromium.org/8036044/diff/6002/content/test/browser_test.h#...
content/test/browser_test.h:17: #if defined(BROWSER_TESTS_USE_CONTENT)
On 2011/09/27 19:11:35, John Abd-El-Malek wrote:
> instead of having each test binary having to define a macro, and the code in
> content to hardcode a mapping from that macro to a header to include, why not
> have the macro be the source to the header?

Do you mean something like:

defines: [
  'BROWSER_TESTS_HEADER': 'chrome/test/base/in_process_browser_test.h'
]

? I thought about this, but the macros are also used in
out_of_proc_test_runner.cc to exclude chrome-specific code. I'd like to get rid
of those chrome-specific bits, but it'd probably need more of the content shell
in place.

[jam, 2011-09-27 20:20:23]

On 2011/09/27 20:05:18, Paweł Hajdan Jr. wrote:
> http://codereview.chromium.org/8036044/diff/6002/content/test/browser_test.h
> File content/test/browser_test.h (right):
> 
>
http://codereview.chromium.org/8036044/diff/6002/content/test/browser_test.h#...
> content/test/browser_test.h:17: #if defined(BROWSER_TESTS_USE_CONTENT)
> On 2011/09/27 19:11:35, John Abd-El-Malek wrote:
> > instead of having each test binary having to define a macro, and the code in
> > content to hardcode a mapping from that macro to a header to include, why
not
> > have the macro be the source to the header?
> 
> Do you mean something like:
> 
> defines: [
>   'BROWSER_TESTS_HEADER': 'chrome/test/base/in_process_browser_test.h'
> ]
> 
> ? I thought about this, but the macros are also used in
> out_of_proc_test_runner.cc to exclude chrome-specific code. I'd like to get
rid
> of those chrome-specific bits, but it'd probably need more of the content
shell
> in place.

it seems a bit of a hack to declare this ifdef, and have code in content include
stuff from chrome and do chrome specific behavior there. i haven't looked in
detail, but can't we accomplish this like how we normally do things with
content/chrome, i.e. through interfaces or callbacks?

[jam, 2011-09-27 20:20:50]

also our goal should be that we can get rid of chrome\test deps rule from
content

[Paweł Hajdan Jr., 2011-09-29 20:42:16]

PTAL, I've dropped the chrome dependency.

[jam, 2011-09-29 21:21:36]

http://codereview.chromium.org/8036044/diff/13001/chrome/test/base/chrome_tes...
File chrome/test/base/chrome_test_launcher.cc (right):

http://codereview.chromium.org/8036044/diff/13001/chrome/test/base/chrome_tes...
chrome/test/base/chrome_test_launcher.cc:46: // TODO(pkasting): This
"single_process vs. single-process" design is terrible
it seems that we would want to share all the sandbox and single-process code
with all embedders that want to have tests?

http://codereview.chromium.org/8036044/diff/13001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/8036044/diff/13001/content/content_tests.gypi#...
content/content_tests.gypi:218: 'BROWSER_TESTS_USE_CONTENT',
not needed anymore?

http://codereview.chromium.org/8036044/diff/13001/content/content_tests.gypi#...
content/content_tests.gypi:223: 'test/content_test_launcher.cc',
did you forget to upload this?

http://codereview.chromium.org/8036044/diff/13001/content/test/browser_test_b...
File content/test/browser_test_base.cc (right):

http://codereview.chromium.org/8036044/diff/13001/content/test/browser_test_b...
content/test/browser_test_base.cc:34:
host_resolver_->AddSimulatedFailure("*.google.com");
do we need this in the core content code or can we keep it in chrome? it seems
that in moving tests to content we can avoid having stuff like this be brought
in, since we'd catch the test if it's ported

[Paweł Hajdan Jr., 2011-09-29 22:55:30]

PTAL

http://codereview.chromium.org/8036044/diff/13001/chrome/test/base/chrome_tes...
File chrome/test/base/chrome_test_launcher.cc (right):

http://codereview.chromium.org/8036044/diff/13001/chrome/test/base/chrome_tes...
chrome/test/base/chrome_test_launcher.cc:46: // TODO(pkasting): This
"single_process vs. single-process" design is terrible
On 2011/09/29 21:21:37, John Abd-El-Malek wrote:
> it seems that we would want to share all the sandbox and single-process code
> with all embedders that want to have tests?

Not sure yet. I don't want to be speculative, so my plan is to try to unify this
when I get content_browsertests to run (preferably in separate CL for less risky
landing).

http://codereview.chromium.org/8036044/diff/13001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/8036044/diff/13001/content/content_tests.gypi#...
content/content_tests.gypi:218: 'BROWSER_TESTS_USE_CONTENT',
On 2011/09/29 21:21:37, John Abd-El-Malek wrote:
> not needed anymore?

Done.

http://codereview.chromium.org/8036044/diff/13001/content/content_tests.gypi#...
content/content_tests.gypi:223: 'test/content_test_launcher.cc',
On 2011/09/29 21:21:37, John Abd-El-Malek wrote:
> did you forget to upload this?

Initially yes, it is in more recent patchsets.

http://codereview.chromium.org/8036044/diff/13001/content/test/browser_test_b...
File content/test/browser_test_base.cc (right):

http://codereview.chromium.org/8036044/diff/13001/content/test/browser_test_b...
content/test/browser_test_base.cc:34:
host_resolver_->AddSimulatedFailure("*.google.com");
On 2011/09/29 21:21:37, John Abd-El-Malek wrote:
> do we need this in the core content code or can we keep it in chrome? it seems
> that in moving tests to content we can avoid having stuff like this be brought
> in, since we'd catch the test if it's ported

Done.

[Dirk Pranke, 2011-09-29 22:58:53]

incidental review comment ... I'm "this close" to enabling component build
for content (hopefully landing this afternoon) and something tells me adding
content_browsertests will need more exports to happen. Can we coordinate
when you land this?

-- Dirk

On Thu, Sep 29, 2011 at 3:55 PM, <phajdan.jr@chromium.org> wrote:

> PTAL
>
>
>
> http://codereview.chromium.**org/8036044/diff/13001/chrome/**
>
test/base/chrome_test_**launcher.cc<http://codereview.chromium.org/8036044/diff/13001/chrome/test/base/chrome_test_launcher.cc>
> File chrome/test/base/chrome_test_**launcher.cc (right):
>
> http://codereview.chromium.**org/8036044/diff/13001/chrome/**
>
test/base/chrome_test_**launcher.cc#newcode46<http://codereview.chromium.org/8036044/diff/13001/chrome/test/base/chrome_test_launcher.cc#newcode46>
> chrome/test/base/chrome_test_**launcher.cc:46: // TODO(pkasting): This
> "single_process vs. single-process" design is terrible
> On 2011/09/29 21:21:37, John Abd-El-Malek wrote:
>
>> it seems that we would want to share all the sandbox and
>>
> single-process code
>
>> with all embedders that want to have tests?
>>
>
> Not sure yet. I don't want to be speculative, so my plan is to try to
> unify this when I get content_browsertests to run (preferably in
> separate CL for less risky landing).
>
>
> http://codereview.chromium.**org/8036044/diff/13001/**
>
content/content_tests.gypi<http://codereview.chromium.org/8036044/diff/13001/content/content_tests.gypi>
> File content/content_tests.gypi (right):
>
> http://codereview.chromium.**org/8036044/diff/13001/**
>
content/content_tests.gypi#**newcode218<http://codereview.chromium.org/8036044/diff/13001/content/content_tests.gypi#newcode218>
> content/content_tests.gypi:**218: 'BROWSER_TESTS_USE_CONTENT',
> On 2011/09/29 21:21:37, John Abd-El-Malek wrote:
>
>> not needed anymore?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8036044/diff/13001/**
>
content/content_tests.gypi#**newcode223<http://codereview.chromium.org/8036044/diff/13001/content/content_tests.gypi#newcode223>
> content/content_tests.gypi:**223: 'test/content_test_launcher.**cc',
> On 2011/09/29 21:21:37, John Abd-El-Malek wrote:
>
>> did you forget to upload this?
>>
>
> Initially yes, it is in more recent patchsets.
>
>
> http://codereview.chromium.**org/8036044/diff/13001/**
>
content/test/browser_test_**base.cc<http://codereview.chromium.org/8036044/diff/13001/content/test/browser_test_base.cc>
> File content/test/browser_test_**base.cc (right):
>
> http://codereview.chromium.**org/8036044/diff/13001/**
>
content/test/browser_test_**base.cc#newcode34<http://codereview.chromium.org/8036044/diff/13001/content/test/browser_test_base.cc#newcode34>
> content/test/browser_test_**base.cc:34:
> host_resolver_->**AddSimulatedFailure("*.google.**com <http://google.com>
> ");
> On 2011/09/29 21:21:37, John Abd-El-Malek wrote:
>
>> do we need this in the core content code or can we keep it in chrome?
>>
> it seems
>
>> that in moving tests to content we can avoid having stuff like this be
>>
> brought
>
>> in, since we'd catch the test if it's ported
>>
>
> Done.
>
>
>
http://codereview.chromium.**org/8036044/<http://codereview.chromium.org/8036...
>

[jam, 2011-09-29 23:07:06]

http://codereview.chromium.org/8036044/diff/13001/chrome/test/base/chrome_tes...
File chrome/test/base/chrome_test_launcher.cc (right):

http://codereview.chromium.org/8036044/diff/13001/chrome/test/base/chrome_tes...
chrome/test/base/chrome_test_launcher.cc:46: // TODO(pkasting): This
"single_process vs. single-process" design is terrible
On 2011/09/29 22:55:30, Paweł Hajdan Jr. wrote:
> On 2011/09/29 21:21:37, John Abd-El-Malek wrote:
> > it seems that we would want to share all the sandbox and single-process code
> > with all embedders that want to have tests?
> 
> Not sure yet. I don't want to be speculative, so my plan is to try to unify
this
> when I get content_browsertests to run (preferably in separate CL for less
risky
> landing).

ok sounds good to move this later. you'll definitely want to share this code
though.

http://codereview.chromium.org/8036044/diff/13028/chrome/test/base/chrome_tes...
File chrome/test/base/chrome_test_launcher.cc (right):

http://codereview.chromium.org/8036044/diff/13028/chrome/test/base/chrome_tes...
chrome/test/base/chrome_test_launcher.cc:46: // TODO(pkasting): This
"single_process vs. single-process" design is terrible
nit: 80 chars, here and below

http://codereview.chromium.org/8036044/diff/13028/content/test/content_test_l...
File content/test/content_test_launcher.cc (right):

http://codereview.chromium.org/8036044/diff/13028/content/test/content_test_l...
content/test/content_test_launcher.cc:26: // TODO(pkasting): This
"single_process vs. single-process" design is terrible
why copy this code now?

[jam, 2011-09-29 23:07:32]

nit: also please fix the cl description. not sure why it keeps getting newlines?

[Paweł Hajdan Jr., 2011-09-29 23:21:47]

About CL description: I already fixed it about 3 times. There seems to be a
ridiculous tools bug that inserts those newlines every time I change the CL or
maybe upload it. I plan to do a final fix right before commit.

http://codereview.chromium.org/8036044/diff/13028/chrome/test/base/chrome_tes...
File chrome/test/base/chrome_test_launcher.cc (right):

http://codereview.chromium.org/8036044/diff/13028/chrome/test/base/chrome_tes...
chrome/test/base/chrome_test_launcher.cc:46: // TODO(pkasting): This
"single_process vs. single-process" design is terrible
On 2011/09/29 23:07:06, John Abd-El-Malek wrote:
> nit: 80 chars, here and below

Done.

http://codereview.chromium.org/8036044/diff/13028/content/test/content_test_l...
File content/test/content_test_launcher.cc (right):

http://codereview.chromium.org/8036044/diff/13028/content/test/content_test_l...
content/test/content_test_launcher.cc:26: // TODO(pkasting): This
"single_process vs. single-process" design is terrible
On 2011/09/29 23:07:06, John Abd-El-Malek wrote:
> why copy this code now?

I think that's simpler than adding even more indirection or ambiguously named
flags. When I get this to work and there is possibility to de-duplicate, I'm
going to do that.

[jam, 2011-09-29 23:32:39]

lgtm

[jam, 2011-09-29 23:33:58]

http://codereview.chromium.org/8036044/diff/13028/content/test/content_test_l...
File content/test/content_test_launcher.cc (right):

http://codereview.chromium.org/8036044/diff/13028/content/test/content_test_l...
content/test/content_test_launcher.cc:26: // TODO(pkasting): This
"single_process vs. single-process" design is terrible
On 2011/09/29 23:21:47, Paweł Hajdan Jr. wrote:
> On 2011/09/29 23:07:06, John Abd-El-Malek wrote:
> > why copy this code now?
> 
> I think that's simpler than adding even more indirection or ambiguously named
> flags. When I get this to work and there is possibility to de-duplicate, I'm
> going to do that.

I think we have to share this code, given how fragile and complicated it is.
This is what we did with ChromeMain, where it was split into ContentMain and
ChromeMain.

[sky, 2011-09-29 23:40:20]

LGTM