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.
Created attachment 253147 [details] Patch
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.
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."
Comment on attachment 253147 [details] Patch r- until there is at least proper justification for this.
(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.
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.
Created attachment 253726 [details] Renaming HasConstructorProperty and improving changelog
Comment on attachment 253726 [details] Renaming HasConstructorProperty and improving changelog Clearing flags on attachment: 253726 Committed r184872: <http://trac.webkit.org/changeset/184872>
All reviewed patches have been landed. Closing bug.
(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.
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))
Re-opened since this is blocked by bug 145396
Created attachment 253777 [details] Rebasing test expectations
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.
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...
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
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
Created attachment 253837 [details] Patch for landing
Comment on attachment 253837 [details] Patch for landing Clearing flags on attachment: 253837 Committed r184953: <http://trac.webkit.org/changeset/184953>