[webkit-reviews] review denied: [Bug 71757] WebWorkerRunLoop wrapper around WorkerRunLoop : [Attachment 116319] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied  review:
Bug 71757: WebWorkerRunLoop wrapper around WorkerRunLoop
https://bugs.webkit.org/show_bug.cgi?id=71757

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116319&action=review


> Source/WebKit/chromium/public/WebKitPlatformSupport.h:338
> +    virtual void workerRunLoopStarted(WebWorkerRunLoop*) { }

Sorry, I should have mentioned this earlier, but for notifications like this,
we normally
prefix them with either "will" or "did"...

workerRunLoopStarted -> didStartWorkerRunLoop

I have a question about workerRunLoopStopped.  On PlatformSupport.h, you call
the
method workerRunLoopStopping, which suggests that the run loop hasn't stopped
yet,
but soon will.	However, workerRunLoopStopped says that the run loop has
already
stopped.  Clearly, these are not consistent.  Assuming, "will stop" is what you
mean,
then I'd call this function "willStopWorkerRunLoop".

> Source/WebKit/chromium/public/WebWorkerRunLoop.h:38
> +    static WebWorkerRunLoop* current();

Static functions like this need to be marked up with WEBKIT_EXPORT, or else you

will break the component=shared_library build.

Can you say more about why you need 'current' to be implemented in the WebKit
layer, where it is apparently not needed by WebCore?  If it is only needed by
the embedder, then it seems like the embedder can maintain the TLS entry itself

based on the notifications didStartWorkerRunLoop and willStopWorkerRunLoop.