[webkit-reviews] review denied: [Bug 60842] [Qt]Qt bridge bindings should be rewritten using JSC API : [Attachment 94377] WIP patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 23 08:55:21 PDT 2011
Oliver Hunt <oliver at apple.com> has denied review:
Bug 60842: [Qt]Qt bridge bindings should be rewritten using JSC API
https://bugs.webkit.org/show_bug.cgi?id=60842
Attachment 94377: WIP patch
https://bugs.webkit.org/attachment.cgi?id=94377&action=review
------- Additional Comments from Oliver Hunt <oliver at apple.com>
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.
More information about the webkit-reviews
mailing list