Start moving code from BrowserMain to content, so that it can be reused by all embedders of conte...

Start moving code from BrowserMain to content, so that it can be reused by all embedders of content. I've based the refactoring on the existing BrowserMainParts. This is the first step; I didn't want to do it all at the same time because it would be too big. Remaining tasks:
-rename the browser_main files in chrome to chrome_browser_main
-move the OS specific implementations of BrowserMainParts that are needed for content
-finish splitting the remaining BrowserMain function (now that's in TemporaryContinue())

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

[viettrungluu, 2011-09-06 21:31:42]

If you can get this to build, pass tests, etc., then you have a
mostly-rubberstamp LGTM from me. Good luck!

http://codereview.chromium.org/7779040/diff/14003/chrome/browser/browser_main...
File chrome/browser/browser_main_mac.mm (right):

http://codereview.chromium.org/7779040/diff/14003/chrome/browser/browser_main...
chrome/browser/browser_main_mac.mm:91: // Tell Cooca to finish its
initalization, which we want to do manually
s/Cooca/Cocoa
s/initalization/initialization

(even if it's not your fault)

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
File content/common/content_switches.cc (right):

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
content/common/content_switches.cc:217: // Enables TLS cached info extension.
alphabetical order (well, match the order in the header)

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
File content/common/content_switches.h (right):

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
content/common/content_switches.h:55: extern const char kDisableSSLFalseStart[];
nit: alphabetical order

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
content/common/content_switches.h:68: extern const char
kEnableDNSCertProvenanceChecking[];
"

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
content/common/content_switches.h:77: extern const char kEnableSSLCachedInfo[];
"

[jam, 2011-09-06 21:50:41]

http://codereview.chromium.org/7779040/diff/14003/chrome/browser/browser_main...
File chrome/browser/browser_main_mac.mm (right):

http://codereview.chromium.org/7779040/diff/14003/chrome/browser/browser_main...
chrome/browser/browser_main_mac.mm:91: // Tell Cooca to finish its
initalization, which we want to do manually
On 2011/09/06 21:31:42, viettrungluu wrote:
> s/Cooca/Cocoa
> s/initalization/initialization
> 
> (even if it's not your fault)

Done.

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
File content/common/content_switches.cc (right):

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
content/common/content_switches.cc:217: // Enables TLS cached info extension.
On 2011/09/06 21:31:42, viettrungluu wrote:
> alphabetical order (well, match the order in the header)

Done.

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
File content/common/content_switches.h (right):

http://codereview.chromium.org/7779040/diff/14003/content/common/content_swit...
content/common/content_switches.h:55: extern const char kDisableSSLFalseStart[];
On 2011/09/06 21:31:42, viettrungluu wrote:
> nit: alphabetical order

(and below) in other places that i've seen this, they've used ascii sorting so
that S is before e etc

[satorux1, 2011-09-07 17:10:31]

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (left):

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main...
chrome/browser/browser_main.cc:2183: parts.reset(NULL);
The comment says the BrowserMainParts is explicitly released, before
CrosLibrary's shutdown. However, CrosLibrary's shutdown is now moved to
~BroserMainPartsChromeos. This is rather worrisome.


http://codereview.chromium.org/7779040/diff/11053/chrome/browser/chromeos/bro...

 55 BrowserMainPartsChromeos::~BrowserMainPartsChromeos() {
 56   if (!parameters().ui_task && chromeos::CrosLibrary::Get())
 57     chromeos::CrosLibrary::Shutdown();

[jam, 2011-09-07 17:37:48]

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (left):

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main...
chrome/browser/browser_main.cc:2183: parts.reset(NULL);
On 2011/09/07 17:10:31, satorux1 wrote:
> The comment says the BrowserMainParts is explicitly released, before
> CrosLibrary's shutdown. However, CrosLibrary's shutdown is now moved to
> ~BroserMainPartsChromeos. This is rather worrisome.

Do you have any specifics that worry you?

We need to be able to reorder startup/shutdown slightly as we refactor existing
code, otherwise we would have to add a lot of callbacks to keep the exact same
order, which may not be necessary at all. a lot, but not all of course, of the
code was put in whatever place where it worked, but that doesn't mean it can't
move around.

> 
> 
>
http://codereview.chromium.org/7779040/diff/11053/chrome/browser/chromeos/bro...
> 
>  55 BrowserMainPartsChromeos::~BrowserMainPartsChromeos() {
>  56   if (!parameters().ui_task && chromeos::CrosLibrary::Get())
>  57     chromeos::CrosLibrary::Shutdown();

[satorux1, 2011-09-07 18:50:14]

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (left):

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main...
chrome/browser/browser_main.cc:2183: parts.reset(NULL);
On 2011/09/07 17:37:48, John Abd-El-Malek wrote:
> On 2011/09/07 17:10:31, satorux1 wrote:
> > The comment says the BrowserMainParts is explicitly released, before
> > CrosLibrary's shutdown. However, CrosLibrary's shutdown is now moved to
> > ~BroserMainPartsChromeos. This is rather worrisome.
> 
> Do you have any specifics that worry you?
> 
> We need to be able to reorder startup/shutdown slightly as we refactor
existing
> code, otherwise we would have to add a lot of callbacks to keep the exact same
> order, which may not be necessary at all. a lot, but not all of course, of the
> code was put in whatever place where it worked, but that doesn't mean it can't
> move around.

I don't have specific concerns. Steven who added CrosLibrary::Shutdown() here
may have some comment on this. I'm fine as long as Steven is fine with this
reorganization. 


> 
> > 
> > 
> >
>
http://codereview.chromium.org/7779040/diff/11053/chrome/browser/chromeos/bro...
> > 
> >  55 BrowserMainPartsChromeos::~BrowserMainPartsChromeos() {
> >  56   if (!parameters().ui_task && chromeos::CrosLibrary::Get())
> >  57     chromeos::CrosLibrary::Shutdown();
>

[stevenjb, 2011-09-07 22:27:55]

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (left):

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main...
chrome/browser/browser_main.cc:2183: parts.reset(NULL);
On 2011/09/07 17:10:31, satorux1 wrote:
> The comment says the BrowserMainParts is explicitly released, before
> CrosLibrary's shutdown. However, CrosLibrary's shutdown is now moved to
> ~BroserMainPartsChromeos. This is rather worrisome.
> 
> 
>
http://codereview.chromium.org/7779040/diff/11053/chrome/browser/chromeos/bro...
> 
>  55 BrowserMainPartsChromeos::~BrowserMainPartsChromeos() {
>  56   if (!parameters().ui_task && chromeos::CrosLibrary::Get())
>  57     chromeos::CrosLibrary::Shutdown();

This definitely changes things. We need to change the comment, now at the bottom
of content/browser/browser_main.cc, as it is no longer accurate.

Because the destructor of BrowserMainPartsChromeos and all of its member
destructors occur before BrowserMainParts, we will not be able to shut down
CrosLibrary before members of BrowserMainParts.

This means that we need to ensure that any class that has CrosLibrary calls in
its destructor is shut down as part of BrowserMainPartsChromeos and not
BrowserMainParts. This is more correct anyway. We will however need to do an
audit before R15 goes to beta.

I am looking at a chromeos shutdown problem anyway so will try to make sure that
all of the auto tests, etc, get exercised in Debug mode and we catch any
shutdown issues.

[jam, 2011-09-08 00:33:56]

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (left):

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/browser_main...
chrome/browser/browser_main.cc:2183: parts.reset(NULL);
On 2011/09/07 22:27:55, Steven Bennetts wrote:
> On 2011/09/07 17:10:31, satorux1 wrote:
> > The comment says the BrowserMainParts is explicitly released, before
> > CrosLibrary's shutdown. However, CrosLibrary's shutdown is now moved to
> > ~BroserMainPartsChromeos. This is rather worrisome.
> > 
> > 
> >
>
http://codereview.chromium.org/7779040/diff/11053/chrome/browser/chromeos/bro...
> > 
> >  55 BrowserMainPartsChromeos::~BrowserMainPartsChromeos() {
> >  56   if (!parameters().ui_task && chromeos::CrosLibrary::Get())
> >  57     chromeos::CrosLibrary::Shutdown();
> 
> This definitely changes things. We need to change the comment, now at the
bottom
> of content/browser/browser_main.cc, as it is no longer accurate.

yep I have removed that in my next cl

> 
> Because the destructor of BrowserMainPartsChromeos and all of its member
> destructors occur before BrowserMainParts, we will not be able to shut down
> CrosLibrary before members of BrowserMainParts.

With my change CrosLibrary is shutdown before members of BrowserMainParts. The
only thing that depended on CrosLibrary is
BrowserMainParts::network_change_notifier_ since the factory is overriden by
BrowserMainPartsChromeos::PreMainMessageLoopStart(). We can add a method to
BrowserMainParts to reset the networkchangenotifier, but that did look hacky to
me, so instead I had made the networkchangenotifier handle the case of
CrosLibrary being gone. The only usage of it that I had seen was removing
observers, so it seemed ok to me, but you know this better than me.

http://codereview.chromium.org/7779040/diff/11053/chrome/browser/chromeos/net...
> 
> This means that we need to ensure that any class that has CrosLibrary calls in
> its destructor is shut down as part of BrowserMainPartsChromeos and not
> BrowserMainParts. This is more correct anyway.

Agreed.

> We will however need to do an audit before R15 goes to beta.

I suspect that if there were anymore, we'd quickly see this as shutdown crashes.

> 
> I am looking at a chromeos shutdown problem anyway so will try to make sure
that
> all of the auto tests, etc, get exercised in Debug mode and we catch any
> shutdown issues.
> 
>