Bug 117183 - Automatically generate WorkerContext constructor attributes
Summary: Automatically generate WorkerContext constructor attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 115853
  Show dependency treegraph
 
Reported: 2013-06-04 01:03 PDT by Chris Dumez
Modified: 2013-06-04 08:23 PDT (History)
14 users (show)

See Also:


Attachments
Patch (30.23 KB, patch)
2013-06-04 02:28 PDT, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (30.20 KB, patch)
2013-06-04 03:51 PDT, Chris Dumez
haraken: review+
Details | Formatted Diff | Diff
Patch (32.91 KB, patch)
2013-06-04 06:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-06-04 01:03:41 PDT
The bindings generator are currently able to generate constructor attributes on the Window object but not yet on the WorkerContext. This should be addressed.
Comment 1 Chris Dumez 2013-06-04 02:28:16 PDT
Created attachment 203667 [details]
Patch
Comment 2 Kentaro Hara 2013-06-04 02:33:06 PDT
(I know I shouldn't say this after you implemented the patch but) another idea might be:

- Keep [NoInterfaceObject] as is, since it's a speced IDL attribute.
- Add [InterfaceObjectOnWorker], which indicates that the interface should be exposed on WorkerContext.
Comment 3 Build Bot 2013-06-04 03:08:58 PDT
Comment on attachment 203667 [details]
Patch

Attachment 203667 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/775053
Comment 4 Chris Dumez 2013-06-04 03:35:38 PDT
(In reply to comment #2)
> (I know I shouldn't say this after you implemented the patch but) another idea might be:
> 
> - Keep [NoInterfaceObject] as is, since it's a speced IDL attribute.
> - Add [InterfaceObjectOnWorker], which indicates that the interface should be exposed on WorkerContext.

I don't fully understand your comment. I kept [NoInterfaceObject] as is already. I merely introduced a new global extended attribute called [GlobalContext].

The approach you propose is fairly similar despite the extended attribute name being different. The issue is how do you indicate that a constructor should be generated the WindowOnly, or on the WorkerContext only?

Note, for example, that according to specification FileReaderSync does not have [NoInterfaceObject]:
http://dev.w3.org/2006/webapi/FileAPI/#FileReaderSync

However, the spec clearly states:
"In environments where the global object is represented by a WorkerGlobalScope object, the FileReaderSync constructor must be available."
-> http://dev.w3.org/2006/webapi/FileAPI/#filereadersyncConstrctr

The Web IDL spec (http://dev.w3.org/2006/webapi/WebIDL/#es-interfaces) says:
"
For every interface that:
- is a callback interface that has constants declared on it, or
- is a non-callback interface that is not declared with the [NoInterfaceObject] 
  extended attribute,
a corresponding property must exist on the ECMAScript global object. The name of the property is the identifier of the interface, and its value is an object called the interface object.
"

The global object may be a Window or a WorkerContext depending on the environment. Currently, this is only defined in prose in the specs. This is the reason why I proposed to introduce [GlobalContext] extended attribute to indicate on which global context the constructor should be exposed.
Comment 5 Kentaro Hara 2013-06-04 03:41:58 PDT
ah, sorry! I was misunderstanding the patch. You approach completely looks reasonable.
Comment 6 Chris Dumez 2013-06-04 03:51:34 PDT
Created attachment 203674 [details]
Patch

Should fix win-ews.
Comment 7 Kentaro Hara 2013-06-04 03:53:15 PDT
Comment on attachment 203674 [details]
Patch

Looks reasonable.
Comment 8 Chris Dumez 2013-06-04 04:10:27 PDT
FYI, I am porting this to Blink as well (but in Python).
Comment 9 Kentaro Hara 2013-06-04 04:24:56 PDT
(In reply to comment #8)
> FYI, I am porting this to Blink as well (but in Python).

Two engineers are working on the rewrite, but it will take time to switch to Python. Please feel free to make changes to the current Perl scripts.
Comment 10 Chris Dumez 2013-06-04 04:26:30 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > FYI, I am porting this to Blink as well (but in Python).
> 
> Two engineers are working on the rewrite, but it will take time to switch to Python. Please feel free to make changes to the current Perl scripts.

The preprocess-idls script was already rewritten in Python in Blink. This is the only script I edited.
Comment 11 Chris Dumez 2013-06-04 06:51:53 PDT
Comment on attachment 203674 [details]
Patch

Seems I broke the bindings tests.
Comment 12 Chris Dumez 2013-06-04 06:57:41 PDT
Created attachment 203691 [details]
Patch

Fixed bindings tests by passing new --workerContextConstructorsFile argument to preprocess-idls.pl script. Kentaro, would you mind taking another look please?
Comment 13 Kentaro Hara 2013-06-04 06:59:13 PDT
Comment on attachment 203691 [details]
Patch

LGTM
Comment 14 WebKit Commit Bot 2013-06-04 07:17:50 PDT
Comment on attachment 203691 [details]
Patch

Clearing flags on attachment: 203691

Committed r151169: <http://trac.webkit.org/changeset/151169>
Comment 15 WebKit Commit Bot 2013-06-04 07:17:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Chris Dumez 2013-06-04 08:23:44 PDT
I updated the WebKit IDL wiki accordingly:
https://trac.webkit.org/wiki/WebKitIDL?action=diff&version=120&old_version=119