[webkit-reviews] review denied: [Bug 66013] [chromium] Add WebThread to WebKitClient : [Attachment 103536] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Nat Duca
<nduca at chromium.org>'s request for review:
Bug 66013: [chromium] Add WebThread to WebKitClient
https://bugs.webkit.org/show_bug.cgi?id=66013

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.