[webkit-reviews] review granted: [Bug 87113] [GTK] Add inspector API to WebKit2 GTK+ : [Attachment 143263] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 87113: [GTK] Add inspector API to WebKit2 GTK+
https://bugs.webkit.org/show_bug.cgi?id=87113

Attachment 143263: Patch
https://bugs.webkit.org/attachment.cgi?id=143263&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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" ?