Binding generator should support interfaces with CustomConstructor and NoInterfaceObject

• Component: Bindings
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

Created attachment 253147 [details]
Patch

[WebKit Commit Bot]

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 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 on attachment 253147 [details]
Patch

r- until there is at least proper justification for this.

[youenn fablet]

(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 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]

Created attachment 253726 [details]
Renaming HasConstructorProperty and improving changelog

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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]

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]

Re-opened since this is blocked by bug 145396

[youenn fablet]

Created attachment 253777 [details]
Rebasing test expectations

[WebKit Commit Bot]

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 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 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]

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]

Created attachment 253837 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 253837 [details]
Patch for landing

Clearing flags on attachment: 253837

Committed r184953: <http://trac.webkit.org/changeset/184953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.