Bug 41995

Summary: [Qt] Enable binding of QObjects to JavaScript environment for inspector frontend
Product: WebKit Reporter: Jamey Hicks <jamey.hicks>
Component: WebKit QtAssignee: QtWebKit Unassigned <webkit-qt-unassigned>
Severity: Normal CC: commit-queue, hausmann, kling, laszlo.gombos, webkit.review.bot
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
patch to enable binding QObject to the javascript environment of a QWebInspector
kling: review-
patch revised to address comments
Patch none

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.

> +          QVariant qvJsObjectMap = property("_q_inspector_js_objects");
Missing #ifndef QT_NO_PROPERTIES

> +          if (qvJsObjectMap.isValid()) {
WebKit normally uses early-return style, so this would be:
if (!qvJsObjectMap.isValid())

> +       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]
Comment 5 Kenneth Rohde Christiansen 2010-07-19 05:26:50 PDT
Comment on attachment 61508 [details]

 +          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.

 +          if (!qvJsObjectMap.isValid())
qvJsObjectMap - please come up with a more descriptive name

 +          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]
Comment 7 Kenneth Rohde Christiansen 2010-07-20 13:51:17 PDT
Comment on attachment 62061 [details]

 +       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]

 +        inspectorPage->setProperty("_q_inspectorJavaScriptWindowObjects", inspectorJavaScriptWindowObjects);
and here as well
Comment 9 Jamey Hicks 2010-07-21 08:01:55 PDT
Created attachment 62180 [details]
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]

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