Allow overriding of X error functions

Allow overriding of X error functions
BUG=50006
(and various other reports)
TEST=Run chrome under nested window manager using Xephyr (see
http://code.google.com/p/chromium/wiki/LayoutTestsLinux)

use --enable-logging=stderr --log-level=0
kill xephyr
examine log. You should see 
X IO Error detected
followed (not necessarily immediately) by 
successfully saved /tmp/tx/Default/Preferences
successfully saved /tmp/tx/Local State
successfully saved /tmp/tx/Local State
successfully saved /tmp/tx/Default/Preferences
along with no crash.

There is a high ranking crash report on both linux and chromeos that happens whenever X sends an error to chrome. This change causes us to log and continue when we get a regular error from X. When we get an IO error, indicating X is gone, we attempt to shut down gracefully.

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

[Evan Martin, 2010-08-24 21:26:56]

For TEST=, can you verify that shutting down X causes us to shut down cleanly? 
You can use a nested X server, see
http://code.google.com/p/chromium/wiki/LayoutTestsLinux , and then just "kill
`pidof Xephyr`" to simulate X death.

http://codereview.chromium.org/3175038/diff/4001/5001
File app/x11_util.cc (right):

http://codereview.chromium.org/3175038/diff/4001/5001#newcode56
app/x11_util.cc:56: int X11ErrorHandler(Display* d, _XErrorEvent* e) {
Maybe rename these to "*Default*X11ErrorHandler"?
Also, should they be declared "static"?

http://codereview.chromium.org/3175038/diff/4001/5002
File app/x11_util.h (right):

http://codereview.chromium.org/3175038/diff/4001/5002#newcode35
app/x11_util.h:35: } _XErrorEvent;
This is ugly, but I see the X header doesn't allow forward declares.  :(

This feels fragile to me.  Why not just make it void*, and then cast it back at
the call site?  You can still do the typedef.  E.g.
typedef void _XErrorEvent;
along with a comment about how you can't forward-declare the right type.

http://codereview.chromium.org/3175038/diff/4001/5004
File chrome/browser/browser_list.cc (left):

http://codereview.chromium.org/3175038/diff/4001/5004#oldcode321
chrome/browser/browser_list.cc:321: NOTIMPLEMENTED();
Shouldn't we do the same thing on Linux here?

http://codereview.chromium.org/3175038/diff/4001/5005
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3175038/diff/4001/5005#newcode682
chrome/browser/browser_main.cc:682: static bool in_x11_io_error_handler = false;
Maybe put all this in an anon namespace near the top of the file?  style guide
frowns on "static" declarations.

Can you put a comment on this variable?

I think we use the pattern "g_foobar" for a static variable that is global, even
if it's global to the file.

http://codereview.chromium.org/3175038/diff/4001/5005#newcode1172
chrome/browser/browser_main.cc:1172: #if defined (OS_LINUX)
no space after "defined"

Is this the right place for setting this?  The location feels kinda random.

http://codereview.chromium.org/3175038/diff/4001/5006
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/3175038/diff/4001/5006#newcode269
chrome/browser/browser_process_impl.cc:269: // Can't run a local loop on linux.
Doesn't this mean the code will behave significantly differently?  Why is this
OK?  Should we have a NOTIMPLEMENTED here?

[Lei Zhang, 2010-08-24 21:55:38]

http://codereview.chromium.org/3175038/diff/4001/5006
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/3175038/diff/4001/5006#newcode269
chrome/browser/browser_process_impl.cc:269: // Can't run a local loop on linux.
On 2010/08/24 21:26:56, Evan Martin wrote:
> Doesn't this mean the code will behave significantly differently?  Why is this
> OK?  Should we have a NOTIMPLEMENTED here?

This code is deceiving. It gets built on all platforms, but it's only called
from BrowserList::WindowsSessionEnding(), which is then only called from Win32
only code. So it's never used on other platforms and doesn't work as is.

Another thing you can do here is:

bool old_state = MessageLoop::current()->NestableTasksAllowed();
MessageLoop::current()->SetNestableTasksAllowed(true);
MessageLoop::current()->Run();
MessageLoop::current()->SetNestableTasksAllowed(old_state);

but when I did this in a now abandoned code review [1], Darin asked me why I
needed this because it's not needed on Windows, and I never got around to
figuring it out.

Willchan's comment on my previous CL is that we're using two separate shutdown
paths, and ideally we should only have one.

[1] http://codereview.chromium.org/1576024/show

[willchan no longer on Chromium, 2010-08-25 17:36:09]

Yes, as Lei pointed out, I said that, and I still think it's right.  That
Windows only session ending control path is broken and evil.

http://codereview.chromium.org/3175038/diff/4001/5001
File app/x11_util.cc (right):

http://codereview.chromium.org/3175038/diff/4001/5001#newcode81
app/x11_util.cc:81: XSetIOErrorHandler(io_error_handler ? io_error_handler
:X11IOErrorHandler);
You forgot a space after the : here.

[DaveMoore, 2010-08-25 18:02:46]

http://codereview.chromium.org/3175038/diff/4001/5001
File app/x11_util.cc (right):

http://codereview.chromium.org/3175038/diff/4001/5001#newcode56
app/x11_util.cc:56: int X11ErrorHandler(Display* d, _XErrorEvent* e) {
Rename done. They're already in a private namespace, along with other members. I
thought the chrome way was to not make them static (they weren't before I made
my change).

On 2010/08/24 21:26:56, Evan Martin wrote:
> Maybe rename these to "*Default*X11ErrorHandler"?
> Also, should they be declared "static"?

http://codereview.chromium.org/3175038/diff/4001/5002
File app/x11_util.h (right):

http://codereview.chromium.org/3175038/diff/4001/5002#newcode35
app/x11_util.h:35: } _XErrorEvent;
On 2010/08/24 21:26:56, Evan Martin wrote:
> This is ugly, but I see the X header doesn't allow forward declares.  :(
> 
> This feels fragile to me.  Why not just make it void*, and then cast it back
at
> the call site?  You can still do the typedef.  E.g.
> typedef void _XErrorEvent;
> along with a comment about how you can't forward-declare the right type.

Done.

http://codereview.chromium.org/3175038/diff/4001/5004
File chrome/browser/browser_list.cc (left):

http://codereview.chromium.org/3175038/diff/4001/5004#oldcode321
chrome/browser/browser_list.cc:321: NOTIMPLEMENTED();
I changed it to call exit().
On 2010/08/24 21:26:56, Evan Martin wrote:
> Shouldn't we do the same thing on Linux here?

http://codereview.chromium.org/3175038/diff/4001/5005
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3175038/diff/4001/5005#newcode682
chrome/browser/browser_main.cc:682: static bool in_x11_io_error_handler = false;
On 2010/08/24 21:26:56, Evan Martin wrote:
> Maybe put all this in an anon namespace near the top of the file?  style guide
> frowns on "static" declarations.
> 
> Can you put a comment on this variable?
> 
> I think we use the pattern "g_foobar" for a static variable that is global,
even
> if it's global to the file.

Done.

http://codereview.chromium.org/3175038/diff/4001/5005#newcode1172
chrome/browser/browser_main.cc:1172: #if defined (OS_LINUX)
Done on extra space. I put it here because it's the profile we want to be sure
to write. I added a comment.
On 2010/08/24 21:26:56, Evan Martin wrote:
> no space after "defined"
> 
> Is this the right place for setting this?  The location feels kinda random.

http://codereview.chromium.org/3175038/diff/4001/5006
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/3175038/diff/4001/5006#newcode269
chrome/browser/browser_process_impl.cc:269: // Can't run a local loop on linux.
I had tried doing as you suggest but I get errors of the following form:
GLib: g_main_context_prepare() called recursively from within a source's check()
or prepare() member.
GLib: g_main_context_check() called recursively from within a source's check()
or prepare() member.

These happen whether or not I attempt to allow nestable tasks.
On 2010/08/24 21:55:39, Lei Zhang wrote:
> On 2010/08/24 21:26:56, Evan Martin wrote:
> > Doesn't this mean the code will behave significantly differently?  Why is
this
> > OK?  Should we have a NOTIMPLEMENTED here?
> 
> This code is deceiving. It gets built on all platforms, but it's only called
> from BrowserList::WindowsSessionEnding(), which is then only called from Win32
> only code. So it's never used on other platforms and doesn't work as is.
> 
> Another thing you can do here is:
> 
> bool old_state = MessageLoop::current()->NestableTasksAllowed();
> MessageLoop::current()->SetNestableTasksAllowed(true);
> MessageLoop::current()->Run();
> MessageLoop::current()->SetNestableTasksAllowed(old_state);
> 
> but when I did this in a now abandoned code review [1], Darin asked me why I
> needed this because it's not needed on Windows, and I never got around to
> figuring it out.
> 
> Willchan's comment on my previous CL is that we're using two separate shutdown
> paths, and ideally we should only have one.
> 
> [1] http://codereview.chromium.org/1576024/show

http://codereview.chromium.org/3175038/diff/4001/5006#newcode269
chrome/browser/browser_process_impl.cc:269: // Can't run a local loop on linux.
The code behaves differently. I don't know why Windows needs the local loop.
Linux seems to work fine w/out it.
On 2010/08/24 21:26:56, Evan Martin wrote:
> Doesn't this mean the code will behave significantly differently?  Why is this
> OK?  Should we have a NOTIMPLEMENTED here?

[Evan Martin, 2010-08-25 18:15:53]

http://codereview.chromium.org/3175038/diff/17001/18004
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/3175038/diff/17001/18004#newcode323
chrome/browser/browser_list.cc:323: exit(ResultCodes::NORMAL_EXIT);
exit() runs the atexit handlers.  I think we might want to use _exit() here.

http://codereview.chromium.org/3175038/diff/17001/18006
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/3175038/diff/17001/18006#newcode270
chrome/browser/browser_process_impl.cc:270: MessageLoop::current()->Run();
I believe this is to wait for the profile/metrics shutdown.

*i)->MarkAsCleanShutdown() above calls, through a soup of layers:
  SavePersistentPrefs
  WritePrefs
  WriteNow()
   file_message_loop_proxy_->PostTask(FROM_HERE,
                                     new WriteToDiskTask(path_, data));

