Bug 23312 - Implement MessageQueue::waitForMessageTimed()
Summary: Implement MessageQueue::waitForMessageTimed()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 22718
  Show dependency treegraph
 
Reported: 2009-01-14 00:07 PST by Dmitry Titov
Modified: 2009-01-14 13:57 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (9.61 KB, patch)
2009-01-14 00:25 PST, Dmitry Titov
ap: review+
Details | Formatted Diff | Diff
Updated patch (10.20 KB, patch)
2009-01-14 12:18 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 2009-01-14 00:07:13 PST
Yet another part of 'timers in workers' (bug 22718).
The Win32 version of ThreadCondition::waitTimed() still needs to be implemented, that will be a separate patch.
Comment 1 Dmitry Titov 2009-01-14 00:25:35 PST
Created attachment 26704 [details]
Proposed patch

Also changed ThreadCondition::timedWait() to take absolute time, as discussed on webkit-dev.
Built Mac and Win.
Comment 2 Alexey Proskuryakov 2009-01-14 10:17:56 PST
Comment on attachment 26704 [details]
Proposed patch

+        (WTF::):

It's best to correct the script in cases like this to have more reasonable ChangeLogs.

+    enum MessageQueueWaitResult {

MessageQueue itself is made visible in global namespace by a using directive, you should do the same for the result type and its values. We generally prefer to put "using" declarations for "public" WTF symbols, and leave ones that are implementation details in the WTF namespace without importing them. Taking this approach has the benefit that no one is tempted to say "using namespace WTF", so risk of collision with the non-public symbols is reduced.

Historically, there was some disagreement on this point, so many public symbols do not have "using" declarations at the moment.

+    // The absoluteTime is in seconds, starting on January 1, 1970.

It could be helpful to clarify what time zone is used (maybe by saying that the time is measured in the same units as currentTime() return value?)

+    // QT defines wait for up to ULONG_MAX milliseconds.

QT is QuickTime, and this file is for Qt.

-    // Empty for now
+    // FIXME: Implement.

There's also notImplemented() - but I guess that you are probably going to implement this soon anyway.

r=me, but please consider my nitpicks.
Comment 3 Alexey Proskuryakov 2009-01-14 10:50:10 PST
> There's also notImplemented()

Sorry, that lives in WebCore/platform.
Comment 4 Dmitry Titov 2009-01-14 12:18:59 PST
Created attachment 26725 [details]
Updated patch

Updated with Alexey's comments:
- updated ChangeLog to be more informative
- added 'using WTF::MessageQueueWaitResult', also for its values
- fixed typo /QT/Qt/
- left FIXME (and yes, I'll implement this very soon)

It seems this one is ready for landing.
Comment 5 Alexey Proskuryakov 2009-01-14 13:57:57 PST
Committed revision 39908.