Bug 67064 - Unzip initialization lists and constructors in JSCell hierarchy (3/7)
Summary: Unzip initialization lists and constructors in JSCell hierarchy (3/7)
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
Depends on: 66957
Blocks: 66567 67174
  Show dependency treegraph
Reported: 2011-08-26 13:55 PDT by Mark Hahnenberg
Modified: 2011-08-29 18:42 PDT (History)
1 user (show)

See Also:

Patch (31.61 KB, patch)
2011-08-26 16:30 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fixing review issues (33.22 KB, patch)
2011-08-27 15:01 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-26 13:55:31 PDT
This is the third level of the unzipping process described in https://bugs.webkit.org/show_bug.cgi?id=66567.
Comment 1 Mark Hahnenberg 2011-08-26 16:30:53 PDT
Created attachment 105422 [details]
Comment 2 Darin Adler 2011-08-27 12:36:20 PDT
Comment on attachment 105422 [details]

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?
Comment 3 Mark Hahnenberg 2011-08-27 15:01:43 PDT
Created attachment 105443 [details]
Fixing review issues
Comment 4 Darin Adler 2011-08-27 16:22:42 PDT
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.
Comment 5 WebKit Review Bot 2011-08-29 18:42:51 PDT
Comment on attachment 105443 [details]
Fixing review issues

Clearing flags on attachment: 105443

Committed r94035: <http://trac.webkit.org/changeset/94035>
Comment 6 WebKit Review Bot 2011-08-29 18:42:55 PDT
All reviewed patches have been landed.  Closing bug.