Small cleanup for ChromeBrowserMain:

Small cleanup for ChromeBrowserMain:
-add ChromeBrowserMainExtraPartsX11 to get rid of the x11 specific calls in ChromeBrowserMain that are right beside ChromeBrowserMainExtraParts calls
-move win specific stuff out of ChromeBrowserMain and into ChromeBrowserMainWin
-remove the static methods in chrome_browser_main.h in favor of private methods in chrome_browser_main.h
-remove the unnecessary toolkit_extra_parts.h header, it doesn't help much since each method was named differently anyways

R=thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210884

[jam, 2013-07-10 00:45:29]

Nico: picked you randomly out of the chrome/OWNERS file so that you can look out
for this stuff when reviewing changes to chrome_browser_main

[tfarina, 2013-07-10 00:48:37]

https://codereview.chromium.org/18967002/diff/14004/chrome/browser/toolkit_ex...
File chrome/browser/toolkit_extra_parts.h (left):

https://codereview.chromium.org/18967002/diff/14004/chrome/browser/toolkit_ex...
chrome/browser/toolkit_extra_parts.h:6: #define
CHROME_BROWSER_TOOLKIT_EXTRA_PARTS_H_
I added this. How are you doing this now?

[tfarina, 2013-07-10 00:50:50]

Copying Ben (just in case), since he reviewed my toolkit_extra_parts.h changes.

https://codereview.chromium.org/18967002/diff/14004/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18967002/diff/14004/chrome/browser/DEPS#newco...
chrome/browser/DEPS:73:
"+chrome/browser/ui/views/chrome_browser_main_extra_parts_views.h",
no, the toolkit_extra_parts.h was done exactly with this purpose in mind. I'd
rather keep it if you don't mind. Is it causing you any trouble?

[jam, 2013-07-10 00:52:44]

https://codereview.chromium.org/18967002/diff/14004/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18967002/diff/14004/chrome/browser/DEPS#newco...
chrome/browser/DEPS:73:
"+chrome/browser/ui/views/chrome_browser_main_extra_parts_views.h",
On 2013/07/10 00:50:50, tfarina wrote:
> no, the toolkit_extra_parts.h was done exactly with this purpose in mind. I'd
> rather keep it if you don't mind. Is it causing you any trouble?

I don't see how that header serves any purpose. all it does is declare a method
that's defined in ui/views. I saw the linked bug, if ui/views gets split into a
component, then it'll have a header which declares this method.

[tfarina, 2013-07-10 00:54:19]

On Tue, Jul 9, 2013 at 9:52 PM,  <jam@chromium.org> wrote:
>
> https://codereview.chromium.org/18967002/diff/14004/chrome/browser/DEPS
> File chrome/browser/DEPS (right):
>
>
https://codereview.chromium.org/18967002/diff/14004/chrome/browser/DEPS#newco...
> chrome/browser/DEPS:73:
> "+chrome/browser/ui/views/chrome_browser_main_extra_parts_views.h",
> On 2013/07/10 00:50:50, tfarina wrote:
>>
>> no, the toolkit_extra_parts.h was done exactly with this purpose in
>
> mind. I'd
>>
>> rather keep it if you don't mind. Is it causing you any trouble?
>
>
> I don't see how that header serves any purpose. all it does is declare a
> method that's defined in ui/views.
defined in chrome/browser/ui/views. There are two things here, we
don't want the ifdefs and we don't want to include
chrome/browser/ui/views here.

-- 
Thiago

[jam, 2013-07-10 00:59:52]

On 2013/07/10 00:54:19, tfarina wrote:
> On Tue, Jul 9, 2013 at 9:52 PM,  <mailto:jam@chromium.org> wrote:
> >
> > https://codereview.chromium.org/18967002/diff/14004/chrome/browser/DEPS
> > File chrome/browser/DEPS (right):
> >
> >
>
https://codereview.chromium.org/18967002/diff/14004/chrome/browser/DEPS#newco...
> > chrome/browser/DEPS:73:
> > "+chrome/browser/ui/views/chrome_browser_main_extra_parts_views.h",
> > On 2013/07/10 00:50:50, tfarina wrote:
> >>
> >> no, the toolkit_extra_parts.h was done exactly with this purpose in
> >
> > mind. I'd
> >>
> >> rather keep it if you don't mind. Is it causing you any trouble?
> >
> >
> > I don't see how that header serves any purpose. all it does is declare a
> > method that's defined in ui/views.
> defined in chrome/browser/ui/views.

