Bug 145016

Summary: Binding generator should support interfaces with CustomConstructor and NoInterfaceObject
Product: WebKit Reporter: youenn fablet <youennf>
Component: BindingsAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, buildbot, cdumez, commit-queue, darin, dbates, ggaren, ossy, rniwa, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 145396    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Renaming HasConstructorProperty and improving changelog
none
Rebasing test expectations
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Patch for landing none

youenn fablet
Reported 2015-05-14 13:29:08 PDT
This case may happen in general, but also in particular for the streams API controller and reader interfaces. This may be either handled in custom bindings or in code generator.
Attachments
Patch (45.68 KB, patch)
2015-05-14 14:38 PDT, youenn fablet
no flags
Renaming HasConstructorProperty and improving changelog (46.03 KB, patch)
2015-05-26 13:23 PDT, youenn fablet
no flags
Rebasing test expectations (45.97 KB, patch)
2015-05-26 22:56 PDT, youenn fablet
no flags
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (604.97 KB, application/zip)
2015-05-27 01:29 PDT, Build Bot
no flags
Patch for landing (45.97 KB, patch)
2015-05-27 23:51 PDT, youenn fablet
no flags
youenn fablet
Comment 1 2015-05-14 14:38:29 PDT
WebKit Commit Bot
Comment 2 2015-05-14 14:41:48 PDT
Attachment 253147 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestCustomConstructorWithNoInterfaceObject.cpp:176: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestCustomConstructorWithNoInterfaceObject.mm:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestCustomConstructorWithNoInterfaceObject.mm:72: More than one command on the same line [whitespace/newline] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 3 2015-05-15 13:13:02 PDT
Comment on attachment 253147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253147&action=review > Source/WebCore/ChangeLog:3 > + Binding generator should support interfaces with CustomConstructor and NoInterfaceObject This is not valid WebIDL: http://heycam.github.io/webidl/#NoInterfaceObject "If the [NoInterfaceObject] extended attribute is specified on an interface, then the [Constructor] extended attribute must not also be specified on that interface."
Chris Dumez
Comment 4 2015-05-15 13:14:15 PDT
Comment on attachment 253147 [details] Patch r- until there is at least proper justification for this.
youenn fablet
Comment 5 2015-05-15 13:29:42 PDT
(In reply to comment #3) > Comment on attachment 253147 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253147&action=review > > > Source/WebCore/ChangeLog:3 > > + Binding generator should support interfaces with CustomConstructor and NoInterfaceObject > > This is not valid WebIDL: > http://heycam.github.io/webidl/#NoInterfaceObject > > "If the [NoInterfaceObject] extended attribute is specified on an interface, > then the [Constructor] extended attribute must not also be specified on that > interface." Well, it is not Constructor but CustomConstructor ;) The spec makes sense. NoInterfaceObject is asking to not make the interface visible, which is contradictory with having a visible constructor of the interface. Our case is a bit different: we do not want to expose the interface but we still want to have a constructor property on the prototype of objects implementing the interface. Edge case right... you can see it at https://streams.spec.whatwg.org/#globals I am not sure CustomConstructor is the best keyword, since we want to convey something like NotGloballyExposedConstructorButStillExposedSomehow... Alternative is to modify on the fly the prototype of these objects when they got created. Downside is that we will check the prototype for every created instance.
Chris Dumez
Comment 6 2015-05-18 10:47:12 PDT
Comment on attachment 253147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253147&action=review Removing my r-. >>> Source/WebCore/ChangeLog:3 >>> + Binding generator should support interfaces with CustomConstructor and NoInterfaceObject >> >> This is not valid WebIDL: >> http://heycam.github.io/webidl/#NoInterfaceObject >> >> "If the [NoInterfaceObject] extended attribute is specified on an interface, then the [Constructor] extended attribute must not also be specified on that interface." > > Well, it is not Constructor but CustomConstructor ;) > > The spec makes sense. NoInterfaceObject is asking to not make the interface visible, which is contradictory with having a visible constructor of the interface. > > Our case is a bit different: we do not want to expose the interface but we still want to have a constructor property on the prototype of objects implementing the interface. > Edge case right... you can see it at https://streams.spec.whatwg.org/#globals > > I am not sure CustomConstructor is the best keyword, since we want to convey something like NotGloballyExposedConstructorButStillExposedSomehow... > > Alternative is to modify on the fly the prototype of these objects when they got created. Downside is that we will check the prototype for every created instance. Ok, maybe update the Changelog to explain why we need this functionality. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2413 > + push(@implContent, " domObject->putDirect(exec->vm(), exec->propertyNames().constructor, constructor, DontEnum | ReadOnly);\n"); Someone like ggaren should probably take a look at this. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4738 > +sub HasConstructorProperty "NeedsConstructorProperty" would be a better name IMHO.
youenn fablet
Comment 7 2015-05-26 13:23:57 PDT
Created attachment 253726 [details] Renaming HasConstructorProperty and improving changelog
WebKit Commit Bot
Comment 8 2015-05-26 14:18:53 PDT
Comment on attachment 253726 [details] Renaming HasConstructorProperty and improving changelog Clearing flags on attachment: 253726 Committed r184872: <http://trac.webkit.org/changeset/184872>
WebKit Commit Bot
Comment 9 2015-05-26 14:18:56 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 10 2015-05-26 15:29:40 PDT
(In reply to comment #8) > Comment on attachment 253726 [details] > Renaming HasConstructorProperty and improving changelog > > Clearing flags on attachment: 253726 > > Committed r184872: <http://trac.webkit.org/changeset/184872> It broke the bindings tests, please fix.
Alexey Proskuryakov
Comment 11 2015-05-26 15:32:18 PDT
Looks like this broke bindings generation tests, and I'm not sure if these are expected changes. FAIL: (JS) JSTestCustomConstructorWithNoInterfaceObject.h --- WebCore/bindings/scripts/test/JS/JSTestCustomConstructorWithNoInterfaceObject.h 2015-05-26 15:00:27.000000000 -0700 +++ /var/folders/k1/1ttd8pg161b8bnbstk_tjmz40000gn/T/tmp6ptpLC/JSTestCustomConstructorWithNoInterfaceObject.h 2015-05-26 15:28:24.000000000 -0700 @@ -27,7 +27,7 @@ namespace WebCore { -class WEBCORE_EXPORT JSTestCustomConstructorWithNoInterfaceObject : public JSDOMWrapper { +class JSTestCustomConstructorWithNoInterfaceObject : public JSDOMWrapper { public: typedef JSDOMWrapper Base; static JSTestCustomConstructorWithNoInterfaceObject* create(JSC::Structure* structure, JSDOMGlobalObject* globalObject, Ref<TestCustomConstructorWithNoInterfaceObject>&& impl) @@ -78,7 +78,7 @@ return &owner.get(); } -WEBCORE_EXPORT JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, TestCustomConstructorWithNoInterfaceObject*); +JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, TestCustomConstructorWithNoInterfaceObject*); inline JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, TestCustomConstructorWithNoInterfaceObject& impl) { return toJS(exec, globalObject, &impl); } // Custom constructor FAIL: (ObjC) DOMTestCustomConstructorWithNoInterfaceObject.mm --- WebCore/bindings/scripts/test/ObjC/DOMTestCustomConstructorWithNoInterfaceObject.mm 2015-05-26 15:00:27.000000000 -0700 +++ /var/folders/k1/1ttd8pg161b8bnbstk_tjmz40000gn/T/tmpu_YJgI/DOMTestCustomConstructorWithNoInterfaceObject.mm 2015-05-26 15:28:28.000000000 -0700 @@ -69,7 +69,7 @@ DOMTestCustomConstructorWithNoInterfaceObject *kit(WebCore::TestCustomConstructorWithNoInterfaceObject* value) { - { DOM_ASSERT_MAIN_THREAD(); WebCoreThreadViolationCheckRoundOne(); }; + WebCoreThreadViolationCheckRoundOne(); if (!value) return nil; if (DOMTestCustomConstructorWithNoInterfaceObject *wrapper = getDOMWrapper(value))
WebKit Commit Bot
Comment 12 2015-05-26 15:56:34 PDT
Re-opened since this is blocked by bug 145396
youenn fablet
Comment 13 2015-05-26 22:56:43 PDT
Created attachment 253777 [details] Rebasing test expectations
WebKit Commit Bot
Comment 14 2015-05-26 22:58:43 PDT
Attachment 253777 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestCustomConstructorWithNoInterfaceObject.cpp:176: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestCustomConstructorWithNoInterfaceObject.mm:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
youenn fablet
Comment 15 2015-05-26 23:03:41 PDT
Comment on attachment 253777 [details] Rebasing test expectations Checking https://bugs.webkit.org/show_bug.cgi?id=145396#c2, the new tests just need to be rebased (see https://bugs.webkit.org/show_bug.cgi?id=145396#c5). The new patch takes care of the rebasing. I am not sure whether I can mark it as 'r?' or 'cq+'. Putting as 'r?' just in case...
Build Bot
Comment 16 2015-05-27 01:28:59 PDT
Comment on attachment 253777 [details] Rebasing test expectations Attachment 253777 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5488677480300544 New failing tests: http/tests/media/video-preload.html
Build Bot
Comment 17 2015-05-27 01:29:03 PDT
Created attachment 253788 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
youenn fablet
Comment 18 2015-05-27 23:51:10 PDT
Created attachment 253837 [details] Patch for landing
WebKit Commit Bot
Comment 19 2015-05-28 03:13:04 PDT
Comment on attachment 253837 [details] Patch for landing Clearing flags on attachment: 253837 Committed r184953: <http://trac.webkit.org/changeset/184953>
WebKit Commit Bot
Comment 20 2015-05-28 03:13:09 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.