[Webkit-unassigned] [Bug 68235] [GTK][WK2] Initial implementation of WebInspector

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68235


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #108317|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #27 from Martin Robinson <mrobinson at webkit.org>  2011-09-22 10:36:08 PST ---
(From update of attachment 108317)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.