(that's what I meant, I was just trying to avoid typing the whole thing)

> There are two things here, we
> don't want the ifdefs 

There are already ifdefs in calling the right method from the header, so i don't
see the harm in having two sets of ifdefs instead of one set in
chrome_content_browser_client.cc.

> and we don't want to include
> chrome/browser/ui/views here.

like i said earlier, using linkage to define a component's boundary is
confusing. to be specific, i mean declaring a method in chrome/browser and
defining it in chrome/browser/ui/views. normally when we split something into a
component, either we put public headers in foo/public (like content and webkit),
or we put private stuff into an internal namespace (like net and others). but we
don't put a header in chrome/ and expect another module to define that method.

> 
> -- 
> Thiago

[tfarina, 2013-07-10 01:13:07]

On Tue, Jul 9, 2013 at 9:59 PM,  <jam@chromium.org> wrote:
I understand your point but you didn't reply to my concern, is it
causing you any problem right now? If it isn't, why would you regress
the progress of it? If you need, can you come with a better solution?


--
Thiago

[jam, 2013-07-10 01:19:59]

On 2013/07/10 01:13:07, tfarina wrote:
> On Tue, Jul 9, 2013 at 9:59 PM,  <mailto:jam@chromium.org> wrote:
> I understand your point but you didn't reply to my concern, is it
> causing you any problem right now? If it isn't, why would you regress
> the progress of it?

My point was that using the header isn't progress, and so I don't think this is
regressing anything.

> If you need, can you come with a better solution?

I don't see anything being broken with src/chrome allowed to include a header
from src/chrome/browser/ui/views, instead of the former declaring that method
and the latter defining it.

When ui/views gets split into a component, then a header will be needed to
accomplish for either of the approaches above.

> 
> 
> --
> Thiago

[tfarina, 2013-07-10 01:24:09]

On Tue, Jul 9, 2013 at 10:19 PM,  <jam@chromium.org> wrote:
> On 2013/07/10 01:13:07, tfarina wrote:
>
>> On Tue, Jul 9, 2013 at 9:59 PM,  <mailto:jam@chromium.org> wrote:
>> I understand your point but you didn't reply to my concern, is it
>> causing you any problem right now? If it isn't, why would you regress
>> the progress of it?
>
>
> My point was that using the header isn't progress, and so I don't think this
> is
> regressing anything.
>
>
>> If you need, can you come with a better solution?
>
>
> I don't see anything being broken with src/chrome allowed to include a
> header
> from src/chrome/browser/ui/views,
there is a circular dependency. browser library includes
browser_ui_views and browser_ui_views includes browser library.

but I won't block you, I just wanted to get to the point, if possible,
where we find a better design for this. May be there isn't. Since you
are more experienced than me. I won't complain about 'regressing' (yes
I still consider it a regression) it anymore.

-- 
Thiago

[jam, 2013-07-10 01:28:35]

On 2013/07/10 01:24:09, tfarina wrote:
> there is a circular dependency. browser library includes
> browser_ui_views and browser_ui_views includes browser library.

The dependency already exists through the method that's declared in
chrome/browser and is implemented in chrome/browser/ui/views. It's just hidden
through this extra header.

[tfarina, 2013-07-10 01:41:14]

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:595: main_parts->AddParts(new
ChromeBrowserMainExtraPartsGtk());
would be possible to have a factory method? 

main_parts->AddParts(ChromeBrowserMainExtraParts::Create());

I think we have gyp filename exclusions for foo_gtk.cc, foo_views.cc,
foo_ash.cc, foo_aura.cc, foo_x11.cc.

Would be that worth? Or is it too much effort that the gain is little?

