Makes it possible to run content_browsertests with --as-browser and fake WebRTC devices.

Makes it possible to run content_browsertests with --as-browser and fake WebRTC devices.

The first part of the patch makes it possible to launch content_browsertests with the --as-browser flag. This is essentially just merging the two Run methods for the content test runner and the chrome test runner.

The second part hooks up the content tests to use fake WebRTC devices. I'm not really happy with the use of a global variable in browser_main_loop, but the tricky thing is that the call needs to happen precisely where it does happen, e.g. after the IO thread has been created but before we start pumping messages. 

It's hard to hook in precisely at that spot. In fact, it's hard to even hook in after the browser main loop has been created from our vantage point in the content browser tests. Hooking into ShellBrowserMainParts::PreMainMessageLoopRun would have worked great, but we can't do our UseFakeDevice call there directly since that's in the content shell code, which can't reach that method due to deps rules.

We could override that method from the content browser tests though - we have control over the creation of the ShellMainDelegate. However, we would have to override our way through ShellContentBrowserClient to get to ShellBrowserMainParts, and I don't see a good way to do that given how the code is written currently. Calling down through these guys doesn't work either, since the content browser tests have already handed off control to the main content code when we need to make the call. Hence, this solution.

BUG=

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

[jam, 2012-09-04 15:18:14]

This looks good in general.

Can you separate out the --as-browser patch from the fake rtc stuff?

re the fake rtc, another option is to use a command line flag that your harness
can set. i.e. it's unclear to me that this should be done for every test?

http://codereview.chromium.org/10912070/diff/1/content/browser/browser_main_l...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10912070/diff/1/content/browser/browser_main_l...
content/browser/browser_main_loop.cc:231:
BrowserMainLoop::use_fake_media_stream_device_ = true;
nit: BrowserMainLoop:: is unnecessary

http://codereview.chromium.org/10912070/diff/1/content/browser/browser_main_l...
content/browser/browser_main_loop.cc:480: if (use_fake_media_stream_device_)
actually, it seems that this static better belongs in MediaStreamManager?

