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
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-01-04 10:48 PST by
Modified: 2010-01-04 11:21 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-01-04 10:48:08 PST
Make it easier to change set of actions taken when passing across the AP boundary.
------- Comment #1 From 2010-01-04 10:48:30 PST -------
s/AP/API/
------- Comment #2 From 2010-01-04 10:50:25 PST -------
Created an attachment (id=45805) [details]
The patch
------- Comment #3 From 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 From 2010-01-04 11:04:46 PST -------
(From update of attachment 45805 [details])
>      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 From 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 From 2010-01-04 11:18:58 PST -------
Committed revision 52751.
------- Comment #7 From 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.