So the write happens on a background thread.  We have to wait for that to
complete before we exit.

[DaveMoore, 2010-08-25 18:55:15]

http://codereview.chromium.org/3175038/diff/17001/18004
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/3175038/diff/17001/18004#newcode323
chrome/browser/browser_list.cc:323: exit(ResultCodes::NORMAL_EXIT);
On 2010/08/25 18:15:54, Evan Martin wrote:
> exit() runs the atexit handlers.  I think we might want to use _exit() here.

I've removed the exit() call. The main message loop will exit when the Quit
message is processed.

http://codereview.chromium.org/3175038/diff/17001/18006
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/3175038/diff/17001/18006#newcode270
chrome/browser/browser_process_impl.cc:270: MessageLoop::current()->Run();
I've made chrome not exit within BrowserList::WindowsSessionEnding() on linux,
as we can't run local message loops. The main loop will exit when the Quit
message is processed.
On 2010/08/25 18:15:54, Evan Martin wrote:
> I believe this is to wait for the profile/metrics shutdown.
> 
> *i)->MarkAsCleanShutdown() above calls, through a soup of layers:
>   SavePersistentPrefs
>   WritePrefs
>   WriteNow()
>    file_message_loop_proxy_->PostTask(FROM_HERE,
>                                      new WriteToDiskTask(path_, data));
> 
> So the write happens on a background thread.  We have to wait for that to
> complete before we exit.

