Bug 117523 - [WK2] Proof of concept to inject custom JavaScript objects on WebWorker context.
Summary: [WK2] Proof of concept to inject custom JavaScript objects on WebWorker context.
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tullio Lucena
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-11 15:04 PDT by Tullio Lucena
Modified: 2013-11-29 10:58 PST (History)
21 users (show)

See Also:


Attachments
Patch with the changes. (21.62 KB, patch)
2013-06-11 15:07 PDT, Tullio Lucena
no flags Details | Formatted Diff | Diff
Patch (22.91 KB, patch)
2013-06-11 15:30 PDT, Tullio Lucena
no flags Details | Formatted Diff | Diff
Patch (26.47 KB, patch)
2013-06-13 13:27 PDT, Tullio Lucena
no flags Details | Formatted Diff | Diff
Patch (32.91 KB, patch)
2013-06-21 14:11 PDT, Tullio Lucena
no flags Details | Formatted Diff | Diff
Patch (25.19 KB, patch)
2013-07-17 18:08 PDT, Tullio Lucena
no flags Details | Formatted Diff | Diff
Patch (33.63 KB, patch)
2013-11-28 14:01 PST, Tullio Lucena
no flags Details | Formatted Diff | Diff
Patch (33.17 KB, patch)
2013-11-28 18:05 PST, Tullio Lucena
sam: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tullio Lucena 2013-06-11 15:04:41 PDT
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.
Comment 1 Tullio Lucena 2013-06-11 15:07:16 PDT
Created attachment 204363 [details]
Patch with the changes.
Comment 2 WebKit Commit Bot 2013-06-11 15:10:22 PDT
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.
Comment 3 Tullio Lucena 2013-06-11 15:30:24 PDT
Created attachment 204364 [details]
Patch
Comment 4 Build Bot 2013-06-11 16:09:19 PDT
Comment on attachment 204364 [details]
Patch

Attachment 204364 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/660923
Comment 5 Gyuyoung Kim 2013-06-11 17:55:30 PDT
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.
Comment 6 Build Bot 2013-06-11 19:58:41 PDT
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
Comment 7 Tullio Lucena 2013-06-13 10:20:07 PDT
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.
Comment 8 Tullio Lucena 2013-06-13 13:27:25 PDT
Created attachment 204638 [details]
Patch
Comment 9 Build Bot 2013-06-13 14:03:37 PDT
Comment on attachment 204638 [details]
Patch

Attachment 204638 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/899052
Comment 10 kov's GTK+ EWS bot 2013-06-13 14:43:49 PDT
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
Comment 11 Build Bot 2013-06-13 15:34:49 PDT
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
Comment 12 Tullio Lucena 2013-06-21 14:11:42 PDT
Created attachment 205214 [details]
Patch
Comment 13 Build Bot 2013-06-21 14:21:33 PDT
Comment on attachment 205214 [details]
Patch

Attachment 205214 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/963075
Comment 14 Build Bot 2013-06-21 14:38:00 PDT
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
Comment 15 Tullio Lucena 2013-07-17 18:08:50 PDT
Created attachment 206940 [details]
Patch
Comment 16 Build Bot 2013-07-17 18:16:57 PDT
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
Comment 17 Build Bot 2013-07-17 18:38:19 PDT
Comment on attachment 206940 [details]
Patch

Attachment 206940 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1090709
Comment 18 Tullio Lucena 2013-07-17 18:49:52 PDT
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?
Comment 19 Tullio Lucena 2013-11-28 14:01:42 PST
Created attachment 218024 [details]
Patch
Comment 20 Build Bot 2013-11-28 14:31:26 PST
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
Comment 21 Gyuyoung Kim 2013-11-28 14:41:44 PST
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 ?
Comment 22 Gyuyoung Kim 2013-11-28 14:42:30 PST
This patch need to be reviewed by WK2 owner. Probably, Sam or Benjamin.
Comment 23 Build Bot 2013-11-28 14:54:42 PST
Comment on attachment 218024 [details]
Patch

Attachment 218024 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/39258032
Comment 24 Tullio Lucena 2013-11-28 17:52:12 PST
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.
Comment 25 Tullio Lucena 2013-11-28 18:05:28 PST
Created attachment 218029 [details]
Patch
Comment 26 Build Bot 2013-11-28 18:39:50 PST
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
Comment 27 Build Bot 2013-11-28 19:03:31 PST
Comment on attachment 218029 [details]
Patch

Attachment 218029 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/39368014
Comment 28 Sam Weinig 2013-11-29 10:58:11 PST
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.