Support sharing of ContentMain and BrowserMain code with embedded use cases.

Support sharing of ContentMain and BrowserMain code with embedded use cases.

For the browser use case it is convenient to have a single ContentMain entry point function that handles all initialization, run and shutdown. For embedded use cases it is often necessary to integrate with existing application message loops where initialization and shutdown must be handled separately.

To support sharing of this code the following changes were required:

1. Refactor the ContentMain function to create a ContentMainRunner class containing separate initialization, run and shutdown functions.

2. Refactor the BrowserMain function and BrowserMainLoop class to create a BrowserMainRunner class containing separate initialization, run and shutdown functions.

3. Add a new BrowserMainParts::GetMainMessageLoop method. This is necessary to support creation of a custom MessageLoop implementation while sharing BrowserMainRunner initialization and shutdown code.

BUG=112507
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120574

[Marshall, 2012-01-11 21:04:06]

Hi John,

This is a preliminary patch to implement the functionality that we discussed in
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre....

I've only tested on Windows with content_shell so far. If you agree with the
approach I'll update the rest of Chromium code and verify compile on OS X and
Linux.

Thanks,
Marshall

[jam, 2012-01-12 18:07:18]

CCing Adam to get his feedback, i.e. just to make sure that this solves both
embedder use cases

comments to content_main_runner also apply to browser_main_runner

http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
File content/app/content_main_runner.cc (right):

http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
content/app/content_main_runner.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
can you "svn copy" content_main.cc to this file so that I can see what changed?

http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
File content/app/content_main_runner.h (right):

http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
content/app/content_main_runner.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
if you intend for this to be a public API to be used by embedders, it belongs in
content/public/app

nit: 2012

http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
content/app/content_main_runner.h:32: static ContentMainRunner*
CreateMainRunner();
nit: we usually call these just "Create" since
ContentMainRunner::CreateMainRunner() is redundant

http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
content/app/content_main_runner.h:57: std::string GetProcessType(int argc, const
char** argv);
is this really needed? a lot of the code knows how to get the process type from
the command line arguments, there's not much to be gained by wrapping it with a
function IMO

http://codereview.chromium.org/9190018/diff/6008/content/public/app/content_m...
File content/public/app/content_main_delegate.h (right):

http://codereview.chromium.org/9190018/diff/6008/content/public/app/content_m...
content/public/app/content_main_delegate.h:37: // Gives the embedder an
opportinity to override a known process. Return -1 to
these two functions are the same. why not just make it still RunProcess, and
call it for known + unknown?

http://codereview.chromium.org/9190018/diff/6008/content/public/browser/brows...
File content/public/browser/browser_main_parts.h (right):

http://codereview.chromium.org/9190018/diff/6008/content/public/browser/brows...
content/public/browser/browser_main_parts.h:67: virtual MessageLoop*
GetMainMessageLoop() = 0;
I'm not sure I understand this. I thought CEF/Awesomium wanted to just call
everything before running the message loop, and then they'll pump whenever they
want, and then when they want to quit they'll call everything currently after
running the ML? In that case, you wouldn't want to give a custom ML?

http://codereview.chromium.org/9190018/diff/6008/content/shell/shell.h
File content/shell/shell.h (right):

http://codereview.chromium.org/9190018/diff/6008/content/shell/shell.h#newcode19
content/shell/shell.h:19: // Define this value to test embedded message loop
integration.
adding test code in production code isn't how we do tests, i.e. we'd write a
unit tests for something like this.

[Marshall, 2012-01-12 18:41:57]

Thanks for the review. Please see my comments inline. I'll update the patch
shortly.

On 2012/01/12 18:07:18, John Abd-El-Malek wrote:
> CCing Adam to get his feedback, i.e. just to make sure that this solves both
> embedder use cases
> 
> comments to content_main_runner also apply to browser_main_runner
> 
>
http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
> File content/app/content_main_runner.cc (right):
> 
>
http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
> content/app/content_main_runner.cc:1: // Copyright (c) 2011 The Chromium
> Authors. All rights reserved.
> can you "svn copy" content_main.cc to this file so that I can see what
changed?

OK.