[Evan Martin, 2010-08-25 18:56:53]

LGTM, one last nit

http://codereview.chromium.org/3175038/diff/22001/23004
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/3175038/diff/22001/23004#newcode326
chrome/browser/browser_list.cc:326: // BrowserProcessImpl::EndSession().
Can you leave the
#else
NOTIMPLEMENTED()

for Mac's benefit?

[Andrew T Wilson (Slow), 2010-08-25 19:09:12]

http://codereview.chromium.org/3175038/diff/22001/23004
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/3175038/diff/22001/23004#newcode305
chrome/browser/browser_list.cc:305: BrowserList::CloseAllBrowsers(false);
I don't think it's cool to shutdown without calling CloseAllBrowsers(). There's
lots of shutdown stuff that's triggered from that routine (sending out
APP_TERMINATING to shutdown extension background pages/trigger session restore,
call to AllBrowsersClosedAndAppExiting(), etc). 

I think a better way to handle this would be to gracefully deal with X errors
while shutting down browsers, or pre-close the browsers before calling
CloseAllBrowsers(), etc.

[brettw, 2010-08-26 00:08:02]

Does this mean if X crashes/exits (control-alt-backspace?) then Chrome will shut
down "cleanly" and I won't get session restore?

[DaveMoore, 2010-08-26 05:15:30]

That is the goal.
On 2010/08/26 00:08:02, brettw wrote:
> Does this mean if X crashes/exits (control-alt-backspace?) then Chrome will
shut
> down "cleanly" and I won't get session restore?

[DaveMoore, 2010-09-02 19:48:19]

There were a few debug checks I needed to make more specific, and I made us wait
with a timeout for the file thread to write its data, in case there was a
deadlock.

