Bug 103306 – Support custom V8 bindings for WebWorkers

• Component: Platform
• Status: RESOLVED WONTFIX

[Marshall Greenblatt]

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.

[Marshall Greenblatt]

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.

[Adam Barth]

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.

[Marshall Greenblatt]

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?

[Marshall Greenblatt]

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?

[Adam Barth]

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.

[Marshall Greenblatt]

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?

[Adam Barth]

Sorry, we're in the process of deleting WebKitPlatformSupport.h.

[Marshall Greenblatt]

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().

[Adam Barth]

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.

[Marshall Greenblatt]

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.

[Marshall Greenblatt]

Ping. Can someone review this? Thanks.

[Adam Barth]

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.

[Marshall Greenblatt]

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?

[Marshall Greenblatt]

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.

[Adam Barth]

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?

[Marshall Greenblatt]

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.

[Adam Barth]

I'm sorry, but I don't have the bandwidth to finish reviewing this patch.  Other folks should feel free to review it, however.