Bug 35340

Summary: [Qt] Enable alternate HTML/JavaScript front-ends for Web Inspector
Product: WebKit Reporter: Jamey Hicks <jamey.hicks>
Component: WebKit QtAssignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: CLOSED FIXED    
Severity: Enhancement CC: commit-queue, dan.podwall, diegohcg, eostroukhov, hausmann, jturcotte, kenneth, laszlo.gombos, vestbo, webkit.review.bot
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 31552, 35784    
Attachments:
Description Flags
Adds QWebInspector::setFrontendURL() and QWebInspector::setDefaultFrontendUrl()
none
Adds QWebInspector::setFrontendURL() and QWebInspector::setDefaultFrontendUrl()
none
revised patch to fix style problems in WebKit/qt/ChangeLog
laszlo.gombos: review-
revised to use QWebSettings::setInspectorUrl() and QWebSettings::inspectorUrl() instead of a change to the QWebInspector API.
none
diagram of remote Web Inspector use case
none
Patch to replace public API with private dynamic property none

Description Jamey Hicks 2010-02-24 05:40:37 PST
This patch enables the use of alternate UIs for Web Inspector by loading different HTML/JavaScript when QWebInspector is instantiated and shown.

We're planning to use this to allow remote use of Web Inspector via HTTP and to use Eclipse or Aptana to debug and profile WebKit.

An earlier patch was submitted but that approach was given review- and the approach used in this patch (i.e., using alternate HTML/JavaScript) was recommended.
Comment 1 Jamey Hicks 2010-02-24 06:21:54 PST
Created attachment 49385 [details]
Adds QWebInspector::setFrontendURL() and QWebInspector::setDefaultFrontendUrl()
Comment 2 Jamey Hicks 2010-02-24 08:10:14 PST
Created attachment 49395 [details]
Adds QWebInspector::setFrontendURL() and QWebInspector::setDefaultFrontendUrl()

Revised ChangeLog
Comment 3 WebKit Review Bot 2010-02-24 10:40:53 PST
Attachment 49395 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring "WebKit/qt/Api/qwebinspector.cpp": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebinspector.h": this file is exempt from the style guide.
WebKit/qt/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
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:11:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:12:  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]
Ignoring "WebKit/qt/Api/qwebpage_p.h": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebpage.h": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebpage.cpp": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebinspector_p.h": this file is exempt from the style guide.
Total errors found: 9 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jamey Hicks 2010-02-24 11:45:16 PST
Created attachment 49418 [details]
revised patch to fix style problems in WebKit/qt/ChangeLog
Comment 5 Laszlo Gombos 2010-02-25 06:46:20 PST
Comment on attachment 49418 [details]
revised patch to fix style problems in WebKit/qt/ChangeLog

Approach looks good to me, this is a great addition to QtWebKit.

ChangeLog does not seems to follow the template. The "Reviewed by NOBODY (OOPS!)." part is missing. Also I have added [Qt] to the title of the bug, so if you could regenerate the ChnageLog that would be good.

Based on IRC discussion with Kenneth, Jamey and myself we concluded the the API for setting/getting the Inspector URL should be:

void QWebSettings::setInspectorUrl (const QUrl &inspector);
QUrl QWebSettings::inspectorUrl() const;

Please document these new APIs well.

This means that changes in InspectorClientQt.h would not be necessary but your style fixes are very welcome.

r- to use the new proposed API; general approach is good.
Comment 6 Jamey Hicks 2010-02-25 13:10:15 PST
Created attachment 49528 [details]
revised to use QWebSettings::setInspectorUrl() and QWebSettings::inspectorUrl() instead of a change to the QWebInspector API.

Revised to use QWebSettings::setInspectorUrl() and QWebSettings::inspectorUrl() instead of a change to the QWebInspector API.

Also added -inspector-url argument to QtLauncher so this can be tested.
Comment 7 Laszlo Gombos 2010-02-25 13:23:16 PST
Comment on attachment 49528 [details]
revised to use QWebSettings::setInspectorUrl() and QWebSettings::inspectorUrl() instead of a change to the QWebInspector API.

