Bug 33158 – Refactor JSC API entry/exit to use RAII instead of copy/pasting code.

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

[Gavin Barraclough]

Make it easier to change set of actions taken when passing across the AP boundary.

[Gavin Barraclough]

s/AP/API/

[Gavin Barraclough]

Created attachment 45805 [details]
The patch

[WebKit Review Bot]

Attachment 45805 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/API/APIShims.h:84:  #endif line should be "#endif // APIShims_h"  [build/header_guard] [5]
JavaScriptCore/API/APIShims.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2

[Sam Weinig]

Comment on attachment 45805 [details]
The patch

>      JSObject* jsThisObject = toJS(thisObject);
>  
>      // evaluate sets "this" to the global object if it is NULL
>      JSGlobalObject* globalObject = exec->dynamicGlobalObject();
>      SourceCode source = makeSource(script->ustring(), sourceURL->ustring(), startingLineNumber);
> -    Completion completion = evaluate(globalObject->globalExec(), globalObject->globalScopeChain(), source, jsThisObject);
> +    Completion completion = evaluate(exec, globalObject->globalScopeChain(), source, jsThisObject);

This seems unrelated and should be removed from this patch.

>  JSType JSValueGetType(JSContextRef ctx, JSValueRef value)
>  {
>      JSC::ExecState* exec = toJS(ctx);
> -    exec->globalData().heap.registerThread();
> -    JSC::JSLock lock(exec);
> +    JSC::APIEntryShim entryShim(exec);

The JSC:: should not be needed here.


r=me if you change these.

[Mark Rowe (bdash)]

The boolean parameter to the APIEntryShim constructor is indecipherable at call sites.  An enum would make it much easier to understand what it is doing.

[Gavin Barraclough]

Committed revision 52751.

[Gavin Barraclough]

(In reply to comment #5)
> The boolean parameter to the APIEntryShim constructor is indecipherable at call
> sites.  An enum would make it much easier to understand what it is doing.

Hrrm, you're right – but I've just landed based on Sam's r+.

I'd like to remove the parameter altogether – it seems odd to not check the thread is registered in a few infrequently called places, but I didn't want to introduce any functional change in this patch.  Let me discuss with Geoff, and I'll follow up with a patch to either change the parameter to an enum, or remove it altogether.