This is the third level of the unzipping process described in https://bugs.webkit.org/show_bug.cgi?id=66567.
Created attachment 105422 [details] Patch
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?
Created attachment 105443 [details] Fixing review issues
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 on attachment 105443 [details] Fixing review issues Clearing flags on attachment: 105443 Committed r94035: <http://trac.webkit.org/changeset/94035>
All reviewed patches have been landed. Closing bug.