Bug 117523 – [WK2] Proof of concept to inject custom JavaScript objects on WebWorker context.

• Component: WebKit2
• Status: UNCONFIRMED

[Tullio Lucena]

Were added callbacks of initialize and terminate for WebWorkers.
An API test was added to check the these callbacks. It verifies the two types of workers, dedicated and shared.

[Tullio Lucena]

Created attachment 204363 [details]
Patch with the changes.

[WebKit Commit Bot]

Attachment 204363 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/WorkerScriptController.cpp', u'Source/WebCore/bindings/js/WorkerScriptController.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/PlatformEfl.cmake', u'Tools/TestWebKitAPI/Tests/WebKit2/WebWorker.cpp', u'Tools/TestWebKitAPI/Tests/WebKit2/WebWorkerSharedTest.html', u'Tools/TestWebKitAPI/Tests/WebKit2/WebWorkerSharedTest.js', u'Tools/TestWebKitAPI/Tests/WebKit2/WebWorkerTest.html', u'Tools/TestWebKitAPI/Tests/WebKit2/WebWorkerTest.js', u'Tools/TestWebKitAPI/Tests/WebKit2/WebWorker_Bundle.cpp']" exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/TestWebKitAPI/Tests/WebKit2/WebWorker.cpp:53:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/TestWebKitAPI/Tests/WebKit2/WebWorker.cpp:57:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/TestWebKitAPI/Tests/WebKit2/WebWorker_Bundle.cpp:67:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/bindings/js/WorkerScriptController.h:87:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/js/WorkerScriptController.h:89:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 9 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tullio Lucena]

Created attachment 204364 [details]
Patch

[Build Bot]

Comment on attachment 204364 [details]
Patch

Attachment 204364 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/660923

[Gyuyoung Kim]

Comment on attachment 204364 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [WK2][EFL] Proof of concept to inject custom JavaScript objects on WebWorker context.

I think this patch is for common WK2 ports rather than EFL WK2 port.

> Source/WebCore/ChangeLog:7
> +

Missing description.

> Source/WebKit2/ChangeLog:7
> +

ditto.

[Build Bot]

Comment on attachment 204364 [details]
Patch

Attachment 204364 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/738583

[Tullio Lucena]

Thanks for the review.