> 
>
http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
> File content/app/content_main_runner.h (right):
> 
>
http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
> content/app/content_main_runner.h:1: // Copyright (c) 2011 The Chromium
Authors.
> All rights reserved.
> if you intend for this to be a public API to be used by embedders, it belongs
in
> content/public/app
> 
> nit: 2012

OK.

> 
>
http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
> content/app/content_main_runner.h:32: static ContentMainRunner*
> CreateMainRunner();
> nit: we usually call these just "Create" since
> ContentMainRunner::CreateMainRunner() is redundant

OK.

> 
>
http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_run...
> content/app/content_main_runner.h:57: std::string GetProcessType(int argc,
const
> char** argv);
> is this really needed? a lot of the code knows how to get the process type
from
> the command line arguments, there's not much to be gained by wrapping it with
a
> function IMO

I'll remove it.

> 
>
http://codereview.chromium.org/9190018/diff/6008/content/public/app/content_m...
> File content/public/app/content_main_delegate.h (right):
> 
>
http://codereview.chromium.org/9190018/diff/6008/content/public/app/content_m...
> content/public/app/content_main_delegate.h:37: // Gives the embedder an
> opportinity to override a known process. Return -1 to
> these two functions are the same. why not just make it still RunProcess, and
> call it for known + unknown?

I made it two separate methods for self documentation purposes (otherwise the
user needs to know which processes are known and which are unknown). I'm OK with
making it a single method if that's what you prefer.

> 
>
http://codereview.chromium.org/9190018/diff/6008/content/public/browser/brows...
> File content/public/browser/browser_main_parts.h (right):
> 
>
http://codereview.chromium.org/9190018/diff/6008/content/public/browser/brows...
> content/public/browser/browser_main_parts.h:67: virtual MessageLoop*
> GetMainMessageLoop() = 0;
> I'm not sure I understand this. I thought CEF/Awesomium wanted to just call
> everything before running the message loop, and then they'll pump whenever
they
> want, and then when they want to quit they'll call everything currently after
> running the ML? In that case, you wouldn't want to give a custom ML?

I believe a MessageLoop always needs to be created because code expects
MessageLoop::current() to return non-NULL (for example, see
BrowserMainLoop::InitializeMainThread). What the embedder wants to control is
when/how the MessageLoop is run. This method lets the embedder return their own
MessageLoop implementation (for example, see the ShellMessageLoopForUI
implementation added in shell_main_delegate.cc) instead of using the default
implementation.

> 
> http://codereview.chromium.org/9190018/diff/6008/content/shell/shell.h
> File content/shell/shell.h (right):
> 
>
http://codereview.chromium.org/9190018/diff/6008/content/shell/shell.h#newcode19
> content/shell/shell.h:19: // Define this value to test embedded message loop
> integration.
> adding test code in production code isn't how we do tests, i.e. we'd write a
> unit tests for something like this.

I agree that the define isn't the right approach. However, in order to test this
functionality we need to use most of the content_shell code. It doesn't look
like we currently have unit test functionality as part of content_shell. Would a
content_shell command-line flag be a reasonable compromise?

[Marshall, 2012-01-12 23:03:12]

Please review this updated patch set that includes all of the requested changes.

The embedded message loop test mode in content_shell is now enabled with a flag
instead of a define.

Thanks,
Marshall

[Adam Simmons, 2012-01-13 05:32:49]

This looks awesome, no critiques here; great job Marshall.

@ John: we also need to define our own MessageLoop in some embedding scenarios.
For example, the default UI MessageLoop on Windows makes calls to
Dispatch/TranslateMessage which may conflict with a WinAPI app that pumps its
own messages.

On 2012/01/12 23:03:12, Marshall wrote:
> Please review this updated patch set that includes all of the requested
changes.
> 
> The embedded message loop test mode in content_shell is now enabled with a
flag
> instead of a define.
> 
> Thanks,
> Marshall

[jam, 2012-01-13 20:42:10]

(small note: in the future, it's slightly better if you can reply to each
point in rietveld instead of by email directly. that way inter-patchset
diffs will show all the comments)

On Thu, Jan 12, 2012 at 10:41 AM, <marshall@chromium.org> wrote:

