Bug 60842

Summary: [Qt] Qt bridge bindings should be rewritten using JSC API
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: WebKit QtAssignee: Simon Hausmann <hausmann>
Status: RESOLVED FIXED    
Severity: Major CC: agbakken, allan.jensen, ararunprasad, arurajku, cmarcelo, cshu, girish, hausmann, kent.hansen, lars.knoll, laszlo.gombos, loki, luiz, noam, oliver, ossy, pvarga, yael, zherczeg
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 62330, 88161, 93462, 93463, 93476, 93649, 93716, 93720, 93868, 93872, 93880, 93889, 93897, 94695, 94702, 95325, 95570    
Bug Blocks: 94691    
Attachments:
Description Flags
WIP patch oliver: review-

Oliver Hunt
Reported 2011-05-14 12:46:13 PDT
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.
Attachments
WIP patch (179.99 KB, patch)
2011-05-23 00:47 PDT, Noam Rosenthal
oliver: review-
Noam Rosenthal
Comment 1 2011-05-15 07:46:02 PDT
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
Comment 2 2011-05-16 02:32:45 PDT
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
Comment 3 2011-05-22 23:50:25 PDT
*** Bug 39746 has been marked as a duplicate of this bug. ***
Noam Rosenthal
Comment 4 2011-05-23 00:47:33 PDT
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 5 2011-05-23 08:55:21 PDT
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
Comment 6 2011-05-23 11:00:51 PDT
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
Comment 7 2012-08-02 04:54:36 PDT
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
Comment 8 2014-02-03 02:32:39 PST
Mostly fixed, out of scope anyway.
Note You need to log in before you can comment on or make changes to this bug.