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

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

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


> Source/WebKit2/Shared/WebPreferencesStore.h:51
> +#define DEFAULT_WEBKIT_INSPECTOR_STARTS_ATTACHED false
>  #else
>  #define DEFAULT_WEBKIT_TABSTOLINKS_ENABLED false
> +#define DEFAULT_WEBKIT_INSPECTOR_STARTS_ATTACHED true
>  #endif

Let's keep this option the same for now. We want it to match in the future.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:60
> +#elif PLATFORM(GTK)
> +typedef struct _GtkWidget GtkWidget;
>  #endif

Are you sure this is needed?

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:49
> +    // Inform WebProcess that inspector has been closed. Otherwise, further
invocation

Please but a newline before this comment so that it's easier to read.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:52
> +    // Inform UIProcess side that window has been destroyed for cleanup.

No need for this comment.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:67
> +static String inspectorFilePath(const char* protocol, const char*
inspectorFile)
> +{
> +    const gchar* environmentPath = g_getenv("WEBKIT_INSPECTOR_PATH");
> +    if (environmentPath && g_file_test(environmentPath, G_FILE_TEST_IS_DIR))
{
> +	   GOwnPtr<gchar> filePath(g_build_filename(protocol, environmentPath,
inspectorFile, NULL));
> +	   return WebCore::filenameToString(filePath.get());
> +    }
> +    GOwnPtr<gchar> filePath(g_build_filename(protocol, DATA_DIR,
"webkitgtk-", WEBKITGTK_API_VERSION_STRING, "webinspector", inspectorFile,
NULL));
> +    return WebCore::filenameToString(filePath.get());
> +}

What's the reasoning for pulling this out of inspectorPageURL? It's only used
there. Since all the bits of GOwnPtr<gchar> filePath(g_build_filename(protocol,
DATA_DIR, "webkitgtk-", WEBKITGTK_API_VERSION_STRING, "webinspector",
inspectorFile, NULL)); are known at compile time you don't have to calculate
the value at runtime. I think that's preferable.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:91
> +    // Set the default width & height of Inspector window.

There's no need for this comment.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:93
> +    // Set the minimum width & height of Inspector window.

Ditto.

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

Is that really the correct location of the l10n underscore?