> Thanks for the review. Please see my comments inline. I'll update the patch
> shortly.
>
>
> On 2012/01/12 18:07:18, John Abd-El-Malek wrote:
>
>> CCing Adam to get his feedback, i.e. just to make sure that this solves
>> both
>> embedder use cases
>>
>
>  comments to content_main_runner also apply to browser_main_runner
>>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
app/content_main_runner.cc<http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_runner.cc>
>
>> File content/app/content_main_**runner.cc (right):
>>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
app/content_main_runner.cc#**newcode1<http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_runner.cc#newcode1>
>
>> content/app/content_main_**runner.cc:1: // Copyright (c) 2011 The
>> Chromium
>> Authors. All rights reserved.
>> can you "svn copy" content_main.cc to this file so that I can see what
>>
> changed?
>
> OK.
>
>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
app/content_main_runner.h<http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_runner.h>
>
>> File content/app/content_main_**runner.h (right):
>>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
app/content_main_runner.h#**newcode1<http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_runner.h#newcode1>
>
>> content/app/content_main_**runner.h:1: // Copyright (c) 2011 The Chromium
>>
> Authors.
>
>> All rights reserved.
>> if you intend for this to be a public API to be used by embedders, it
>> belongs
>>
> in
>
>> content/public/app
>>
>
>  nit: 2012
>>
>
> OK.
>
>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
app/content_main_runner.h#**newcode32<http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_runner.h#newcode32>
>
>> content/app/content_main_**runner.h:32: static ContentMainRunner*
>> CreateMainRunner();
>> nit: we usually call these just "Create" since
>> ContentMainRunner::**CreateMainRunner() is redundant
>>
>
> OK.
>
>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
app/content_main_runner.h#**newcode57<http://codereview.chromium.org/9190018/diff/6008/content/app/content_main_runner.h#newcode57>
>
>> content/app/content_main_**runner.h:57: std::string GetProcessType(int
>> argc,
>>
> const
>
>> char** argv);
>> is this really needed? a lot of the code knows how to get the process type
>>
> from
>
>> the command line arguments, there's not much to be gained by wrapping it
>> with
>>
> a
>
>> function IMO
>>
>
> I'll remove it.
>
>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
public/app/content_main_**delegate.h<http://codereview.chromium.org/9190018/diff/6008/content/public/app/content_main_delegate.h>
>
>> File content/public/app/content_**main_delegate.h (right):
>>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
public/app/content_main_**delegate.h#newcode37<http://codereview.chromium.org/9190018/diff/6008/content/public/app/content_main_delegate.h#newcode37>
>
>> content/public/app/content_**main_delegate.h:37: // Gives the embedder an
>> opportinity to override a known process. Return -1 to
>> these two functions are the same. why not just make it still RunProcess,
>> and
>> call it for known + unknown?
>>
>
> I made it two separate methods for self documentation purposes (otherwise
> the
> user needs to know which processes are known and which are unknown). I'm
> OK with
> making it a single method if that's what you prefer.


please update the existing comment, since now it's out-of-date


