[webkit-reviews] review denied: [Bug 19392] [Gtk] Enable WebInspector in the Gtk port : [Attachment 22364] proposed patch (forgot a commit on the other)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jul 26 16:21:43 PDT 2008


Christian Dywan <christian at imendio.com> has denied Gustavo Noronha Silva
<gns at gnome.org>'s request for review:
Bug 19392: [Gtk] Enable WebInspector in the Gtk port
https://bugs.webkit.org/show_bug.cgi?id=19392

Attachment 22364: proposed patch (forgot a commit on the other)
https://bugs.webkit.org/attachment.cgi?id=22364&action=edit

------- Additional Comments from Christian Dywan <christian at imendio.com>
>+    /**
>+     * WebKitWebInspector::destroy:
>+     * @web_inspector: the object on which the signal is emitted
>+     *
>+     * Emitted when the inspector is destroyed.
>+     */
>+    webkit_web_inspector_signals[DESTROY] = g_signal_new("destroy",
>+	      G_TYPE_FROM_CLASS(klass),
>+	      (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
>+	      0,
>+	      NULL,
>+	      NULL,
>+	      g_cclosure_marshal_VOID__VOID,
>+	      G_TYPE_NONE , 0);

GtkObject, and therefore every GtkWidget, has a "destroy" signal with that same
signature, that means "the object is disposed, with no way to stop that". But
this signal merely means "finished" and it's still possible to keep a reference
of the instance.
I suggest the signal should have a different name to avoid confusion about how
this signal works.

>+static void webkit_web_inspector_finalize(GObject* object)
>+{
>+    WebKitWebInspector* web_inspector = WEBKIT_WEB_INSPECTOR(object);
>+    WebKitWebInspectorPrivate* priv = web_inspector->priv;
>+
>+    if (priv->inspector_view)
>+	  g_object_unref(priv->inspector_view);
>+
>+    if (priv->inspected_uri)
>+	  g_free(priv->inspected_uri);

No need to check whether a string array is non-NULL, g_free does that
implicitly.

>+void webkit_web_inspector_set_web_view(WebKitWebInspector *web_inspector,
WebKitWebView *web_view)
>+{
>+    g_return_if_fail(WEBKIT_IS_WEB_INSPECTOR(web_inspector));
>+    g_return_if_fail(WEBKIT_IS_WEB_VIEW(web_view));
>+
>+    WebKitWebInspectorPrivate* priv = web_inspector->priv;
>+
>+    if (priv->inspector_view)
>+	  g_object_unref(priv->inspector_view);
>+    
>+    g_object_ref(web_view);
>+    priv->inspector_view = web_view;
>+}

No documentation at all here... Please add that.

And, would it be a good idea to allow _set_web_view with a NULL web_view to
unset the web view?

>+void webkit_web_inspector_set_inspected_uri(WebKitWebInspector*
web_inspector, const gchar* inspected_uri)
>+{
>+    g_return_if_fail(WEBKIT_IS_WEB_INSPECTOR(web_inspector));
>+
>+    WebKitWebInspectorPrivate* priv = web_inspector->priv;
>+
>+    if (priv->inspected_uri)
>+	  g_free(priv->inspected_uri);
>+    
>+    priv->inspected_uri = g_strdup(inspected_uri);
>+}

Again, no need for the if (priv->inspected_uri).

>+    /**
>+    * WebKitWebSettings:enable-developer-extras:
>+    *
>+    * Whether developer extensions should be enabled.
>+    *
>+    * Since: 1.0.2
>+    */

I suggest you should mention the Web Inspector expressly here. Otherwise
"special extensions" is rather meaningless to newcomers.

>@@ -1206,6 +1212,13 @@ static void
webkit_web_view_class_init(WebKitWebViewClass* webViewClass)
>							 
WEBKIT_TYPE_WEB_SETTINGS,
>							 
WEBKIT_PARAM_READWRITE));
> 
>+    g_object_class_install_property(objectClass, PROP_WEB_INSPECTOR,
>+				      g_param_spec_object("web-inspector",
>+							  "Web Inspector",
>+							  "The associated
WebKitWebInspector instance",
>+							 
WEBKIT_TYPE_WEB_INSPECTOR,
>+							 
WEBKIT_PARAM_READABLE));

This needs documentation, particularly a Since tag.

>+WebKitWebInspector* webkit_web_view_get_inspector(WebKitWebView* webView)
>+{
>+    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);
>+
>+    WebKitWebViewPrivate* priv = webView->priv;
>+    return priv->webInspector;
>+}

Documentation missing. Particularly it should be documented if this can be
NULL.


More information about the webkit-reviews mailing list