[webkit-reviews] review denied: [Bug 68235] [GTK][WK2] Initial implementation of WebInspector : [Attachment 108466] Initial implementation of WebInspector for GTK - revision 5

[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 108466: Initial implementation of WebInspector for GTK - revision 5
https://bugs.webkit.org/attachment.cgi?id=108466&action=review

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


> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:50
> +    // Inform WebProcess that inspector has been closed. 
> +    // Otherwise, further invocation of inspector fails to invoke
::platformOpen().

Please even out these comment lines.  The line break is in weird place.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:104
> +    GOwnPtr<gchar> title(g_strdup_printf(_("Web Inspector - %s"),
url.utf8().data()));

If I'm not mistaken this needs to be:

GOwnPtr<gchar> title(g_strdup_printf("%s - %s",  _("Web Inspector"),
url.utf8().data()));

I could be wrong though.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:110
> +    // Hardcoding protocol separator to :// is not wrong since protocol
separator is same for all the platforms.

I think you can drop this comment completely.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:116
> +    static const char* inspectorFilePath =
"file://"DATA_DIR""G_DIR_SEPARATOR_S"webkitgtk-"WEBKITGTK_API_VERSION_STRING""G
_DIR_SEPARATOR_S"webinspector"G_DIR_SEPARATOR_S"inspector.html";

Please insert a bit of space here. I'm imaginging something like

static const char* inspectorFilePath = "file://"DATA_DIR" "G_DIR_SEPARATOR_S
"webkitgtk-"
					      WEBKITGTK_API_VERSION_STRING
G_DIR_SEPARATOR_S
					      "webinspector" G_DIR_SEPARATOR_S
"inspector.html";

(except aligned properly)