Bug 76012 - [chromium] Add enter/exitRunLoop to WebThread API
Summary: [chromium] Add enter/exitRunLoop to WebThread API
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
Depends on:
Reported: 2012-01-10 17:30 PST by James Robinson
Modified: 2012-01-26 14:26 PST (History)
4 users (show)

See Also:

Patch (6.23 KB, patch)
2012-01-10 17:31 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (6.40 KB, patch)
2012-01-18 15:53 PST, James Robinson
fishd: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-01-10 17:30:04 PST
[chromium] Add runMessageLoop, quitMessageLoop, and runAllPendingMessages to WebThread API
Comment 1 James Robinson 2012-01-10 17:31:15 PST
Created attachment 121948 [details]
Comment 2 James Robinson 2012-01-10 17:31:48 PST
Depends on http://codereview.chromium.org/9167034/
Comment 3 WebKit Review Bot 2012-01-10 17:33:49 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Nat Duca 2012-01-10 17:41:03 PST
Comment on attachment 121948 [details]

Comment 5 WebKit Review Bot 2012-01-10 17:54:45 PST
Comment on attachment 121948 [details]

Attachment 121948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11141721
Comment 6 Darin Fisher (:fishd, Google) 2012-01-11 12:50:35 PST
Comment on attachment 121948 [details]

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.
Comment 7 James Robinson 2012-01-18 15:53:28 PST
Created attachment 123027 [details]
Comment 8 James Robinson 2012-01-18 15:54:48 PST
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
Comment 9 WebKit Review Bot 2012-01-18 16:26:58 PST
Comment on attachment 123027 [details]

Attachment 123027 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11211844
Comment 10 Darin Fisher (:fishd, Google) 2012-01-18 21:24:05 PST
Comment on attachment 123027 [details]

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"
Comment 11 James Robinson 2012-01-26 14:26:35 PST
Committed r106044: <http://trac.webkit.org/changeset/106044>