Bug 22551 – [GTK] API to start inspector for a WebView

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Kalle Vahlman]

The only way to open the inspector currently in the GTK+ port is to use the context menu. This is kind of hard to find, and in any case it would be nice to initiate the inspector from a keyboard shortcut or a button/menuitem in the client UI, specially when the context menu isn't available (touchscreen UI for example).

So we need an API to do that for a WebView.

[Kalle Vahlman]

Created attachment 25589 [details]
New API:  void webkit_web_view_inspect(WebKitWebView* web_view)

The inspect() method starts the inspector with the document node of either the focused frame or the main frame for the view. I think that's a sane default thing to do...

It would be nice to have API to inspect a certain node too, but since we don't have suitable DOM bindings ready yet, that needs to wait.

[Kalle Vahlman]

Created attachment 25590 [details]
A simple patch for GtkLauncher to test the API

[Holger Freyther]

Why did you put this in WebKitWebView and not in the WebKitWebInspector?

[Kalle Vahlman]

(In reply to comment #3)
> Why did you put this in WebKitWebView and not in the WebKitWebInspector?

I operated purely on instinct. :)

"inspect this view" was the task I set out to accomplish, and webkit_web_view_inspect() seemed the natural way to do that. Logically of course the method does belong to the inspector, the only bad side of that is that it introduces a otherwise (seemingly) worthless get() call (which usually sucks in C API).

But I guess it's better to put the implementation where it belongs so I'll just redo the patch to add the API on the inspector.

[Luca Ferretti]

Just a question/request: could you extend the API in order to open a specific section of Web Inspector? Something like 

  webkit_web_view_inspect (view, INSPECTOR_RESOURCES);

to directly open the Resources section of Web Inspector, (by now the inspector opens in Element section).

This could be used by applications like Epiphany to provide a Safari-like "Develop" menu, as well as in features like View->Page Source (see [1] for this)

[1] http://bugzilla.gnome.org/show_bug.cgi?id=562446#c7

[Christian Dywan]

