Bug 87113 – [GTK] Add inspector API to WebKit2 GTK+

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

API to allow interact with the inspector and also implement custom attach/detach

[Carlos Garcia Campos]

Created attachment 143263 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 143263 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143263&action=review

Looks great. Couple nits below, but in general I really like the new API.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:134
> +     * To prevent the inspector from being shown you can connecto the this

Nit connectto -> "connect to"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:150
> +     * WebKitWebInspector::bring-to-front:

Perhaps this should be called "raise" to match "gdk_window_raise" or "present" to match "gtk_window_present"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:161
> +     * In both cases, if this signal is not hanlded, the default implementation

hanlded -> handled

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:200
> +     * attached to the inspected view, so you only need tyo handle this signal

need tyo -> need to

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:224
> +     * Emitted when the inspector is requested to be detached from the window
> +     * where it currently is. The inspector is detached when the inspector page

Perhaps better phrased: "Emitted when the #WebKitWebInspector is requesting to be detached from the window it is currently attached to."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:226
> +     * #WebKitWebInspector::closed, or when the user clicks on detach button

on detach button -> on the detach button

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:340
> + * Returns: the URI that is currently being inspected.

Can this be %NULL? If so, it would be good to mention it.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspector.cpp:351
> +    CustomInspectorTest::add("WebKitWebInspector", "custom", testInspectorCustom);

This could probably be more specific about what it's testing. Perhaps "manual-attach-detach"

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspector.cpp:352
> +    CustomInspectorTest::add("WebKitWebInspector", "inspector-window-destroyed", testInspectorWindowDestroyed);

Perhaps "containing-window-destroyed" ?

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 143263 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143263&action=review
> 
> Looks great. Couple nits below, but in general I really like the new API.

Thanks!, and sorry for the typos :-(

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:150
> > +     * WebKitWebInspector::bring-to-front:
> 
> Perhaps this should be called "raise" to match "gdk_window_raise" or "present" to match "gtk_window_present"

Well, this depends on the implementation, for example, if the inspector is attached in a browser tab, bring-to-front is not just raise or present the window, but also make the tab active.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:224
> > +     * Emitted when the inspector is requested to be detached from the window
> > +     * where it currently is. The inspector is detached when the inspector page
> 
> Perhaps better phrased: "Emitted when the #WebKitWebInspector is requesting to be detached from the window it is currently attached to."

Indeed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:340
> > + * Returns: the URI that is currently being inspected.
> 
> Can this be %NULL? If so, it would be good to mention it.

I think it's about:blank, but I'll check it.
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspector.cpp:351
> > +    CustomInspectorTest::add("WebKitWebInspector", "custom", testInspectorCustom);
> 
> This could probably be more specific about what it's testing. Perhaps "manual-attach-detach"

Ok.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspector.cpp:352
> > +    CustomInspectorTest::add("WebKitWebInspector", "inspector-window-destroyed", testInspectorWindowDestroyed);
> 
> Perhaps "containing-window-destroyed" ?

containing window might be the inspected window when the inspector is attached, inspector window always refers to the external window that contains the inspector. This is testing the case when the external custom window, which not known by wk, is destroyed.

[Martin Robinson]

(In reply to comment #4)

> > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspector.cpp:352
> > > +    CustomInspectorTest::add("WebKitWebInspector", "inspector-window-destroyed", testInspectorWindowDestroyed);
> > 
> > Perhaps "containing-window-destroyed" ?
> 
> containing window might be the inspected window when the inspector is attached, inspector window always refers to the external window that contains the inspector. This is testing the case when the external custom window, which not known by wk, is destroyed.

"custom-container-destroyed"  or something similar?

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> 
> > > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspector.cpp:352
> > > > +    CustomInspectorTest::add("WebKitWebInspector", "inspector-window-destroyed", testInspectorWindowDestroyed);
> > > 
> > > Perhaps "containing-window-destroyed" ?
> > 
> > containing window might be the inspected window when the inspector is attached, inspector window always refers to the external window that contains the inspector. This is testing the case when the external custom window, which not known by wk, is destroyed.
> 
> "custom-container-destroyed"  or something similar?

yeah, that sounds better.

[Carlos Garcia Campos]

Committed r118146: <http://trac.webkit.org/changeset/118146>