[chromium] Add WebThread to WebKitClient
Created attachment 103536 [details] Patch
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.
> 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; }
Created attachment 103572 [details] Pass tasks not fnptrs.
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)?
Committed r92965: <http://trac.webkit.org/changeset/92965>