Bug 67692 - Unzip initialization lists and constructors in JSCell hierarchy (6/7)
Summary: Unzip initialization lists and constructors in JSCell hierarchy (6/7)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 67420 68104
Blocks: 66567 68122
  Show dependency treegraph
 
Reported: 2011-09-06 19:08 PDT by Mark Hahnenberg
Modified: 2011-09-14 15:17 PDT (History)
3 users (show)

See Also:


Attachments
Patch (92.95 KB, patch)
2011-09-12 18:17 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fixing issues (123.83 KB, patch)
2011-09-13 16:59 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-09-06 19:08:33 PDT
This is the sixth level of the unzipping process described in https://bugs.webkit.org/show_bug.cgi?id=66567.
Comment 1 Mark Hahnenberg 2011-09-12 18:17:29 PDT
Created attachment 107121 [details]
Patch
Comment 2 Mark Hahnenberg 2011-09-13 14:50:15 PDT
Comment on attachment 107121 [details]
Patch

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

From review discussion with Geoff.

> Source/JavaScriptCore/ChangeLog:13
> +        into the constructors of the subclasses of the second level of the hierarchy 

Not true.  Needs to be fifth level instead of second.

> Source/JavaScriptCore/ChangeLog:20
> +        File names truncated for brevity. 

Need more details on why this is done.

> Source/WebCore/ChangeLog:24
> +        Changed the DOM bindings generation script to move the calls to finishCreation into the
> +        create methods of all the generated classes except for children of JSDOMWindowBase and 
> +        JSWorkerContextBase.

Because JSDOMWindowBase and JSWorkerContextBase are the next level down.  However, so are some of the classes that get changed with this patch, but it's just easier to change all of them at once.  I added generation of finishCreation methods for all of these classes
Comment 3 Darin Adler 2011-09-13 16:27:03 PDT
Comment on attachment 107121 [details]
Patch

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

review+ assuming you do the run-bindings-tests thing I requested

>> Source/JavaScriptCore/ChangeLog:20
>> +        File names truncated for brevity. 
> 
> Need more details on why this is done.

Please do not do that. We do want you to include the filenames and in most cases, the function names too.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:733
> +        push(@headerContent, "        $className* ptr = new (JSC::allocateCell<$className>(globalData.heap)) ${className}(globalData, structure, impl, windowShell);\n");

Since you are changing the code generator, you need to run "run-bindings-tests --reset-results" and check in the changes to the output of the bindings code generation tests.
Comment 4 Mark Hahnenberg 2011-09-13 16:59:44 PDT
Created attachment 107262 [details]
Fixing issues
Comment 5 Geoffrey Garen 2011-09-13 17:03:50 PDT
Comment on attachment 107262 [details]
Fixing issues

This fixes Darin's comments, so r=me.
Comment 6 WebKit Review Bot 2011-09-14 11:54:36 PDT
Comment on attachment 107262 [details]
Fixing issues

Clearing flags on attachment: 107262

Committed r95108: <http://trac.webkit.org/changeset/95108>
Comment 7 WebKit Review Bot 2011-09-14 11:54:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Mark Hahnenberg 2011-09-14 12:37:19 PDT
Broke some tests on Qt.  Working on a fix.
Comment 9 Mark Hahnenberg 2011-09-14 15:17:59 PDT
Build issue has been resolved.