Bug 67692

Summary: Unzip initialization lists and constructors in JSCell hierarchy (6/7)
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 67420, 68104    
Bug Blocks: 66567, 68122    
Attachments:
Description Flags
Patch
none
Fixing issues none

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.