Monitor sleep/wake on Mac. Should fix some networking issues arising after sl...

Monitor sleep/wake on Mac. Should fix some networking issues arising after sleep.

BUG=29669
TEST=turn logging up; see logging about power being suspended/resumed when Mac sleeps/awakes


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

[Mark Mentovai, 2010-11-24 18:52:44]

I don’t know if “TEST=as in bug” works for this one, because the bug doesn’t
contain any specific testing instructions.

Your TEST= can be something end-to-end involving socket data from
about:net-internals, or it can be about watching for output like this from
system_monitor.cc:

VLOG(1) << L"Power Suspending";

While I’m picking on that, there’s no reason for any of the LOGging strings in
system_monitor.cc to use wchar_t*. You can get rid of all of those Ls.

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm
File app/system_monitor_mac.mm (right):

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm#ne...
app/system_monitor_mac.mm:7: #import <Cocoa/Cocoa.h>
Use <AppKit/AppKit.h> for this one. I’m trying to keep these scoped just a
little bit more narrowly when possible.

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm#ne...
app/system_monitor_mac.mm:27: // See QA1340
<http://developer.apple.com/library/mac/#qa/qa2004/qa1340.html>
80.

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm#ne...
app/system_monitor_mac.mm:33: object:NULL];
This is an |id|, use nil here and on line 38.

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm#ne...
app/system_monitor_mac.mm:61: void SystemMonitor::PlatformInit() {
PlatformInit and PlatformDestroy aren’t static methods on SystemMonitor. You
shouldn’t have g_system_monitor_bridge. You should have a member variable on
SystemMonitor instead. The Objective-C bridge object belongs to the
SystemMonitor, not to this file, and it’s someone else’s problem that the
application is limited elsewhere to one SystemMonitor instance.

[Avi (use Gerrit), 2010-11-24 19:02:15]

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm
File app/system_monitor_mac.mm (right):

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm#ne...
app/system_monitor_mac.mm:27: // See QA1340
<http://developer.apple.com/library/mac/#qa/qa2004/qa1340.html>
On 2010/11/24 18:52:44, Mark Mentovai wrote:
> 80.

I thought we could let URLs slide? Or do we want to put it on its own line?

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm#ne...
app/system_monitor_mac.mm:61: void SystemMonitor::PlatformInit() {
Something like the 

#if OBJC
@class bridge
#else
class bridge
#endif

deal? We've done that with parameters of that type before, but have we done that
with member variables? The ODR keeps dancing in my head when I think of that.

[Mark Mentovai, 2010-11-24 19:13:35]

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm
File app/system_monitor_mac.mm (right):

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm#ne...
app/system_monitor_mac.mm:27: // See QA1340
<http://developer.apple.com/library/mac/#qa/qa2004/qa1340.html>
Avi wrote:
> On 2010/11/24 18:52:44, Mark Mentovai wrote:
> > 80.
> 
> I thought we could let URLs slide? Or do we want to put it on its own line?

You can let a URL slide, but if you can make it fit within 80 by putting it on
its own line, you should.

http://codereview.chromium.org/5310005/diff/7001/app/system_monitor_mac.mm#ne...
app/system_monitor_mac.mm:61: void SystemMonitor::PlatformInit() {
Avi wrote:
> Something like the 
> 
> #if OBJC
> @class bridge
> #else
> class bridge
> #endif
> 
> deal?

Yup, but:

#ifdef __OBJC__

and use semicolons.

> We've done that with parameters of that type before, but have we done that
> with member variables? The ODR keeps dancing in my head when I think of that.

If you’re worried about ODR, parameters and member variables (and return types
and anything else) should be analyzed in exactly the same way. Yes, we’ve
already got instances where we use this trick for member variables, see
base/file_version_info_mac.h for one example. It also comes up fairly regularly
in WebKit.

The reason that this isn’t an ODR violation is that the D in the rule stands for
Definition, not Declaration. The declarations are equivalent in the sense that
they’re sufficient to deal properly with a pointer to type “bridge.” It’ll
always be an error to dereference a “bridge*” when all you’ve got is the
non-__OBJC__ class declaration, because there isn’t any matching definition. The
definition has an @ on it, there’s only one of them, and (because of the @) it
only makes sense in an .mm. C++ is restricted to shuffling pointers around,
which is safe, and is all that we need it to do. An attempt to do anything more
with it in C++-land is a compile-time error, which is exactly what we want it to
be.

Would void* and casts make you feel better? (I hope not.)

[Avi (use Gerrit), 2010-11-24 19:24:09]

I didn't see that we were already doing this. Yay. No, going void* would make me
feel bad but in a different way.

For your review.

[Mark Mentovai, 2010-11-24 19:29:36]

LGTM