Make GTK and Aura parts orthogonal to OS parts

Make GTK and Aura parts orthogonal to OS parts

This CL moves GTK and Aura "Parts" out of the primary BrowserMainParts tree and into orthogonal parts that can be added independently. This was done in a way that will facilitate adding additional parts if (when) we need them.

The motivation for this was to 
a) eliminate the existing typedef in chrome_browser_main_chromeos.h 
b) reduce the number of #ifdefs required in the setup / parts code

For an outline of the new parts see:

https://docs.google.com/drawings/d/1-gIMl-81c4SvcMrT1xaxnDGibDe7VQfMkFT1bMnIvrg/edit?hl=en_US

Please consider this a proposal; I am entirely open to feedback.

BUG=none
TEST=Chrome compiles and passes tests on all platfroms.

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

[stevenjb, 2011-10-15 01:31:09]

First draft of the Parts cleanup. Some additional cleanup (migrating some global
functions to pure virtuals in ChromeBrowserMainParts) seemed pretty natural,
which strikes me as a good sign, but I am wide open to feedback.

[tfarina, 2011-10-15 22:17:29]

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

http://codereview.chromium.org/8302016/diff/2001/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.h:25:
DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsChromeos);
Can we move this to the end of private section?

[DaveMoore, 2011-10-17 17:44:21]

vtl should review. Also, is there some way to unify the OS parts and the Extra
parts, so that there's just a single list of parts? If there needs to be
inheritance in the OS side that's fine, but can you enumerate the OS ones along
with the others?

[stevenjb, 2011-10-17 23:53:08]

OK, I think this is better. I'd look at browser_main.h/cc first, then
chrome_content_browser_client.cc. Everything else pretty much falls out of that.

[DaveMoore, 2011-10-19 21:31:18]

http://codereview.chromium.org/8302016/diff/10001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.h (right):

http://codereview.chromium.org/8302016/diff/10001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.h:54: // New virtual methods
Nit: this comment probably doesn't need to be here

http://codereview.chromium.org/8302016/diff/10001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_aura.h (right):

