Bug 87113 - [GTK] Add inspector API to WebKit2 GTK+
Summary: [GTK] Add inspector API to WebKit2 GTK+
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2012-05-22 03:40 PDT by Carlos Garcia Campos
Modified: 2012-05-23 00:56 PDT (History)
3 users (show)

See Also:


Attachments
Patch (65.87 KB, patch)
2012-05-22 03:57 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-05-22 03:40:50 PDT
API to allow interact with the inspector and also implement custom attach/detach
Comment 1 Carlos Garcia Campos 2012-05-22 03:57:34 PDT
Created attachment 143263 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-22 04:04:16 PDT
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
Comment 3 Martin Robinson 2012-05-22 07:47:12 PDT
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" ?
Comment 4 Carlos Garcia Campos 2012-05-22 08:19:13 PDT
(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.
Comment 5 Martin Robinson 2012-05-22 08:35:01 PDT
(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?
Comment 6 Carlos Garcia Campos 2012-05-22 08:38:43 PDT
(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.
Comment 7 Carlos Garcia Campos 2012-05-23 00:56:04 PDT
Committed r118146: <http://trac.webkit.org/changeset/118146>