Bug 103306 - Support custom V8 bindings for WebWorkers
Summary: Support custom V8 bindings for WebWorkers
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Marshall Greenblatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-26 15:30 PST by Marshall Greenblatt
Modified: 2013-04-11 13:19 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (4.13 KB, patch)
2012-11-28 12:37 PST, Marshall Greenblatt
abarth: review-
Details | Formatted Diff | Diff
Proposed patch (22.97 KB, patch)
2012-11-30 17:11 PST, Marshall Greenblatt
no flags Details | Formatted Diff | Diff
Updated patch (23.74 KB, patch)
2013-01-02 05:53 PST, Marshall Greenblatt
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marshall Greenblatt 2012-11-26 15:30:47 PST
Applications embedding WebKit and Chromium can use existing C++ APIs to dynamically add custom V8 JavaScript bindings. These bindings allow closer integration between the script execution environment and the host application and are currently supported for page contexts via the FrameLoaderClient::didCreateScriptContext callback. However, custom V8 bindings are not currently supported for WebWorkers. Combining the multi-threaded capabilities of WebWorkers with the flexibility offered by custom V8 bindings would be an extremely useful feature for some applications.

The initial implementation idea is to add new callbacks similar to Platform::didStartWorkerRunLoop and Platform::didStopWorkerRunLoop.
Comment 1 Marshall Greenblatt 2012-11-28 12:37:33 PST
Created attachment 176547 [details]
Proposed patch

Attached is a proposed patch for this capability. If tests are required where would be the correct place to add them? Thanks.
Comment 2 Adam Barth 2012-11-28 12:43:45 PST
Comment on attachment 176547 [details]
Proposed patch

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

This generally looks like a good approach.  Would you be willing to write a unit test in Source/WebKit/chromium/tests ?  That's especially important for an API like this that won't be tested by the Chromium project's continuous integration system otherwise.

> WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:60
> +#if PLATFORM(CHROMIUM)

No need for PLATFORM(CHROMIUM) in bindings/v8.  Chromium is the only port that uses V8 currently.
Comment 3 Marshall Greenblatt 2012-11-28 14:10:46 PST
In order to build the webkit_unit_tests target it seems that I need to add V8 dependencies to the webkit_support and webkit_support_common targets in Chromium's webkit/support/webkit_support.gypi file. This is due to the #include <v8.h> added in Platform.h. I guess I need to get that change landed in Chromium and update Source/WebKit/chromium/DEPS before proceeding with this issue?
Comment 4 Marshall Greenblatt 2012-11-28 14:36:03 PST
I'm a bit stuck here. It seems that I'll need to land a stub implementation of the Platform changes on the Chromium side and update Source/WebKit/chromium/DEPS before I can add the necessary tests to webkit_unit_tests. Is this a reasonable approach?
Comment 5 Adam Barth 2012-11-28 14:47:07 PST
I wonder if it would be better to send the notification out via a header in Source/WebKit/chromium/public.  Those headers already depend on v8.h, so we wouldn't be adding any extra dependencies.
Comment 6 Marshall Greenblatt 2012-11-28 17:11:05 PST
We currently have WebKitPlatformSupport (Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h) which extends Platform and whose singleton instance is retrievable via WebKit::webKitPlatformSupport(). Would it be allowed to use that from Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp?
Comment 7 Adam Barth 2012-11-28 17:49:32 PST
Sorry, we're in the process of deleting WebKitPlatformSupport.h.
Comment 8 Marshall Greenblatt 2012-11-28 18:34:50 PST
How about adding a new observer interface (WebWorkerObserver) that can be registered using a static method on WorkerThread (WebKit/Source/WebCore/workers/WorkerThread.h)?

It could then be exposed via a new WebKit class under Source/WebKit/chromium/public/platform similar to how WebKit::WebWorkerInfo::dedicatedWorkerCount() (WebKit/Source/WebKit/chromium/public/WebWorkerInfo.h) currently calls WebCore::WorkerThread::workerThreadCount().
Comment 9 Adam Barth 2012-11-29 08:38:41 PST
Something like that sounds plausible.  You probably want to talk with fishd about your patch.  I'm going on vacation soon, so he's likely the one who will review your patch.
Comment 10 Marshall Greenblatt 2012-11-30 17:11:14 PST
Created attachment 177057 [details]
Proposed patch

