Bug 33158 - Refactor JSC API entry/exit to use RAII instead of copy/pasting code.
: Refactor JSC API entry/exit to use RAII instead of copy/pasting code.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Gavin Barraclough
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-04 10:48 PST by Gavin Barraclough
Modified: 2010-01-04 11:21 PST (History)
1 user (show)

See Also:


Attachments
The patch (37.50 KB, patch)
2010-01-04 10:50 PST, Gavin Barraclough
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2010-01-04 10:48:08 PST
Make it easier to change set of actions taken when passing across the AP boundary.
Comment 1 Gavin Barraclough 2010-01-04 10:48:30 PST
s/AP/API/
Comment 2 Gavin Barraclough 2010-01-04 10:50:25 PST
Created attachment 45805 [details]
The patch
Comment 3 WebKit Review Bot 2010-01-04 10:52:45 PST
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
Comment 4 Sam Weinig 2010-01-04 11:04:46 PST
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.
Comment 5 Mark Rowe (bdash) 2010-01-04 11:09:13 PST
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.
Comment 6 Gavin Barraclough 2010-01-04 11:18:58 PST
Committed revision 52751.
Comment 7 Gavin Barraclough 2010-01-04 11:21:24 PST
(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.