LGTM, Thanks Jamey.
Comment 8 WebKit Commit Bot 2010-02-26 01:56:12 PST
Comment on attachment 49528 [details]
revised to use QWebSettings::setInspectorUrl() and QWebSettings::inspectorUrl() instead of a change to the QWebInspector API.

Clearing flags on attachment: 49528

Committed r55273: <http://trac.webkit.org/changeset/55273>
Comment 9 WebKit Commit Bot 2010-02-26 01:56:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Jocelyn Turcotte 2010-02-26 05:47:43 PST
Is there any plan to make the server side of the remote inspector available in WebKit trunk somehow?

It would be nice to have a complete solution available to users without having to rely on external components possibly not compatible with the version of WebKit they use.

For example a separate executable compiled with the rest or eventually some kind of QRemoteWebInspector widget class that takes in some network parameters and start the HTTP server.
Comment 11 Jamey Hicks 2010-02-26 06:18:44 PST
I agree that it would be helpful to have the remote inspector available in WebKit trunk.

There are two pieces of this that are platform specific: The first is the piece I just patched for Qt -- the platform specific code that launches a particular html/javascript page for the inspector frontend. 

Someone would need to do this for other platforms.

The second is the server part. Using WebSockets to connect the UI and the inspector might remove the need for an HTTP server in WebKit, which would reduce the platform dependences.

In the short term, I'll be working on the HTTP server in Qt/WebKit and I do plan on making that public.
Comment 12 Simon Hausmann 2010-03-01 00:24:48 PST
I don't think this is the right API and I would like to roll out this patch.

Properties in QWebSettings should be useful for anyone using QWebPage, QWebView and QGraphicsWebView, QtWebKit in general.

This property is highly specific to the web inspector and therefore belongs into QWebInspector I think. The documentation should make it clear what it is useful for.

Jamey, can you explain exactly what you're doing and when you set this property? What features do you need of the inspector itself when you run it on the desktop?
Comment 13 Laszlo Gombos 2010-03-01 12:28:48 PST
Simon, the patch before (https://bug-35340-attachments.webkit.org/attachment.cgi?id=49418) use to have the interface on QWebInspector. Are you proposing a similar interface ?

This API tells Webkit the URL for the front-end resource files.

I would suggest to discuss (and fix if needed) the API without rolling the patch out.
Comment 14 Jamey Hicks 2010-03-01 12:53:01 PST
Simon,

We are enabling remote usage of all the inspector functionality. I'm working on two such remote cases:
1) Using Web Inspector UI remoted over HTTP to enable on-device debug and profiling.
2) Implementing ChromeDevTools protocol for Eclipse or Aptana debug and profiling on-device

The previous version of the patch extended the QWebInspector API but a discussion with laszlo and kenne on #qtwebkit led to this QWebSettings API. I have no particular preference between the two.

Jamey
Comment 15 Jocelyn Turcotte 2010-03-02 04:34:26 PST
(In reply to comment #14)
Just to dump how I see both alternatives:
- The API in QWebSettings is nice since it make it simple to apply it on all the pages. It also don't bind to the QWebInspector class which is currently only a widget class. Though its name suggests it could do more.
- The API in QWebInspector is nice since it don't clutter the normal web page usage API.

Both of them goes in the opposite was that we see usual remote debugging where the debuggee would open the server and the client would attach to the debuggee. If we want to change the way the implementation is done in the future this might cause us headaches.
Comment 16 Simon Hausmann 2010-03-08 01:52:38 PST
(In reply to comment #14)
> Simon,
> 
> We are enabling remote usage of all the inspector functionality. I'm working on
> two such remote cases:
> 1) Using Web Inspector UI remoted over HTTP to enable on-device debug and
> profiling.
> 2) Implementing ChromeDevTools protocol for Eclipse or Aptana debug and
> profiling on-device
> 
> The previous version of the patch extended the QWebInspector API but a
> discussion with laszlo and kenne on #qtwebkit led to this QWebSettings API. I
> have no particular preference between the two.

Ah, so when you bring up the inspector with a remote URL, do you actually use any functionaliy of the inspector backend?

