Bug 76012 – [chromium] Add enter/exitRunLoop to WebThread API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Add runMessageLoop, quitMessageLoop, and runAllPendingMessages to WebThread API

[James Robinson]

Created attachment 121948 [details]
Patch

[James Robinson]

Depends on http://codereview.chromium.org/9167034/

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Nat Duca]

Comment on attachment 121948 [details]
Patch

Hot.

[WebKit Review Bot]

Comment on attachment 121948 [details]
Patch

Attachment 121948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11141721

[Darin Fisher (:fishd, Google)]

Comment on attachment 121948 [details]
Patch

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.

[James Robinson]

Created attachment 123027 [details]
Patch

[James Robinson]

Turns out runAllPending...() isn't need at all.  Removed that, renamed the other functions, and added some docs.  PTAL.

Will fail to compile until https://chromiumcodereview.appspot.com/9167034 is landed and rolled in to Source/WebKit/chromium/DEPS

[WebKit Review Bot]

Comment on attachment 123027 [details]
Patch

Attachment 123027 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11211844

[Darin Fisher (:fishd, Google)]

Comment on attachment 123027 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123027&action=review

> Source/WebKit/chromium/public/platform/WebThread.h:59
> +    // WebThread does not support nesting, meaning that once the run loop is entered for a given WebThread is is not valid to

nit: "is is" ... i think you meant "it is"

[James Robinson]

Committed r106044: <http://trac.webkit.org/changeset/106044>