Bug 67064 – Unzip initialization lists and constructors in JSCell hierarchy (3/7)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

This is the third level of the unzipping process described in https://bugs.webkit.org/show_bug.cgi?id=66567.

[Mark Hahnenberg]

Created attachment 105422 [details]
Patch

[Darin Adler]

Comment on attachment 105422 [details]
Patch

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?

[Mark Hahnenberg]

Created attachment 105443 [details]
Fixing review issues

[Darin Adler]

Comment on attachment 105443 [details]
Fixing review issues

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

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

This way of creating an empty string is so non-obvious that I think we should probably put a function in UString.h to do it or some other way that won’t look so strange at the call site. Need not be part of this patch, though. Follow-up seems better.

[WebKit Review Bot]

Comment on attachment 105443 [details]
Fixing review issues

Clearing flags on attachment: 105443

Committed r94035: <http://trac.webkit.org/changeset/94035>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.