http://codereview.chromium.org/3175038/diff/22001/23004
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/3175038/diff/22001/23004#newcode305
chrome/browser/browser_list.cc:305: BrowserList::CloseAllBrowsers(false);
I've pulled the notification parts out of CloseAllBrowsers() in this case.
On 2010/08/25 19:09:12, Andrew T Wilson wrote:
> I don't think it's cool to shutdown without calling CloseAllBrowsers().
There's
> lots of shutdown stuff that's triggered from that routine (sending out
> APP_TERMINATING to shutdown extension background pages/trigger session
restore,
> call to AllBrowsersClosedAndAppExiting(), etc). 
> 
> I think a better way to handle this would be to gracefully deal with X errors
> while shutting down browsers, or pre-close the browsers before calling
> CloseAllBrowsers(), etc.

http://codereview.chromium.org/3175038/diff/22001/23004#newcode326
chrome/browser/browser_list.cc:326: // BrowserProcessImpl::EndSession().
On 2010/08/25 18:56:53, Evan Martin wrote:
> Can you leave the
> #else
> NOTIMPLEMENTED()
> 
> for Mac's benefit?

Done.

[darin (slow to review), 2010-09-08 18:16:14]

http://codereview.chromium.org/3175038/diff/35001/36001
File app/x11_util.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36001#newcode77
app/x11_util.cc:77: // consumers of x11_util.h to have to include XLib.h which
conflicts
nit: XLib.h -> Xlib.h

http://codereview.chromium.org/3175038/diff/35001/36002
File app/x11_util.h (right):

http://codereview.chromium.org/3175038/diff/35001/36002#newcode27
app/x11_util.h:27: typedef int (*_XErrorHandler) (Display*, void*);
nit: no space between "(" and ")"

http://codereview.chromium.org/3175038/diff/35001/36002#newcode27
app/x11_util.h:27: typedef int (*_XErrorHandler) (Display*, void*);
this is a pretty ugly hack.  how about just forking the error handler business
out into its own .h and .cc files.  that way you can have the .h include Xlib.h.
 afterall, any consumer of SetX11ErrorHandlers will need Xlib.h.

x11_util could still define a SetDefaultX11ErrorHandlers function.

http://codereview.chromium.org/3175038/diff/35001/36004
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36004#newcode306
chrome/browser/browser_list.cc:306: #if defined(OS_LINUX)
i don't understand why this logic needs to be linux specific.  it seems like it
will lead to more problems if our shutdown behavior isn't as consistent as
possible between platforms.

http://codereview.chromium.org/3175038/diff/35001/36004#newcode316
chrome/browser/browser_list.cc:316: // If there are no browsers, send the
APP_TERMINATING action here. Otherwise,
this comment seems to be from CloseAllBrowsers, where it actually does a check
of browsers_.empty before calling Notify.  here you are not doing that check,
which means that the comment is wrong.

perhaps on OS_LINUX you just want to have another parameter to CloseAllBrowsers
that forces it to go down the AllBrowsersClosedAndAppExiting path even if all
browsers aren't closed yet?

forking the code here seems bad.

http://codereview.chromium.org/3175038/diff/35001/36004#newcode336
chrome/browser/browser_list.cc:336: #if defined(OS_WIN)
this feels like something that should be abstracted into process_util.h.  we
shouldn't need #ifdefs here.

base::Exit(int result_code);

http://codereview.chromium.org/3175038/diff/35001/36005
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36005#newcode1185
chrome/browser/browser_main.cc:1185: #if defined(OS_LINUX)
perhaps this should be #if defined(USE_X11) instead?

http://codereview.chromium.org/3175038/diff/35001/36006
File chrome/browser/browser_main_gtk.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36006#newcode7
chrome/browser/browser_main_gtk.cc:7: #include "X11/Xlib.h"
nit: include using angle brackets

http://codereview.chromium.org/3175038/diff/35001/36006#newcode56
chrome/browser/browser_main_gtk.cc:56: static bool g_in_x11_io_error_handler =
false;
it is usually best practice to put static data and static helpers at the top of
the file, leaving the bottom of the file for external functions.

though not strictly required, convention in chromium is to use an anonymous
namespace instead of the static keyword.

http://codereview.chromium.org/3175038/diff/35001/36006#newcode60
chrome/browser/browser_main_gtk.cc:60: XErrorEvent* e =
static_cast<XErrorEvent*>(error);
it seems like it would be nice to just call the default X11ErrorHandler here
instead of duplicating the code for error logging here.

i realize that you want LOG(ERROR) and that code uses LOG(FATAL).  maybe there
is a way around that issue.  it is definitely bad to duplicate code though.

http://codereview.chromium.org/3175038/diff/35001/36007
File chrome/browser/browser_main_gtk.h (right):

