[Webkit-unassigned] [Bug 60842] [Qt]Qt bridge bindings should be rewritten using JSC API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60842


Oliver Hunt <oliver at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #94377|                            |review-
               Flag|                            |




--- Comment #5 from Oliver Hunt <oliver at apple.com>  2011-05-23 08:55:21 PST ---
(From update of attachment 94377)
View in context: https://bugs.webkit.org/attachment.cgi?id=94377&action=review

There are a lot of changes that i don't understand in this patch.  All that should be necessary is to change how your runtime classes are defined, everything else should "just work".

> Source/JavaScriptCore/API/JSObjectRef.cpp:37
>  #include "APICast.h"
>  #include "CodeBlock.h"
>  #include "DateConstructor.h"
> +#include "DateInstance.h"
>  #include "ErrorConstructor.h"
>  #include "FunctionConstructor.h"
>  #include "Identifier.h"

Why this change?

> Source/JavaScriptCore/API/JSObjectRef.cpp:218
> -    
> +

Remove this

> Source/JavaScriptCore/API/JSObjectRef.h:365
> -} JSClassDefinition;
> +}   JSClassDefinition;
>  

Don't change our API headers unless you have absolutely no choice.

> Source/JavaScriptCore/API/JSObjectRef.h:468
> +

Again don't change the API headers

> Source/JavaScriptCore/API/JSValueRef.cpp:88
> +bool JSValueIsNaN(JSContextRef ctx, JSValueRef value)
> +{
> +    ExecState* exec = toJS(ctx);
> +    APIEntryShim entryShim(exec);
> +
> +    JSValue jsValue = toJS(exec, value);
> +
> +    return jsValue == jsNaN();
> +}
> +

This function it potentially broken in the presence of denormalised nan's.

> Source/JavaScriptCore/API/JSValueRef.h:87
> + at abstract       Tests whether a JavaScript value's type is the undefined type.
> + at param ctx  The execution context to use.
> + at param value    The JSValue to test.
> + at result         true if value's type is the undefined type, otherwise false.
> +*/
> +JS_EXPORT bool JSValueIsNaN(JSContextRef ctx, JSValueRef value);
> +
> +/*!
> + at function

You cannot add API without review, and this function is unnecessary, so isn't going to pass muster.

JSValueIsNumber(), JSValueToNumber() and isnan will do the required checks.

> Source/WebCore/bindings/js/JSPluginElementFunctions.cpp:96
> +    // Then, we check if the
> +    if (JSObject* scriptObject = pluginElement->getScriptObject())
> +        return scriptObject;
> +

This change seems unrelated

> Source/WebCore/bindings/js/ScriptControllerQt.cpp:-51
> -PassRefPtr<JSC::Bindings::Instance> ScriptController::createScriptInstanceForWidget(WebCore::Widget* widget)
> +JSC::JSObject* ScriptController::createScriptObjectForWidget(Widget* widget)
>  {
> -    if (widget->isPluginView()) {

I don't get why all these changes are necessary.  All the this patch should need is for your current JSCell subclasses to switch over to the API for their construction.  So these entirely new functions should not be too necessary.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.