Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(178)

Issue 16554: WaitableEvent (Closed)

Created:
10 years, 7 months ago by agl
Modified:
6 years, 2 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

WaitableEvent is the replacement for Windows events. Previously in the code, a HANDLE from CreateEvent was used for signaling, both within a process and across processes. WaitableEvent is the cross platform replacement for this. To convert: * HANDLE -> base::WaitableEvent* * ScopedHandle -> scoped_ptr<base::WaitableEvent> * CreateEvent -> new base::WaitableEvent * SetEvent -> base::WaitableEvent::Signal * ResetEvent -> base::WaitableEvent::Reset * ObjectWatcher -> base::WaitableEventWatcher * WaitForMultipleObjects -> static base::WaitableEvent::WaitMany ObjectWatcher remains for Windows specific code. WaitableEventWatcher has an identical interface save, * It uses WaitableEvents, not HANDLEs * It returns void from StartWatching and StopWatcher, rather than errors. System internal errors are fatal to the address space IMPORTANT: There are semantic differences between the different platforms. WaitableEvents on Windows are implemented on top of events. Windows events work across process and this is used mostly for modal dialog support. Windows events can be duplicated with DuplicateHandle. On other platforms, WaitableEvent works only within a single process. In the future we shall have to replace the current uses of cross-process events with IPCs. BEWARE: HANDLE, on Windows, is a void *. Since any pointer type coerces to void *, you can pass a WaitableEvent * where a HANDLE is expected without any build-time errors.

Patch Set 1 #

Patch Set 2 : WaitableEvent changes #

Total comments: 6

Patch Set 3 : Addressing comments #

Patch Set 4 : Minor build fix #

Patch Set 5 : Whitespace fixups #

Total comments: 125

Patch Set 6 : Addressing darin's comments #

Patch Set 7 : Make Waiter public again and I forgot to change OnEvent #

Patch Set 8 : Typo s/r/result #

