Bug 33680 - [V8] Generate header declaration for custom constructor callbacks
Summary: [V8] Generate header declaration for custom constructor callbacks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kavita Kanetkar
URL:
Keywords:
: 33483 (view as bug list)
Depends on:
Blocks: 32638
  Show dependency treegraph
 
Reported: 2010-01-14 10:51 PST by Nate Chapin
Modified: 2010-02-12 10:29 PST (History)
5 users (show)

See Also:


Attachments
SetCallHandler() is now in the generated bindings. (30.31 KB, patch)
2010-01-21 16:21 PST, Kavita Kanetkar
levin: review-
Details | Formatted Diff | Diff
Addressed webkit style issues. (26.01 KB, patch)
2010-01-22 16:57 PST, Kavita Kanetkar
dglazkov: review-
Details | Formatted Diff | Diff
Added handling of 'CanBeConstructed' and 'OmitConstructor' in addition to 'CustomConstructor' (39.68 KB, patch)
2010-01-27 17:20 PST, Kavita Kanetkar
dglazkov: review-
Details | Formatted Diff | Diff
Added ChangeLog (45.09 KB, patch)
2010-01-28 15:41 PST, Kavita Kanetkar
no flags Details | Formatted Diff | Diff
ChangeLog had 1 tab, fixed. (45.10 KB, patch)
2010-01-28 15:53 PST, Kavita Kanetkar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-01-14 10:51:45 PST
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.
Comment 1 Nate Chapin 2010-01-14 11:15:23 PST
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.
Comment 2 Kavita Kanetkar 2010-01-21 16:21:46 PST
Created attachment 47156 [details]
SetCallHandler() is now in the generated bindings.
Comment 3 WebKit Review Bot 2010-01-21 16:26:40 PST
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.
Comment 4 David Levin 2010-01-21 23:42:22 PST
Comment on attachment 47156 [details]
SetCallHandler() is now in the generated bindings.

Lots of style issues to address, see previous comment.
Comment 5 Kavita Kanetkar 2010-01-22 16:57:29 PST
Created attachment 47247 [details]
Addressed webkit style issues.
Comment 6 Dimitri Glazkov (Google) 2010-01-25 12:43:52 PST
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.
Comment 7 Nate Chapin 2010-01-25 14:28:50 PST
(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
Comment 8 Dimitri Glazkov (Google) 2010-01-26 15:37:47 PST
Comment on attachment 47247 [details]
Addressed webkit style issues.

We talked offline -- new, better, cooler patch is coming up.
Comment 9 Kavita Kanetkar 2010-01-27 17:20:07 PST
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.
Comment 10 Dimitri Glazkov (Google) 2010-01-28 10:25:25 PST
Comment on attachment 47580 [details]
Added handling of 'CanBeConstructed' and 'OmitConstructor' in addition to 'CustomConstructor'

Awesome! Now all it needs is a ChangeLog entry.
Comment 11 Dimitri Glazkov (Google) 2010-01-28 10:41:19 PST
.. and the patch should be re-rooted. It looks like it's being created in WebCore/ dir.

I highly recommend using webkit-patch tool.
Comment 12 Kavita Kanetkar 2010-01-28 15:41:17 PST
Created attachment 47653 [details]
Added ChangeLog

Added ChangeLog.
Comment 13 WebKit Review Bot 2010-01-28 15:42:16 PST
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.
Comment 14 Kavita Kanetkar 2010-01-28 15:53:56 PST
Created attachment 47654 [details]
ChangeLog had 1 tab, fixed.

Tab removed.
Comment 15 David Levin 2010-01-28 16:16:02 PST
Comment on attachment 47653 [details]
Added ChangeLog

Please mark your previous patch as obsolete when you add one that replaces it.
Comment 16 Dimitri Glazkov (Google) 2010-01-28 16:39:27 PST
Comment on attachment 47654 [details]
ChangeLog had 1 tab, fixed.

r=me.
Comment 17 WebKit Commit Bot 2010-01-28 20:49:46 PST
Comment on attachment 47654 [details]
ChangeLog had 1 tab, fixed.

Clearing flags on attachment: 47654

Committed r54042: <http://trac.webkit.org/changeset/54042>
Comment 18 WebKit Commit Bot 2010-01-28 20:49:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Nate Chapin 2010-02-12 10:29:25 PST
*** Bug 33483 has been marked as a duplicate of this bug. ***