Make CF net tests a content::BrowserMainParts.

This goes most of the way to re-enabling chrome_frame_net_tests by making it a content::BrowserMainParts.  The tests are still disabled until we are able to test them on IE8 and older; on IE9 all tests currently pass except for a few that hang.

Additionally, this makes chrome_frame_net_tests determine protocol version using the exact same version information as npchrome_frame.dll does, so that version mismatch due to the RC dependency problems we have sometimes seen should not happen any more with this test suite.

While in there, I also updated the poor_mans_trybot.bat script.

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

[Jói, 2011-12-09 20:57:34]

Hi guys,

Consider this a preview, if you have feedback at this point please send it along
so I can incorporate it into my cleanup on Monday morning.

At this point, all tests but the ones added to the filter pass on my machine,
and shutdown is clean up to the point where we call ExitProcess.  I've only
tested a debug component build so far, so I'll see what happens with shutdown in
release builds, but there should not be much left at that point.

Obviously I can't leave the "DO NOT SUBMIT" comment and change in
content/browser/browser_main_loop.cc, I'll see what the best way is to fix that.
 The issue is duplicate registration of a NetworkChangeNotifier.

Cheers,
Jói

[robertshield, 2011-12-12 04:05:49]

Huge thanks for doing this Jói :-) The approach looks sound to me, it looks like
the net tests are become a fairly small implementation of a /content-based app
which is great. 

The comment about teardown of webkit globals makes me think these tests may now
be running more code than they need (and more than before), but if that's the
price of coding to the content api then so be it. That will be infinitely more
stable than the previous approach of coding to no api at all.

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
File chrome_frame/test/net/fake_external_tab.cc (right):

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
chrome_frame/test/net/fake_external_tab.cc:74: char** g_argv = NULL;
nit: should mark these as static

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
chrome_frame/test/net/fake_external_tab.cc:185: // Initialize the content client
which that code uses to talk to Chrome.
nit: which code?

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
chrome_frame/test/net/fake_external_tab.cc:592:
reinterpret_cast<FakeBrowserProcessImpl*>(g_browser_process)->PreStartThread(
nit: why the casts here and below?

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
chrome_frame/test/net/fake_external_tab.cc:650: ExitProcess(test_result());
How much more of what is now content is being run in these tests now compared to
what they were doing before? Are the cf net tests now running something quite
similar to content_shell?

If we're now quite close to content shell, then cf_net_tests should be much more
stable over time than its previous attempt at starting up parts of Chrome in an
ad hoc way.

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/poor_mans_...
File chrome_frame/test/poor_mans_trybot.bat (right):

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/poor_mans_...
chrome_frame/test/poor_mans_trybot.bat:2: REM Copyright (c) 2009 The Chromium
Authors. All rights reserved.
nit: 2009 -> 2011

[Jói, 2011-12-12 14:09:02]

I've uploaded a quick patch to address review nits, and below are answers to a
couple of your questions.  Will follow up later with a fully polished change.

FYI, the webkit comment was already in the code, I just moved it around.

Cheers,
Jói

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
File chrome_frame/test/net/fake_external_tab.cc (right):

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
chrome_frame/test/net/fake_external_tab.cc:74: char** g_argv = NULL;
On 2011/12/12 04:05:49, robertshield wrote:
> nit: should mark these as static

Done.

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
chrome_frame/test/net/fake_external_tab.cc:185: // Initialize the content client
which that code uses to talk to Chrome.
On 2011/12/12 04:05:49, robertshield wrote:
> nit: which code?

Removed the comment, it added nothing to what you get from reading the line of
code.

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
chrome_frame/test/net/fake_external_tab.cc:592:
reinterpret_cast<FakeBrowserProcessImpl*>(g_browser_process)->PreStartThread(
On 2011/12/12 04:05:49, robertshield wrote:
> nit: why the casts here and below?

Because g_browser_process (a variable created in the Chrome code) is a
BrowserProcess, which does not have the methods we need to get at
(FakeBrowserProcessImpl inherits from BrowserProcessImpl, which does).  I have
consolidated the casts in a single static function in FakeBrowserProcessImpl and
added documentation.

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/net/fake_e...
chrome_frame/test/net/fake_external_tab.cc:650: ExitProcess(test_result());
On 2011/12/12 04:05:49, robertshield wrote:
> How much more of what is now content is being run in these tests now compared
to
> what they were doing before? Are the cf net tests now running something quite
> similar to content_shell?
> 
> If we're now quite close to content shell, then cf_net_tests should be much
more
> stable over time than its previous attempt at starting up parts of Chrome in
an
> ad hoc way.

I don't think a lot more code gets run than before, although I haven't
quantified it.

cf_net_tests is now just a content/ client and pretty close to content shell, so
yeah it should be a lot stabler.

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/poor_mans_...
File chrome_frame/test/poor_mans_trybot.bat (right):

http://codereview.chromium.org/8894008/diff/2001/chrome_frame/test/poor_mans_...
chrome_frame/test/poor_mans_trybot.bat:2: REM Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2011/12/12 04:05:49, robertshield wrote:
> nit: 2009 -> 2011

Done.

[Jói, 2011-12-12 16:03:49]

robertshield:  Main reviewer.
eroman:  net/OWNERS approval re net/base/net_test_suite.h/cc

Robert, I'd like to commit as-is with the early exit path still turned on, and
request that either you or Ananta try enabling the tests and running them
against IE8 or older before we remove it.  Everything now passes against IE9
except for a few tests that always or often hang, but per Ananta that is likely
a previously encountered issue and not related to my change.

Note that I updated the description of the change.

Cheers,
Jói

[robertshield, 2011-12-12 16:09:40]

lgtm

http://codereview.chromium.org/8894008/diff/8001/chrome_frame/test/net/fake_e...
File chrome_frame/test/net/fake_external_tab.cc (right):

http://codereview.chromium.org/8894008/diff/8001/chrome_frame/test/net/fake_e...
chrome_frame/test/net/fake_external_tab.cc:141: }
Slightly cleaner might be to expose FakeExternalTab's browser_process_ member
and use that (making it a FakeBrowserProcessImpl* while we're at it). But you
could legitimately call that nitpicking.

[Jói, 2011-12-12 16:38:02]

Please take another look.  I addressed your review nit, and fixed a link-time
issue that only occurs in non-component builds.

Cheers,
Jói

[ananta, 2011-12-12 18:24:26]

LGTM

[robertshield, 2011-12-12 18:30:11]

On 2011/12/12 16:38:02, Jói wrote:
> Please take another look.  I addressed your review nit, and fixed a link-time
> issue that only occurs in non-component builds.
> 
> Cheers,
> Jói

LGTM. Thanks again!

[eroman, 2011-12-12 20:29:27]

Doesn't look like the latest patchset includes net/base anymore, so my review
isn't necessary. Let me know if you need me to review anything, otherwise
removing myself.

[Jói, 2011-12-12 21:11:19]

Whoops, that was a couple of bad git cl uploads in a row.  I've
uploaded a clean patch.

Eric, I need your approval for net/base/net_test_suite.h and .cc

Cheers,
Joi


On Mon, Dec 12, 2011 at 8:29 PM,  <eroman@chromium.org> wrote:
> Doesn't look like the latest patchset includes net/base anymore, so my
> review
> isn't necessary. Let me know if you need me to review anything, otherwise
> removing myself.
>
> http://codereview.chromium.org/8894008/

[eroman, 2011-12-12 21:15:14]

lgtm for net/base/*