(In reply to comment #5)
> Just a question/request: could you extend the API in order to open a specific
> section of Web Inspector? Something like 
> 
>   webkit_web_view_inspect (view, INSPECTOR_RESOURCES);
> 
> to directly open the Resources section of Web Inspector, (by now the inspector
> opens in Element section).

I like that idea very much. I'd like that to also switch an existing inspector to the specified section. I wonder if the 'section' should be a string, since pages might be added in the future or even by the user, I remember seeing that described on a website.

Which makes me think of native integration of the inspector. If the artifical toolbar could be hidden, the application could add its own, and merge other application specific views in the same interface.

[Gustavo Noronha (kov)]

(In reply to comment #6)
> I like that idea very much. I'd like that to also switch an existing inspector
> to the specified section. I wonder if the 'section' should be a string, since
> pages might be added in the future or even by the user, I remember seeing that
> described on a website.

The panel seems to currently be indexed by an enum, in the inspector controller. The client doesn't provide facilities for that. What do you suggest for indexing the panel, if not strings?

    typedef enum {
        CurrentPanel,
        ConsolePanel,
        DatabasesPanel,
        ElementsPanel,
        ProfilesPanel,
        ResourcesPanel,
        ScriptsPanel
    } SpecialPanels;

> Which makes me think of native integration of the inspector. If the artifical
> toolbar could be hidden, the application could add its own, and merge other
> application specific views in the same interface.
 
I would love that, yeah. I'm not sure how feasible that is.

[Christian Dywan]

(In reply to comment #7)
> (In reply to comment #6)
> > I like that idea very much. I'd like that to also switch an existing inspector
> > to the specified section. I wonder if the 'section' should be a string, since
> > pages might be added in the future or even by the user, I remember seeing that
> > described on a website.
> 
> The panel seems to currently be indexed by an enum, in the inspector
> controller. The client doesn't provide facilities for that. What do you suggest
> for indexing the panel, if not strings?

I actually assumed an enumeration when I read the suggestion, so I suggested a string.

> > Which makes me think of native integration of the inspector. If the artifical
> > toolbar could be hidden, the application could add its own, and merge other
> > application specific views in the same interface.
> 
> I would love that, yeah. I'm not sure how feasible that is.

I think it should be easy enough. What I'm more worried about is the controls inside the inspector, that is WebKit doesn't support native elements to the extend the inspector would need it. So maybe it would actually be more useful to investigate that and for instance if the toolbar and statusbar had a (mostly) native appearance, we could avoid such workarounds.

[Gustavo Noronha (kov)]

I suggest adding this API as proposed, since having a way to programatically open the inspector is important. I agree that providing more flexibility regarding what to inspect, and what tab to open the inspector on are good to have features, but we could add those as APIs to the inspector object, instead, after we work out the dependencies (DOM bindings, and how to specify the "tab"). If nobody is against, I'll r+ this patch.

[Christian Dywan]

(In reply to comment #9)
> I suggest adding this API as proposed, since having a way to programatically
> open the inspector is important. I agree that providing more flexibility
> regarding what to inspect, and what tab to open the inspector on are good to
> have features, but we could add those as APIs to the inspector object, instead,
> after we work out the dependencies (DOM bindings, and how to specify the
> "tab"). If nobody is against, I'll r+ this patch.

I'm fine with it. No sense in blocking patches forever.

[Xan Lopez]

I think I'd rather have all the API in one place, so if there's a consensus that this should go in WebKitWebInspector I think we should put it there to begin with.

We can later add functions to inspect specific nodes or to open it in specific pages.

[Xan Lopez]

(In reply to comment #11)
> I think I'd rather have all the API in one place, so if there's a consensus
> that this should go in WebKitWebInspector I think we should put it there to
> begin with.
> 
> We can later add functions to inspect specific nodes or to open it in specific
> pages.
> 

Well, and that being said, is there a reason for not only having the API to open it in a specific page? I don't think that depends on anything, and we could do just that unless somebody thinks having one parameter-less "open in the default page" is a big win.

[Christian Dywan]

(In reply to comment #12)
> (In reply to comment #11)
> > I think I'd rather have all the API in one place, so if there's a consensus
> > that this should go in WebKitWebInspector I think we should put it there to
> > begin with.
> > 
> > We can later add functions to inspect specific nodes or to open it in specific
> > pages.
> > 
> 
> Well, and that being said, is there a reason for not only having the API to
> open it in a specific page? I don't think that depends on anything, and we
> could do just that unless somebody thinks having one parameter-less "open in
> the default page" is a big win.

I think Gustavo mainly didn't want to let the bug vanish rot, and after the question of choosing pages was raised nobody replied anymore.

For what I want, we can have webkit_web_inspector_open (const gchar* page) if that works for you. Using a string to specify the page was suggested earlier and to me seems like the way to go.

[Kalle Vahlman]

(In reply to comment #13)
> (In reply to comment #12)
> > Well, and that being said, is there a reason for not only having the API to
> > open it in a specific page? I don't think that depends on anything, and we
> > could do just that unless somebody thinks having one parameter-less "open in
> > the default page" is a big win.
> 
> I think Gustavo mainly didn't want to let the bug vanish rot, and after the
> question of choosing pages was raised nobody replied anymore.
> 
> For what I want, we can have webkit_web_inspector_open (const gchar* page) if
> that works for you. Using a string to specify the page was suggested earlier
> and to me seems like the way to go.

It's slightly nasty, but does give a bit more future- and change-proof API.

So something like this perhaps:

/**
 * webkit_web_inspector_inspect:
 * @web_inspector: a #WebKitWebInspector
 * @inspector_panel: a string identifying the panel to show
 *
 * Activates the #WebKitWebInspector and switches it to the specified inspector
 * panel. The inspector is activated with the document node of the currently
 * focused or the main frame of the associated #WebKitWebView. If the panel
 * string is NULL or specifies an unkown panel, no switching will occur.
 *
 * Since: 1.1.6
 **/
void webkit_web_inspector_inspect(WebKitWebInspector *web_inspector,
                                  const gchar *inspector_panel)

followed by inspect_element(web_inspector, inspector_panel, dom_element) as soon as the dom bindings are available.

If the above is ok, I'll redo the patch accordingly. Actually I started already, but it's going to take few nights since someone forgot to add the 12 extra hours to a day so there's good time for feedback...

[Kalle Vahlman]

Comment on attachment 25589 [details]
New API:  void webkit_web_view_inspect(WebKitWebView* web_view)

Will be redone

[Gustavo Noronha (kov)]

(In reply to comment #14)
> /**
>  * webkit_web_inspector_inspect:
>  * @web_inspector: a #WebKitWebInspector
>  * @inspector_panel: a string identifying the panel to show
>  *
>  * Activates the #WebKitWebInspector and switches it to the specified inspector
>  * panel. The inspector is activated with the document node of the currently
>  * focused or the main frame of the associated #WebKitWebView. If the panel
>  * string is NULL or specifies an unkown panel, no switching will occur.
>  *
>  * Since: 1.1.6
>  **/
> void webkit_web_inspector_inspect(WebKitWebInspector *web_inspector,
>                                   const gchar *inspector_panel)

That sounds good to me, but I think the verb has to be changed from inspect to something else. The best option I could come up with up to now is 'start'; I had considered things such as 'show', 'launch', 'present', but these have well-defined meanings in GTK+ already, so I propose using 'start' or 'open' as suggested by Christian, whatever works best for you.

> followed by inspect_element(web_inspector, inspector_panel, dom_element) as
> soon as the dom bindings are available.

Right =).

> If the above is ok, I'll redo the patch accordingly. Actually I started
> already, but it's going to take few nights since someone forgot to add the 12
> extra hours to a day so there's good time for feedback...

Thanks!

[Kalle Vahlman]

(In reply to comment #16)
> (In reply to comment #14)
> > /**
> >  * webkit_web_inspector_inspect:
> >  * @web_inspector: a #WebKitWebInspector
> >  * @inspector_panel: a string identifying the panel to show
> >  *
> >  * Activates the #WebKitWebInspector and switches it to the specified inspector
> >  * panel. The inspector is activated with the document node of the currently
> >  * focused or the main frame of the associated #WebKitWebView. If the panel
> >  * string is NULL or specifies an unkown panel, no switching will occur.
> >  *
> >  * Since: 1.1.6
> >  **/
> > void webkit_web_inspector_inspect(WebKitWebInspector *web_inspector,
> >                                   const gchar *inspector_panel)
> 
> That sounds good to me, but I think the verb has to be changed from inspect to
> something else.

Oh, good point. It worked for the view API but is clearly wrong here.

> The best option I could come up with up to now is 'start'; I
> had considered things such as 'show', 'launch', 'present', but these have
> well-defined meanings in GTK+ already, so I propose using 'start' or 'open' as
> suggested by Christian, whatever works best for you.

What about _activate()? It would then make sense to use this method to switch the panel in the already visible inspector. And maybe then provide _activate_panel() if someone wants a version that doesn't imply activating the inspector itself. Though _open() works for that too...

[Gustavo Noronha (kov)]

(In reply to comment #17)
> > The best option I could come up with up to now is 'start'; I
> > had considered things such as 'show', 'launch', 'present', but these have
> > well-defined meanings in GTK+ already, so I propose using 'start' or 'open' as
> > suggested by Christian, whatever works best for you.
> 
> What about _activate()? It would then make sense to use this method to switch
> the panel in the already visible inspector. And maybe then provide
> _activate_panel() if someone wants a version that doesn't imply activating the
> inspector itself. Though _open() works for that too...

Could be confused with the activate signal (or at least with its meaning). Ok, I vote for open.

[Christian Dywan]

At this point have webkit_web_inspector_open (inspector, string) would be crucial now that implementing a context menu in the application is feasible.

[Gustavo Noronha (kov)]

Created attachment 41940 [details]
proposed API

Here's my take on it. I propose we start with this simple coordinates-based API, and later on we can add more APIs to inspect with a tab already open, and to inspect a specific DOM element.

[Gustavo Noronha (kov)]

Created attachment 41941 [details]
Epiphany usage for the API

As usual, a patch for Epiphany to use the new API.

[Jan Alonzo]

Comment on attachment 41940 [details]
proposed API

> +void webkit_web_inspector_inspect_coordinates(WebKitWebInspector* webInspector, gdouble x, gdouble y)

What do you think of inspect_node? Also, does x & y need to be (g)doubles?

>  
> +WEBKIT_API void
> +webkit_web_inspector_inspect_coordinates(WebKitWebInspector* web_inspector, gdouble x, gdouble y);

With this patch, I suppose we can also start enabling the WebInspector tests (which are currently Skipped)?

[Gustavo Noronha (kov)]

(In reply to comment #22)
> (From update of attachment 41940 [details])
> > +void webkit_web_inspector_inspect_coordinates(WebKitWebInspector* webInspector, gdouble x, gdouble y)
> 
> What do you think of inspect_node? Also, does x & y need to be (g)doubles?

I think inspect_node should be reserved for when we have DOM bindings and can give it a proper GDOMNode or something. I am using gdoubles here to match GdkEvent.

> > +WEBKIT_API void
> > +webkit_web_inspector_inspect_coordinates(WebKitWebInspector* web_inspector, gdouble x, gdouble y);
> 
> With this patch, I suppose we can also start enabling the WebInspector tests
> (which are currently Skipped)?

Hrm. I'll look at that, good catch! =)

[Xan Lopez]

Comment on attachment 41940 [details]
proposed API

>+/**
>+ * webkit_web_inspector_inspect_coordinates:
>+ * @web_inspector: the #WebKitWebInspector that will do the inspection
>+ * @x: the X coordinate of the node to be inspected
>+ * @y: the Y coordinate of the node to be inspected
>+ *
>+ * Causes the Web Inspector to inspect the node that is located at the
>+ * given coordinates.
>+ */

Missing the "Since:" stuff.

>+void webkit_web_inspector_inspect_coordinates(WebKitWebInspector* webInspector, gdouble x, gdouble y)
>+{
>+    g_return_if_fail(WEBKIT_IS_WEB_INSPECTOR(webInspector));

Perhaps you could check the coordinates are not negative, while you are it.

>+
>+    WebKitWebInspectorPrivate* priv = webInspector->priv;
>+
>+    Frame* frame = priv->page->focusController()->focusedOrMainFrame();
>+    FrameView* view = frame->view();
>+
>+    if (!view)
>+        return;
>+
>+    HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active);
>+    IntPoint documentPoint = view->windowToContents(IntPoint(static_cast<int>(x), static_cast<int>(y)));
>+    HitTestResult result(documentPoint);
>+
>+    frame->contentRenderer()->layer()->hitTest(request, result);
>+    priv->page->inspectorController()->inspect(result.innerNonSharedNode());
>+}

Looks good to me otherwise, you have my 1/2 r+.

[Gustavo Noronha (kov)]

Created attachment 41996 [details]
take 2

Now with tests being enabled - required two more APIs, I decided to make one of them public, and the other private, but we may consider making close() private if we are not sure about it.

[Jan Alonzo]

Comment on attachment 41996 [details]
take 2

> +    // Make the Web Inspector work when running tests
> +    if (g_file_test("WebCore/inspector/front-end/inspector.html", G_FILE_TEST_EXISTS)) {

I believe this will fail in srcdir != builddir scenario when running the tests. Ditto with localizedStrings.js.

> +/**
> + * webkit_web_inspector_inspect_coordinates:
> + * @web_inspector: the #WebKitWebInspector that will do the inspection
> + * @x: the X coordinate of the node to be inspected
> + * @y: the Y coordinate of the node to be inspected
> + *
> + * Causes the Web Inspector to inspect the node that is located at the
> + * given coordinates.

It would be nice to put a  note here where to get those coordinates from, and valid values of x & y.

> + *
> + * Since: 1.1.17
> + */
> +void webkit_web_inspector_inspect_coordinates(WebKitWebInspector* webInspector, gdouble x, gdouble y)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_INSPECTOR(webInspector));
> +    g_return_if_fail((x >= 0) && (y >= 0));

I really think we should just make x & y guints so we don't need to do checks like these and the int casts below.
The usage in the epiphany patch also casts the int to double unnecessarily.

> +    HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active);
> +    IntPoint documentPoint = view->windowToContents(IntPoint(static_cast<int>(x), static_cast<int>(y)));
> +    HitTestResult result(documentPoint);
> +

[Gustavo Noronha (kov)]

(In reply to comment #26)
> (From update of attachment 41996 [details])
> > +    // Make the Web Inspector work when running tests
> > +    if (g_file_test("WebCore/inspector/front-end/inspector.html", G_FILE_TEST_EXISTS)) {
> 
> I believe this will fail in srcdir != builddir scenario when running the tests.
> Ditto with localizedStrings.js.

It should work fine, I am testing in such a scenario - the same used by the buildbot. The only thing that needs to happen is you need to be at the $srcdir when running the tests (which is what happens in the bot). I would prefer doing something more flexible, but have no idea of how.

> > + * Causes the Web Inspector to inspect the node that is located at the
> > + * given coordinates.
> 
> It would be nice to put a  note here where to get those coordinates from, and
> valid values of x & y.

Sounds fair. The most common way would be getting them from a mouse press GdkEvent, I believe (coming from handling the button_press_event).

> > +    g_return_if_fail(WEBKIT_IS_WEB_INSPECTOR(webInspector));
> > +    g_return_if_fail((x >= 0) && (y >= 0));
> 
> I really think we should just make x & y guints so we don't need to do checks
> like these and the int casts below.
> The usage in the epiphany patch also casts the int to double unnecessarily.

I don't have a too strong opinion on this. I am just following what's in GdkEvent, really. In Epiphany we have to cast back because the EphyEmbedEvent object stores this information as int. I am assuming that is a historical artifact from gecko times, and most API users would instead use GdkEvent directly.

http://library.gnome.org/devel/gdk/unstable/gdk-Events.html#gdk-event-get-coords

Xan, do you want to untie this? =)

[Xan Lopez]

(In reply to comment #27)
> I don't have a too strong opinion on this. I am just following what's in
> GdkEvent, really. In Epiphany we have to cast back because the EphyEmbedEvent
> object stores this information as int. I am assuming that is a historical
> artifact from gecko times, and most API users would instead use GdkEvent
> directly.
> 
> http://library.gnome.org/devel/gdk/unstable/gdk-Events.html#gdk-event-get-coords
> 
> Xan, do you want to untie this? =)

Since doubles are used in GTK+ to store coordinates I prefer having the API accept doubles. Better for us to cast it than having everyone do so.

Also,

+    g_return_if_fail((x >= 0) && (y >= 0));

Unneeded parenthesis.

+void webkit_web_inspector_execute_script(WebKitWebInspector* webInspector, long callId, const gchar* script)
+{
+    g_return_if_fail(WEBKIT_IS_WEB_INSPECTOR(webInspector));

That could at least test script is not NULL, I guess?

[Gustavo Noronha (kov)]

Created attachment 42014 [details]
take 3

Improved docs, and checks as requested by Jan, and Xan.

[Jan Alonzo]

(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 41996 [details] [details])
> > > +    // Make the Web Inspector work when running tests
> > > +    if (g_file_test("WebCore/inspector/front-end/inspector.html", G_FILE_TEST_EXISTS)) {
> > 
> > I believe this will fail in srcdir != builddir scenario when running the tests.
> > Ditto with localizedStrings.js.
> 
> It should work fine, I am testing in such a scenario - the same used by the
> buildbot. The only thing that needs to happen is you need to be at the $srcdir
> when running the tests (which is what happens in the bot). I would prefer doing
> something more flexible, but have no idea of how.

It's ok for now. r=me.

[Gustavo Noronha (kov)]

Landed as r50245.