Bug 82046

Summary: The JSC code generator doesn't generate correct code for Constructors
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: JavaScriptCoreAssignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, haraken, japhet, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 81657    
Attachments:
Description Flags
Patch
none
Patch none

Description Tommy Widenflycht 2012-03-23 04:27:48 PDT
The main bulk of code uses the name jsConstructor for the created object, and then calls GenerateParametersCheck which generates code that uses the name castedThis.
Comment 1 Tommy Widenflycht 2012-03-23 04:39:27 PDT
Created attachment 133461 [details]
Patch
Comment 2 Kentaro Hara 2012-03-23 04:54:09 PDT
Comment on attachment 133461 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        The JSC code generator doesn't generate correct code for Constructors

This patch looks OK, but it is just renaming a variable. "[JSC] Rename jsConstructor to castedThis for naming consistency in CodeGeneratorJS.pm" would be a better title.
Comment 3 Tommy Widenflycht 2012-03-23 05:12:28 PDT
It's more than just a rename, without this rename the generated code doesn't compile if the constructor takes parameters.
Comment 4 Kentaro Hara 2012-03-23 05:14:05 PDT
Comment on attachment 133461 [details]
Patch

The change will affect exisiting run-bindings-tests. Would you update them?
Comment 5 Tommy Widenflycht 2012-03-23 05:14:53 PDT
Sure
Comment 6 Tommy Widenflycht 2012-03-23 05:46:14 PDT
Just for documentation purposes this is the generated code before my patch:


EncodedJSValue JSC_HOST_CALL JSTestObjConstructor::constructJSTestObj(ExecState* exec)
{
    JSTestObjConstructor* jsConstructor = static_cast<JSTestObjConstructor*>(exec->callee());
    if (exec->argumentCount() < 1)
        return throwVMError(exec, createTypeError(exec, "Not enough arguments"));
    if (exec->argumentCount() <= 0 || !exec->argument(0).isFunction()) {
        setDOMException(exec, TYPE_MISMATCH_ERR);
        return JSValue::encode(jsUndefined());
    }
    RefPtr<TestCallback> testCallback = JSTestCallback::create(asObject(exec->argument(0)), castedThis->globalObject());
    RefPtr<TestObj> object = TestObj::create(testCallback);
    return JSValue::encode(asObject(toJS(exec, jsConstructor->globalObject(), object.get())));
}
Comment 7 Tommy Widenflycht 2012-03-23 05:48:02 PDT
Created attachment 133467 [details]
Patch
Comment 8 Kentaro Hara 2012-03-23 05:50:22 PDT
Comment on attachment 133467 [details]
Patch

OK. Thanks for the patch.
Comment 9 WebKit Review Bot 2012-03-23 06:28:14 PDT
Comment on attachment 133467 [details]
Patch

Clearing flags on attachment: 133467

Committed r111856: <http://trac.webkit.org/changeset/111856>
Comment 10 WebKit Review Bot 2012-03-23 06:28:20 PDT
All reviewed patches have been landed.  Closing bug.