[webkit-reviews] review denied: [Bug 76012] [chromium] Add runMessageLoop, quitMessageLoop, and runAllPendingMessages to WebThread API : [Attachment 121948] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied James Robinson
<jamesr at chromium.org>'s request for review:
Bug 76012: [chromium] Add runMessageLoop, quitMessageLoop, and
runAllPendingMessages to WebThread API
https://bugs.webkit.org/show_bug.cgi?id=76012

Attachment 121948: Patch
https://bugs.webkit.org/attachment.cgi?id=121948&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121948&action=review


> Source/WebKit/chromium/public/platform/WebThread.h:58
> +    virtual void runMessageLoop() = 0;

nit: We don't have the term "Message" or "MessageLoop" anywhere else.
It might be nice to avoid those terms.

Some ideas:
runMessageLoop -> enterRunLoop()
quitMessageLoop -> exitRunLoop()
runAllPendingMessages -> run{All}PendingTasks()

Please document the behavior (roughly).  For example, if exitRunLoop() maps
to MessageLoop::Quit(), then it equates to running all pending messages and
then quitting.	It may actually make more sense to add the QuitNow primitive
since you could simulate MessageLoop::Quit() by using postTask(exitRunLoop).

On the implementation side, you might want to lock this API down to not support

nested run loops since nested run loops are problematic and may require
additional
support (e.g., postNonNestableTask).

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:370
> +	   webKitPlatformSupport()->currentThread()->runAllPendingMessages();

do you know why this needs to call runAllPendingMessages after runMessageLoop?
i guess the idea is that other messages could enter the queue after the Quit
message, and you want to flush those out as well?

MessageLoop::Quit() doesn't post a task though.  it just sets a flag, telling
the ML to exit once it has nothing else to do.	so i think
runAllPendingMessages
here should be a no-op.