This patch implements the observer approach via a new WebKit::WebWorkerScriptObserver interface and adds a new WebWorkerTest.WorkerBinding test.
Comment 11 Marshall Greenblatt 2012-12-06 13:07:33 PST
Ping. Can someone review this? Thanks.
Comment 12 Adam Barth 2012-12-06 17:22:53 PST
Comment on attachment 177057 [details]
Proposed patch

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

I'm sorry, but I'm going on vacation and won't be reviewing patches again until January.  The mutex concerns me.  It doesn't seem worth having a mutex to support this feature.  I'm afraid someone else will need to review this patch.

> WebCore/bindings/v8/WorkerScriptController.cpp:67
> +static Mutex& observerMutex()

A mutex...  Hum.
Comment 13 Marshall Greenblatt 2012-12-07 06:59:59 PST
Thanks for taking the time Adam, hopefully fishd can help me finish this one up.

What specifically concerns you about the mutex? It will almost never be contested so there shouldn't be any noticeable performance impact. This is similar in concept to WebWorkerInfo::dedicatedWorkerCount() which also currently uses a mutex.

As an alternative to the mutex we could have WorkerScriptController::addObserver and removeObserver fail (return false) if dedicatedWorkerCount() > 0. Would that be preferable?
Comment 14 Marshall Greenblatt 2013-01-02 05:53:46 PST
Created attachment 181028 [details]
Updated patch

This patch contains the following changes:

- Remove observerMutex.
- Pass the worker URL as a parameter to the WebWorkerScriptObserver callbacks.
- Ensure that calls to the WebWorkerScriptObserver callbacks are balanced.

Adam, please review when you have a chance. Thanks.
Comment 15 Adam Barth 2013-01-02 10:48:05 PST
Comment on attachment 181028 [details]
Updated patch

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

> Platform/chromium/public/WebWorkerScriptObserver.h:35
> +    class WebWorkerScriptObserver {

This shouldn't be indented.  I know some code in WebKit indents inside a namespace, but we're trying to slowly remove that style.

> WebCore/bindings/v8/WorkerScriptController.cpp:278
> +      return;

four-space indent

> WebCore/bindings/v8/WorkerScriptController.cpp:286
> +      return;

ditto

> WebCore/bindings/v8/WorkerScriptController.h:45
> +namespace WebKit {
> +class WebWorkerScriptObserver;
> +}

v8/WorkerScriptController.h isn't a Chromium-specific file (even though its only used in Chromium).  Therefore, it shouldn't depend on Chromium-specific platform types.

> WebKit/chromium/tests/WebWorkerTest.cpp:135
> +    bool got_create() { return m_got_create; }
> +    bool got_release() { return m_got_release; }
> +    bool got_callback() { return m_got_callback; }

These are in the wrong style.  We use WebKit style for code in WebKit.

> WebKit/chromium/tests/WebWorkerTest.cpp:149
> +        delete [] buf;

We shouldn't be calling new and delete manually.  Can't we use a smart point from either WTF or base?

> WebKit/chromium/tests/data/worker_test.js:6
>  Property changes on: WebKit\chromium\tests\data\worker_test.js
>  ___________________________________________________________________
>  Added: svn:eol-style
>    + LF

Why are we making this property change?
Comment 16 Marshall Greenblatt 2013-01-03 02:08:19 PST
Comment on attachment 181028 [details]
Updated patch

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

>> WebCore/bindings/v8/WorkerScriptController.h:45
>> +}
> 
> v8/WorkerScriptController.h isn't a Chromium-specific file (even though its only used in Chromium).  Therefore, it shouldn't depend on Chromium-specific platform types.

A few questions:

1. We currently use WebKit types (WebKit::WebWorkerRunLoop) in v8/WorkerScriptController.cpp behind a PLATFORM(CHROMIUM) define. Would it be viable to continue using WebKit::WebWorkerScriptObserver in WorkerScriptController but move it behind this define?

2. Alternately, is there a good location/namespace to put an interface that will be used by WebCore/bindings/v8 and exposed via Platform/chromium/public?

If neither of the above are options I can introduce a new WebCore::WorkerScriptObserver interface that wraps the WebKit::WebWorkerScriptObserver interface. However, this makes it more complicated to unregister observers in WorkerScriptController because I can no longer directly compare pointers.

>> WebKit/chromium/tests/data/worker_test.js:6
>>    + LF
> 
> Why are we making this property change?

It was accidental, I'll remove it.
Comment 17 Adam Barth 2013-01-03 13:36:30 PST
I'm sorry, but I don't have the bandwidth to finish reviewing this patch.  Other folks should feel free to review it, however.