http://codereview.chromium.org/10912070/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10912070/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:725: // Check if the
embedder isn't interested in the notification.
nit: no need to comment this, per the style guide (i.e. no need to comment
something when it's obvious by looking at the code)

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
File content/public/test/test_launcher.cc (right):

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.cc:649: extern const char kLaunchAsBrowser[] =
"as-browser";
nit: no extern

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.cc:657: bool launch_chrome =
nit: now that this code is in content, launch_chrome -> launch_main

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
File content/public/test/test_launcher.h (right):

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.h:37: extern const char kLaunchAsBrowser[];
nit: order

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.h:45: 
TestLauncherDelegate is an interface for the delegate of browser tests to
implement. if you have some shared code, the place to put it would be in
test_launcher.cc

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.h:55: virtual content::ContentMainDelegate*
CreateContentMainDelegate() = 0;
... and then this would be in the public section like the above methods

[phoglund_chromium, 2012-09-05 09:41:04]

True, the patches did turn out to be completely independent, so I will move the
RTC stuff to a separate patch.

This patch will just be the --as-browser patch going forward.

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
File content/public/test/test_launcher.cc (right):

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.cc:649: extern const char kLaunchAsBrowser[] =
"as-browser";
On 2012/09/04 15:18:14, John Abd-El-Malek wrote:
> nit: no extern

Done.

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.cc:657: bool launch_chrome =
On 2012/09/04 15:18:14, John Abd-El-Malek wrote:
> nit: now that this code is in content, launch_chrome -> launch_main

I split this method into two instead, so that variable disappeared.

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
File content/public/test/test_launcher.h (right):

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.h:37: extern const char kLaunchAsBrowser[];
On 2012/09/04 15:18:14, John Abd-El-Malek wrote:
> nit: order

Done.

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.h:45: 
On 2012/09/04 15:18:14, John Abd-El-Malek wrote:
> TestLauncherDelegate is an interface for the delegate of browser tests to
> implement. if you have some shared code, the place to put it would be in
> test_launcher.cc

Done.

http://codereview.chromium.org/10912070/diff/1/content/public/test/test_launc...
content/public/test/test_launcher.h:55: virtual content::ContentMainDelegate*
CreateContentMainDelegate() = 0;
On 2012/09/04 15:18:14, John Abd-El-Malek wrote:
> ... and then this would be in the public section like the above methods

Done.

[phoglund_chromium, 2012-09-05 11:43:54]

The new fake rtc patch is here: https://chromiumcodereview.appspot.com/10919095

http://codereview.chromium.org/10912070/diff/1/content/browser/browser_main_l...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10912070/diff/1/content/browser/browser_main_l...
content/browser/browser_main_loop.cc:231:
BrowserMainLoop::use_fake_media_stream_device_ = true;
On 2012/09/04 15:18:14, John Abd-El-Malek wrote:
> nit: BrowserMainLoop:: is unnecessary

Done.

http://codereview.chromium.org/10912070/diff/1/content/browser/browser_main_l...
content/browser/browser_main_loop.cc:480: if (use_fake_media_stream_device_)
On 2012/09/04 15:18:14, John Abd-El-Malek wrote:
> actually, it seems that this static better belongs in MediaStreamManager?
Good point, I'll move it.

http://codereview.chromium.org/10912070/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10912070/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:725: // Check if the
embedder isn't interested in the notification.
On 2012/09/04 15:18:14, John Abd-El-Malek wrote:
> nit: no need to comment this, per the style guide (i.e. no need to comment
> something when it's obvious by looking at the code)

Done.

[jam, 2012-09-05 15:26:49]

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

https://codereview.chromium.org/10912070/diff/8001/content/public/test/test_l...
content/public/test/test_launcher.cc:29: #include
"content/public/common/content_switches.h"
nit: order

https://codereview.chromium.org/10912070/diff/8001/content/public/test/test_l...
content/public/test/test_launcher.cc:649: const char kLaunchAsBrowser[] =
"as-browser";
nit: order

https://codereview.chromium.org/10912070/diff/8001/content/public/test/test_l...
content/public/test/test_launcher.cc:654: bool ShouldRunInBrowserMode() {
nit: this isn't just for running in browser, it's also for all child processes
(i.e. renderer), so to make it clearer, name this

ShouldRunMain(). and also below, RunMain()

https://codereview.chromium.org/10912070/diff/8001/content/public/test/test_l...
content/public/test/test_launcher.cc:717: if (ShouldRunInBrowserMode()) {
what's the point of splitting this into two functions, when they're called from
one place together?

nit: this file doesn't use brace brackets for one line statements

[phoglund_chromium, 2012-09-06 08:45:28]

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

https://codereview.chromium.org/10912070/diff/8001/content/public/test/test_l...
content/public/test/test_launcher.cc:29: #include
"content/public/common/content_switches.h"
On 2012/09/05 15:26:51, John Abd-El-Malek wrote:
> nit: order

Done.

https://codereview.chromium.org/10912070/diff/8001/content/public/test/test_l...
content/public/test/test_launcher.cc:649: const char kLaunchAsBrowser[] =
"as-browser";
On 2012/09/05 15:26:51, John Abd-El-Malek wrote:
> nit: order

Done. The other switches weren't in order so I sorted them. I assume you meant
alphabetical order?

https://codereview.chromium.org/10912070/diff/8001/content/public/test/test_l...
content/public/test/test_launcher.cc:654: bool ShouldRunInBrowserMode() {
On 2012/09/05 15:26:51, John Abd-El-Malek wrote:
> nit: this isn't just for running in browser, it's also for all child processes
> (i.e. renderer), so to make it clearer, name this
> 
> ShouldRunMain(). and also below, RunMain()
Ok, then maybe it should even be ShouldRunContentMain and RunContentMain since
that's what we are doing?

https://codereview.chromium.org/10912070/diff/8001/content/public/test/test_l...
content/public/test/test_launcher.cc:717: if (ShouldRunInBrowserMode()) {
On 2012/09/05 15:26:51, John Abd-El-Malek wrote:
> what's the point of splitting this into two functions, when they're called
from
> one place together?
> 
> nit: this file doesn't use brace brackets for one line statements
> 
I thought the code read better like this. The semantics of
launcher_delegate->Run() returning false seemed far from obvious to me - does it
mean it failed to run or that it doesn't want to run, or what? I think this is a
bit clearer.

[tommi (sloooow) - chröme, 2012-09-06 09:57:29]

lgtm

http://codereview.chromium.org/10912070/diff/7003/content/public/test/test_la...
File content/public/test/test_launcher.cc (right):

http://codereview.chromium.org/10912070/diff/7003/content/public/test/test_la...
content/public/test/test_launcher.cc:630: const char kChildProcessFlag[]   =
"child";
only one space on each side of =

http://codereview.chromium.org/10912070/diff/7003/content/public/test/test_la...
content/public/test/test_launcher.cc:633: const char kGTestHelpFlag[]   =
"gtest_help";
please fix here as well and for "help" + "single-process" + "single_process"
below.

[jam, 2012-09-06 16:24:02]

lgtm

[commit-bot: I haz the power, 2012-09-07 08:10:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phoglund@chromium.org/10912070/7003

[commit-bot: I haz the power, 2012-09-07 09:10:25]

Try job failure for 10912070-7003 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-09 09:24:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phoglund@chromium.org/10912070/7003

[commit-bot: I haz the power, 2012-09-09 10:10:58]

Try job failure for 10912070-7003 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-09-09 10:50:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phoglund@chromium.org/10912070/5013

[commit-bot: I haz the power, 2012-09-09 13:36:35]

Failed to apply patch for content/test/content_test_launcher.cc:
While running patch -p1 --forward --force;
patching file content/test/content_test_launcher.cc
Hunk #1 FAILED at 9.
Hunk #2 succeeded at 95 (offset 1 line).
Hunk #3 succeeded at 106 (offset 1 line).
1 out of 3 hunks FAILED -- saving rejects to file
content/test/content_test_launcher.cc.rej

[commit-bot: I haz the power, 2012-09-10 12:21:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phoglund@chromium.org/10912070/11003

[commit-bot: I haz the power, 2012-09-10 14:19:35]

Try job failure for 10912070-11003 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-09-10 15:35:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phoglund@chromium.org/10912070/11003

[commit-bot: I haz the power, 2012-09-10 17:51:53]

Change committed as 155760