Bug 67174 - Unzip initialization lists and constructors in JSCell hierarchy (4/7)
Summary: Unzip initialization lists and constructors in JSCell hierarchy (4/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: 67064
Blocks: 66567 67420
  Show dependency treegraph
 
Reported: 2011-08-29 18:36 PDT by Mark Hahnenberg
Modified: 2011-09-01 16:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch (91.76 KB, patch)
2011-08-30 13:53 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (91.77 KB, patch)
2011-08-30 14:58 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (91.77 KB, patch)
2011-08-31 11:34 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (83.88 KB, patch)
2011-08-31 18:22 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (84.83 KB, patch)
2011-08-31 19:10 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fixing review issues (84.97 KB, patch)
2011-09-01 09:04 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Resetting test bindings results (88.72 KB, patch)
2011-09-01 10:31 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Random improvement (88.36 KB, patch)
2011-09-01 12:34 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (99.50 KB, patch)
2011-09-01 15:54 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-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.