Bug 152171

Summary: WebIDL generator should support the possibility for C++ classes to have a JS Builtin constructor
Product: WebKit Reporter: youenn fablet <youennf>
Component: Tools / TestsAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: adam.bergkvist, cdumez, cgarcia, commit-queue, esprehn+autocc, kondapallykalyan, lforschler
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Minor refactoring
none
Rebasing
none
Rebasing
none
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2015-12-11 06:26:15 PST
It might be handy to use JS builtin to process the parameters passed to create a DOM object.
One solution is to create the C++ object using standard XX::create() and then call a JS builtin function similarly to JSBuiltin classes.
This might need the creation of a JSBuiltinConstructor
Comment 1 youenn fablet 2015-12-11 06:34:06 PST
Created attachment 267168 [details]
Patch
Comment 2 WebKit Commit Bot 2015-12-11 06:35:11 PST
Attachment 267168 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestClassWithJSBuiltinConstructor.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/JS/JSTestClassWithJSBuiltinConstructor.cpp:153:  Extra space before ( in function call  [whitespace/parens] [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 3 youenn fablet 2015-12-14 07:42:41 PST
Created attachment 267295 [details]
Minor refactoring
Comment 4 youenn fablet 2015-12-14 08:06:37 PST
Created attachment 267297 [details]
Rebasing
Comment 5 youenn fablet 2015-12-14 08:45:36 PST
Created attachment 267298 [details]
Rebasing
Comment 6 WebKit Commit Bot 2015-12-14 08:48:02 PST
Attachment 267298 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestClassWithJSBuiltinConstructor.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/JS/JSTestClassWithJSBuiltinConstructor.cpp:158:  Extra space before ( in function call  [whitespace/parens] [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 7 youenn fablet 2015-12-14 09:18:44 PST
Created attachment 267299 [details]
Patch
Comment 8 youenn fablet 2015-12-14 09:20:30 PST
Created attachment 267300 [details]
Patch
Comment 9 WebKit Commit Bot 2015-12-14 09:21:18 PST
Attachment 267300 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestClassWithJSBuiltinConstructor.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/JS/JSTestClassWithJSBuiltinConstructor.cpp:158:  Extra space before ( in function call  [whitespace/parens] [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 10 youenn fablet 2015-12-14 09:23:32 PST
(In reply to comment #8)
> Created attachment 267300 [details]
> Patch

@adambe, patch is currently not supporting ConstructorCallWith for JSBuiltinConstructor. Is that ok with your needs?
Comment 11 Darin Adler 2015-12-14 09:51:48 PST
Comment on attachment 267300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267300&action=review

> Source/WebCore/ChangeLog:23
> +        (GenerateConstructorDefinition):
> +        (IsConstructable):
> +        (IsJSBuiltinConstructor):
> +        (AddJSBuiltinIncludesIfNeeded):
> +        (GenerateHeader): Deleted.
> +        (GetJSBuiltinScopeName): Deleted.

Is this list of functions accurate? Did we actually delete two functions? If not, please fix this before landing.

> Source/WebCore/ChangeLog:31
> +        (WebKit::kit):
> +        (WebKit::core):
> +        (WebKit::wrapTestClassWithJSBuiltinConstructor):
> +        (webkit_dom_test_class_with_js_builtin_constructor_finalize):
> +        (webkit_dom_test_class_with_js_builtin_constructor_constructor):
> +        (webkit_dom_test_class_with_js_builtin_constructor_class_init):
> +        (webkit_dom_test_class_with_js_builtin_constructor_init):

These lists of functions are pointless in test expectations files like this. I suggest omitting them.

> Source/WebCore/ChangeLog:53
> +        (WebCore::JSTestClassWithJSBuiltinConstructorPrototype::create):
> +        (WebCore::JSTestClassWithJSBuiltinConstructorPrototype::createStructure):
> +        (WebCore::JSTestClassWithJSBuiltinConstructorPrototype::JSTestClassWithJSBuiltinConstructorPrototype):
> +        (WebCore::JSTestClassWithJSBuiltinConstructorConstructor::createJSObject):
> +        (WebCore::JSTestClassWithJSBuiltinConstructorConstructor::initializeProperties):
> +        (WebCore::JSTestClassWithJSBuiltinConstructorConstructor::initializeExecutable):
> +        (WebCore::JSTestClassWithJSBuiltinConstructorPrototype::finishCreation):
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::JSTestClassWithJSBuiltinConstructor):
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::createPrototype):
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::getPrototype):
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::destroy):
> +        (WebCore::jsTestClassWithJSBuiltinConstructorConstructor):
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::getConstructor):
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::visitChildren):
> +        (WebCore::JSTestClassWithJSBuiltinConstructorOwner::isReachableFromOpaqueRoots):
> +        (WebCore::JSTestClassWithJSBuiltinConstructorOwner::finalize):
> +        (WebCore::toJSNewlyCreated):
> +        (WebCore::toJS):
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::toWrapped):

These lists of functions are pointless in test expectations files like this. I suggest omitting them.

> Source/WebCore/ChangeLog:59
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::create):
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::createStructure):
> +        (WebCore::JSTestClassWithJSBuiltinConstructor::finishCreation):
> +        (WebCore::wrapperOwner):
> +        (WebCore::toJS):

These lists of functions are pointless in test expectations files like this. I suggest omitting them.

> Source/WebCore/ChangeLog:64
> +        (-[DOMTestClassWithJSBuiltinConstructor dealloc]):
> +        (core):
> +        (kit):

These lists of functions are pointless in test expectations files like this. I suggest omitting them.
Comment 12 youenn fablet 2015-12-14 10:07:50 PST
> > Source/WebCore/ChangeLog:23
> > +        (GenerateConstructorDefinition):
> > +        (IsConstructable):
> > +        (IsJSBuiltinConstructor):
> > +        (AddJSBuiltinIncludesIfNeeded):
> > +        (GenerateHeader): Deleted.
> > +        (GetJSBuiltinScopeName): Deleted.
> 
> Is this list of functions accurate? Did we actually delete two functions? If
> not, please fix this before landing.

OK

> 
> > Source/WebCore/ChangeLog:31
> > +        (WebKit::kit):
> > +        (WebKit::core):
> > +        (WebKit::wrapTestClassWithJSBuiltinConstructor):
> > +        (webkit_dom_test_class_with_js_builtin_constructor_finalize):
> > +        (webkit_dom_test_class_with_js_builtin_constructor_constructor):
> > +        (webkit_dom_test_class_with_js_builtin_constructor_class_init):
> > +        (webkit_dom_test_class_with_js_builtin_constructor_init):
> 
> These lists of functions are pointless in test expectations files like this.
> I suggest omitting them.

Are you referring to the changelog or the test expectation files?
If the latter, how can that be done?
Also functions related to WebCore::JSTestClassWithJSBuiltinConstructor are important to keep.
Comment 13 youenn fablet 2015-12-15 00:00:06 PST
Created attachment 267350 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2015-12-15 00:55:49 PST
Comment on attachment 267350 [details]
Patch for landing

Clearing flags on attachment: 267350

Committed r194100: <http://trac.webkit.org/changeset/194100>
Comment 15 WebKit Commit Bot 2015-12-15 00:55:55 PST
All reviewed patches have been landed.  Closing bug.