Bug 41995 - [Qt] Enable binding of QObjects to JavaScript environment for inspector frontend
Summary: [Qt] Enable binding of QObjects to JavaScript environment for inspector frontend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-07-09 14:52 PDT by Jamey Hicks
Modified: 2010-08-24 07:11 PDT (History)
5 users (show)

See Also:


Attachments
patch to enable binding QObject to the javascript environment of a QWebInspector (2.08 KB, patch)
2010-07-09 14:52 PDT, Jamey Hicks
akling: review-
Details | Formatted Diff | Diff
patch revised to address comments (3.43 KB, patch)
2010-07-12 06:23 PDT, Jamey Hicks
no flags Details | Formatted Diff | Diff
Patch (3.64 KB, patch)
2010-07-14 05:36 PDT, Jamey Hicks
no flags Details | Formatted Diff | Diff
Patch (3.83 KB, patch)
2010-07-20 06:05 PDT, Jamey Hicks
no flags Details | Formatted Diff | Diff
Patch (3.94 KB, patch)
2010-07-21 08:01 PDT, Jamey Hicks
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jamey Hicks 2010-07-09 14:52:09 PDT
Created attachment 61104 [details]
patch to enable binding QObject to the javascript environment of a QWebInspector

The attached patch enables QObjects to be bound to global variables in the javascript environment of the inspector frontend. This is useful for writing alternate inspector frontends in javascript which need additional native functionality. It's a lighter-weight alternative from using an NPAPI plugin.

The code which instantiates QWebInspector uses this by setting the dynamic property "q_inspector_js_objects" on the QWebInspector. The value of this property should be a QMap<QString,QVariant>mapping global variable name to QObject.

I have used this to add support for eclipse/chrome remote debugging in QtTestBrowser.
Comment 1 Andreas Kling 2010-07-10 09:30:22 PDT
Comment on attachment 61104 [details]
patch to enable binding QObject to the javascript environment of a QWebInspector

First off, every patch needs a ChangeLog entry.
You can use WebKitTools/Scripts/prepare-ChangeLog to generate most of it.

>WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:79
> +          QVariant qvJsObjectMap = property("_q_inspector_js_objects");
Missing #ifndef QT_NO_PROPERTIES

>WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:80
> +          if (qvJsObjectMap.isValid()) {
WebKit normally uses early-return style, so this would be:
if (!qvJsObjectMap.isValid())
    return;

>WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:123
> +       QVariant qvJsObjectMap = inspector->property("_q_inspector_js_objects");
One too many spaces in the indentation here.

r- for missing QT_NO_PROPERTIES check. Change itself looks reasonable to me.
Comment 2 Jamey Hicks 2010-07-12 06:23:06 PDT
Created attachment 61219 [details]
patch revised to address comments

Patch revised to address comments.
Comment 3 WebKit Review Bot 2010-07-14 05:20:37 PDT
Attachment 61219 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:20:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:21:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 11 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jamey Hicks 2010-07-14 05:36:22 PDT
Created attachment 61508 [details]
Patch
Comment 5 Kenneth Rohde Christiansen 2010-07-19 05:26:50 PDT
Comment on attachment 61508 [details]
Patch

WebKit/qt/ChangeLog:14
 +          dynamic property "q_inspector_js_objects" on the
That is not normally how we name our private dynamic properties, plus I find the name quite non-descriptive. I believe the others are prefixed with _qt_ or _qt_webkit_ - you will have to check.

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:81
 +          if (!qvJsObjectMap.isValid())
qvJsObjectMap - please come up with a more descriptive name

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:85
 +          for (QMap<QString, QVariant>::const_iterator it = jsObjectMap.constBegin(); it != jsObjectMap.constEnd(); ++it) {
I would move QMap<QString, QVariant>::const_iterator it = jsObjectMap.constBegin() outside the look.
Comment 6 Jamey Hicks 2010-07-20 06:05:53 PDT
Created attachment 62061 [details]
Patch
Comment 7 Kenneth Rohde Christiansen 2010-07-20 13:51:17 PDT
Comment on attachment 62061 [details]
Patch

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:126
 +       QVariant inspectorJavaScriptWindowObjects = inspector->property("_q_inspectorJavaScriptWindowObjects");
indentation error
Comment 8 Kenneth Rohde Christiansen 2010-07-20 13:51:47 PDT
Comment on attachment 62061 [details]
Patch

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:128
 +        inspectorPage->setProperty("_q_inspectorJavaScriptWindowObjects", inspectorJavaScriptWindowObjects);
and here as well
Comment 9 Jamey Hicks 2010-07-21 08:01:55 PDT
Created attachment 62180 [details]
Patch
Comment 10 Jamey Hicks 2010-07-22 07:30:17 PDT
We would like this included in QtWebKit 2.1 for use in on-device debugging and profiling.
Comment 11 WebKit Commit Bot 2010-07-22 10:02:27 PDT
Comment on attachment 62180 [details]
Patch

Clearing flags on attachment: 62180

Committed r63899: <http://trac.webkit.org/changeset/63899>
Comment 12 WebKit Commit Bot 2010-07-22 10:02:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Simon Hausmann 2010-07-30 04:11:42 PDT
Revision r63899 cherry-picked into qtwebkit-2.1 with commit e4306d5c4e0a48a34152e7d2498feaf4654e2bf9