Bug 33680 – [V8] Generate header declaration for custom constructor callbacks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

This involves vaguely the following:
1. Add an attribute in the relevant .idls to indicate that there's a custom constructor callback.
2. Update CodeGeneratorV8.pm to look for that flag and make use of a custom constructor when appropriate.
3. Remove manual calls to FunctionTemplate::SetCallHandler in V8DOMWrapper.cpp.
4. Remove manual constructor declarations in V8CustomBinding.h.

[Nate Chapin]

Does it seem reasonable to add an attribute (e.g., CustomConstructor) to some idls for this purpose?  I don't have a good idea of what the criteria are for something being attribute-worthy.

[Kavita Kanetkar]

Created attachment 47156 [details]
SetCallHandler() is now in the generated bindings.

[WebKit Review Bot]

Attachment 47156 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:39:  "V8XMLHttpRequest.h" already included at WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:32  [build/include] [4]
WebCore/bindings/v8/custom/V8XMLSerializerConstructor.cpp:36:  "V8XMLSerializer.h" already included at WebCore/bindings/v8/custom/V8XMLSerializerConstructor.cpp:32  [build/include] [4]
WebCore/bindings/v8/custom/V8SharedWorkerCustom.cpp:48:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/v8/custom/V8SharedWorkerCustom.cpp:48:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/html/canvas/WebGLUnsignedIntArray.idl:35:  Line contains tab character.  [whitespace/tab] [5]
WebCore/bindings/v8/custom/V8WebKitPointConstructor.cpp:38:  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: 6


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 47156 [details]
SetCallHandler() is now in the generated bindings.

Lots of style issues to address, see previous comment.

[Kavita Kanetkar]

Created attachment 47247 [details]
Addressed webkit style issues.

[Dimitri Glazkov (Google)]

I think this is a good start. A couple of comments:

1) it would be great to eliminate the dichotomy of OmitConstructor + CustomConstructor by modifying CodeGeneratorJS.pm. If there's [CustomConstructor], for JSC bindings it means [OmitConstructor]. No need to list both.

2) What is the overlap between CustomConstructor + CanBeConstructor? If it's 100%, it would be great to do the same as the first comment.

[Nate Chapin]

(In reply to comment #6)
> I think this is a good start. A couple of comments:
> 
> 1) it would be great to eliminate the dichotomy of OmitConstructor +
> CustomConstructor by modifying CodeGeneratorJS.pm. If there's
> [CustomConstructor], for JSC bindings it means [OmitConstructor]. No need to
> list both.
> 
> 2) What is the overlap between CustomConstructor + CanBeConstructor? If it's
> 100%, it would be great to do the same as the first comment.

1) AFAICT, there is not much correlation between OmitConstructor and CustomConstructor.

2. CanBeConstructed is a strict subset of CustomConstructor

[Dimitri Glazkov (Google)]

Comment on attachment 47247 [details]
Addressed webkit style issues.

We talked offline -- new, better, cooler patch is coming up.

[Kavita Kanetkar]

Created attachment 47580 [details]
Added handling of 'CanBeConstructed' and 'OmitConstructor' in addition to 'CustomConstructor'

Changed the following from the previous patch:
- Deleted implementation .cpp files for corresponding 'CanBeConstructed' extended attributes. These are now generated via codegeneratorv8.pm
- Treating 'OmitConstructor' and 'CustomConstructor' to be the same in codegeneratorjs.pm
- Cleaned idls that had 'CustomConstructor' and 'OmitConstructor' together, and same with CustomConstructor and CanBeConstructed.

[Dimitri Glazkov (Google)]

Comment on attachment 47580 [details]
Added handling of 'CanBeConstructed' and 'OmitConstructor' in addition to 'CustomConstructor'

Awesome! Now all it needs is a ChangeLog entry.

[Dimitri Glazkov (Google)]

.. and the patch should be re-rooted. It looks like it's being created in WebCore/ dir.

I highly recommend using webkit-patch tool.

[Kavita Kanetkar]

Created attachment 47653 [details]
Added ChangeLog

Added ChangeLog.

[WebKit Review Bot]

Attachment 47653 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'WebCore/bindings/v8/custom/V8XMLSerializerConstructor.cpp': Can't open for reading
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Skipping input 'WebCore/bindings/v8/custom/V8XPathEvaluatorConstructor.cpp': Can't open for reading
Skipping input 'WebCore/bindings/v8/custom/V8DOMParserConstructor.cpp': Can't open for reading
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kavita Kanetkar]

Created attachment 47654 [details]
ChangeLog had 1 tab, fixed.

Tab removed.

[David Levin]

Comment on attachment 47653 [details]
Added ChangeLog

Please mark your previous patch as obsolete when you add one that replaces it.

[Dimitri Glazkov (Google)]

Comment on attachment 47654 [details]
ChangeLog had 1 tab, fixed.

r=me.

[WebKit Commit Bot]

Comment on attachment 47654 [details]
ChangeLog had 1 tab, fixed.

Clearing flags on attachment: 47654

Committed r54042: <http://trac.webkit.org/changeset/54042>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nate Chapin]

*** Bug 33483 has been marked as a duplicate of this bug. ***