http://codereview.chromium.org/3175038/diff/35001/36007#newcode16
chrome/browser/browser_main_gtk.h:16: 
nit: extra new line here?

http://codereview.chromium.org/3175038/diff/35001/36008
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36008#newcode77
chrome/browser/browser_process_impl.cc:77: #if defined(OS_LINUX)
maybe this should be USE_X11 also?

http://codereview.chromium.org/3175038/diff/35001/36008#newcode256
chrome/browser/browser_process_impl.cc:256: #if defined(OS_WIN) ||
defined(OS_LINUX)
maybe this should be USE_X11 too?

http://codereview.chromium.org/3175038/diff/35001/36008#newcode280
chrome/browser/browser_process_impl.cc:280:
g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
nit: I recommend switching this code over to use ChromeThread::PostTask instead.

have you considered using the same code on all platforms here?  the block until
signaled approach should work on windows and mac as well.  also the comment in
the !OS_LINUX section equally applies to OS_LINUX so it should be moved.

http://codereview.chromium.org/3175038/diff/35001/36009
File chrome/browser/chromeos/tab_closeable_state_watcher.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36009#newcode77
chrome/browser/chromeos/tab_closeable_state_watcher.cc:77: if
(browser_shutdown::GetShutdownType() != browser_shutdown::END_SESSION)
i'm very concerned about how the expected code paths are not being taken now. 
there could be more subtle dependencies on this ordering.

at the very least, from a code cleanliness point of view, it seems like it would
be better to have a function in the browser_shutdown namespace that answers the
question of whether or not we are in a state where we'd expect things to have
shutdown cleanly.

i'm not sure how to describe such a function.  maybe
browser_shutdown::IsShuttingDownCleanly?

then, without #ifdefs, you could write:

  if (browser_shutdown::IsShuttingDownCleanly())
    DCHECK(tabstrip_watchers_.empty());

i'm not sure if "cleanly" is the write description here.

http://codereview.chromium.org/3175038/diff/35001/36010
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36010#newcode972
chrome/browser/views/bookmark_bar_view.cc:972: // In normal shutdown The
bookmark model should never be deleted before us.
same as in the tabstrip watchers case.  this should not be #ifdef'd code.

[DaveMoore, 2010-09-08 23:25:29]

http://codereview.chromium.org/3175038/diff/35001/36001
File app/x11_util.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36001#newcode77
app/x11_util.cc:77: // consumers of x11_util.h to have to include XLib.h which
conflicts
On 2010/09/08 18:16:14, darin wrote:
> nit: XLib.h -> Xlib.h

Done.

http://codereview.chromium.org/3175038/diff/35001/36002
File app/x11_util.h (right):

http://codereview.chromium.org/3175038/diff/35001/36002#newcode27
app/x11_util.h:27: typedef int (*_XErrorHandler) (Display*, void*);
On 2010/09/08 18:16:14, darin wrote:
> nit: no space between "(" and ")"

Done.

http://codereview.chromium.org/3175038/diff/35001/36002#newcode27
app/x11_util.h:27: typedef int (*_XErrorHandler) (Display*, void*);
I moved the declaration to x11_util_internal.h which includes XLib.h and removed
the hack.
On 2010/09/08 18:16:14, darin wrote:
> this is a pretty ugly hack.  how about just forking the error handler business
> out into its own .h and .cc files.  that way you can have the .h include
Xlib.h.
>  afterall, any consumer of SetX11ErrorHandlers will need Xlib.h.
> 
> x11_util could still define a SetDefaultX11ErrorHandlers function.

http://codereview.chromium.org/3175038/diff/35001/36004
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36004#newcode306
chrome/browser/browser_list.cc:306: #if defined(OS_LINUX)
If we could run the message loop here on linux we would. Instead we've
introduced a timeout. I don't think that's as good as the windows version but
it's the best we can do on linux. I don't think we should make the windows
version worse in the name of consistency.

On 2010/09/08 18:16:14, darin wrote:
> i don't understand why this logic needs to be linux specific.  it seems like
it
> will lead to more problems if our shutdown behavior isn't as consistent as
> possible between platforms.

http://codereview.chromium.org/3175038/diff/35001/36004#newcode316
chrome/browser/browser_list.cc:316: // If there are no browsers, send the
APP_TERMINATING action here. Otherwise,
I've removed the use_post param and made this method private. It checks the
browser_shutdown::GetShutdownType() and handles it differently on linux. So no
code needed to be forked.
On 2010/09/08 18:16:14, darin wrote:
> this comment seems to be from CloseAllBrowsers, where it actually does a check
> of browsers_.empty before calling Notify.  here you are not doing that check,
> which means that the comment is wrong.
> 
> perhaps on OS_LINUX you just want to have another parameter to
CloseAllBrowsers
> that forces it to go down the AllBrowsersClosedAndAppExiting path even if all
> browsers aren't closed yet?
> 
> forking the code here seems bad.

