Move creation and ownership of ResourceDispatcherHost and PluginService to content. This gives a ...

Move creation and ownership of ResourceDispatcherHost and PluginService to content. This gives a few benefits:
-avoid having each embedder know when to create/destruct these objects, as well as contained objects (i.e. those related to downloads)
-avoid having to tell embedders about specifics of BrowserThread startup/shutdown
-move ResourceDispatcherHost's getter to content where it belongs

I've taken out the DnsParallelism field trial (not used anymore, confirmed with jar) as it was the only thing that caused MetricsService to depend on IOThread initialization, which also depended on MetricsService (through FieldTrials). This two-sided dependency always annoyed me and made the code hard to restructure.

BUG=98716
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117078
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117171

[Jói, 2012-01-10 10:58:57]

LGTM, very nice, great to see BrowserMainParts become simpler again.

Cheers,
Jói

http://codereview.chromium.org/9150016/diff/10121/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9150016/diff/10121/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:301:
g_resource_dispatcher_host = new ResourceDispatcherHost();
The assignment is not needed, since the ResourceDispatcherHost constructor
already assigns the new object to g_resource_dispatcher_host.  Should perhaps
just have a DCHECK here that it became non-NULL.

[jam, 2012-01-10 17:34:08]

http://codereview.chromium.org/9150016/diff/10121/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9150016/diff/10121/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:301:
g_resource_dispatcher_host = new ResourceDispatcherHost();
On 2012/01/10 10:58:57, Jói wrote:
> The assignment is not needed, since the ResourceDispatcherHost constructor
> already assigns the new object to g_resource_dispatcher_host.  Should perhaps
> just have a DCHECK here that it became non-NULL.

Done.

[Jói, 2012-01-10 18:30:40]

LGTM

On Tue, Jan 10, 2012 at 5:34 PM,  <jam@chromium.org> wrote:
>
>
http://codereview.chromium.org/9150016/diff/10121/content/browser/renderer_ho...
> File content/browser/renderer_host/resource_dispatcher_host.cc (right):
>
>
http://codereview.chromium.org/9150016/diff/10121/content/browser/renderer_ho...
> content/browser/renderer_host/resource_dispatcher_host.cc:301:
> g_resource_dispatcher_host = new ResourceDispatcherHost();
> On 2012/01/10 10:58:57, Jói wrote:
>>
>> The assignment is not needed, since the ResourceDispatcherHost
>
> constructor
>>
>> already assigns the new object to g_resource_dispatcher_host.  Should
>
> perhaps
>>
>> just have a DCHECK here that it became non-NULL.
>
>
> Done.
>
> http://codereview.chromium.org/9150016/

[jam, 2012-01-11 00:58:06]

This patch got reverted. I've uploaded the reverted version as ps10. I gave up
on trying to make the getter for RDH create it, since too many tests depended on
that behavior implicitly. Instead I create RDH in content explicitly, and that
allows me to get rid of the two hacks in chrome_browser_main.cc and
extension_service.cc which tried to create RDH.

[Jói, 2012-01-11 10:32:34]

Latest patchset LGTM

[Finnur, 2012-01-18 17:11:06]

http://codereview.chromium.org/9150016/diff/11168/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/9150016/diff/11168/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1313:
browser_process_->PreCreateThreads();
I believe this CL caused a crasher in the toast code due to early return around
lines 1223 above.

Run chrome.exe with --try-chrome-again=10001 and Chrome will... KABOOM!

A cursory glance revealed that io_thread_ (normally created as a result of
PreCreateThreads) is not created and so it crashes when it is first used
(safebrowsing in my case).

I'm writing a test to catch this in the future, but we need to figure out what
to do with the crash before I can enable it.

[jam, 2012-01-18 17:31:47]

thanks for the repro, i'll take a look

On Wed, Jan 18, 2012 at 9:11 AM, <finnur@chromium.org> wrote:

>
> http://codereview.chromium.**org/9150016/diff/11168/chrome/**
>
browser/chrome_browser_main.cc<http://codereview.chromium.org/9150016/diff/11168/chrome/browser/chrome_browser_main.cc>
> File chrome/browser/chrome_browser_**main.cc (right):
>
> http://codereview.chromium.**org/9150016/diff/11168/chrome/**
>
browser/chrome_browser_main.**cc#newcode1313<http://codereview.chromium.org/9150016/diff/11168/chrome/browser/chrome_browser_main.cc#newcode1313>
> chrome/browser/chrome_browser_**main.cc:1313:
> browser_process_->**PreCreateThreads();
> I believe this CL caused a crasher in the toast code due to early return
> around lines 1223 above.
>
> Run chrome.exe with --try-chrome-again=10001 and Chrome will... KABOOM!
>
> A cursory glance revealed that io_thread_ (normally created as a result
> of PreCreateThreads) is not created and so it crashes when it is first
> used (safebrowsing in my case).
>
> I'm writing a test to catch this in the future, but we need to figure
> out what to do with the crash before I can enable it.
>
>
http://codereview.chromium.**org/9150016/<http://codereview.chromium.org/9150...
>

[Jói, 2012-01-18 17:32:00]

What does the crash look like, is somebody using IOThread after we
return from the function?  It might be reasonable to just set the
desired return value and return at the end after more initialization
has been done, if that initialization is required.

Cheers,
Jói


On Wed, Jan 18, 2012 at 5:11 PM,  <finnur@chromium.org> wrote:
>
>
http://codereview.chromium.org/9150016/diff/11168/chrome/browser/chrome_brows...
> File chrome/browser/chrome_browser_main.cc (right):
>
>
http://codereview.chromium.org/9150016/diff/11168/chrome/browser/chrome_brows...
> chrome/browser/chrome_browser_main.cc:1313:
> browser_process_->PreCreateThreads();
> I believe this CL caused a crasher in the toast code due to early return
> around lines 1223 above.
>
> Run chrome.exe with --try-chrome-again=10001 and Chrome will... KABOOM!
>
> A cursory glance revealed that io_thread_ (normally created as a result
> of PreCreateThreads) is not created and so it crashes when it is first
> used (safebrowsing in my case).
>
> I'm writing a test to catch this in the future, but we need to figure
> out what to do with the crash before I can enable it.
>
> http://codereview.chromium.org/9150016/