Bug 23282 - JSImmediate should be private.
Summary: JSImmediate should be private.
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
Depends on:
Reported: 2009-01-12 20:01 PST by Gavin Barraclough
Modified: 2009-01-13 07:40 PST (History)
0 users

See Also:

The patch (117.53 KB, patch)
2009-01-12 20:01 PST, Gavin Barraclough
oliver: 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 2009-01-12 20:01:01 PST
JSImmediate  exposes the internal implementation of JSValuePtr, and commonly there are multiple interfaces that can be used to achieve the same effect.  We should rationalize down the interfaces, and move all operations on JSValuePtrs to use the interface provided by JSValuePtr.
Comment 1 Gavin Barraclough 2009-01-12 20:01:34 PST
Created attachment 26658 [details]
The patch
Comment 2 Oliver Hunt 2009-01-12 20:42:52 PST
Comment on attachment 26658 [details]
The patch


@@ double JSCallbackObject<Base>::toNumber(
378378         if (JSObjectConvertToTypeCallback convertToType = jsClass->convertToType) {
379379             JSLock::DropAllLocks dropAllLocks(exec);
380380             if (JSValueRef value = convertToType(ctx, thisRef, kJSTypeNumber, toRef(exec->exceptionSlot())))
381                  return toJS(value)->getNumber();
 381                 return toJS(value)->uncheckedGetNumber();
382382         }
384384     return Base::toNumber(exec);

This isn't safe -- convertToType is developer defined so we can't guarantee good behaviour, so this can't be unchecked, i think

double dValue;
return toJS(value)->getNumber(dValue) ? dValue : NaN;

Would be the correct version

Otherwise r=me
Comment 3 Gavin Barraclough 2009-01-12 20:51:58 PST
Review comment fixerated.

Transmitting file data ......................................
Committed revision 39851.

Comment 4 Darin Adler 2009-01-13 07:40:16 PST
This is a great change. I've been hoping to see this for *ages*. Thanks, Gavin!