Created attachment 100886 [details] The patch to be committed if/when the bug is approved This is my initial implementation for giving C++/COM access to javascript methods. For the most part I am simply filling in the skeleton that was already there, but I did need to make some design decisions that I would like guidance/feedback on, specifically in regards to converting the COM VARIANT type into various javascript values.
Created attachment 100888 [details] A patch for testing the primary patch. This changes WinLauncher to allow testing.
More documentation on this to come. Please do not review until I've explained it a little more.
Overall design summary: 1. I created static methods on WebScriptObject, primarily for sub-classes to access. a. The main method that's doing most of the work is WebScriptObject::callWebScriptMethodWithJSDOMWrapper(). This method is primarily a port from Objective-C at Source/WebCore/bindings/objc/WebScriptObject.mm. I found that most c++ interaction requires a JSDOMWrapper around a DOM element, so I created the method on WebScriptObject, and subclasses then call that method, passing their associated wrappers to the method. b. Secondarily, I created two static methods on WebScriptObject: convertVariantToValue() and convertValueToVariant(), for translating from VARIANT parameters to Javascript values, and back. At this point, these methods are only partially filled out as I want to get feedback on the overall design before I go ahead with the rest of it. 2. I've implemented callWebScriptMethod() methods on the DOMElement, and the DOMNode. Each of these essentially gets its wrapper, and then calls the WebScriptObject static method. 3. I also filled out DOMHTMLElement::innerHTML() and DOMHTMLElement::setInnerHTML(), as I use these in my tests. These are primarily just translated from DOMHTMLElement::innerText() and DOMHTMLElement::setInnerText(). 4. Regarding translation from VARIANT to JSValue, most of the primitive types are pretty simple, but I want feedback on my decision for array types. I would like to give the user a "fastest possible" option so that the user can pass large amounts of data between C++/COM and javascript with the least amount of overhead. Ideally this would simply be passing a pointer to a memory buffer, and javascript would somehow properly create the right wrappers around that buffer to make it usable. However, as far as I can tell, this isn't feasible. So, the next fastest option would be to have just one memory copy of the entire buffer. This is what I've implemented. For a VARIANT array of primitive types, I create a javascript ArrayBufferView, which essentially effects a one-time copy of the VARIANT's buffer. I want to create type-specific ArrayBufferView's, i.e. for a VARIANT array of int's I would create an Int32Array. However, I haven't yet figured out how to get these properly translated to javascript. I can create the Int32Array in c++, but when I look at it in javascript, it just looks like an ArrayBufferView, i.e. the length attribute as well as getters and setters are not present (any help here would be greatly appreciated). For now, I think it's fine as it is since it will be forward-compatible once we get that piece working, and the I (the user) can do work-around's in javascript until we get to that point. I later plan on translating a VARIANT array of VARIANT's into a javascript array type (yet to be implemented).
Comment on attachment 100886 [details] The patch to be committed if/when the bug is approved View in context: https://bugs.webkit.org/attachment.cgi?id=100886&action=review > Source/WebKit/win/DOMCoreClasses.cpp:112 > + if (!wrapper) I've noticed that in some cases, m_node's wrapper is NULL. I don't remember the specific case, but it was somewhat random, i.e. a pretty innocuous change in the HTML made the node's wrapper NULL, and another change would make it valid again. I couldn't figure out how to create a wrapper if it is not already there, so here I simply fail if it is NULL. Any suggestions? > Source/WebKit/win/DOMCoreClasses.cpp:1006 > + if (!wrapper) Ditto above on the wrapper > Source/WebKit/win/WebScriptObject.cpp:36 > +#include "../../WebCore/html/canvas/ArrayBufferView.h" I don't know if this is best practices to include "../../Webcore...", but it was the only way I could figure out how to get the definition of the various ArrayBufferView types, as these headers are not copied to the distribution directory, which is the <WebCore...> directory. And should I intermingle these includes with the above in alphabetical order as the code guidelines say, or should they be out on their own? > Source/WebKit/win/WebScriptObject.cpp:119 > + if (arg.vt & VT_ARRAY) { // it's an array This is the main array part. For now I've only filled out short's, int's, float's, and double's, design feedback pending, but hopefully you get the idea. > Source/WebKit/win/WebScriptObject.cpp:165 > + default: Here's the straight-forward translation piece. Pretty simple, although there are still a few things I still need to fill out going forward. > Source/WebKit/win/WebScriptObject.cpp:200 > +{ This is mostly adapted from WebCore/bindings/objc/WebScriptObject.mm
Created attachment 100928 [details] Updated patch v2 fat fingers on implementing style guidelines on the first patch
Attachment 100928 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/win/DOMCoreClasses.cpp', u'S..." exit_code: 1 Source/WebKit/win/WebScriptObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 100886 [details] The patch to be committed if/when the bug is approved View in context: https://bugs.webkit.org/attachment.cgi?id=100886&action=review > Source/WebKit/win/WebScriptObject.cpp:158 > + if (arrayBufferView.get()) { You may want to look at the next few lines regarding life-cycle management, i.e. make sure I'm not creating memory leaks here. > Source/WebKit/win/WebScriptObject.cpp:161 > + return new (exec) WebCore::JSArrayBufferView(structure, globalObject, arrayBufferView); This is the piece I believe where I'm not getting a proper javascript Int32Array, as I'm just creating a JSArrayBufferView. However I couldn't find a JSInt32Array, or maybe I couldn't properly include it without getting compile errors. For now I think this should be fine, though, until I figure all that out.
Comment on attachment 100928 [details] Updated patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=100928&action=review > Source/WebKit/win/DOMCoreClasses.cpp:112 > + if (!wrapper) I've noticed that in some cases, m_node's wrapper is NULL. I don't remember the specific case, but it was somewhat random, i.e. a pretty innocuous change in the HTML made the node's wrapper NULL, and another change would make it valid again. I couldn't figure out how to create a wrapper if it is not already there, so here I simply fail if it is NULL. Any suggestions? > Source/WebKit/win/DOMCoreClasses.cpp:1006 > + if (!wrapper) Ditto above on the wrapper >> Source/WebKit/win/WebScriptObject.cpp:36 >> +#include "../../WebCore/html/canvas/ArrayBufferView.h" > > Alphabetical sorting problem. [build/include_order] [4] I don't know if this is best practices to include "../../Webcore...", but it was the only way I could figure out how to get the definition of the various ArrayBufferView types, as these headers are not copied to the distribution directory, which is the <WebCore...> directory. And should I intermingle these includes with the above in alphabetical order as the code guidelines say, or should they be out on their own? > Source/WebKit/win/WebScriptObject.cpp:119 > + if (arg.vt & VT_ARRAY) { // it's an array This is the main array part. For now I've only filled out short's, int's, float's, and double's, design feedback pending, but hopefully you get the idea. > Source/WebKit/win/WebScriptObject.cpp:165 > + default: Here's the straight-forward translation piece. Pretty simple, although there are still a few things I still need to fill out going forward. > Source/WebKit/win/WebScriptObject.cpp:200 > +{ This is mostly adapted from WebCore/bindings/objc/WebScriptObject.mm
Attachment 100886 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/win/DOMCoreClasses.cpp', u'S..." exit_code: 1 Source/WebKit/win/WebScriptObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 100928 [details] Updated patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=100928&action=review >>> Source/WebKit/win/WebScriptObject.cpp:36 >>> +#include "../../WebCore/html/canvas/ArrayBufferView.h" >> >> Alphabetical sorting problem. [build/include_order] [4] > > I don't know if this is best practices to include "../../Webcore...", but it was the only way I could figure out how to get the definition of the various ArrayBufferView types, as these headers are not copied to the distribution directory, which is the <WebCore...> directory. And should I intermingle these includes with the above in alphabetical order as the code guidelines say, or should they be out on their own? I guess the review bot says they need to be with the includes above. I'll do that for the next patch update. Still, is this method of including kosher, or is the proper way to do it something else, i.e. should these headers be copied to the distribution directory <WebCore...>, in which case I don't know how to do that?
Created attachment 101367 [details] Update patch v3 Updated #includes to pass style guidelines
Comment on attachment 101367 [details] Update patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=101367&action=review > Source/WebKit/win/DOMCoreClasses.cpp:112 > + if (!wrapper) I've noticed that in some cases, m_node's wrapper is NULL. I don't remember the specific case, but it was somewhat random, i.e. a pretty innocuous change in the HTML made the node's wrapper NULL, and another change would make it valid again. I couldn't figure out how to create a wrapper if it is not already there, so here I simply fail if it is NULL. Any suggestions for future improvements are welcome. > Source/WebKit/win/WebScriptObject.cpp:118 > + if (arg.vt & VT_ARRAY) { // it's an array This is the main array part. For now I've only filled out short's, int's, float's, and double's, design feedback pending, but hopefully you get the idea. > Source/WebKit/win/WebScriptObject.cpp:163 > + switch (vt) { Here's the straight-forward translation piece. Pretty simple, although there are still a few things I still need to fill out going forward. > Source/WebKit/win/WebScriptObject.cpp:199 > +{ This is mostly adapted from WebCore/bindings/objc/WebScriptObject.mm
It's been a month and I haven't got a review on this yet. Could someone take a look at this and at least give me an opinion as to whether the main design will work, as we're moving forward implementing this into our software. Should I create a new, cleaner bug report with my latest patch (I have made a few further changes since the v3 patch)? Thanks
Many apologies for not getting around to this earlier. Thank you very much for putting in the effort to get this working, and for so clearly explaining the issues you ran into while implementing this. I wish everyone were as detailed as you have been! WebScriptObject on OS X has not had any substantial improvements made to it in quite some time. It is considered a dead end. The current recommendation is that client applications use the JavaScriptCore API directly rather than indirecting through WebScriptObject. Given this, it may not be wise to add WebScriptObject support to the COM-based WebKit API (even though the interfaces/methods are already declared; that's just a byproduct of the way the COM-based API was initially created). Would the JSC API work for your application?
I see. Thanks for the feedback. I'll look into the Javascript Core API and see if it has what I need, and if so, I'll go that route.
Sounds good. Implementing WebScriptObject is still a possibility, but it would be good to explore the other options first. Sorry again for taking so long to get this feedback to you.
Adam, I've looked through the JavaScriptCore API (http://developer.apple.com/library/mac/#documentation/Carbon/Reference/WebKit_JavaScriptCore_Ref/javascriptcore_fw-datatypes.html), and there are some primary things that I can't figure out how to do for my application if it's to access the JavaScriptCore API directly. First, let me explain what I want my application to do. The goal is for our c++ windows client application to display real time graphs within the client application, i.e. bar charts, line charts, histograms, pie charts, etc. as a simulation runs. Then we also want to make those graphs and charts accessible remotely, i.e. a browser connects to our application which is acting as a server, and the application sends data to the browser that essentially renders those bar charts the same as they look in the client application. So our solution is to have a "web widget" in the client application, that is essentially an html canvas that draws a chart. The canvas DOM element is given methods such as updateData(), which updates the data to be drawn. From the c++ side, when the client application wants to update the chart data, it gets a reference to the canvas DOM element (DOMDocument::getElementById()), and then calls the updateData method on that DOM element, passing in the appropriate parameters. At this point our client application works as designed above. We are just now getting into moving that to a remote server scenario. But the idea is once we've designed the client app's chart widget, the remote widget will be almost exactly the same code, i.e. a canvas DOM element that has some methods on it that are called to update the chart display, albeit this time through some HTTP/AJAX request/response mechanism. So, for our given design, here are the things we need: 1. An HTML/CSS/JavaScript-based window, with it's associated DOM structure. 2. When we want to update the display, we need to get reference to DOM elements. 3. We need to call javascript methods on those elements. 4. Right now we are running everything through the COM-API provided. For the JavaScriptCore API I wasn't able to find anything that allowed me to get the JSObject that is associated with the DOM element in an html document. I think once I have that, I could use the JavaScriptCore API; I just don't see how to tap into the DOM/JSObject binding. Perhaps it might work if the javascript methods are global, but how to get global javascript in an html document using the JavaScript Core API? The other issue is, I don't know exactly how to get access to the JavaScriptCore API via the COM API, as I don't see interfaces that bridge the gap. Anyway, thanks for your time and help, and any design suggestions here are more than welcome.
Thanks for the detailed explanation. I think we can make the JSC API work for you. Responses below. (In reply to comment #17) > For the JavaScriptCore API I wasn't able to find anything that allowed me to get the JSObject that is associated with the DOM element in an html document. That's right. This is the main limitation of the JSC API as compared to WebScriptObject. > I think once I have that, I could use the JavaScriptCore API; I just don't see how to tap into the DOM/JSObject binding. Perhaps it might work if the javascript methods are global, but how to get global javascript in an html document using the JavaScript Core API? The other issue is, I don't know exactly how to get access to the JavaScriptCore API via the COM API, as I don't see interfaces that bridge the gap. The main entry point for the JSC API for a WebKit app is IWebFrameLoadDelegate::didClearWindowObject. That function gets called once per frame load. It is passed the frame's JSGlobalContextRef (though the function signature only calls it a JSContextRef) and the frame's window object's JSObjectRef. To get to global JavaScript functions and variables, you can use JSObjectGetProperty(globalContext, windowObject, propertyName, &exception). One technique we have found useful when writing WebKit-based apps that use the JSC API is to put as much logic in actual JavaScript code as possible. By putting logic into JavaScript itself, you get to use the succinct JS-based DOM APIs rather than having to use the verbose JSC API for everything. You can use the JSC API to inject some hooks into the page to allow your JavaScript code to call back up into your C/C++ code as needed.
To further expand on this: The basic technique is to add your hooks to the window object inside didClearWindowObject. One clean way to do this is to add a single object as a property on the window object, and have that object have all the hooks hanging off of it. That allows you to write code like this in JavaScript: window.apiHooks.hookFunction(someData); (As in all JavaScript, you can omit "window.".) So, inside didClearWindowObject, you'd create a JSClassRef that has all the hook functions as staticFunctions, then create a JSObjectRef using that JSClassRef, then use JSObjectSetProperty(context, windowObject, "apiHooks", apiHooksObject, attributes, &exception) to add the hooks object to the window. Since didClearWindowObject is called before any JavaScript code can run in the page, your JavaScript code can just assume that the apiHooks object is present.
Awesome!! Thanks for your help. I'll go ahead and close this case once I've got all this stuff figured out.
I assume the fact that you closed this bug means that the JSC approach worked for you. That's great! I hope you'll still want to contribute to WebKit again in the future, even though this particular patch didn't pan out. It would be great to work with you more!
Comment on attachment 100886 [details] The patch to be committed if/when the bug is approved Cleared review? from obsolete attachment 100886 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).