[webkit-reviews] review denied: [Bug 68235] [GTK][WK2] Initial implementation of WebInspector : [Attachment 107661] WebKit2 GTK changes for WebInspector

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Ravi Phaneendra Kasibhatla
<ravi.kasibhatla at motorola.com>'s request for review:
Bug 68235: [GTK][WK2] Initial implementation of WebInspector
https://bugs.webkit.org/show_bug.cgi?id=68235

Attachment 107661: WebKit2 GTK changes for WebInspector
https://bugs.webkit.org/attachment.cgi?id=107661&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107661&action=review


> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:63
> +    // FIXME:: DATA_DIR maps to /usr/local/share by default.
> +    // Installed path from apt-get install maps to /usr/share.
> +    // Should this gap be addressed or it will be invalid usecase to
consider?

DATA_DIR should depend on the prefix supplied during configuration.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:64
> +    return makeString(DATA_DIR, "/webkitgtk-", WEBKITGTK_API_VERSION_STRING,
"/webinspector");

You should use the functions GLib supplies for building file names. This will
break on Windows. Does makeString properly convert from the file system locale?


> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:95
> +    gtk_window_set_title(GTK_WINDOW(m_inspectorWindow), "Web Inspector
GTK");
> +    gtk_window_set_default_size(GTK_WINDOW(m_inspectorWindow),
initialWindowWidth, initialWindowHeight);
> +    g_signal_connect(m_inspectorWindow, "delete-event",
G_CALLBACK(inspectorWindowDestroyed), this);
> +
> +    gtk_container_add(GTK_CONTAINER(m_inspectorWindow), m_inspectorView);
> +    gtk_widget_show_all(m_inspectorWindow);

This doesn't seem to handle the embedded view? Almost certainly this should be
passed up to the API-layer somehow.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:107
> +	   gtk_widget_destroy(m_inspectorView);
> +	   m_inspectorView = 0;
> +    }
> +    if (m_inspectorWindow) {
> +	   gtk_widget_destroy(m_inspectorWindow);
> +	   m_inspectorWindow = 0;
> +    }

Ditto.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:117
> +    String title = makeString("Web Inspector GTK - ", url);

This needs to be localized probably. Maybe it should be handled by the client
completely.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:123
> +    return makeString("file://", inspectorFilesPath(), "/inspector.html");

This needs to be done in a platform independent way.