http://codereview.chromium.org/3175038/diff/35001/36004#newcode336
chrome/browser/browser_list.cc:336: #if defined(OS_WIN)
These things are pretty different from each other. ExitProcess() calls each
dll's entry point with DLL_PROCESS_DETACH, although the active threads don't get
DLL_THREAD_DETACH.

I think the strangenesses around exit are best handled by whoever is calling it.
Abstracting it could lead to surprising behavior.

On 2010/09/08 18:16:14, darin wrote:
> this feels like something that should be abstracted into process_util.h.  we
> shouldn't need #ifdefs here.
> 
> base::Exit(int result_code);

http://codereview.chromium.org/3175038/diff/35001/36005
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36005#newcode1185
chrome/browser/browser_main.cc:1185: #if defined(OS_LINUX)
On 2010/09/08 18:16:14, darin wrote:
> perhaps this should be #if defined(USE_X11) instead?

Done.

http://codereview.chromium.org/3175038/diff/35001/36006
File chrome/browser/browser_main_gtk.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36006#newcode7
chrome/browser/browser_main_gtk.cc:7: #include "X11/Xlib.h"
On 2010/09/08 18:16:14, darin wrote:
> nit: include using angle brackets

Done.

http://codereview.chromium.org/3175038/diff/35001/36006#newcode56
chrome/browser/browser_main_gtk.cc:56: static bool g_in_x11_io_error_handler =
false;
On 2010/09/08 18:16:14, darin wrote:
> it is usually best practice to put static data and static helpers at the top
of
> the file, leaving the bottom of the file for external functions.
> 
> though not strictly required, convention in chromium is to use an anonymous
> namespace instead of the static keyword.

Done.

http://codereview.chromium.org/3175038/diff/35001/36006#newcode60
chrome/browser/browser_main_gtk.cc:60: XErrorEvent* e =
static_cast<XErrorEvent*>(error);
On 2010/09/08 18:16:14, darin wrote:
> it seems like it would be nice to just call the default X11ErrorHandler here
> instead of duplicating the code for error logging here.
> 
> i realize that you want LOG(ERROR) and that code uses LOG(FATAL).  maybe there
> is a way around that issue.  it is definitely bad to duplicate code though.

Done.

http://codereview.chromium.org/3175038/diff/35001/36007
File chrome/browser/browser_main_gtk.h (right):

http://codereview.chromium.org/3175038/diff/35001/36007#newcode16
chrome/browser/browser_main_gtk.h:16: 
On 2010/09/08 18:16:14, darin wrote:
> nit: extra new line here?

Done.

http://codereview.chromium.org/3175038/diff/35001/36008
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36008#newcode77
chrome/browser/browser_process_impl.cc:77: #if defined(OS_LINUX)
On 2010/09/08 18:16:14, darin wrote:
> maybe this should be USE_X11 also?

Done.

http://codereview.chromium.org/3175038/diff/35001/36008#newcode256
chrome/browser/browser_process_impl.cc:256: #if defined(OS_WIN) ||
defined(OS_LINUX)
On 2010/09/08 18:16:14, darin wrote:
> maybe this should be USE_X11 too?

Done.

http://codereview.chromium.org/3175038/diff/35001/36008#newcode280
chrome/browser/browser_process_impl.cc:280:
g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
nit: Done
Wait instead of loop on windows: see comment above
Comment: Done

On 2010/09/08 18:16:14, darin wrote:
> nit: I recommend switching this code over to use ChromeThread::PostTask
instead.
> 
> have you considered using the same code on all platforms here?  the block
until
> signaled approach should work on windows and mac as well.  also the comment in
> the !OS_LINUX section equally applies to OS_LINUX so it should be moved.

http://codereview.chromium.org/3175038/diff/35001/36009
File chrome/browser/chromeos/tab_closeable_state_watcher.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36009#newcode77
chrome/browser/chromeos/tab_closeable_state_watcher.cc:77: if
(browser_shutdown::GetShutdownType() != browser_shutdown::END_SESSION)
I added browser_shutdown::ShuttingDownWithoutClosingBrowsers()

