Bug 60842 – [Qt] Qt bridge bindings should be rewritten using JSC API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Oliver Hunt]

The Qt bridge bindings are a frequent source of pain and frustration for those of us working on the actual engine as they aren't obviously maintained and no one seems to claim any real ownership of them.  For this reason they should be rewritten using the JSC API and so be protected from underlying changes in JSC.

[Noam Rosenthal]

I'm trying to see how feasible it is and what we'd need to change in the API.
So far it seems the main missing part is being able to access internal types that are known to WebCore, like ByteArray or HTMLElement.

[Zoltan Herczeg]

I have already fixed several bugs in the bridge/qt, and in my experience the changes in the GC mechanism affects badly this code, like all the others in the bridge directory. I am thinking now what could be done here.

[Noam Rosenthal]

*** Bug 39746 has been marked as a duplicate of this bug. ***

[Noam Rosenthal]

Created attachment 94377 [details]
WIP patch

This patch still needs lots of work, but it covers the basics.
Still doesn't work:
1. __qt_sender__ (the code is there, but crashes...)
2. ScriptOwnership
3. deleting properties

It also doesn't pass the style tests and other things but I wanted people to see where things stand.

[Oliver Hunt]

Comment on attachment 94377 [details]
WIP patch

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
> +@abstract       Tests whether a JavaScript value's type is the undefined type.
> +@param ctx  The execution context to use.
> +@param value    The JSValue to test.
> +@result         true if value's type is the undefined type, otherwise false.
> +*/
> +JS_EXPORT bool JSValueIsNaN(JSContextRef ctx, JSValueRef value);
> +
> +/*!
> +@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.

[Noam Rosenthal]

Why are you spending your time reviewing this?
I haven't put an r? on it and made clear that this is WIP and needs a lot of more work.
I've put it up so that people can track what's being done.
For now it includes some API changes that of course I'd review separately.

[Simon Hausmann]

I'm working on this and am maintaining a work-in-progress patch set at https://github.com/tronical/webkit/commits/jsc-runtime . I'll create sub-tasks once I'm ready for inclusion to trunk. I'm doing the changes only for the Qt 5 code path, so I'm making this bug depend on bug #88161

[Allan Sandfeld Jensen]

Mostly fixed, out of scope anyway.