Bug 145016 - Binding generator should support interfaces with CustomConstructor and NoInterfaceObject
Summary: Binding generator should support interfaces with CustomConstructor and NoInte...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on: 145396
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-14 13:29 PDT by youenn fablet
Modified: 2015-05-28 03:13 PDT (History)
11 users (show)

See Also:


Attachments
Patch (45.68 KB, patch)
2015-05-14 14:38 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Renaming HasConstructorProperty and improving changelog (46.03 KB, patch)
2015-05-26 13:23 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing test expectations (45.97 KB, patch)
2015-05-26 22:56 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
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 Details
Patch for landing (45.97 KB, patch)
2015-05-27 23:51 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 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.
Comment 1 youenn fablet 2015-05-14 14:38:29 PDT
Created attachment 253147 [details]
Patch
Comment 2 WebKit Commit Bot 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.
Comment 3 Chris Dumez 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."
Comment 4 Chris Dumez 2015-05-15 13:14:15 PDT
Comment on attachment 253147 [details]
Patch

r- until there is at least proper justification for this.
Comment 5 youenn fablet 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.
Comment 6 Chris Dumez 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.
Comment 7 youenn fablet 2015-05-26 13:23:57 PDT
Created attachment 253726 [details]
Renaming HasConstructorProperty and improving changelog
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2015-05-26 14:18:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Csaba Osztrogonác 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.
Comment 11 Alexey Proskuryakov 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))
Comment 12 WebKit Commit Bot 2015-05-26 15:56:34 PDT
Re-opened since this is blocked by bug 145396
Comment 13 youenn fablet 2015-05-26 22:56:43 PDT
Created attachment 253777 [details]
Rebasing test expectations
Comment 14 WebKit Commit Bot 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.
Comment 15 youenn fablet 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...
Comment 16 Build Bot 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
Comment 17 Build Bot 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
Comment 18 youenn fablet 2015-05-27 23:51:10 PDT
Created attachment 253837 [details]
Patch for landing
Comment 19 WebKit Commit Bot 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>
Comment 20 WebKit Commit Bot 2015-05-28 03:13:09 PDT
All reviewed patches have been landed.  Closing bug.