Add ChromeBrowserMainExtraParts for non main parts.

Add ChromeBrowserParts for non main parts.

This reverts the code back to a single instance of BrowserMainParts, with auxillary parts (Gtk, Views, Aura, Touch) implemented from a new base class, ChromeBrowserParts, which has a Chrome specific interface, allowing initialization to be better subdivided.

This should fix the notifications auto tests.

BUG=103821
TEST=Make sure all tests and autotests run

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

[stevenjb, 2011-11-12 03:10:16]

OK, third time is a charm. I think this is a better solution than the other
attempts. It certainly makes it easier for us to add new stages and pull more
platform specific code out of ChromeBrowserMain and into the specific parts,
without touching Content.

I will plan to test this on a chromeos device on Monday, but will not be able to
test on Stumpy + Aura until Tues. Nikita, if you have a chance to try this patch
on stumpy before then, that would be great.

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (left):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:1861:
OptionallyRunChromeOSLoginManager(parsed_command_line(), profile_);
No longer necessary to split this function or derfer setting the notification UI
manager, now that PostBrowserProcessInit() exists for the auxiliary parts
(Views, Aura).

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:1592:
OptionallyRunChromeOSLoginManager(parsed_command_line(), profile_);
Note: This is exactly where this code used to be before the re-factoring @
109069. It is OK to move it back here now that the required desktop
initialization is being done in PostBrowserProcessInit() instead of at the end
of this function (PreMainMessageLoopRun()) in PartsViews and PartsAura.

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_parts_aura.cc (left):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_parts_aura.cc:33: void
ChromeBrowserPartsAura::PreMainMessageLoopRun() {
Note: this is now PostBrowserProcessInit() so that the desktop is created at the
point that it used to be.

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_parts_views.cc (left):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_parts_views.cc:30: void
ChromeBrowserPartsViews::PreMainMessageLoopRun() {
Note: this is now PostBrowserProcessInit() so that the desktop is created at the
point that it used to be.

[Nirnimesh, 2011-11-12 06:17:35]

Please reference BUG=103427 too

[jam, 2011-11-14 19:45:54]

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.h (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.h:28: class ChromeBrowserParts;
nit: order

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_parts.h (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_parts.h:18: class ChromeBrowserParts {
it's confusing to have both ChromeBrowserParts and ChromeBrowserMainParts since
both are parts for the BrowserMain function.

As I understand it, the existing ChromeBrowserMainParts is really the main
BrowserMainparts for Chrome. So how about we call that
ChromeMainBrowserMainParts? Then it has a collection of ChromeBrowserMainParts
implementations.

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:223: ChromeBrowserMainParts
*main_parts;
nit: ChromeBrowserMainParts* main_parts;

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.h:24: 
nit: the blank lines are unnecessary, we usually put all overrides right after
each other

http://codereview.chromium.org/8539038/diff/2001/content/browser/browser_main...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/8539038/diff/2001/content/browser/browser_main...
content/browser/browser_main_loop.h:9: #include <vector>
nit: not needed anymore

http://codereview.chromium.org/8539038/diff/2001/content/browser/browser_main...
content/browser/browser_main_loop.h:57: BrowserMainParts* parts_;
nit: scoped_ptr?

[stevenjb, 2011-11-14 23:31:10]

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.h (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.h:28: class ChromeBrowserParts;
On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
> nit: order

Done.

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_parts.h (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_parts.h:18: class ChromeBrowserParts {
On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
> it's confusing to have both ChromeBrowserParts and ChromeBrowserMainParts
since
> both are parts for the BrowserMain function.
> 
> As I understand it, the existing ChromeBrowserMainParts is really the main
> BrowserMainparts for Chrome. So how about we call that
> ChromeMainBrowserMainParts? Then it has a collection of ChromeBrowserMainParts
> implementations.

I agree it is confusing, but I specifically avoided "Main" in the name of
ChromeBrowserParts. These parts are owned by the Chrome specific
BrowserMainParts class (ChromeBrowserMainParts), however these are not really
"main" parts, they are orthogonal sub-parts, and they do not derive from
BrowserMainParts.

Also, the names are currently consistent with the derived sub-classes, e.g.
ChromeBrowserMainPartsLinux, and ChromeBrowserPartsViews. (See:
https://docs.google.com/drawings/d/1nlCHttFS6BAFawce6AM-AcyOhgTl7ptCod1NZznoc...
 for a diagram).

I'd much rather discuss naming strategies offline and do any renaming as a
separate CL.

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:223: ChromeBrowserMainParts
*main_parts;
On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
> nit: ChromeBrowserMainParts* main_parts;

Done.

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.h:24: 
On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
> nit: the blank lines are unnecessary, we usually put all overrides right after
> each other

Done.

http://codereview.chromium.org/8539038/diff/2001/content/browser/browser_main...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/8539038/diff/2001/content/browser/browser_main...
content/browser/browser_main_loop.h:9: #include <vector>
On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
> nit: not needed anymore

Done.

http://codereview.chromium.org/8539038/diff/2001/content/browser/browser_main...
content/browser/browser_main_loop.h:57: BrowserMainParts* parts_;
On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
> nit: scoped_ptr?

Done.

[jam, 2011-11-15 00:10:51]

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_parts.h (right):

http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_parts.h:18: class ChromeBrowserParts {
On 2011/11/14 23:31:10, Steven Bennetts wrote:
> On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
> > it's confusing to have both ChromeBrowserParts and ChromeBrowserMainParts
> since
> > both are parts for the BrowserMain function.
> > 
> > As I understand it, the existing ChromeBrowserMainParts is really the main
> > BrowserMainparts for Chrome. So how about we call that
> > ChromeMainBrowserMainParts? Then it has a collection of
ChromeBrowserMainParts
> > implementations.
> 
> I agree it is confusing, but I specifically avoided "Main" in the name of
> ChromeBrowserParts. These parts are owned by the Chrome specific
> BrowserMainParts class (ChromeBrowserMainParts), however these are not really
> "main" parts, they are orthogonal sub-parts, and they do not derive from
> BrowserMainParts.

Sorry to nit, but I don't quite follow this argument. These classes are all
called from BrowserMain, so they are all "parts" of the BrowserMain function,
and not just a generic Browser scope. There is already a "Browser" class in
chrome/browser/ui, so BrowserParts would indicate that a class is part of that,
and not BrowserMain.

> 
> Also, the names are currently consistent with the derived sub-classes, e.g.
> ChromeBrowserMainPartsLinux, and ChromeBrowserPartsViews. (See:
>
https://docs.google.com/drawings/d/1nlCHttFS6BAFawce6AM-AcyOhgTl7ptCod1NZznoc...
>  for a diagram).

I think these all have to be renamed as well. All of these classes are in use
during the BrowserMain function, and making their name contain "BrowserMain"
instead of just "Main" makes it obvious that these are called inside the
BrowserMain function, and not throughout the browser process' lifetime.

> 
> I'd much rather discuss naming strategies offline and do any renaming as a
> separate CL. 

If you want to rename this in the change after this, that's fine too. However
this class shouldn't go in as ChromeBrowserParts only to be renamed right after
to ChromeBrowserMainParts.

[stevenjb, 2011-11-15 01:19:56]

On 2011/11/15 00:10:51, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
> File chrome/browser/chrome_browser_parts.h (right):
> 
>
http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browse...
> chrome/browser/chrome_browser_parts.h:18: class ChromeBrowserParts {
> On 2011/11/14 23:31:10, Steven Bennetts wrote:
> > On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
> > > it's confusing to have both ChromeBrowserParts and ChromeBrowserMainParts
> > since
> > > both are parts for the BrowserMain function.
> > > 
> > > As I understand it, the existing ChromeBrowserMainParts is really the main
> > > BrowserMainparts for Chrome. So how about we call that
> > > ChromeMainBrowserMainParts? Then it has a collection of
> ChromeBrowserMainParts
> > > implementations.
> > 
> > I agree it is confusing, but I specifically avoided "Main" in the name of
> > ChromeBrowserParts. These parts are owned by the Chrome specific
> > BrowserMainParts class (ChromeBrowserMainParts), however these are not
really
> > "main" parts, they are orthogonal sub-parts, and they do not derive from
> > BrowserMainParts.
> 
> Sorry to nit, but I don't quite follow this argument. These classes are all
> called from BrowserMain, so they are all "parts" of the BrowserMain function,
> and not just a generic Browser scope. There is already a "Browser" class in
> chrome/browser/ui, so BrowserParts would indicate that a class is part of
that,
> and not BrowserMain.
> 
> > 
> > Also, the names are currently consistent with the derived sub-classes, e.g.
> > ChromeBrowserMainPartsLinux, and ChromeBrowserPartsViews. (See:
> >
>
https://docs.google.com/drawings/d/1nlCHttFS6BAFawce6AM-AcyOhgTl7ptCod1NZznoc...
> >  for a diagram).
> 
> I think these all have to be renamed as well. All of these classes are in use
> during the BrowserMain function, and making their name contain "BrowserMain"
> instead of just "Main" makes it obvious that these are called inside the
> BrowserMain function, and not throughout the browser process' lifetime.
> 
> > 
> > I'd much rather discuss naming strategies offline and do any renaming as a
> > separate CL. 
> 
> If you want to rename this in the change after this, that's fine too. However
> this class shouldn't go in as ChromeBrowserParts only to be renamed right
after
> to ChromeBrowserMainParts.

Honestly I'd much rather push this forward and carefully rename
ChromeBrowserParts and everything else later, but if this really bothers you:

I think that ChromeMainBrowserMainParts is pure insanity :) Not to mention the
derived classes, e.g ChromeMainBrowserMainPartsLinux.
I also think we really need to leave ChromeBrowserMainParts as-is until all of
this is reasonably stable. This class is central to everything and renaming it
is an invitation for problems and confusion.

What if we name "ChromeMainParts" as "ChromeBrowserMainExtraParts" and rename
the derived classes appropriately:
ChromeBrowserMainExtraParts[Gtx|Views|Aura|Touch]. I'm not a huge fan of
"ExtraParts", but it's the best I can come up with under duress :P

[jam, 2011-11-15 06:28:10]

On Mon, Nov 14, 2011 at 5:19 PM, <stevenjb@chromium.org> wrote:

> On 2011/11/15 00:10:51, John Abd-El-Malek wrote:
>
> http://codereview.chromium.**org/8539038/diff/2001/chrome/**
>
browser/chrome_browser_parts.h<http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browser_parts.h>
>
>> File chrome/browser/chrome_browser_**parts.h (right):
>>
>
>
> http://codereview.chromium.**org/8539038/diff/2001/chrome/**
>
browser/chrome_browser_parts.**h#newcode18<http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browser_parts.h#newcode18>
>
>> chrome/browser/chrome_browser_**parts.h:18: class ChromeBrowserParts {
>> On 2011/11/14 23:31:10, Steven Bennetts wrote:
>> > On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
>> > > it's confusing to have both ChromeBrowserParts and
>> ChromeBrowserMainParts
>> > since
>> > > both are parts for the BrowserMain function.
>> > >
>> > > As I understand it, the existing ChromeBrowserMainParts is really the
>> main
>> > > BrowserMainparts for Chrome. So how about we call that
>> > > ChromeMainBrowserMainParts? Then it has a collection of
>> ChromeBrowserMainParts
>> > > implementations.
>> >
>> > I agree it is confusing, but I specifically avoided "Main" in the name
>> of
>> > ChromeBrowserParts. These parts are owned by the Chrome specific
>> > BrowserMainParts class (ChromeBrowserMainParts), however these are not
>>
> really
>
>> > "main" parts, they are orthogonal sub-parts, and they do not derive from
>> > BrowserMainParts.
>>
>
>  Sorry to nit, but I don't quite follow this argument. These classes are
>> all
>> called from BrowserMain, so they are all "parts" of the BrowserMain
>> function,
>> and not just a generic Browser scope. There is already a "Browser" class
>> in
>> chrome/browser/ui, so BrowserParts would indicate that a class is part of
>>
> that,
>
>> and not BrowserMain.
>>
>
>  >
>> > Also, the names are currently consistent with the derived sub-classes,
>> e.g.
>> > ChromeBrowserMainPartsLinux, and ChromeBrowserPartsViews. (See:
>> >
>>
>
> https://docs.google.com/**drawings/d/**1nlCHttFS6BAFawce6AM-**
>
AcyOhgTl7ptCod1NZznocP8/edit?**authkey=CMixsP8H<https://docs.google.com/drawings/d/1nlCHttFS6BAFawce6AM-AcyOhgTl7ptCod1NZznocP8/edit?authkey=CMixsP8H>
>
>> >  for a diagram).
>>
>
>  I think these all have to be renamed as well. All of these classes are in
>> use
>> during the BrowserMain function, and making their name contain
>> "BrowserMain"
>> instead of just "Main" makes it obvious that these are called inside the
>> BrowserMain function, and not throughout the browser process' lifetime.
>>
>
>  >
>> > I'd much rather discuss naming strategies offline and do any renaming
>> as a
>> > separate CL.
>>
>
>  If you want to rename this in the change after this, that's fine too.
>> However
>> this class shouldn't go in as ChromeBrowserParts only to be renamed right
>>
> after
>
>> to ChromeBrowserMainParts.
>>
>
> Honestly I'd much rather push this forward and carefully rename
> ChromeBrowserParts and everything else later, but if this really bothers
> you:
>
> I think that ChromeMainBrowserMainParts is pure insanity :) Not to mention
> the
> derived classes, e.g ChromeMainBrowserMainPartsLinu**x.
> I also think we really need to leave ChromeBrowserMainParts as-is until
> all of
> this is reasonably stable. This class is central to everything and
> renaming it
> is an invitation for problems and confusion.
>
> What if we name "ChromeMainParts" as "ChromeBrowserMainExtraParts" and
> rename
> the derived classes appropriately:
> ChromeBrowserMainExtraParts[**Gtx|Views|Aura|Touch]. I'm not a huge fan of
> "ExtraParts", but it's the best I can come up with under duress :P
>

sgtm. Whatever you prefer as long as it has BrowserMainParts somewhere in
it :)


>
>
>
>
>
>
>
>
http://codereview.chromium.**org/8539038/<http://codereview.chromium.org/8539...
>

[jam, 2011-11-15 06:28:57]

btw lgtm if it's easier for you to commit this as is, and then do the
renaming to ChromeBrowserMainExtraParts in a followup so that it's a
rubberstamp lgtm from anyone

On Mon, Nov 14, 2011 at 10:28 PM, John Abd-El-Malek <jam@chromium.org>wrote:

>
>
> On Mon, Nov 14, 2011 at 5:19 PM, <stevenjb@chromium.org> wrote:
>
>> On 2011/11/15 00:10:51, John Abd-El-Malek wrote:
>>
>> http://codereview.chromium.**org/8539038/diff/2001/chrome/**
>>
browser/chrome_browser_parts.h<http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browser_parts.h>
>>
>>> File chrome/browser/chrome_browser_**parts.h (right):
>>>
>>
>>
>> http://codereview.chromium.**org/8539038/diff/2001/chrome/**
>>
browser/chrome_browser_parts.**h#newcode18<http://codereview.chromium.org/8539038/diff/2001/chrome/browser/chrome_browser_parts.h#newcode18>
>>
>>> chrome/browser/chrome_browser_**parts.h:18: class ChromeBrowserParts {
>>> On 2011/11/14 23:31:10, Steven Bennetts wrote:
>>> > On 2011/11/14 19:45:55, John Abd-El-Malek wrote:
>>> > > it's confusing to have both ChromeBrowserParts and
>>> ChromeBrowserMainParts
>>> > since
>>> > > both are parts for the BrowserMain function.
>>> > >
>>> > > As I understand it, the existing ChromeBrowserMainParts is really
>>> the main
>>> > > BrowserMainparts for Chrome. So how about we call that
>>> > > ChromeMainBrowserMainParts? Then it has a collection of
>>> ChromeBrowserMainParts
>>> > > implementations.
>>> >
>>> > I agree it is confusing, but I specifically avoided "Main" in the name
>>> of
>>> > ChromeBrowserParts. These parts are owned by the Chrome specific
>>> > BrowserMainParts class (ChromeBrowserMainParts), however these are not
>>>
>> really
>>
>>> > "main" parts, they are orthogonal sub-parts, and they do not derive
>>> from
>>> > BrowserMainParts.
>>>
>>
>>  Sorry to nit, but I don't quite follow this argument. These classes are
>>> all
>>> called from BrowserMain, so they are all "parts" of the BrowserMain
>>> function,
>>> and not just a generic Browser scope. There is already a "Browser" class
>>> in
>>> chrome/browser/ui, so BrowserParts would indicate that a class is part of
>>>
>> that,
>>
>>> and not BrowserMain.
>>>
>>
>>  >
>>> > Also, the names are currently consistent with the derived sub-classes,
>>> e.g.
>>> > ChromeBrowserMainPartsLinux, and ChromeBrowserPartsViews. (See:
>>> >
>>>
>>
>> https://docs.google.com/**drawings/d/**1nlCHttFS6BAFawce6AM-**
>>
AcyOhgTl7ptCod1NZznocP8/edit?**authkey=CMixsP8H<https://docs.google.com/drawings/d/1nlCHttFS6BAFawce6AM-AcyOhgTl7ptCod1NZznocP8/edit?authkey=CMixsP8H>
>>
>>> >  for a diagram).
>>>
>>
>>  I think these all have to be renamed as well. All of these classes are
>>> in use
>>> during the BrowserMain function, and making their name contain
>>> "BrowserMain"
>>> instead of just "Main" makes it obvious that these are called inside the
>>> BrowserMain function, and not throughout the browser process' lifetime.
>>>
>>
>>  >
>>> > I'd much rather discuss naming strategies offline and do any renaming
>>> as a
>>> > separate CL.
>>>
>>
>>  If you want to rename this in the change after this, that's fine too.
>>> However
>>> this class shouldn't go in as ChromeBrowserParts only to be renamed right
>>>
>> after
>>
>>> to ChromeBrowserMainParts.
>>>
>>
>> Honestly I'd much rather push this forward and carefully rename
>> ChromeBrowserParts and everything else later, but if this really bothers
>> you:
>>
>> I think that ChromeMainBrowserMainParts is pure insanity :) Not to
>> mention the
>> derived classes, e.g ChromeMainBrowserMainPartsLinu**x.
>> I also think we really need to leave ChromeBrowserMainParts as-is until
>> all of
>> this is reasonably stable. This class is central to everything and
>> renaming it
>> is an invitation for problems and confusion.
>>
>> What if we name "ChromeMainParts" as "ChromeBrowserMainExtraParts" and
>> rename
>> the derived classes appropriately:
>> ChromeBrowserMainExtraParts[**Gtx|Views|Aura|Touch]. I'm not a huge fan
>> of
>> "ExtraParts", but it's the best I can come up with under duress :P
>>
>
> sgtm. Whatever you prefer as long as it has BrowserMainParts somewhere in
> it :)
>
>
>>
>>
>>
>>
>>
>>
>>
>>
http://codereview.chromium.**org/8539038/<http://codereview.chromium.org/8539...
>>
>
>

[stevenjb, 2011-11-15 18:41:01]

PTAL
Went ahead and did the rename.
Will do a rebase + bot run next.

[jam, 2011-11-15 21:28:29]

lgtm