[webkit-reviews] review granted: [Bug 23312] Implement MessageQueue::waitForMessageTimed() : [Attachment 26704] Proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 14 10:17:56 PST 2009


Alexey Proskuryakov <ap at webkit.org> has granted Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 23312: Implement MessageQueue::waitForMessageTimed()
https://bugs.webkit.org/show_bug.cgi?id=23312

Attachment 26704: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=26704&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 (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.


More information about the webkit-reviews mailing list