Hum, maybe that would still hide it anyway :(

If there is no way, go with what you have.

[jam, 2013-07-10 05:50:58]

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:595: main_parts->AddParts(new
ChromeBrowserMainExtraPartsGtk());
On 2013/07/10 01:41:14, tfarina wrote:
> would be possible to have a factory method? 
> 
> main_parts->AddParts(ChromeBrowserMainExtraParts::Create());
> 
> I think we have gyp filename exclusions for foo_gtk.cc, foo_views.cc,
> foo_ash.cc, foo_aura.cc, foo_x11.cc.
> 
> Would be that worth? Or is it too much effort that the gain is little?

two things
-there could be more than one extra parts per config. in that case, one factory
method wouldn't work
-a factory method is exactly the approach i'm trying to remove, since it's
declaring a method in one "component" and defining it n another

> 
> Hum, maybe that would still hide it anyway :(
> 
> If there is no way, go with what you have.

[Nico, 2013-07-10 17:32:50]

The CL description is great and made reviewing this much easier, thanks.

Two minor things:

1. Make sure you checked with Ben that he is happy with the DEPS change.
2. I don't know if dropping RecordBreakpadRegistration() is safe (+thestig) on
Linux and Mac.

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/DEPS#newco...
chrome/browser/DEPS:74:
"+chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h",
This seems to run counter to https://crbug.com/125846 . Do you intend to clean
this up again?

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:724: #endif
I would've guessed that this is here to make sure it runs as the first extra
part (and chrome_content_browser_client.cc now adds the extra x11 part as last
component). Then again, it was added in
https://chromiumcodereview.appspot.com/17084014 by oshima and neither that CL
description nor the current code have a comment to that end, so I suppose this
is fine, but +oshima just in case.

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1475: base::debug::BeingDebugged());
On Mac and Linux, this used to call
metrics->RecordBreakpadRegistration(IsCrashReporterEnabled()) too. Can that call
just be dropped?

Add BUG=139023 for the Android version.

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_win.cc:194: // TODO(viettrungluu): why don't
we run this earlier?
This now runs earlier, maybe this comment can go now?

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_win.cc:209: if (do_first_run_tasks() &&
!profile()->IsOffTheRecord()) {
(+gab to check that do_first_run_tasks_ isn't reset anywhere. From what I can
tell, it's written correctly at this point and isn't reset anywhere, so this is
fine.)

[jam, 2013-07-10 17:47:02]

On 2013/07/10 17:32:50, Nico wrote:
> The CL description is great and made reviewing this much easier, thanks.
> 
> Two minor things:
> 
> 1. Make sure you checked with Ben that he is happy with the DEPS change.

yep (per IM)

> 2. I don't know if dropping RecordBreakpadRegistration() is safe (+thestig) on
> Linux and Mac.

I didn't drop it. I moved the generic part to chrome_browser_main.cc. The
Linux/Mac specific parts are in chrome/browser/chrome_browser_main_mac.mm and
chrome/browser/chrome_browser_main_linux.cc. see PostProfileInit in those files.

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/DEPS#newco...
chrome/browser/DEPS:74:
"+chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h",
On 2013/07/10 17:32:50, Nico wrote:
> This seems to run counter to https://crbug.com/125846 . Do you intend to clean
> this up again?

see the discussion between tfarina and me here. adding an extra header and
declaring a method in one component and definining it in another didn't solve
anything (i.e. it wouldn't link), it just made hid the dependency through an
intermediate file

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:724: #endif
On 2013/07/10 17:32:50, Nico wrote:
> I would've guessed that this is here to make sure it runs as the first extra
> part (and chrome_content_browser_client.cc now adds the extra x11 part as last
> component). Then again, it was added in
> https://chromiumcodereview.appspot.com/17084014 by oshima and neither that CL
> description nor the current code have a comment to that end, so I suppose this
> is fine, but +oshima just in case.

I didn't see any indication that it was put here because of that. if it really
needs to be run before other extra parts, then we can change the order that it's
added

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1475: base::debug::BeingDebugged());
On 2013/07/10 17:32:50, Nico wrote:
> On Mac and Linux, this used to call
> metrics->RecordBreakpadRegistration(IsCrashReporterEnabled()) too. Can that
call
> just be dropped?

not quite. mac and linux used to do this as well, see
line 154
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...

and line 86
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
> 
> Add BUG=139023 for the Android version.

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_win.cc:194: // TODO(viettrungluu): why don't
we run this earlier?
On 2013/07/10 17:32:50, Nico wrote:
> This now runs earlier, maybe this comment can go now?

only slightly earlier. i think vtl's comment was why doesn't it run in the
beginning, which this doesn't address

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_win.cc:209: if (do_first_run_tasks() &&
!profile()->IsOffTheRecord()) {
On 2013/07/10 17:32:50, Nico wrote:
> (+gab to check that do_first_run_tasks_ isn't reset anywhere. From what I can
> tell, it's written correctly at this point and isn't reset anywhere, so this
is
> fine.)

right, it's not reset. (i had checked that)

[gab, 2013-07-10 17:48:57]

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_win.cc:209: if (do_first_run_tasks() &&
!profile()->IsOffTheRecord()) {
On 2013/07/10 17:47:02, jam wrote:
> On 2013/07/10 17:32:50, Nico wrote:
> > (+gab to check that do_first_run_tasks_ isn't reset anywhere. From what I
can
> > tell, it's written correctly at this point and isn't reset anywhere, so this
> is
> > fine.)
> 
> right, it's not reset. (i had checked that)

Ya, it's never reset, let me know if you want me to look at the rest of the CL.

Cheers!
Gab

[Nico, 2013-07-10 17:50:52]

lgtm!

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.cc:146: #endif
(and this)

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_mac.mm (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_mac.mm:277: IsCrashReporterEnabled());
Ah, I missed this.

[oshima, 2013-07-10 18:50:37]

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:724: #endif
On 2013/07/10 17:47:02, jam wrote:
> On 2013/07/10 17:32:50, Nico wrote:
> > I would've guessed that this is here to make sure it runs as the first extra
> > part (and chrome_content_browser_client.cc now adds the extra x11 part as
last
> > component). Then again, it was added in
> > https://chromiumcodereview.appspot.com/17084014 by oshima and neither that
CL
> > description nor the current code have a comment to that end, so I suppose
this
> > is fine, but +oshima just in case.
> 
> I didn't see any indication that it was put here because of that. if it really
> needs to be run before other extra parts, then we can change the order that
it's
> added

SetBrowserX11ErrorHandlersPreEarlyInitialization should be called as early as
possible (but less critical)
and SetBrowserX11ErrorHandlersPostMainMessageLoopStart() should be called as
late as possible (more critical).

in which order X11 parts will be called?

[jam, 2013-07-11 16:42:07]

On 2013/07/10 18:50:37, oshima wrote:
>
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
> File chrome/browser/chrome_browser_main.cc (left):
> 
>
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
> chrome/browser/chrome_browser_main.cc:724: #endif
> On 2013/07/10 17:47:02, jam wrote:
> > On 2013/07/10 17:32:50, Nico wrote:
> > > I would've guessed that this is here to make sure it runs as the first
extra
> > > part (and chrome_content_browser_client.cc now adds the extra x11 part as
> last
> > > component). Then again, it was added in
> > > https://chromiumcodereview.appspot.com/17084014 by oshima and neither that
> CL
> > > description nor the current code have a comment to that end, so I suppose
> this
> > > is fine, but +oshima just in case.
> > 
> > I didn't see any indication that it was put here because of that. if it
really
> > needs to be run before other extra parts, then we can change the order that
> it's
> > added
> 
> SetBrowserX11ErrorHandlersPreEarlyInitialization should be called as early as
> possible (but less critical)
> and SetBrowserX11ErrorHandlersPostMainMessageLoopStart() should be called as
> late as possible (more critical).
> 
> in which order X11 parts will be called?

The X11 parts is currently the last added one, see
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_con...

are you sure it needs to run before other extra parts? if so we can move the
extra parts creation order, but i just want to know if we need to do that or not
before doing so

[petarj, 2013-07-11 23:31:04]

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.cc:145:
g_browser_process->metrics_service()->RecordBreakpadRegistration(false);
You can not use g_browser_process variable here, since it is not
defined (chrome/browser/browser_process.h is not included).
I.e. this breaks the build if USE_LINUX_BREAKPAD is not defined.

Can you please fix this?

Thank you.

[oshima, 2013-07-11 23:50:08]

On 2013/07/11 16:42:07, jam wrote:
> On 2013/07/10 18:50:37, oshima wrote:
> >
>
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
> > File chrome/browser/chrome_browser_main.cc (left):
> > 
> >
>
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
> > chrome/browser/chrome_browser_main.cc:724: #endif
> > On 2013/07/10 17:47:02, jam wrote:
> > > On 2013/07/10 17:32:50, Nico wrote:
> > > > I would've guessed that this is here to make sure it runs as the first
> extra
> > > > part (and chrome_content_browser_client.cc now adds the extra x11 part
as
> > last
> > > > component). Then again, it was added in
> > > > https://chromiumcodereview.appspot.com/17084014 by oshima and neither
that
> > CL
> > > > description nor the current code have a comment to that end, so I
suppose
> > this
> > > > is fine, but +oshima just in case.
> > > 
> > > I didn't see any indication that it was put here because of that. if it
> really
> > > needs to be run before other extra parts, then we can change the order
that
> > it's
> > > added
> > 
> > SetBrowserX11ErrorHandlersPreEarlyInitialization should be called as early
as
> > possible (but less critical)
> > and SetBrowserX11ErrorHandlersPostMainMessageLoopStart() should be called as
> > late as possible (more critical).
> > 
> > in which order X11 parts will be called?
> 
> The X11 parts is currently the last added one, see
>
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_con...

O, the order for SetBrowserX11ErrorHandlersPreEarlyInitialization is less
important, but
SetBrowserX11ErrorHandlersPostMainMessageLoopStart() has to be called after all
initializations
are done, so the current order is fine.

> are you sure it needs to run before other extra parts? if so we can move the
> extra parts creation order, but i just want to know if we need to do that or
not
> before doing so


If we can someone guarantee/add check so that we can catch if someone change the
order of the X11 parts,
that'd be good.

[jam, 2013-07-12 00:06:05]

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.cc:145:
g_browser_process->metrics_service()->RecordBreakpadRegistration(false);
On 2013/07/11 23:31:04, petarj wrote:
> You can not use g_browser_process variable here, since it is not
> defined (chrome/browser/browser_process.h is not included).
> I.e. this breaks the build if USE_LINUX_BREAKPAD is not defined.
> 
> Can you please fix this?
> 
> Thank you.

derat has fixed this

[Nico, 2013-07-12 03:41:16]

On Thu, Jul 11, 2013 at 4:50 PM, <oshima@chromium.org> wrote:

> On 2013/07/11 16:42:07, jam wrote:
>
>> On 2013/07/10 18:50:37, oshima wrote:
>> >
>>
>
> https://codereview.chromium.**org/18967002/diff/45002/**
>
chrome/browser/chrome_browser_**main.cc<https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_browser_main.cc>
>
>> > File chrome/browser/chrome_browser_**main.cc (left):
>> >
>> >
>>
>
> https://codereview.chromium.**org/18967002/diff/45002/**
>
chrome/browser/chrome_browser_**main.cc#oldcode724<https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_browser_main.cc#oldcode724>
>
>> > chrome/browser/chrome_browser_**main.cc:724: #endif
>> > On 2013/07/10 17:47:02, jam wrote:
>> > > On 2013/07/10 17:32:50, Nico wrote:
>> > > > I would've guessed that this is here to make sure it runs as the
>> first
>> extra
>> > > > part (and chrome_content_browser_client.**cc now adds the extra
>> x11 part
>>
> as
>
>> > last
>> > > > component). Then again, it was added in
>> > > >
https://chromiumcodereview.**appspot.com/17084014<https://chromiumcodereview....
oshima and neither
>>
> that
>
>> > CL
>> > > > description nor the current code have a comment to that end, so I
>>
> suppose
>
>> > this
>> > > > is fine, but +oshima just in case.
>> > >
>> > > I didn't see any indication that it was put here because of that. if
>> it
>> really
>> > > needs to be run before other extra parts, then we can change the order
>>
> that
>
>> > it's
>> > > added
>> >
>> > SetBrowserX11ErrorHandlersPreE**arlyInitialization should be called as
>> early
>>
> as
>
>> > possible (but less critical)
>> > and SetBrowserX11ErrorHandlersPost**MainMessageLoopStart() should be
>> called as
>> > late as possible (more critical).
>> >
>> > in which order X11 parts will be called?
>>
>
>  The X11 parts is currently the last added one, see
>>
>
> https://codereview.chromium.**org/18967002/diff/45002/**
>
chrome/browser/chrome_content_**browser_client.cc<https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_content_browser_client.cc>
>
> O, the order for SetBrowserX11ErrorHandlersPreE**arlyInitialization is
> less
> important, but
> SetBrowserX11ErrorHandlersPost**MainMessageLoopStart() has to be called
> after all
> initializations
> are done, so the current order is fine.
>
>
>  are you sure it needs to run before other extra parts? if so we can move
>> the
>> extra parts creation order, but i just want to know if we need to do that
>> or
>>
> not
>
>> before doing so
>>
>
>
> If we can someone guarantee/add check so that we can catch if someone
> change the
> order of the X11 parts,
> that'd be good.
>

Worst case, add a comment that this is important. (Would've been nice if
there had been a comment for this already :-) )


>
>
>
>
>
https://codereview.chromium.**org/18967002/<https://codereview.chromium.org/1...
>