Bug 66013 - [chromium] Add WebThread to WebKitClient
Summary: [chromium] Add WebThread to WebKitClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nat Duca
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-10 14:43 PDT by Nat Duca
Modified: 2011-08-12 09:29 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.82 KB, patch)
2011-08-10 14:44 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Pass tasks not fnptrs. (3.85 KB, patch)
2011-08-10 20:14 PDT, Nat Duca
fishd: review+
fishd: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-08-10 14:43:26 PDT
[chromium] Add WebThread to WebKitClient
Comment 1 Nat Duca 2011-08-10 14:44:35 PDT
Created attachment 103536 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-08-10 14:56:44 PDT
Comment on attachment 103536 [details]
Patch

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

> Source/WebKit/chromium/public/WebKitClient.h:182
> +    virtual WebThread* createThread(const char* name) { return 0; }

nit: probably best to avoid mentioning Chrome specific implementation details
like messageloop.  it also seems like this documentation, if we were to have it,
would be better on the WebThread interface.  here, you might just say "Creates
an embedder defined thread."

Also, the "createFoo" function naming convention in the WebKit API implies that the
caller will have to manage the lifetime of the returned object and know to delete it
when they are done with it.

you probably want to document that upon deletion of a WebThread, all pending tasks
will be run before the thread is terminated.  this documentation should probably be
in WebThread.h.

> Source/WebKit/chromium/public/WebKitClient.h:183
> +

nit: two new lines between sections.

> Source/WebKit/chromium/public/WebThread.h:32
> +// Provides an interface to a message loop. Deleting the message loop blocks

s/message loop/thread/.

s/until the message loop drains/until all pending, non-deferred tasks have been run/.

do we care that the context pointed to by a delayed task will leak?  would it be helpful
to also provide a context_dtor function?  we might struggle to support this on the
chrome side.
Comment 3 Nat Duca 2011-08-10 18:36:05 PDT
> do we care that the context pointed to by a delayed task will leak?  would it be helpful
> to also provide a context_dtor function?  we might struggle to support this on the
> chrome side.

Ergh, great point.

Howabout...

class WebThread {
public:
    class Task {
    public:
        virtual ~Task() { }
        virtual void performTask() = 0;
    };
    void postTask(Task*) = 0;
    void postDelayedTask(Task*) = 0;
}
Comment 4 Nat Duca 2011-08-10 20:14:49 PDT
Created attachment 103572 [details]
Pass tasks not fnptrs.
Comment 5 Darin Fisher (:fishd, Google) 2011-08-11 22:48:54 PDT
Comment on attachment 103572 [details]
Pass tasks not fnptrs.

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

> Source/WebKit/chromium/public/WebKitClient.h:182
> +

nit: insert an extra new line here to preserve spacing between sections

> Source/WebKit/chromium/public/WebThread.h:34
> +// Deleting the thread blocks until all pending, non-deferred tasks have been

nit: deferred -> delayed (to use the same jargon as the method name)

> Source/WebKit/chromium/public/WebThread.h:41
> +        virtual void performTask() = 0;

maybe: s/performTask/run/ or just "perform" since it would be task->perform() (the extra "task" in the function name seems a bit redundant)?
Comment 6 Nat Duca 2011-08-12 09:29:37 PDT
Committed r92965: <http://trac.webkit.org/changeset/92965>