On 2010/09/08 18:16:14, darin wrote:
> i'm very concerned about how the expected code paths are not being taken now. 
> there could be more subtle dependencies on this ordering.
> 
> at the very least, from a code cleanliness point of view, it seems like it
would
> be better to have a function in the browser_shutdown namespace that answers
the
> question of whether or not we are in a state where we'd expect things to have
> shutdown cleanly.
> 
> i'm not sure how to describe such a function.  maybe
> browser_shutdown::IsShuttingDownCleanly?
> 
> then, without #ifdefs, you could write:
> 
>   if (browser_shutdown::IsShuttingDownCleanly())
>     DCHECK(tabstrip_watchers_.empty());
> 
> i'm not sure if "cleanly" is the write description here.

http://codereview.chromium.org/3175038/diff/35001/36010
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/3175038/diff/35001/36010#newcode972
chrome/browser/views/bookmark_bar_view.cc:972: // In normal shutdown The
bookmark model should never be deleted before us.
On 2010/09/08 18:16:14, darin wrote:
> same as in the tabstrip watchers case.  this should not be #ifdef'd code.

Done.

[darin (slow to review), 2010-09-10 16:39:01]

http://codereview.chromium.org/3175038/diff/53001/28012
File app/x11_util.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28012#newcode70
app/x11_util.cc:70: std::string GetErrorEventDescription(XErrorEvent* e) {
nit: it seems like it would be nice if all of the functions declared in
x11_util_internal.h were grouped together.

http://codereview.chromium.org/3175038/diff/53001/28014
File app/x11_util_internal.h (right):

http://codereview.chromium.org/3175038/diff/53001/28014#newcode23
app/x11_util_internal.h:23: // NOTE: these function caches the results and must
be called from the UI
given the placement of this comment, it would appear to apply to everything
defined in this header file.

maybe this file would benefit from some division comments?

// ---------------------------------- (out to 80 chars)
// NOTE: these functions cache their results and must...

... GetRenderARGB32Format ...
... GetRenderVisualFormat ...

// ---------------------------------- (out to 80 chars)
// X11 error handling

...

http://codereview.chromium.org/3175038/diff/53001/28014#newcode35
app/x11_util_internal.h:35: // Sets the X Error Handlers. Passing NULL for
either will cause the process
nit: "Passing NULL for either will enable the default error handler, which if
called will log the error and abort the process."

http://codereview.chromium.org/3175038/diff/53001/28014#newcode41
app/x11_util_internal.h:41: std::string GetErrorEventDescription(XErrorEvent*
e);
nit: "e" -> "error_event"

http://codereview.chromium.org/3175038/diff/53001/28015
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28015#newcode73
chrome/app/chrome_dll_main.cc:73: #include "app/x11_util_internal.h"
wouldn't it be nice to define a SetDefaultX11ErrorHandlers() method in
x11_util.h so that you don't need to include x11_util_internal.h here?  it seems
risky to include Xlib.h in cross platform .cc files.  this might cause grief for
someone developing on Windows or Mac.

http://codereview.chromium.org/3175038/diff/53001/28017
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28017#newcode231
chrome/browser/browser_list.cc:231: #if defined(OS_LINUX)
is this really only for OS_LINUX or USE_X11?

http://codereview.chromium.org/3175038/diff/53001/28018
File chrome/browser/browser_list.h (right):

http://codereview.chromium.org/3175038/diff/53001/28018#newcode108
chrome/browser/browser_list.h:108: static void CloseAllBrowsers();
nit: add a new line below here to separate the functions

http://codereview.chromium.org/3175038/diff/53001/28020
File chrome/browser/browser_main_gtk.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28020#newcode7
chrome/browser/browser_main_gtk.cc:7: #include <X11/Xlib.h>
nit: you can kill this header now

http://codereview.chromium.org/3175038/diff/53001/28020#newcode23
chrome/browser/browser_main_gtk.cc:23: namespace {
nit: new line below "namespace {"

http://codereview.chromium.org/3175038/diff/53001/28020#newcode43
chrome/browser/browser_main_gtk.cc:43: }
nit: new line above "}", and please add this comment:

}  // namespace

http://codereview.chromium.org/3175038/diff/53001/28021
File chrome/browser/browser_main_gtk.h (right):

http://codereview.chromium.org/3175038/diff/53001/28021#newcode5
chrome/browser/browser_main_gtk.h:5: // Contains functions used by BrowserMain()
that are win32-specific.
nit: win32 -> gtk

http://codereview.chromium.org/3175038/diff/53001/28023
File chrome/browser/browser_shutdown.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28023#newcode293
chrome/browser/browser_shutdown.cc:293: #if defined(OS_LINUX)
OS_LINUX -> USE_X11

