[webkit-reviews] review granted: [Bug 67064] Unzip initialization lists and constructors in JSCell hierarchy (3/7) : [Attachment 105422] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 67064: Unzip initialization lists and constructors in JSCell hierarchy
(3/7)
https://bugs.webkit.org/show_bug.cgi?id=67064

Attachment 105422: Patch
https://bugs.webkit.org/attachment.cgi?id=105422&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105422&action=review


> Source/JavaScriptCore/ChangeLog:10
> +	   Completed the third level of the refactoring to add finishCreation()

> +	   methods to all classes within the JSCell hierarchy with non-trivial 

> +	   constructor bodies.

This is an unclear comment. It would be better if this talked about what the
actual changes in the patch are. It’s sort of a mixed bag.

In some cases, the change is to factor code into the finishCreation function
and then adopt finishCreation in create functions. In other cases, the changes
seem to be an earlier refactoring step where we adopt finishCreation inside
constructors and rewrite create functions to use local variables, presumably to
make the change later to adopt finishCreation a smaller clearer patch. But
grouping those two kinds of changes together doesn’t make all that much sense.
Then there are other changes that don’t fall into either category.

The patch would be better if it was a more consistently chosen set of changes.

But it is small enough that it can be effectively reviewed, so it’s OK as is,
even though it could be better.

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:32
> -    constructorBody(globalData);
> +    finishCreation(globalData, "");

Would it be more efficient to pass something that’s already a UString so we
don’t have to run code that discovers that the char* is an empty string?

> Source/JavaScriptCore/runtime/Executable.cpp:-157
> -    m_firstLine = firstLine;
> -    m_lastLine = lastLine;

Change log should have a comment about why this change is OK.

> Source/JavaScriptCore/runtime/JSObjectWithGlobalObject.cpp:53
> -	   putAnonymousValue(globalData, GlobalObjectSlot, globalObject);
> +	   putAnonymousValue(globalObject->globalData(), GlobalObjectSlot,
globalObject);

This change seems unnecessary and not really an improvement. Should we instead
be asserting that globalObject->globalData() is == globalData?