message_pump_libevent refactor:

message_pump_libevent refactor:
* Unify WatchSocket & WatchFileHandle.
* Better encapsulate libevent.
* Fix a bug with blocking writes in ipc_posix.cc

[jeremy, 2008-12-12 17:18:15]

Fixes for dkegel's comments, ready for another look...

[pink (ping after 24hrs), 2008-12-12 17:40:49]

http://codereview.chromium.org/13757/diff/23/214
File base/message_pump_libevent.cc (right):

http://codereview.chromium.org/13757/diff/23/214#newcode20
Line 20: if (-1 == flags)
use |if (flags == -1)|. The constant coming first is jarring and impairs
readability.

http://codereview.chromium.org/13757/diff/23/214#newcode135
Line 135: event_set(evt.get(), fd, event_mask | EV_READ,
initial indent here looks messed up.

http://codereview.chromium.org/13757/diff/23/215
File base/message_pump_libevent.h (right):

http://codereview.chromium.org/13757/diff/23/215#newcode28
Line 28: // NOTE: These methods aren't called StartWatching()/StopWatching() to
s/to to/to/

http://codereview.chromium.org/13757/diff/23/215#newcode36
Line 36: // Called by MessagePumpLibevent, ownership of e is transferred to this
use pipes around |e|, eg "ownership of |e| is..." to make it more clear.

http://codereview.chromium.org/13757/diff/23/215#newcode45
Line 45: event *event_;
comment this as a strong, or owning, reference. Why not use a scoped_ptr if
that's the case?

[jeremy, 2008-12-12 18:04:10]

All fixed.
dkegel: lgty?

http://codereview.chromium.org/13757/diff/23/214
File base/message_pump_libevent.cc (right):

http://codereview.chromium.org/13757/diff/23/214#newcode20
Line 20: if (-1 == flags)
On 2008/12/12 17:40:49, pink wrote:
> use |if (flags == -1)|. The constant coming first is jarring and impairs
> readability.

Done.

http://codereview.chromium.org/13757/diff/23/214#newcode135
Line 135: event_set(evt.get(), fd, event_mask | EV_READ,
On 2008/12/12 17:40:49, pink wrote:
> initial indent here looks messed up.

Done.

http://codereview.chromium.org/13757/diff/23/215
File base/message_pump_libevent.h (right):

http://codereview.chromium.org/13757/diff/23/215#newcode28
Line 28: // NOTE: These methods aren't called StartWatching()/StopWatching() to
On 2008/12/12 17:40:49, pink wrote:
> s/to to/to/

Done.

http://codereview.chromium.org/13757/diff/23/215#newcode36
Line 36: // Called by MessagePumpLibevent, ownership of e is transferred to this
On 2008/12/12 17:40:49, pink wrote:
> use pipes around |e|, eg "ownership of |e| is..." to make it more clear.

Done.

http://codereview.chromium.org/13757/diff/23/215#newcode45
Line 45: event *event_;
On 2008/12/12 17:40:49, pink wrote:
> comment this as a strong, or owning, reference. Why not use a scoped_ptr if
> that's the case?

Done.

[Mark Mentovai, 2008-12-12 20:01:21]

lgtm

http://codereview.chromium.org/13757/diff/32/233
File base/message_pump_libevent.cc (right):

http://codereview.chromium.org/13757/diff/32/233#newcode45
Line 45: is_persistent = is_persistent_;
Check that, looks backwards.

http://codereview.chromium.org/13757/diff/32/234
File base/message_pump_libevent.h (right):

http://codereview.chromium.org/13757/diff/32/234#newcode67
Line 67: WATCH_READ_WRITE
Don'tcha think you should just have WATCH_READ = 1 << 0 and WATCH_WRITE = 1 <<
1?  If you want to have WATCH_READ_WRITE, you can make it be WATCH_READ |
WATCH_WRITE, but since read and write are guaranteed to be bits now, the code in
the .cc becomes simpler because you can just check (mode & WATCH_READ) != 0
instead of mode == WATCH_READ || mode == WATCH_WRITE.

http://codereview.chromium.org/13757/diff/32/235
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/13757/diff/32/235#newcode14
Line 14: #include <linux/un.h>
I think we can get away with <sys/un.h> on Linux too.

http://codereview.chromium.org/13757/diff/32/236
File chrome/common/ipc_channel_posix.h (right):

http://codereview.chromium.org/13757/diff/32/236#newcode44
Line 44: // Are we currently blocked waiting for a write to complete.
Is that a question?

http://codereview.chromium.org/13757/diff/32/238
File net/base/listen_socket.h (right):

http://codereview.chromium.org/13757/diff/32/238#newcode14
Line 14: #include <string>
C++ system headers after C system headers.

[jeremy, 2008-12-15 20:49:55]

Fix for TestShell unit test bustage.

I've made WatchFileDescriptor() behavior cumulative.

http://codereview.chromium.org/13757/diff/32/233
File base/message_pump_libevent.cc (right):

http://codereview.chromium.org/13757/diff/32/233#newcode45
Line 45: is_persistent = is_persistent_;
On 2008/12/12 20:01:22, Mark Mentovai wrote:
> Check that, looks backwards.

Done.

http://codereview.chromium.org/13757/diff/32/238
File net/base/listen_socket.h (right):

http://codereview.chromium.org/13757/diff/32/238#newcode14
Line 14: #include <string>
On 2008/12/12 20:01:22, Mark Mentovai wrote:
> C++ system headers after C system headers.

Done.

[Mark Mentovai, 2008-12-15 21:43:40]

LG
tm
okaaaaaaaaaaa

http://codereview.chromium.org/13757/diff/56/60
File base/message_pump_libevent.h (right):

http://codereview.chromium.org/13757/diff/56/60#newcode27
Line 27: ~FileDescriptorWatcher();  // Implicitly calls StopWatching.
Implicitly calls StopWatchingFileDescriptor?

http://codereview.chromium.org/13757/diff/56/61
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/13757/diff/56/61#newcode16
Line 16: #include <sys/un.h>
Did you try to use <sys/un.h> on Linux here?

http://codereview.chromium.org/13757/diff/56/61#newcode153
Line 153: is_blocked_on_write_(false),
Indentation messed up through here.