http://codereview.chromium.org/8302016/diff/10001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_aura.h:10: class ChromeBrowserPartsAura :
public content::BrowserParts {
We should rename this file to "chrome_browser_parts_aura"

http://codereview.chromium.org/8302016/diff/10001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_gtk.h (right):

http://codereview.chromium.org/8302016/diff/10001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_gtk.h:14: class ChromeBrowserPartsGtk :
public content::BrowserParts {
Rename file to chrome_browser_parts_gtk

http://codereview.chromium.org/8302016/diff/10001/content/browser/browser_main.h
File content/browser/browser_main.h (right):

http://codereview.chromium.org/8302016/diff/10001/content/browser/browser_mai...
content/browser/browser_main.h:50: //    specifc code (e.g. Gtk or Aura).
Nit: spelling

[stevenjb, 2011-10-20 00:35:50]

Broadening the review list for this CL. Please take a look if you can.

[Emmanuel Saint-loubert-Bié, 2011-10-20 00:44:21]

Looks great, and less #ifdef too!! Thanks Steven.

[sky, 2011-10-20 16:36:23]

Trung is a better reviewer than I am for this.

  -Scott

[jam, 2011-10-20 18:01:18]

http://codereview.chromium.org/8302016/diff/18025/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.h (right):

http://codereview.chromium.org/8302016/diff/18025/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.h:62: virtual void
PrepareRestartOnCrashEnviroment() = 0;
the last three methods are Windows specific. can we make the windows
implementation hook at the correct place (even if that means moving it slightly)
so that we don't have to put these functions on all the other platforms that
don't need it?

http://codereview.chromium.org/8302016/diff/18025/content/browser/browser_main.h
File content/browser/browser_main.h (right):

http://codereview.chromium.org/8302016/diff/18025/content/browser/browser_mai...
content/browser/browser_main.h:42: //   inverse order.
While I understand why you did it, the concept of having two different types of
parts, one which we can have many of, and one only one instance, is a little
confusing to me.

Did you consider just having all instances be of the same type? And then when we
call RunMainMessageLoopParts on all the instances, obviously only the first part
which accepts to run it gets a go, and we don't call any of the rest.

[stevenjb, 2011-10-21 22:25:49]

PTAL

This re-factoring should be much improved after input from John. I would start
with content/public/browser/browser_main_parts.h, then
content/browser/browser_main.cc/h. 

I have also updated
https://docs.google.com/a/chromium.org/drawings/d/1-gIMl-81c4SvcMrT1xaxnDGibD...
with the new, cleaner organization.

Note: In a follow-up I think I would like to extract ChromeBrowserMainParts[OS]
from ChromeBrowserMainParts (so that they are all siblings with no inheritance),
however I need to look closely at the behavior of each piece before I do that,
and figure out a better/consistent way to communicated
parameters/parsed_command_line across the classes.

http://codereview.chromium.org/8302016/diff/18025/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.h (right):

http://codereview.chromium.org/8302016/diff/18025/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.h:62: virtual void
PrepareRestartOnCrashEnviroment() = 0;
On 2011/10/20 18:01:19, John Abd-El-Malek wrote:
> the last three methods are Windows specific. can we make the windows
> implementation hook at the correct place (even if that means moving it
slightly)
> so that we don't have to put these functions on all the other platforms that
> don't need it?

OK, I wasn't sure if there were plans for these. Made them static members of
ChromeBrowserMainPartsWin only and put them into #ifdefs. (I think at some point
we will want to break up ChromeBrowserMainParts::PreMainMessageLoopRun() and
move specialized code into the correct module, but probably not here).

Done.

http://codereview.chromium.org/8302016/diff/18025/content/browser/browser_main.h
File content/browser/browser_main.h (right):

http://codereview.chromium.org/8302016/diff/18025/content/browser/browser_mai...
content/browser/browser_main.h:42: //   inverse order.
On 2011/10/20 18:01:19, John Abd-El-Malek wrote:
> While I understand why you did it, the concept of having two different types
of
> parts, one which we can have many of, and one only one instance, is a little
> confusing to me.
> 
> Did you consider just having all instances be of the same type? And then when
we
> call RunMainMessageLoopParts on all the instances, obviously only the first
part
> which accepts to run it gets a go, and we don't call any of the rest.

After some conversation and a closer look at browser_main.h, I realized that:
* most of the original BrowserMainParts could be pulled into the .cc file (now
BrowserMainLoop)
* BrowserParts should be restored to BrowserMainParts as a pure interface class
and moved into public/browser/browser_main_parts.h
* MainMessageLoopRun() should be in the new BrowserMainParts and return a bool
if it runs the main loop

[stevenjb, 2011-10-24 19:26:00]

Ping.

If we're happy with this direction I'd like to get it in sooner than later.
Please weigh in if you have concerns, otherwise I'll work with John to get this
in good shape and in the tree soon.

[Emmanuel Saint-loubert-Bié, 2011-10-24 19:27:41]

Hi Steven

I am very happy with this direction and CL and hope that we can land it shortly.

-- Emmanuel

On Mon, Oct 24, 2011 at 12:26 PM,  <stevenjb@chromium.org> wrote:
> Ping.
>
> If we're happy with this direction I'd like to get it in sooner than later.
> Please weigh in if you have concerns, otherwise I'll work with John to get
> this
> in good shape and in the tree soon.
>
>
> http://codereview.chromium.org/8302016/
>

[jam, 2011-10-24 19:36:00]

(didn't review all the chrome platform specific implementations yet, figured I'd
send this first)

in general this looks much better, thanks for cleaning things up

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

http://codereview.chromium.org/8302016/diff/26036/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:688: : content::BrowserMainParts(),
not necessary

http://codereview.chromium.org/8302016/diff/26036/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:712: parsed_command_line(), local_state);
nit: now that this is a member variable, just parsed_command_line_

http://codereview.chromium.org/8302016/diff/26036/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1470: parsed_command_line());
ditto

http://codereview.chromium.org/8302016/diff/26036/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1697:
StartupTimeBomb::Arm(base::TimeDelta::FromSeconds(300));
let's move this to line  1746 so that we can one OS_WIN ifdef from 1688 to 1743.
we can then move 1711-1743 inside that one ifdef, and also surround it with
GOOGLE_CHROME_BUILD

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
File content/browser/browser_main.cc (right):

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.cc:170: // BrowserMainLoop
nit: this should go into its own file, and also have a header

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.cc:172: // This class contains different "stages"
to be executed in |BrowserMain()|,
nit: these comments are really meant for embedders in order to figure out how to
hook startup, so it seems that putting them in the implementation file will be
hard to find.

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.cc:345: for (PartsList::iterator iter =
parts_.begin();
convention is usually to not use an iterator for a vector..

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_main.h
File content/browser/browser_main.h (left):

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.h:30: // This class contains different "stages" to
be executed in |BrowserMain()|,
this is a lot of nice comments, why remove it all? shouldn't into the new
browser_parts.h?

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_main.h
File content/browser/browser_main.h (right):

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.h:9: #include <vector>
nit: not needed, also most of the forward declares below

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.h:38: //  LocalWords:  BrowserMain BrowserMainLoop
BrowserParts MainMessageLoopRun
?

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
File content/public/browser/browser_main_parts.h (right):

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:5: #ifndef
CONTENT_PUBLIC_BROWSER_CONTENT_BROWSER_BROWSER_MAIN_PARTS_H_
nit: CONTENT_PUBLIC_BROWSER_BOWSER_MAIN_PARTS_H_

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:17: // initialization and shutdown.
we don't really need to tell embedders where to put their global variables etc.
it's also obvious that they'll have to implement the virtual functions that they
care about, so i think this comment (line 14-17) isn't necessary

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:22: 
nit: extra line

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:41: virtual int
PreMainMessageLoopRun() = 0;
why did you change this function to return a value? it seems confusing that a
function which runs before the message loop returns an exit code.

i believe you're doing this because the chrome impl sometimes doesn't want to
run the main message loop, and stashes the result code in its
PreMainMessageLoopRun function. so why not just have it return true there? it
can also have a chance to return the error code, i.e.

bool MainMessageLoopRun(int* exit_code)

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:50: private:
nit: space before private

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/cont...
content/public/browser/content_browser_client.h:86: typedef
std::vector<BrowserMainParts*> BrowserMainPartsList;
nit: i think a typedef isn't necessary in the interface, we usually have that
when we put it as member variables and want to create iterators etc (which we
normally don't use for vectors anyways)

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/cont...
content/public/browser/content_browser_client.h:89: // for the browser staratup
code. Can return NULL, in which case the default
nit: update the comment for the return value

http://codereview.chromium.org/8302016/diff/26036/content/shell/shell_browser...
File content/shell/shell_browser_main.h (right):

http://codereview.chromium.org/8302016/diff/26036/content/shell/shell_browser...
content/shell/shell_browser_main.h:33: virtual void PreEarlyInitialization()
OVERRIDE {};
nit: don't need semicolon

http://codereview.chromium.org/8302016/diff/26036/content/shell/shell_browser...
content/shell/shell_browser_main.h:37: virtual void PostMainMessageLoopStart()
OVERRIDE {};
ditto

http://codereview.chromium.org/8302016/diff/26036/content/shell/shell_browser...
content/shell/shell_browser_main.h:39: virtual bool MainMessageLoopRun()
OVERRIDE { return false; }
i believe this will give a clang error

[stevenjb, 2011-10-25 02:51:03]

PTAL

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

http://codereview.chromium.org/8302016/diff/26036/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:688: : content::BrowserMainParts(),
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> not necessary

Done.

http://codereview.chromium.org/8302016/diff/26036/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:712: parsed_command_line(), local_state);
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: now that this is a member variable, just parsed_command_line_

Done.

http://codereview.chromium.org/8302016/diff/26036/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1470: parsed_command_line());
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/8302016/diff/26036/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1697:
StartupTimeBomb::Arm(base::TimeDelta::FromSeconds(300));
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> let's move this to line  1746 so that we can one OS_WIN ifdef from 1688 to
1743.
> we can then move 1711-1743 inside that one ifdef, and also surround it with
> GOOGLE_CHROME_BUILD

Done. (Had considered doing that but wasn't certain there wouldn't be side
effects, although it looks safe to me)

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
File content/browser/browser_main.cc (right):

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.cc:170: // BrowserMainLoop
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: this should go into its own file, and also have a header

Done.

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.cc:172: // This class contains different "stages"
to be executed in |BrowserMain()|,
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: these comments are really meant for embedders in order to figure out how
to
> hook startup, so it seems that putting them in the implementation file will be
> hard to find.

Done.

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.cc:345: for (PartsList::iterator iter =
parts_.begin();
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> convention is usually to not use an iterator for a vector..

Interesting, I did not know that. That reduces flexibility and debug validity
checks, although it is easier to read. I didn't see that anywhere in our style
guide? Anyway, it's easy enough to change.

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_main.h
File content/browser/browser_main.h (left):

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.h:30: // This class contains different "stages" to
be executed in |BrowserMain()|,
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> this is a lot of nice comments, why remove it all? shouldn't into the new
> browser_parts.h?
This got moved into browser_main.cc, but should have been moved into
browser_main_parts.h; will do so.

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_main.h
File content/browser/browser_main.h (right):

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.h:9: #include <vector>
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: not needed, also most of the forward declares below

Done.

http://codereview.chromium.org/8302016/diff/26036/content/browser/browser_mai...
content/browser/browser_main.h:38: //  LocalWords:  BrowserMain BrowserMainLoop
BrowserParts MainMessageLoopRun
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> ?
wtf? maybe got added when I spellchecked the comments? removed.

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
File content/public/browser/browser_main_parts.h (right):

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:5: #ifndef
CONTENT_PUBLIC_BROWSER_CONTENT_BROWSER_BROWSER_MAIN_PARTS_H_
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: CONTENT_PUBLIC_BROWSER_BOWSER_MAIN_PARTS_H_
Oops. Done.

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:17: // initialization and shutdown.
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> we don't really need to tell embedders where to put their global variables
etc.
> it's also obvious that they'll have to implement the virtual functions that
they
> care about, so i think this comment (line 14-17) isn't necessary

Replaced with comment block from browser_main.cc

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:22: 
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: extra line

Done.

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:41: virtual int
PreMainMessageLoopRun() = 0;
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> why did you change this function to return a value? it seems confusing that a
> function which runs before the message loop returns an exit code.
> 
> i believe you're doing this because the chrome impl sometimes doesn't want to
> run the main message loop, and stashes the result code in its
> PreMainMessageLoopRun function. so why not just have it return true there? it
> can also have a chance to return the error code, i.e.
> 
> bool MainMessageLoopRun(int* exit_code)

That's much better. Done.

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/brow...
content/public/browser/browser_main_parts.h:50: private:
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: space before private

Done.

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/cont...
content/public/browser/content_browser_client.h:86: typedef
std::vector<BrowserMainParts*> BrowserMainPartsList;
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: i think a typedef isn't necessary in the interface, we usually have that
> when we put it as member variables and want to create iterators etc (which we
> normally don't use for vectors anyways)

Done.

http://codereview.chromium.org/8302016/diff/26036/content/public/browser/cont...
content/public/browser/content_browser_client.h:89: // for the browser staratup
code. Can return NULL, in which case the default
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: update the comment for the return value

Done.

http://codereview.chromium.org/8302016/diff/26036/content/shell/shell_browser...
File content/shell/shell_browser_main.h (right):

http://codereview.chromium.org/8302016/diff/26036/content/shell/shell_browser...
content/shell/shell_browser_main.h:33: virtual void PreEarlyInitialization()
OVERRIDE {};
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> nit: don't need semicolon

Done.

http://codereview.chromium.org/8302016/diff/26036/content/shell/shell_browser...
content/shell/shell_browser_main.h:37: virtual void PostMainMessageLoopStart()
OVERRIDE {};
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/8302016/diff/26036/content/shell/shell_browser...
content/shell/shell_browser_main.h:39: virtual bool MainMessageLoopRun()
OVERRIDE { return false; }
On 2011/10/24 19:36:00, John Abd-El-Malek wrote:
> i believe this will give a clang error

Done.

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

http://codereview.chromium.org/8302016/diff/34001/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.h (right):

http://codereview.chromium.org/8302016/diff/34001/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.h:15: class BrowserMainParts;
nit: not needed, since this is already in content_browser_client.h

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
content/browser/browser_main_loop.cc:18: #include
"content/public/common/content_client.h"
nit: by convention we don't include this when including one of the
process-specific contentclient interfaces

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
content/browser/browser_main_loop.h:10: #include "base/command_line.h"
nit: most of these headers aren't referenced in this file. just forward declare
MainFunctionParams

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
content/browser/browser_main_loop.h:22: class BrowserMainLoop {
sorry maybe I wasn't clear. I had meant just putting the class definition in
this file. i don't think we need to add an interface and implement it in the cc
file. BrowserMain can instantiate the BrowserMainLoop object directly

http://codereview.chromium.org/8302016/diff/34001/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/8302016/diff/34001/content/public/browser/cont...
content/public/browser/content_browser_client.h:12: #include
"content/public/browser/browser_main_parts.h"
nit: don't need to add this, just forward declare

http://codereview.chromium.org/8302016/diff/34001/content/public/browser/cont...
content/public/browser/content_browser_client.h:91:
std::vector<BrowserMainParts*>& parts_list) = 0;
I didn't notice last time that you're passing a non-const reference (against
style guide). please make this a pointer to the vector.

nit: parts instead of parts_list

[stevenjb, 2011-10-25 16:24:34]

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
content/browser/browser_main_loop.h:22: class BrowserMainLoop {
On 2011/10/25 06:11:28, John Abd-El-Malek wrote:
> sorry maybe I wasn't clear. I had meant just putting the class definition in
> this file. i don't think we need to add an interface and implement it in the
cc
> file. BrowserMain can instantiate the BrowserMainLoop object directly

I generally prefer to keep implementation details out of the header as much as
possible. I can wrap the Impl calls instead of using a static method to create
the implementation class if you prefer that?

[jam, 2011-10-25 16:33:58]

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
content/browser/browser_main_loop.h:22: class BrowserMainLoop {
On 2011/10/25 16:24:34, Steven Bennetts wrote:
> On 2011/10/25 06:11:28, John Abd-El-Malek wrote:
> > sorry maybe I wasn't clear. I had meant just putting the class definition in
> > this file. i don't think we need to add an interface and implement it in the
> cc
> > file. BrowserMain can instantiate the BrowserMainLoop object directly
> 
> I generally prefer to keep implementation details out of the header as much as
> possible. I can wrap the Impl calls instead of using a static method to create
> the implementation class if you prefer that?

hmm I haven't encountered this anywhere else in chrome code (although I haven't
looked closely at cros code). I'm hesitant of starting to use this pattern. When
someone is looking at the header, it's clear which are public methods, and where
the implementation is. Why should this class be treated special?

also, part of the reason for splitting content from chrome is to make embedders
see less code. This class will be entirely in the implementation part (i.e.
content/src) and won't be visible in the API dir (content/public).

[stevenjb, 2011-10-25 19:21:17]

http://codereview.chromium.org/8302016/diff/34001/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.h (right):

http://codereview.chromium.org/8302016/diff/34001/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.h:15: class BrowserMainParts;
On 2011/10/25 06:11:28, John Abd-El-Malek wrote:
> nit: not needed, since this is already in content_browser_client.h

Done.

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
content/browser/browser_main_loop.cc:18: #include
"content/public/common/content_client.h"
On 2011/10/25 06:11:28, John Abd-El-Malek wrote:
> nit: by convention we don't include this when including one of the
> process-specific contentclient interfaces

Done.

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
content/browser/browser_main_loop.h:10: #include "base/command_line.h"
On 2011/10/25 06:11:28, John Abd-El-Malek wrote:
> nit: most of these headers aren't referenced in this file. just forward
declare
> MainFunctionParams

Done.

http://codereview.chromium.org/8302016/diff/34001/content/browser/browser_mai...
content/browser/browser_main_loop.h:22: class BrowserMainLoop {
On 2011/10/25 16:33:59, John Abd-El-Malek wrote:
> On 2011/10/25 16:24:34, Steven Bennetts wrote:
> > On 2011/10/25 06:11:28, John Abd-El-Malek wrote:
> > > sorry maybe I wasn't clear. I had meant just putting the class definition
in
> > > this file. i don't think we need to add an interface and implement it in
the
> > cc
> > > file. BrowserMain can instantiate the BrowserMainLoop object directly
> > 
> > I generally prefer to keep implementation details out of the header as much
as
> > possible. I can wrap the Impl calls instead of using a static method to
create
> > the implementation class if you prefer that?
> 
> hmm I haven't encountered this anywhere else in chrome code (although I
haven't
> looked closely at cros code). I'm hesitant of starting to use this pattern.
When
> someone is looking at the header, it's clear which are public methods, and
where
> the implementation is. Why should this class be treated special?
> 
> also, part of the reason for splitting content from chrome is to make
embedders
> see less code. This class will be entirely in the implementation part (i.e.
> content/src) and won't be visible in the API dir (content/public).

OK, I will change this.

http://codereview.chromium.org/8302016/diff/34001/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/8302016/diff/34001/content/public/browser/cont...
content/public/browser/content_browser_client.h:12: #include
"content/public/browser/browser_main_parts.h"
On 2011/10/25 06:11:28, John Abd-El-Malek wrote:
> nit: don't need to add this, just forward declare

Done.

http://codereview.chromium.org/8302016/diff/34001/content/public/browser/cont...
content/public/browser/content_browser_client.h:91:
std::vector<BrowserMainParts*>& parts_list) = 0;
On 2011/10/25 06:11:28, John Abd-El-Malek wrote:
> I didn't notice last time that you're passing a non-const reference (against
> style guide). please make this a pointer to the vector.
> 
> nit: parts instead of parts_list

Done. Also, it's parts_list everywhere except the member variable in
BrowserMainLoop, so I changed that.

[jam, 2011-10-25 19:43:32]

lgtm

http://codereview.chromium.org/8302016/diff/34001/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/8302016/diff/34001/content/public/browser/cont...
content/public/browser/content_browser_client.h:91:
std::vector<BrowserMainParts*>& parts_list) = 0;
On 2011/10/25 19:21:18, Steven Bennetts wrote:
> On 2011/10/25 06:11:28, John Abd-El-Malek wrote:
> > I didn't notice last time that you're passing a non-const reference (against
> > style guide). please make this a pointer to the vector.
> > 
> > nit: parts instead of parts_list
> 
> Done. Also, it's parts_list everywhere except the member variable in
> BrowserMainLoop, so I changed that.

nit: parts_list isn't that accurate since it's not a list container anymore.. i
think parts is just as descriptive since anyone looking at it can see that it's
a container, but up to you.

http://codereview.chromium.org/8302016/diff/40002/content/browser/browser_mai...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/8302016/diff/40002/content/browser/browser_mai...
content/browser/browser_main_loop.h:42: // BrowserMainLoop implementation
nit: remove