(In reply to comment #5)
> (From update of attachment 204364 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204364&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [WK2][EFL] Proof of concept to inject custom JavaScript objects on WebWorker context.
> 
> I think this patch is for common WK2 ports rather than EFL WK2 port.
> 
I put the [EFL] because I changed a build file from EFL to run the API test. But I agreed with you, this is a change for all WK2, and removed this tag from summary.
> > Source/WebCore/ChangeLog:7
> > +
> 
> Missing description.
> 
> > Source/WebKit2/ChangeLog:7
> > +
> 
> ditto.
I will put a description of the bug on a next patch.


I'm working on fix the error at mac/mac-wk2 ews error to submit a new patch.

[Tullio Lucena]

Created attachment 204638 [details]
Patch

[Build Bot]

Comment on attachment 204638 [details]
Patch

Attachment 204638 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/899052

[kov's GTK+ EWS bot]

Comment on attachment 204638 [details]
Patch

Attachment 204638 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.appspot.com/results/874066

[Build Bot]

Comment on attachment 204638 [details]
Patch

Attachment 204638 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/905006

[Tullio Lucena]

Created attachment 205214 [details]
Patch

[Build Bot]

Comment on attachment 205214 [details]
Patch

Attachment 205214 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/963075

[Build Bot]

Comment on attachment 205214 [details]
Patch

Attachment 205214 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/884619

[Tullio Lucena]

Created attachment 206940 [details]
Patch

[Build Bot]

Comment on attachment 206940 [details]
Patch

Attachment 206940 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1100328

[Build Bot]

Comment on attachment 206940 [details]
Patch

Attachment 206940 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1090709

[Tullio Lucena]

The API test is running and passing on GTK and EFL ports.

I don't have a mac build system available, so could someone help me with this build problem and run the test?

[Tullio Lucena]

Created attachment 218024 [details]
Patch

[Build Bot]

Comment on attachment 218024 [details]
Patch

Attachment 218024 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/39228028

[Gyuyoung Kim]

Comment on attachment 218024 [details]
Patch

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

> Tools/TestWebKitAPI/PlatformEfl.cmake:57
> +    ${TESTWEBKITAPI_DIR}/Tests/WebKit2/WebWorker_Bundle.cpp

Wrong alphabetical order.

> Tools/TestWebKitAPI/PlatformEfl.cmake:100
> +    WebWorker

Generally, lower case is placed below upper case.

> Tools/TestWebKitAPI/Tests/WebKit2/WebWorker_Bundle.cpp:35
> +#include <cstdio>

Add a new line.

> Tools/TestWebKitAPI/Tests/WebKit2/WebWorker_Bundle.cpp:76
> +    WebWorkerTest(const std::string& identifier)

explicit ?

[Gyuyoung Kim]

This patch need to be reviewed by WK2 owner. Probably, Sam or Benjamin.

[Build Bot]

Comment on attachment 218024 [details]
Patch

Attachment 218024 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/39258032

[Tullio Lucena]

Comment on attachment 218024 [details]
Patch

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

Thanks for your comments and for have added others reviewers.
I will fix these mistakes now.

>> Tools/TestWebKitAPI/PlatformEfl.cmake:57
>> +    ${TESTWEBKITAPI_DIR}/Tests/WebKit2/WebWorker_Bundle.cpp
> 
> Wrong alphabetical order.

When I run the webkit-check-style, the way it is, there's no alphabetical order error.

In the other place I have these errors:

Tools/TestWebKitAPI/PlatformEfl.cmake:58:  Alphabetical sorting problem. "${TESTWEBKITAPI_DIR}/Tests/WebKit2/WebWorker_Bundle.cpp" should be before "${TESTWEBKITAPI_DIR}/Tests/WebKit2/CoordinatedGraphics/WKViewIsActiveSetIsActive_Bundle.cpp".  [list/order] [5]

Tools/TestWebKitAPI/PlatformEfl.cmake:59:  Alphabetical sorting problem. "${TESTWEBKITAPI_DIR}/Tests/WebKit2/WebWorker_Bundle.cpp" should be before "${TESTWEBKITAPI_DIR}/Tests/WebKit2/efl/WKViewClientWebProcessCallbacks_Bundle.cpp".  [list/order] [5]

>> Tools/TestWebKitAPI/PlatformEfl.cmake:100
>> +    WebWorker
> 
> Generally, lower case is placed below upper case.

Ok.

>> Tools/TestWebKitAPI/Tests/WebKit2/WebWorker_Bundle.cpp:35
>> +#include <cstdio>
> 
> Add a new line.

Ok.

> Tools/TestWebKitAPI/Tests/WebKit2/WebWorker_Bundle.cpp:36
> +using namespace std;

This using is useless, I will remove it too.

>> Tools/TestWebKitAPI/Tests/WebKit2/WebWorker_Bundle.cpp:76
>> +    WebWorkerTest(const std::string& identifier)
> 
> explicit ?

Ok.

[Tullio Lucena]

Created attachment 218029 [details]
Patch

[Build Bot]

Comment on attachment 218029 [details]
Patch

Attachment 218029 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/39318061

[Build Bot]

Comment on attachment 218029 [details]
Patch

Attachment 218029 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/39368014

[Sam Weinig]

Comment on attachment 218029 [details]
Patch

I am not completely sure of the utility of this, so please add use cases.

Some general concerns:
- Use of global callback functions is a bad design.  Please use a per-object client interface.
- I am not clear on how this would work for SharedWorkers since the InjectedBundle will probably not run in shared worker processes (at least as currently designed).
- The use of a boolean to indicate Dedicated/Shared should be avoided. Please use an enum to allow for more types of workers in the future.
- The callbacks need more contextual information.  For instance, in the non-worker versions of these, we pass a WebFrame, which includes things like the URL.
- This should be modeled on the DOMWindowExtension concept, so that it can deal with being the page cache. 
- Perhaps Workers should have their own separate bundle they load. That would allow for a clear threading model and work for Shared workers.