Do I understand correctly that the _use case_ behind this is that you have WebKit running on a mobile platform, with the embedded web server enabled. Then on your _desktop_ machine you take a QWebInspector, set the remote inspector url to the address of the mobile device and fire up the inspector?

In that case, how much of QWebInspector is actually used? Or could you also just point a QWebView to the remote url and it would work just as well?

Please elaborate and explain :), then it's easier for us to understand the API.
Comment 17 Jamey Hicks 2010-03-08 05:12:56 PST
Created attachment 50213 [details]
diagram of remote Web Inspector use case

In this use case, the inspected webkit (on the left) exports a JSON over HTTP interface to the full inspector backend functionality, enabling the Web Inspector UI to be run on a different machine.

In order for the javascript to accept a connection, either it will have to use an intermediary, so that both inspected and inspector javascript use HTTP client sockets, or one of the browsers will need to use an extension enabling HTTP server sockets.
Comment 18 Jamey Hicks 2010-03-08 05:18:06 PST
The frontend URL being specified in this patch is actually a local URL. In the use cases we are most interested in, the URL exposes an interface to the inspector backend so that the backend may be used remotely.
Comment 19 Simon Hausmann 2010-03-08 07:30:27 PST
I see, thanks Jamey for the explanation!

I understand that this feature is still in development, so for the moment you don't really need a finalized public API, right?

What about the option of building WebKit without the embedded .qrc?

Then you could simply load your own resource with with the different inspector html/js/css files at run-time through QResource::registerResource and qrc:/webkit/inspector as mapping root?