http://codereview.chromium.org/3175038/diff/53001/28029
File chrome/gpu/gpu_thread.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28029#newcode61
chrome/gpu/gpu_thread.cc:61: x11_util::SetX11ErrorHandlers(NULL, NULL);
please use x11_util::SetDefaultX11ErrorHandlers() here

http://codereview.chromium.org/3175038/diff/53001/28030
File chrome/plugin/plugin_thread.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28030#newcode91
chrome/plugin/plugin_thread.cc:91: x11_util::SetX11ErrorHandlers(NULL, NULL);
SetDefaultX11ErrorHandlers()

[DaveMoore, 2010-09-10 19:05:41]

http://codereview.chromium.org/3175038/diff/53001/28012
File app/x11_util.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28012#newcode70
app/x11_util.cc:70: std::string GetErrorEventDescription(XErrorEvent* e) {
On 2010/09/10 16:39:02, darin wrote:
> nit: it seems like it would be nice if all of the functions declared in
> x11_util_internal.h were grouped together.

Done.

http://codereview.chromium.org/3175038/diff/53001/28014
File app/x11_util_internal.h (right):

http://codereview.chromium.org/3175038/diff/53001/28014#newcode23
app/x11_util_internal.h:23: // NOTE: these function caches the results and must
be called from the UI
On 2010/09/10 16:39:02, darin wrote:
> given the placement of this comment, it would appear to apply to everything
> defined in this header file.
> 
> maybe this file would benefit from some division comments?
> 
> // ---------------------------------- (out to 80 chars)
> // NOTE: these functions cache their results and must...
> 
> ... GetRenderARGB32Format ...
> ... GetRenderVisualFormat ...
> 
> // ---------------------------------- (out to 80 chars)
> // X11 error handling
> 
> ...

Done.

http://codereview.chromium.org/3175038/diff/53001/28014#newcode35
app/x11_util_internal.h:35: // Sets the X Error Handlers. Passing NULL for
either will cause the process
On 2010/09/10 16:39:02, darin wrote:
> nit: "Passing NULL for either will enable the default error handler, which if
> called will log the error and abort the process."

Done.

http://codereview.chromium.org/3175038/diff/53001/28014#newcode41
app/x11_util_internal.h:41: std::string GetErrorEventDescription(XErrorEvent*
e);
On 2010/09/10 16:39:02, darin wrote:
> nit: "e" -> "error_event"

Done.

http://codereview.chromium.org/3175038/diff/53001/28015
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28015#newcode73
chrome/app/chrome_dll_main.cc:73: #include "app/x11_util_internal.h"
On 2010/09/10 16:39:02, darin wrote:
> wouldn't it be nice to define a SetDefaultX11ErrorHandlers() method in
> x11_util.h so that you don't need to include x11_util_internal.h here?  it
seems
> risky to include Xlib.h in cross platform .cc files.  this might cause grief
for
> someone developing on Windows or Mac.

Done.

http://codereview.chromium.org/3175038/diff/53001/28017
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28017#newcode231
chrome/browser/browser_list.cc:231: #if defined(OS_LINUX)
On 2010/09/10 16:39:02, darin wrote:
> is this really only for OS_LINUX or USE_X11?

Done.

http://codereview.chromium.org/3175038/diff/53001/28018
File chrome/browser/browser_list.h (right):

http://codereview.chromium.org/3175038/diff/53001/28018#newcode108
chrome/browser/browser_list.h:108: static void CloseAllBrowsers();
On 2010/09/10 16:39:02, darin wrote:
> nit: add a new line below here to separate the functions

Done.

http://codereview.chromium.org/3175038/diff/53001/28020
File chrome/browser/browser_main_gtk.cc (right):

http://codereview.chromium.org/3175038/diff/53001/28020#newcode7
chrome/browser/browser_main_gtk.cc:7: #include <X11/Xlib.h>
On 2010/09/10 16:39:02, darin wrote:
> nit: you can kill this header now

Done.

http://codereview.chromium.org/3175038/diff/53001/28020#newcode43
chrome/browser/browser_main_gtk.cc:43: }
On 2010/09/10 16:39:02, darin wrote:
> nit: new line above "}", and please add this comment:
> 
> }  // namespace

Done.

http://codereview.chromium.org/3175038/diff/53001/28021
File chrome/browser/browser_main_gtk.h (right):

http://codereview.chromium.org/3175038/diff/53001/28021#newcode5
chrome/browser/browser_main_gtk.h:5: // Contains functions used by BrowserMain()
that are win32-specific.
On 2010/09/10 16:39:02, darin wrote:
> nit: win32 -> gtk

Done.

[darin (slow to review), 2010-09-10 20:25:04]

LGTM