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

[bugzilla-daemon at webkit.org]

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





--- Comment #31 from Ravi Phaneendra Kasibhatla <ravi.kasibhatla at motorola.com>  2011-09-22 12:03:40 PST ---
(In reply to comment #27)
> (From update of attachment 108317 [details])
> 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.
I still feel this is the best way to avoid sending docking url for loading in current implementation. We can revert this change, once we add docking support in coming patches. As per discussion, I feel other ways look lot more hacky. Let me know if you think otherwise.
> 
> > Source/WebKit2/UIProcess/WebInspectorProxy.h:60
> > +#elif PLATFORM(GTK)
> > +typedef struct _GtkWidget GtkWidget;
> >  #endif
> 
> Are you sure this is needed?
Removed.
> 
> > 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.
Done.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:52
> > +    // Inform UIProcess side that window has been destroyed for cleanup.
> 
> No need for this comment.
Removed.
> 
> > 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.
Removed the function. Doing the path creation inside the function inspectorPageURL itself.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:91
> > +    // Set the default width & height of Inspector window.
> 
> There's no need for this comment.
Removed.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:93
> > +    // Set the minimum width & height of Inspector window.
> 
> Ditto.
Removed.
> 
> > 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?
Corrected.

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