Perhaps we should always build WebKit with the inspector frontend separated and just install the bundle together with WebKit.
Comment 20 Jamey Hicks 2010-03-08 07:45:47 PST
(In reply to comment #19)
> I see, thanks Jamey for the explanation!
> 
> I understand that this feature is still in development, so for the moment you
> don't really need a finalized public API, right?

We need a finalized public API this month to get it into the next software release for phones. The development of HTML and javascript for remote access to the backend can proceed in parallel with that.

> What about the option of building WebKit without the embedded .qrc?
> 
> Then you could simply load your own resource with with the different inspector
> html/js/css files at run-time through QResource::registerResource and
> qrc:/webkit/inspector as mapping root?

That would work. It makes it difficult to specify per page inspector URLs but then again that's not a requirement for our use cases.
Comment 21 Laszlo Gombos 2010-03-08 07:54:47 PST
> I understand that this feature is still in development, so for the moment you
> don't really need a finalized public API, right?
> 
> What about the option of building WebKit without the embedded .qrc?


At least in my mind that was very much one of the use-cases as the resources are not enabled by default on Symbian (and maybe they should not be enabled for maemo either).

I did try to use this API to see if it enables this use-case and heppy to report that it did work.
Comment 22 Laszlo Gombos 2010-03-23 09:06:26 PDT
Simon, do you have a better proposal for the API, or should we just close this bug so that it is ready for the release ?
Comment 23 Diego Gonzalez 2010-04-15 16:28:45 PDT
(In reply to comment #22)
> Simon, do you have a better proposal for the API, or should we just close this
> bug so that it is ready for the release ?

It seems this bug could be closed? Isn't it?
Comment 24 Simon Hausmann 2010-04-16 15:54:02 PDT
How about using a dynamic property on QWebInspector instead of this public API?

AFAICS the purpose of this API is to develop the remote inspector feature. It is not meant to be useful for developers using QtWebKit for application development.
Comment 25 Jamey Hicks 2010-04-19 14:19:01 PDT
(In reply to comment #24)
> How about using a dynamic property on QWebInspector instead of this public API?
> 

A dynamic property would also work. As an internal property, should the name of the property begin with _q_? In this case _q_inspectorFrontendUrl ?

As we are debugging the remote debugger, a second useful property would be to create the inspector page without showing it, which could be controlled by property _q_createInspectorImmediately or something like that.
Comment 26 Simon Hausmann 2010-04-19 18:45:37 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > How about using a dynamic property on QWebInspector instead of this public API?
> > 
> 
> A dynamic property would also work. As an internal property, should the name of
> the property begin with _q_? In this case _q_inspectorFrontendUrl ?

Yeah, that's okay.

Just remember to never use these properties in application code outside the WebKit source tree. So the use in DRT or QtLauncher is fine. Using it in other application is bad and will cause regressions.

> As we are debugging the remote debugger, a second useful property would be to
> create the inspector page without showing it, which could be controlled by
> property _q_createInspectorImmediately or something like that.

Hm, when should it be created instead? The first time show() is called?
Comment 27 Jocelyn Turcotte 2010-05-04 10:59:01 PDT
Any update on this?
Comment 28 Laszlo Gombos 2010-05-04 11:09:03 PDT
(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > How about using a dynamic property on QWebInspector instead of this public API?
> > > 
> > 
> > A dynamic property would also work. As an internal property, should the name of
> > the property begin with _q_? In this case _q_inspectorFrontendUrl ?
> 
> Yeah, that's okay.
> 
> Just remember to never use these properties in application code outside the
> WebKit source tree. So the use in DRT or QtLauncher is fine. Using it in other
> application is bad and will cause regressions.

I think the use-case requires using this API outside of WebKit. I'd like to see this functionality API publicly exposed as a supported API.
Comment 29 Simon Hausmann 2010-05-05 07:53:15 PDT
(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (In reply to comment #24)
> > > > How about using a dynamic property on QWebInspector instead of this public API?
> > > > 
> > > 
> > > A dynamic property would also work. As an internal property, should the name of
> > > the property begin with _q_? In this case _q_inspectorFrontendUrl ?
> > 
> > Yeah, that's okay.
> > 
> > Just remember to never use these properties in application code outside the
> > WebKit source tree. So the use in DRT or QtLauncher is fine. Using it in other
> > application is bad and will cause regressions.
> 
> I think the use-case requires using this API outside of WebKit. I'd like to see
> this functionality API publicly exposed as a supported API.

I think it makes sense to expose this as a supported API once the feature - remote debugging with the web inspector - is complete, so that it becomes an actually useful feature for third-party developers.

It is my understanding that at this point this is merely a tool during the development.
Comment 30 Simon Hausmann 2010-05-05 08:07:10 PDT
Created attachment 55124 [details]
Patch to replace public API with private dynamic property
Comment 31 WebKit Commit Bot 2010-05-06 06:01:09 PDT
Comment on attachment 55124 [details]
Patch to replace public API with private dynamic property

Clearing flags on attachment: 55124

Committed r58876: <http://trac.webkit.org/changeset/58876>
Comment 32 WebKit Commit Bot 2010-05-06 06:01:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Simon Hausmann 2010-05-06 07:07:25 PDT
Re-opening
Comment 34 Kenneth Rohde Christiansen 2010-05-07 06:33:18 PDT
(In reply to comment #33)
> Re-opening

Why was this reopened?
Comment 35 Simon Hausmann 2010-05-12 07:22:58 PDT
Landed http://trac.webkit.org/changeset/59231 to clarify that the dynamic property is there on purpose.
Comment 36 Simon Hausmann 2010-05-21 01:02:33 PDT
<cherry-pick-for-backport: r59231>
Comment 37 Simon Hausmann 2010-05-21 01:13:31 PDT
Revision r55273 cherry-picked into qtwebkit-2.0 with commit d63440e7437006c1105bc17f769a07812512fa57
Revision r58876 cherry-picked into qtwebkit-2.0 with commit 015d13573930a82c684dec0f30a1c92bbb5534e2
Revision r59231 cherry-picked into qtwebkit-2.0 with commit dc1fbb8ce49e6066c6136e5dfcf66fd35fb77af8
Comment 38 Eugene Ostroukhov 2010-09-09 10:24:29 PDT
> We are enabling remote usage of all the inspector functionality. I'm working on two such remote cases:
> 1) Using Web Inspector UI remoted over HTTP to enable on-device debug and profiling.
> 2) Implementing ChromeDevTools protocol for Eclipse or Aptana debug and profiling on-device

Will these parts be available as a part of Qt or they will be parts of proprietary code?