Patch Set 9 : Addresssing darin's comments (round 2) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1418 lines, -266 lines) Patch
M base/atomic_ref_count.h View 3 chunks +5 lines, -5 lines 0 comments Download
M base/base_lib.scons View 1 2 3 4 5 2 chunks +3 lines, -1 line 0 comments Download
M base/base_unittests.scons View 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M base/build/base.vcproj View 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
M base/waitable_event.h View 1 2 3 4 5 6 7 8 4 chunks +79 lines, -8 lines 0 comments Download
M base/waitable_event_generic.cc View 1 chunk +0 lines, -71 lines 0 comments Download
A base/waitable_event_posix.cc View 1 2 3 4 5 6 7 8 1 chunk +392 lines, -0 lines 0 comments Download
M base/waitable_event_unittest.cc View 1 2 3 4 5 2 chunks +55 lines, -0 lines 0 comments Download
A base/waitable_event_watcher.h View 2 3 4 5 6 7 8 1 chunk +148 lines, -0 lines 0 comments Download
A base/waitable_event_watcher_posix.cc View 2 3 4 5 6 7 8 1 chunk +253 lines, -0 lines 0 comments Download
A base/waitable_event_watcher_unittest.cc View 3 4 5 6 7 8 1 chunk +136 lines, -0 lines 0 comments Download
A base/waitable_event_watcher_win.cc View 2 3 4 5 6 7 8 1 chunk +60 lines, -0 lines 0 comments Download
M base/waitable_event_win.cc View 2 3 4 5 6 7 8 4 chunks +33 lines, -7 lines 0 comments Download
M chrome/browser/browser_process.h View 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/browser_process_impl.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/browser_process_impl.cc View 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/browser_shutdown.cc View 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/render_view_host.h View 2 5 chunks +8 lines, -4 lines 0 comments Download
M chrome/browser/render_view_host.cc View 2 12 chunks +14 lines, -12 lines 0 comments Download
M chrome/browser/render_view_host_delegate.h View 2 3 4 5 2 chunks +8 lines, -1 line 0 comments Download
M chrome/browser/render_view_host_manager.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/render_view_host_manager.cc View 4 chunks +7 lines, -3 lines 0 comments Download
M chrome/browser/web_contents.h View 2 2 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/web_contents.cc View 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/web_contents_unittest.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/web_contents_view.h View 3 chunks +8 lines, -3 lines 0 comments Download
M chrome/browser/web_contents_view.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/web_contents_view_win.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/web_contents_view_win.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/child_process.h View 3 chunks +5 lines, -2 lines 0 comments Download
M chrome/common/child_process.cc View 5 chunks +6 lines, -5 lines 0 comments Download
M chrome/common/common.scons View 2 3 4 5 2 chunks +1 line, -1 line 0 comments Download
M chrome/common/ipc_logging.h View 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/common/ipc_message.h View 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/ipc_sync_channel.h View 2 3 4 5 6 7 8 8 chunks +22 lines, -20 lines 0 comments Download
M chrome/common/ipc_sync_channel.cc View 2 3 4 5 6 7 8 20 chunks +51 lines, -43 lines 0 comments Download
M chrome/common/ipc_sync_message.h View 2 3 4 5 4 chunks +11 lines, -10 lines 0 comments Download
M chrome/common/ipc_sync_message.cc View 2 3 4 5 3 chunks +5 lines, -7 lines 0 comments Download
M chrome/plugin/npobject_proxy.h View 2 3 4 5 4 chunks +8 lines, -3 lines 0 comments Download
M chrome/plugin/npobject_proxy.cc View 6 chunks +6 lines, -5 lines 0 comments Download
M chrome/plugin/npobject_util.h View 2 chunks +4 lines, -1 line 0 comments Download
M chrome/plugin/npobject_util.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/plugin/webplugin_proxy.h View 2 chunks +6 lines, -1 line 0 comments Download
M chrome/plugin/webplugin_proxy.cc View 2 3 4 5 5 chunks +7 lines, -5 lines 0 comments Download
M chrome/renderer/render_thread.cc View 2 3 4 5 1 chunk +3 lines, -1 line 0 comments Download
M chrome/renderer/render_view.h View 2 5 chunks +9 lines, -5 lines 0 comments Download
M chrome/renderer/render_view.cc View 2 3 4 5 8 chunks +10 lines, -8 lines 0 comments Download
M chrome/renderer/webplugin_delegate_proxy.h View 2 3 4 5 2 chunks +5 lines, -1 line 0 comments Download
M chrome/renderer/webplugin_delegate_proxy.cc View 2 3 4 5 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/test/testing_browser_process.h View 2 3 4 5 6 7 2 chunks +6 lines, -10 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
agl
I think POSIX support is working now. I still have to do the Windows support, ...
10 years, 7 months ago (2009-01-06 21:50:27 UTC) #1
Evan Martin
I skimmed. http://codereview.chromium.org/16554/diff/210/214 File base/waitable_event.h (right): http://codereview.chromium.org/16554/diff/210/214#newcode68 Line 68: WaitableEvent(HANDLE h); explicit ? http://codereview.chromium.org/16554/diff/210/214#newcode101 Line ...
10 years, 7 months ago (2009-01-13 23:11:37 UTC) #2
jam
cool, some comments http://codereview.chromium.org/16554/diff/210/245 File chrome/common/ipc_sync_channel_unittest.cc (right): http://codereview.chromium.org/16554/diff/210/245#newcode16 Line 16: #include "base/waitable_event.h" curious why this ...
10 years, 7 months ago (2009-01-13 23:22:03 UTC) #3
agl
(Thanks to previous reviewers: all points addressed.) darin: This building and running happily on Linux ...
10 years, 7 months ago (2009-01-14 02:20:29 UTC) #4
darin (slow to review)
overall, this looks great. my comments are mostly just style nits. i haven't reviewed the ...
10 years, 7 months ago (2009-01-14 22:50:02 UTC) #5
agl
http://codereview.chromium.org/16554/diff/518/326 File base/waitable_event.h (right): http://codereview.chromium.org/16554/diff/518/326#newcode28 Line 28: #if defined(OS_WIN) On 2009/01/14 22:50:02, darin wrote: > ...
10 years, 7 months ago (2009-01-15 00:29:16 UTC) #6
darin (slow to review)
http://codereview.chromium.org/16554/diff/518/326 File base/waitable_event.h (right): http://codereview.chromium.org/16554/diff/518/326#newcode141 Line 141: static unsigned WaitMany(WaitableEvent** waitables, unsigned count); I totally ...
10 years, 7 months ago (2009-01-15 06:51:48 UTC) #7
agl
http://codereview.chromium.org/16554/diff/518/326 File base/waitable_event.h (right): http://codereview.chromium.org/16554/diff/518/326#newcode95 Line 95: #if defined(OS_WIN) On 2009/01/15 00:29:16, agl wrote: > ...
10 years, 7 months ago (2009-01-15 19:42:18 UTC) #8
darin (slow to review)
10 years, 7 months ago (2009-01-15 22:46:34 UTC) #9
On 2009/01/15 19:42:18, agl wrote:
> http://codereview.chromium.org/16554/diff/518/326
> File base/waitable_event.h (right):
> 
> http://codereview.chromium.org/16554/diff/518/326#newcode95
> Line 95: #if defined(OS_WIN)
> On 2009/01/15 00:29:16, agl wrote:
> > The Waiter class can be private - I have made it so.
> 
> I take it back, it has to be public. Friendship isn't sufficient to allow a
> class to inherit from a private class.

Ah, right.  OK.

LGTM for everything except the actual posix implementations since I haven't
reviewed them yet.

Powered by Google App Engine
This is Rietveld 408576698