>
>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
public/browser/browser_main_**parts.h<http://codereview.chromium.org/9190018/diff/6008/content/public/browser/browser_main_parts.h>
>
>> File content/public/browser/**browser_main_parts.h (right):
>>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
public/browser/browser_main_**parts.h#newcode67<http://codereview.chromium.org/9190018/diff/6008/content/public/browser/browser_main_parts.h#newcode67>
>
>> content/public/browser/**browser_main_parts.h:67: virtual MessageLoop*
>> GetMainMessageLoop() = 0;
>> I'm not sure I understand this. I thought CEF/Awesomium wanted to just
>> call
>> everything before running the message loop, and then they'll pump whenever
>>
> they
>
>> want, and then when they want to quit they'll call everything currently
>> after
>> running the ML? In that case, you wouldn't want to give a custom ML?
>>
>
> I believe a MessageLoop always needs to be created because code expects
> MessageLoop::current() to return non-NULL (for example, see
> BrowserMainLoop::**InitializeMainThread). What the embedder wants to
> control is
> when/how the MessageLoop is run. This method lets the embedder return
> their own
> MessageLoop implementation (for example, see the ShellMessageLoopForUI
> implementation added in shell_main_delegate.cc) instead of using the
> default
> implementation.


I see. ok if this is good enough for you and Adam, it's good enough for me
:)


>
>
>
>  http://codereview.chromium.**org/9190018/diff/6008/content/**
>>
shell/shell.h<http://codereview.chromium.org/9190018/diff/6008/content/shell/shell.h>
>> File content/shell/shell.h (right):
>>
>
>
> http://codereview.chromium.**org/9190018/diff/6008/content/**
>
shell/shell.h#newcode19<http://codereview.chromium.org/9190018/diff/6008/content/shell/shell.h#newcode19>
>
>> content/shell/shell.h:19: // Define this value to test embedded message
>> loop
>> integration.
>> adding test code in production code isn't how we do tests, i.e. we'd
>> write a
>> unit tests for something like this.
>>
>
> I agree that the define isn't the right approach. However, in order to
> test this
> functionality we need to use most of the content_shell code. It doesn't
> look
> like we currently have unit test functionality as part of content_shell.
> Would a
> content_shell command-line flag be a reasonable compromise?
>

sorry if I wasn't clear. a define/command line flag is still the same
thing, because the issue is mixing test code inside non-test code. if you
need to reuse the content shell code in tests, then you'll have to refactor
it to make it usable from test code.

>
>
>
http://codereview.chromium.**org/9190018/<http://codereview.chromium.org/9190...
>

[Marshall, 2012-01-13 22:37:09]

On 2012/01/13 20:42:10, John Abd-El-Malek wrote:
> (small note: in the future, it's slightly better if you can reply to each
> point in rietveld instead of by email directly. that way inter-patchset
> diffs will show all the comments)

I'm not sure I understand what you mean? I responded in the text field on the
codereview page.

[snip]
public/app/content_main_**delegate.h#newcode37<http://codereview.chromium.org/9190018/diff/6008/content/public/app/content_main_delegate.h#newcode37>
> >
> >> content/public/app/content_**main_delegate.h:37: // Gives the embedder an
> >> opportinity to override a known process. Return -1 to
> >> these two functions are the same. why not just make it still RunProcess,
> >> and
> >> call it for known + unknown?
> >>
> >
> > I made it two separate methods for self documentation purposes (otherwise
> > the
> > user needs to know which processes are known and which are unknown). I'm
> > OK with
> > making it a single method if that's what you prefer.
> 
> 
> please update the existing comment, since now it's out-of-date

Done.

[snip]
>
shell/shell.h#newcode19<http://codereview.chromium.org/9190018/diff/6008/content/shell/shell.h#newcode19>
> >
> >> content/shell/shell.h:19: // Define this value to test embedded message
> >> loop
> >> integration.
> >> adding test code in production code isn't how we do tests, i.e. we'd
> >> write a
> >> unit tests for something like this.
> >>
> >
> > I agree that the define isn't the right approach. However, in order to
> > test this
> > functionality we need to use most of the content_shell code. It doesn't
> > look
> > like we currently have unit test functionality as part of content_shell.
> > Would a
> > content_shell command-line flag be a reasonable compromise?
> >
> 
> sorry if I wasn't clear. a define/command line flag is still the same
> thing, because the issue is mixing test code inside non-test code. if you
> need to reuse the content shell code in tests, then you'll have to refactor
> it to make it usable from test code.

It doesn't look like we have any unit tests currently for start-up code
(ContentMain, BrowserMain, BrowserMainLoop, etc). Is there an existing unit test
target or strategy that would be appropriate for this? Would you accept the
content changes without the test component?

[jam, 2012-01-13 23:08:44]

On 2012/01/13 22:37:09, Marshall wrote:
> On 2012/01/13 20:42:10, John Abd-El-Malek wrote:
> > (small note: in the future, it's slightly better if you can reply to each
> > point in rietveld instead of by email directly. that way inter-patchset
> > diffs will show all the comments)
> 
> I'm not sure I understand what you mean? I responded in the text field on the
> codereview page.

really? that's strange, because it didn't show up as a reply there. i.e. compare
that with this:
http://codereview.chromium.org/9188056/diff/103/content/public/browser/web_ui...

> 
> [snip]
>
public/app/content_main_**delegate.h#newcode37<http://codereview.chromium.org/9190018/diff/6008/content/public/app/content_main_delegate.h#newcode37>
> > >
> > >> content/public/app/content_**main_delegate.h:37: // Gives the embedder an
> > >> opportinity to override a known process. Return -1 to
> > >> these two functions are the same. why not just make it still RunProcess,
> > >> and
> > >> call it for known + unknown?
> > >>
> > >
> > > I made it two separate methods for self documentation purposes (otherwise
> > > the
> > > user needs to know which processes are known and which are unknown). I'm
> > > OK with
> > > making it a single method if that's what you prefer.
> > 
> > 
> > please update the existing comment, since now it's out-of-date
> 
> Done.
> 
> [snip]
> >
>
shell/shell.h#newcode19<http://codereview.chromium.org/9190018/diff/6008/content/shell/shell.h#newcode19>
> > >
> > >> content/shell/shell.h:19: // Define this value to test embedded message
> > >> loop
> > >> integration.
> > >> adding test code in production code isn't how we do tests, i.e. we'd
> > >> write a
> > >> unit tests for something like this.
> > >>
> > >
> > > I agree that the define isn't the right approach. However, in order to
> > > test this
> > > functionality we need to use most of the content_shell code. It doesn't
> > > look
> > > like we currently have unit test functionality as part of content_shell.
> > > Would a
> > > content_shell command-line flag be a reasonable compromise?
> > >
> > 
> > sorry if I wasn't clear. a define/command line flag is still the same
> > thing, because the issue is mixing test code inside non-test code. if you
> > need to reuse the content shell code in tests, then you'll have to refactor
> > it to make it usable from test code.
> 
> It doesn't look like we have any unit tests currently for start-up code
> (ContentMain, BrowserMain, BrowserMainLoop, etc). Is there an existing unit
test
> target or strategy that would be appropriate for this? Would you accept the
> content changes without the test component?

There are no direct tests for this area, but it's also hard to test as you can
imagine (i.e. what does it mean to check that pre-run-ml worked? a naive test
which is tied to what the impl currently does wouldn't be that helpful.).
however this is tested indirectly through chrome, because if these stopped
working chrome would stop.

Since this is a code path that won't be exercised by chrome, if we can't get
tests in there for it, you risk having it regress. so tests would be preferable,
although not required by me (it's to your benefit :) ).

I think this would be difficult to test as a unittest because of singletons that
would get created or reused and still leave pointers behind. perhaps a
content_browsertest that ensures that a simple page loads. but i dont know how
far along the content_browsertests framework is.

[jam, 2012-01-24 22:40:39]

btw are you still pursuing this change?

[Marshall, 2012-01-24 22:49:51]

On 2012/01/24 22:40:39, John Abd-El-Malek wrote:
> btw are you still pursuing this change?

I'm sort of stuck choosing between three options:

1. Commit without any specific tests or content_shell integration.
2. Commit with a new test suite based on content_shell.
3. Just keep the changes locally in CEF.

I think #2 would be best, but I'm not sure how to go about it. There's been
mention recently of running layout tests with content_shell -- perhaps this test
could be part of that in some way.

[jam, 2012-01-24 22:55:35]

On Tue, Jan 24, 2012 at 2:49 PM, <marshall@chromium.org> wrote:

> On 2012/01/24 22:40:39, John Abd-El-Malek wrote:
>
>> btw are you still pursuing this change?
>>
>
> I'm sort of stuck choosing between three options:
>
> 1. Commit without any specific tests or content_shell integration.
> 2. Commit with a new test suite based on content_shell.
> 3. Just keep the changes locally in CEF.
>
> I think #2 would be best, but I'm not sure how to go about it. There's been
> mention recently of running layout tests with content_shell -- perhaps
> this test
> could be part of that in some way.
>

If you can write a test, that'd be good _for you_, to prevent others
regressing it. Unfortunately I don't have spare cycles to investigate in
depth how to write such a test.

Otherwise, I think it would still be good to get this landed, since other
embedders want it as well, and keeping it in your branch will be an
ongoing maintenance work for you.

>
>
http://codereview.chromium.**org/9190018/<http://codereview.chromium.org/9190...
>

[Marshall, 2012-01-24 23:18:15]

On 2012/01/24 22:55:35, John Abd-El-Malek wrote:
> If you can write a test, that'd be good _for you_, to prevent others
> regressing it. Unfortunately I don't have spare cycles to investigate in
> depth how to write such a test.
> 
> Otherwise, I think it would still be good to get this landed, since other
> embedders want it as well, and keeping it in your branch will be an
> ongoing maintenance work for you.

OK, I'll stop worrying about the testing aspect for the time being. I probably
won't have a chance to test the changes on all platforms until next week. I'll
keep this patch set updated in the mean time.

[Marshall, 2012-01-31 19:07:16]

Hi John,

Can you give this a final review before I commit? Are there any other try bots I
should run it on first?

Thanks,
Marshall

[jam, 2012-01-31 21:11:28]

http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
content/app/content_main_runner.cc:288: ContentMainRunnerImpl();
since this class is only in the cc file, there's no need to have separate
declarations/definitions. can you just do it all inline?

http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
content/app/content_main_runner.cc:372: #if !defined(OS_ANDROID)
merge issue?

http://codereview.chromium.org/9190018/diff/41010/content/browser/browser_mai...
File content/browser/browser_main_runner.cc (right):

http://codereview.chromium.org/9190018/diff/41010/content/browser/browser_mai...
content/browser/browser_main_runner.cc:27: class BrowserMainRunnerImpl : public
content::BrowserMainRunner {
ditto for inliing these methods

http://codereview.chromium.org/9190018/diff/41010/content/public/browser/brow...
File content/public/browser/browser_main_runner.h (right):

http://codereview.chromium.org/9190018/diff/41010/content/public/browser/brow...
content/public/browser/browser_main_runner.h:35: bool ExitedMainMessageLoop();
why not make this a static method on BrowserMainRunner? and also add
documentation please

[Marshall, 2012-02-01 16:22:32]

On 2012/01/31 21:11:28, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
> File content/app/content_main_runner.cc (right):
> 
>
http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
> content/app/content_main_runner.cc:288: ContentMainRunnerImpl();
> since this class is only in the cc file, there's no need to have separate
> declarations/definitions. can you just do it all inline?
> 
>
http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
> content/app/content_main_runner.cc:372: #if !defined(OS_ANDROID)
> merge issue?
> 
>
http://codereview.chromium.org/9190018/diff/41010/content/browser/browser_mai...
> File content/browser/browser_main_runner.cc (right):
> 
>
http://codereview.chromium.org/9190018/diff/41010/content/browser/browser_mai...
> content/browser/browser_main_runner.cc:27: class BrowserMainRunnerImpl :
public
> content::BrowserMainRunner {
> ditto for inliing these methods
> 
>
http://codereview.chromium.org/9190018/diff/41010/content/public/browser/brow...
> File content/public/browser/browser_main_runner.h (right):
> 
>
http://codereview.chromium.org/9190018/diff/41010/content/public/browser/brow...
> content/public/browser/browser_main_runner.h:35: bool ExitedMainMessageLoop();
> why not make this a static method on BrowserMainRunner? and also add
> documentation please

All of the requested changes are done except for making ExitedMainMessageLoop a
static method of BrowserMainRunner. The single caller of ExitedMainMessageLoop
is content/browser/renderer_host/render_process_host_impl.cc, which I already
have in a different svn change set. I seem to have reached my svn multi-tasking
limits :-). Is it OK if I make the ExitedMainMessageLoop change in a later
commit?

[jam, 2012-02-01 17:19:32]

On 2012/02/01 16:22:32, Marshall wrote:
> On 2012/01/31 21:11:28, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
> > File content/app/content_main_runner.cc (right):
> > 
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
> > content/app/content_main_runner.cc:288: ContentMainRunnerImpl();
> > since this class is only in the cc file, there's no need to have separate
> > declarations/definitions. can you just do it all inline?
> > 
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
> > content/app/content_main_runner.cc:372: #if !defined(OS_ANDROID)
> > merge issue?
> > 
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/browser/browser_mai...
> > File content/browser/browser_main_runner.cc (right):
> > 
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/browser/browser_mai...
> > content/browser/browser_main_runner.cc:27: class BrowserMainRunnerImpl :
> public
> > content::BrowserMainRunner {
> > ditto for inliing these methods
> > 
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/public/browser/brow...
> > File content/public/browser/browser_main_runner.h (right):
> > 
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/public/browser/brow...
> > content/public/browser/browser_main_runner.h:35: bool
ExitedMainMessageLoop();
> > why not make this a static method on BrowserMainRunner? and also add
> > documentation please
> 
> All of the requested changes are done except for making ExitedMainMessageLoop
a
> static method of BrowserMainRunner. The single caller of ExitedMainMessageLoop
> is content/browser/renderer_host/render_process_host_impl.cc, which I already
> have in a different svn change set. I seem to have reached my svn
multi-tasking
> limits :-). Is it OK if I make the ExitedMainMessageLoop change in a later
> commit?

ah, if this isn't called by chrome, then this doesn't belong in content/public.
Can you just put "extern bool..." in
content/browser/renderer_host/render_process_host_impl.cc to access it in this
change?

[jam, 2012-02-01 17:21:13]

On 2012/02/01 17:19:32, John Abd-El-Malek wrote:
> On 2012/02/01 16:22:32, Marshall wrote:
> > On 2012/01/31 21:11:28, John Abd-El-Malek wrote:
> > >
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
> > > File content/app/content_main_runner.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
> > > content/app/content_main_runner.cc:288: ContentMainRunnerImpl();
> > > since this class is only in the cc file, there's no need to have separate
> > > declarations/definitions. can you just do it all inline?
> > > 
> > >
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/app/content_main_ru...
> > > content/app/content_main_runner.cc:372: #if !defined(OS_ANDROID)
> > > merge issue?
> > > 
> > >
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/browser/browser_mai...
> > > File content/browser/browser_main_runner.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/browser/browser_mai...
> > > content/browser/browser_main_runner.cc:27: class BrowserMainRunnerImpl :
> > public
> > > content::BrowserMainRunner {
> > > ditto for inliing these methods
> > > 
> > >
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/public/browser/brow...
> > > File content/public/browser/browser_main_runner.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9190018/diff/41010/content/public/browser/brow...
> > > content/public/browser/browser_main_runner.h:35: bool
> ExitedMainMessageLoop();
> > > why not make this a static method on BrowserMainRunner? and also add
> > > documentation please
> > 
> > All of the requested changes are done except for making
ExitedMainMessageLoop
> a
> > static method of BrowserMainRunner. The single caller of
ExitedMainMessageLoop
> > is content/browser/renderer_host/render_process_host_impl.cc, which I
already
> > have in a different svn change set. I seem to have reached my svn
> multi-tasking
> > limits :-). Is it OK if I make the ExitedMainMessageLoop change in a later
> > commit?
> 
> ah, if this isn't called by chrome, then this doesn't belong in
content/public.
> Can you just put "extern bool..." in
> content/browser/renderer_host/render_process_host_impl.cc to access it in this
> change?

also, please can you double check how you're replying to comments? your replies
are the only ones i only see in email and not in rietveld. Can you hit  the
"Reply" link beside "Done" in comments like these
http://codereview.chromium.org/9190018/diff/41010/content/public/browser/brow...

[jam, 2012-02-01 17:28:39]

http://codereview.chromium.org/9190018/diff/48001/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

http://codereview.chromium.org/9190018/diff/48001/content/app/content_main_ru...
content/app/content_main_runner.cc:317: #else   // !OS_WIN
nit only two spaces before //

http://codereview.chromium.org/9190018/diff/48001/content/app/content_main_ru...
content/app/content_main_runner.cc:321: is_initialized_ = true;
nit: just move this common line (with 302) to 339

http://codereview.chromium.org/9190018/diff/48001/content/browser/browser_mai...
File content/browser/browser_main_runner.cc (right):

http://codereview.chromium.org/9190018/diff/48001/content/browser/browser_mai...
content/browser/browser_main_runner.cc:66: // !!!!!!!!!! READ ME !!!!!!!!!!
actually this comment is so out of date especially now :) can you remove it
(line 66-85)

[Marshall, 2012-02-01 19:37:13]

All changes done as requested. I think I even replied correctly this time :-).

http://codereview.chromium.org/9190018/diff/48001/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

http://codereview.chromium.org/9190018/diff/48001/content/app/content_main_ru...
content/app/content_main_runner.cc:317: #else   // !OS_WIN
On 2012/02/01 17:28:39, John Abd-El-Malek wrote:
> nit only two spaces before //

Done.

http://codereview.chromium.org/9190018/diff/48001/content/app/content_main_ru...
content/app/content_main_runner.cc:321: is_initialized_ = true;
On 2012/02/01 17:28:39, John Abd-El-Malek wrote:
> nit: just move this common line (with 302) to 339

Done.

http://codereview.chromium.org/9190018/diff/48001/content/browser/browser_mai...
File content/browser/browser_main_runner.cc (right):

http://codereview.chromium.org/9190018/diff/48001/content/browser/browser_mai...
content/browser/browser_main_runner.cc:66: // !!!!!!!!!! READ ME !!!!!!!!!!
On 2012/02/01 17:28:39, John Abd-El-Malek wrote:
> actually this comment is so out of date especially now :) can you remove it
> (line 66-85)

Done.

[Marshall, 2012-02-02 19:58:57]

On 2012/02/01 17:19:32, John Abd-El-Malek wrote:
> On 2012/02/01 16:22:32, Marshall wrote:
> > All of the requested changes are done except for making
ExitedMainMessageLoop
> a
> > static method of BrowserMainRunner. The single caller of
ExitedMainMessageLoop
> > is content/browser/renderer_host/render_process_host_impl.cc, which I
already
> > have in a different svn change set. I seem to have reached my svn
> multi-tasking
> > limits :-). Is it OK if I make the ExitedMainMessageLoop change in a later
> > commit?
> 
> ah, if this isn't called by chrome, then this doesn't belong in
content/public.
> Can you just put "extern bool..." in
> content/browser/renderer_host/render_process_host_impl.cc to access it in this
> change?

Done. I believe this completes all requested changes.

[jam, 2012-02-02 21:43:35]

lgtm, thanks for following this through, this change is in high demand by many
groups :)

http://codereview.chromium.org/9190018/diff/52001/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/9190018/diff/52001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:318: extern bool
g_exited_main_message_loop;
nit: usually i see externs at the top of the file

[Marshall, 2012-02-02 22:02:10]

http://codereview.chromium.org/9190018/diff/52001/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/9190018/diff/52001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:318: extern bool
g_exited_main_message_loop;
On 2012/02/02 21:43:36, John Abd-El-Malek wrote:
> nit: usually i see externs at the top of the file

Done.

[commit-bot: I haz the power, 2012-02-02 22:02:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marshall@chromium.org/9190018/43048

[commit-bot: I haz the power, 2012-02-02 22:02:44]

Can't apply patch for file content/app/content_main_runner.cc.
While running patch -p0 --forward --force;
patching file content/app/content_main_runner.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 27.
Hunk #3 FAILED at 41.
Hunk #4 FAILED at 75.
Hunk #5 FAILED at 260.
Hunk #6 FAILED at 279.
6 out of 6 hunks FAILED -- saving rejects to file
content/app/content_main_runner.cc.rej

[commit-bot: I haz the power, 2012-02-03 15:30:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marshall@chromium.org/9190018/43092

[commit-bot: I haz the power, 2012-02-03 15:30:23]

Can't apply patch for file content/app/content_main_runner.cc.
While running patch -p0 --forward --force;
patching file content/app/content_main_runner.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 27.
Hunk #3 FAILED at 41.
Hunk #4 FAILED at 75.
Hunk #5 FAILED at 260.
Hunk #6 FAILED at 279.
6 out of 6 hunks FAILED -- saving rejects to file
content/app/content_main_runner.cc.rej