Bug 67174

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

Description Mark Hahnenberg 2011-08-29 18:36:58 PDT
This is the fourth level of the unzipping process described in https://bugs.webkit.org/show_bug.cgi?id=66567.
Comment 1 Mark Hahnenberg 2011-08-30 13:53:33 PDT
Created attachment 105687 [details]
Patch
Comment 2 Mark Hahnenberg 2011-08-30 14:58:26 PDT
Created attachment 105705 [details]
Patch
Comment 3 Gustavo Noronha (kov) 2011-08-31 07:30:00 PDT
Comment on attachment 105705 [details]
Patch

Attachment 105705 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9572543
Comment 4 Mark Hahnenberg 2011-08-31 11:34:04 PDT
Created attachment 105803 [details]
Patch
Comment 5 Darin Adler 2011-08-31 16:39:32 PDT
Comment on attachment 105803 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSArray.h:77
> +        static JSArray* create(JSGlobalData&, Structure*, unsigned initialLength, ArrayCreationMode createMode);

The argument name here, createMode, is not helpful and should be omitted.

> Source/JavaScriptCore/runtime/JSFunction.h:55
> +        static JSFunction* create(ExecState*, JSGlobalObject*, Structure*, int length, const Identifier& name, NativeFunction);

Is making these functions non-inline a good choice speed-wise?

> Source/JavaScriptCore/runtime/JSStaticScopeObject.h:53
> +        void finishCreation(ExecState* exec, const Identifier& ident, JSValue value, unsigned attributes)

Normally we try not to abbreviate. So the name here would be identifier rather than ident.
Comment 6 Mark Hahnenberg 2011-08-31 18:22:55 PDT
Created attachment 105874 [details]
Patch
Comment 7 Mark Hahnenberg 2011-08-31 19:10:57 PDT
Created attachment 105882 [details]
Patch
Comment 8 Darin Adler 2011-09-01 08:36:56 PDT
Comment on attachment 105882 [details]
Patch

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

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:82
> +void JSCallbackObject<Parent>::finishCreation(JSGlobalData& globalData)
> +{
> +    UNUSED_PARAM(globalData);

Why not just leave the parameter name out instead? That’s the best way to mark an argument as unused in C++ functions.

> Source/JavaScriptCore/runtime/DateInstance.cpp:46
> +    finishCreation(exec->globalData(), jsNumber(timeClip(time)));

I would think we’d want the jsNumber function call inside the finishCreation function.
Comment 9 Mark Hahnenberg 2011-09-01 09:04:10 PDT
Created attachment 105975 [details]
Fixing review issues
Comment 10 Mark Hahnenberg 2011-09-01 10:31:08 PDT
Created attachment 105990 [details]
Resetting test bindings results
Comment 11 Mark Hahnenberg 2011-09-01 12:34:08 PDT
Created attachment 106010 [details]
Random improvement
Comment 12 Oliver Hunt 2011-09-01 14:49:08 PDT
Comment on attachment 106010 [details]
Random improvement

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

Hmmm, there are no changelogs :-(

> Source/JavaScriptCore/runtime/InternalFunction.cpp:47
> +    finishCreation(*globalData, globalObject, name);

Is it intentional the InternalFunction retains the finishCreation in its constructor?
Comment 13 Mark Hahnenberg 2011-09-01 14:57:46 PDT
> Hmmm, there are no changelogs :-(

Hmm dunno what happened to them…they were in earlier versions of the patch lol.

> Is it intentional the InternalFunction retains the finishCreation in its constructor?

Yep.  It had its own isolated version of constructorBody() before, and now it's being changed to finishCreation().  InternalFunction is on the 4th level of the hierarchy below JSCell.  If you look at its immediate superclass, JSObjectWithGlobalObject, its finishCreation() was removed from the constructor, meaning that it needs to be pushed down into the constructor body of InternalFunction.
Comment 14 Mark Hahnenberg 2011-09-01 15:54:46 PDT
Created attachment 106051 [details]
Patch
Comment 15 WebKit Review Bot 2011-09-01 16:48:31 PDT
Comment on attachment 106051 [details]
Patch

Clearing flags on attachment: 106051

Committed r94364: <http://trac.webkit.org/changeset/94364>
Comment 16 WebKit Review Bot 2011-09-01 16:48:37 PDT
All reviewed patches have been landed.  Closing bug.