Bug 68235 – [GTK][WK2] Initial implementation of WebInspector

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Ravi Phaneendra Kasibhatla]

First cut implementation of WebInspector for WebKit2 GTK port. 
Further enhancements:
- Docking inspector window with main window

[Ravi Phaneendra Kasibhatla]

Created attachment 107661 [details]
WebKit2 GTK changes for WebInspector

[Ravi Phaneendra Kasibhatla]

Created attachment 107662 [details]
Minor changes in MiniBrowser for enabling WebInspector in WebKit2 GTK

[Adam Roben (:aroben)]

Comment on attachment 107662 [details]
Minor changes in MiniBrowser for enabling WebInspector in WebKit2 GTK

View in context: https://bugs.webkit.org/attachment.cgi?id=107662&action=review

> Tools/MiniBrowser/gtk/main.c:37
> +    const gchar* webBundlePath = g_build_filename(bundleEnvPath ? bundleEnvPath : LIBEXECDIR, "libMiniBrowserWebBundle.so", NULL);

Do we need to free this?

[Martin Robinson]

Comment on attachment 107661 [details]
WebKit2 GTK changes for WebInspector

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.

[Martin Robinson]

(In reply to comment #3)
> (From update of attachment 107662 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107662&action=review
> 
> > Tools/MiniBrowser/gtk/main.c:37
> > +    const gchar* webBundlePath = g_build_filename(bundleEnvPath ? bundleEnvPath : LIBEXECDIR, "libMiniBrowserWebBundle.so", NULL);
> 
> Do we need to free this?

Yes, this is definitely a leak.

[Carlos Garcia Campos]

Comment on attachment 107661 [details]
WebKit2 GTK changes for WebInspector

View in context: https://bugs.webkit.org/attachment.cgi?id=107661&action=review

> Source/WebKit2/UIProcess/API/C/gtk/WKView.cpp:50
> +void WKViewSetDeveloperExtrasEnabled(WKPageRef pageRef, bool enable)
> +{
> +    toImpl(pageRef)->setDeveloperExtrasEnabled(enable);
> +}

I don't think we need specific API for this, we should use the existing preferences API, WKPreferencesSetDeveloperExtrasEnabled

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:79
> +    m_inspectorView = GTK_WIDGET(webkitWebViewBaseCreate(page()->process()->context(), inspectorPageGroup()));

You can use the C API here instead of the private view base API, m_inspectorView = GTK_WIDGET(WKViewCreate(toAPI(page()->process()->context()), toAPI(inspectorPageGroup()));

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:80
> +    gtk_widget_set_size_request(m_inspectorView, initialWindowWidth, initialWindowHeight);

Do you need to enforce the view size to the initial window size?

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:82
> +    return webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(m_inspectorView));

WKViewGetPage()

>> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:95
>> +    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.

Please, don't use show_all(), show the view widget and the window.

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

You don't need to destroy the view, just destroy the window and the view will be destroyed too.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:118
> +    gtk_window_set_title(GTK_WINDOW(m_inspectorWindow), title.latin1().data());

gtk_window_set_title() expects utf8 not latin1

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:129
> +    GtkWidget* inspectorWindow = m_page->viewWidget();
> +    return static_cast<unsigned>(gtk_widget_get_allocated_height(inspectorWindow));

Why do you need to get the view from the page? you have m_inspectorWindow

[Ravi Phaneendra Kasibhatla]

Created attachment 107830 [details]
Initial implementation of WebInspector for GTK - revision 1

Address comments.

[Ravi Phaneendra Kasibhatla]

Created attachment 107831 [details]
App changes for WebInspector implementation - revision 1

Address comments.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #3)
> (From update of attachment 107662 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107662&action=review
> 
> > Tools/MiniBrowser/gtk/main.c:37
> > +    const gchar* webBundlePath = g_build_filename(bundleEnvPath ? bundleEnvPath : LIBEXECDIR, "libMiniBrowserWebBundle.so", NULL);
> 
> Do we need to free this?

Yes. It was missing. Corrected in the new patch.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #4)
> (From update of attachment 107661 [details])
> 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.
Done.
> 
> > 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?
Done.
> 
> > 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.
As we discussed, the embedded view patch will be coming in the next patch.
> 
> > 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.
Done as per our discussion. Using GLIB functions for localization.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:123
> > +    return makeString("file://", inspectorFilesPath(), "/inspector.html");
> 
> This needs to be done in a platform independent way.
Done.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #6)
> (From update of attachment 107661 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107661&action=review
> 
> > Source/WebKit2/UIProcess/API/C/gtk/WKView.cpp:50
> > +void WKViewSetDeveloperExtrasEnabled(WKPageRef pageRef, bool enable)
> > +{
> > +    toImpl(pageRef)->setDeveloperExtrasEnabled(enable);
> > +}
> 
> I don't think we need specific API for this, we should use the existing preferences API, WKPreferencesSetDeveloperExtrasEnabled
Thanks for pointing. Done now.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:79
> > +    m_inspectorView = GTK_WIDGET(webkitWebViewBaseCreate(page()->process()->context(), inspectorPageGroup()));
> 
> You can use the C API here instead of the private view base API, m_inspectorView = GTK_WIDGET(WKViewCreate(toAPI(page()->process()->context()), toAPI(inspectorPageGroup()));
Using C API will cause unnecessary back & forth between the exposed opaque structure and actual implementation structure. Current implementation is rather simple. If the concern is about using WebKitWebViewBasePrivate.h, we should move some basic WebView related functions like creation, getting PageRef of a view etc to WebKitWebViewBase.h from WebKitWebViewBasePrivate.h.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:80
> > +    gtk_widget_set_size_request(m_inspectorView, initialWindowWidth, initialWindowHeight);
> 
> Do you need to enforce the view size to the initial window size?
All ports have actually defined that as starting size, so I followed the tradition. :)
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:82
> > +    return webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(m_inspectorView));
> 
> WKViewGetPage()
Please refer to above comment.
> 
> >> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:95
> >> +    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.
> 
> Please, don't use show_all(), show the view widget and the window.
Done.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:103
> > +    if (m_inspectorView) {
> > +        gtk_widget_destroy(m_inspectorView);
> > +        m_inspectorView = 0;
> > +    }
> 
> You don't need to destroy the view, just destroy the window and the view will be destroyed too.
Done.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:118
> > +    gtk_window_set_title(GTK_WINDOW(m_inspectorWindow), title.latin1().data());
> 
> gtk_window_set_title() expects utf8 not latin1
Done.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:129
> > +    GtkWidget* inspectorWindow = m_page->viewWidget();
> > +    return static_cast<unsigned>(gtk_widget_get_allocated_height(inspectorWindow));
> 
> Why do you need to get the view from the page? you have m_inspectorWindow
Name was misleading and it should have been inspectedView. Corrected that now.

[Carlos Garcia Campos]

(In reply to comment #11)

> > > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:79
> > > +    m_inspectorView = GTK_WIDGET(webkitWebViewBaseCreate(page()->process()->context(), inspectorPageGroup()));
> > 
> > You can use the C API here instead of the private view base API, m_inspectorView = GTK_WIDGET(WKViewCreate(toAPI(page()->process()->context()), toAPI(inspectorPageGroup()));
> Using C API will cause unnecessary back & forth between the exposed opaque structure and actual implementation structure. Current implementation is rather simple. If the concern is about using WebKitWebViewBasePrivate.h, we should move some basic WebView related functions like creation, getting PageRef of a view etc to WebKitWebViewBase.h from WebKitWebViewBasePrivate.h.


No, they are private on purpose. In most of the cases toAPI/toImpl are just casts.

> > 
> > > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:80
> > > +    gtk_widget_set_size_request(m_inspectorView, initialWindowWidth, initialWindowHeight);
> > 
> > Do you need to enforce the view size to the initial window size?
> All ports have actually defined that as starting size, so I followed the tradition. :)

That's not the starting size, but the minimum size of the widget, which means you won't be able to make the window smaller.

> > > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:129
> > > +    GtkWidget* inspectorWindow = m_page->viewWidget();
> > > +    return static_cast<unsigned>(gtk_widget_get_allocated_height(inspectorWindow));
> > 
> > Why do you need to get the view from the page? you have m_inspectorWindow
> Name was misleading and it should have been inspectedView. Corrected that now.

Why not use m_inspectorView then?

[Ravi Phaneendra Kasibhatla]

Created attachment 107856 [details]
Initial implementation of WebInspector for GTK - revision 2

Address comments from carlos.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #12)
> (In reply to comment #11)
> 
> > > > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:79
> > > > +    m_inspectorView = GTK_WIDGET(webkitWebViewBaseCreate(page()->process()->context(), inspectorPageGroup()));
> > > 
> > > You can use the C API here instead of the private view base API, m_inspectorView = GTK_WIDGET(WKViewCreate(toAPI(page()->process()->context()), toAPI(inspectorPageGroup()));
> > Using C API will cause unnecessary back & forth between the exposed opaque structure and actual implementation structure. Current implementation is rather simple. If the concern is about using WebKitWebViewBasePrivate.h, we should move some basic WebView related functions like creation, getting PageRef of a view etc to WebKitWebViewBase.h from WebKitWebViewBasePrivate.h.
> 
> 
> No, they are private on purpose. In most of the cases toAPI/toImpl are just casts.
> 
> > > 
> > > > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:80
> > > > +    gtk_widget_set_size_request(m_inspectorView, initialWindowWidth, initialWindowHeight);
> > > 
> > > Do you need to enforce the view size to the initial window size?
> > All ports have actually defined that as starting size, so I followed the tradition. :)
> 
> That's not the starting size, but the minimum size of the widget, which means you won't be able to make the window smaller.
Corrected it. Now not setting any size separately for the view, but setting the default size & minimum size for the window itself.
> 
> > > > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:129
> > > > +    GtkWidget* inspectorWindow = m_page->viewWidget();
> > > > +    return static_cast<unsigned>(gtk_widget_get_allocated_height(inspectorWindow));
> > > 
> > > Why do you need to get the view from the page? you have m_inspectorWindow
> > Name was misleading and it should have been inspectedView. Corrected that now.
> 
> Why not use m_inspectorView then?
The height is for the webview to which the inspector view will be docked. This is not required with the current implementation of new window. I have removed it to clear the confusion and will be adding with the implementation of the docking patch.

[Philippe Normand]

Comment on attachment 107831 [details]
App changes for WebInspector implementation - revision 1

View in context: https://bugs.webkit.org/attachment.cgi?id=107831&action=review

> Tools/MiniBrowser/gtk/main.c:-36
> -    WKStringRef bundlePath = WKStringCreateWithUTF8CString("Libraries/.libs/libMiniBrowserWebBundle.so");

That change looks unrelated, altough it totally make sense I think it should be split out from this patch.

[Philippe Normand]

Comment on attachment 107856 [details]
Initial implementation of WebInspector for GTK - revision 2

View in context: https://bugs.webkit.org/attachment.cgi?id=107856&action=review

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:90
> +    gtk_window_set_title(GTK_WINDOW(m_inspectorWindow), "Web Inspector GTK");

missing l10n call wrapping the string.

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:118
> +    GOwnPtr<gchar> title(g_strconcat("Web Inspector GTK - ", url.utf8().data(), NULL));

Ditto

[Martin Robinson]

Comment on attachment 107856 [details]
Initial implementation of WebInspector for GTK - revision 2

View in context: https://bugs.webkit.org/attachment.cgi?id=107856&action=review

>> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:90
>> +    gtk_window_set_title(GTK_WINDOW(m_inspectorWindow), "Web Inspector GTK");
> 
> missing l10n call wrapping the string.

Bike shed moment: Probably should just title this  "Web Inspector" Users don't really care what toolkit they're using as long as it deliver to them the kittens.

>> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:118
>> +    GOwnPtr<gchar> title(g_strconcat("Web Inspector GTK - ", url.utf8().data(), NULL));
> 
> Ditto

Ditto.

[Gustavo Noronha (kov)]

Comment on attachment 107856 [details]
Initial implementation of WebInspector for GTK - revision 2

View in context: https://bugs.webkit.org/attachment.cgi?id=107856&action=review

>>> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:90
>>> +    ASSERT(!m_inspectorWindow);
>>> +    m_inspectorWindow = gtk_window_new(GTK_WINDOW_TOPLEVEL);
>>> +
>>> +    gtk_window_set_title(GTK_WINDOW(m_inspectorWindow), "Web Inspector GTK");
>> 
>> missing l10n call wrapping the string.
> 
> Bike shed moment: Probably should just title this  "Web Inspector" Users don't really care what toolkit they're using as long as it deliver to them the kittens.

hmm; do we really want to have WebKit itself create the window? What if the user agent has specific requirements for the window, wants to add more chrome to it, wants to position it, wants it to be a popup window? I feel like this makes the API less flexible without making it much more convenient. Why not use the same approach we use in wk1?

[Martin Robinson]

(In reply to comment #18)

> hmm; do we really want to have WebKit itself create the window? What if the user agent has specific requirements for the window, wants to add more chrome to it, wants to position it, wants it to be a popup window? I feel like this makes the API less flexible without making it much more convenient. Why not use the same approach we use in wk1?

One thing we can do is to make this the default behavior. Later we can add signals and this will transition to be the default signal handler. Thoughts?

[Ravi Phaneendra Kasibhatla]

(In reply to comment #15)
> (From update of attachment 107831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107831&action=review
> 
> > Tools/MiniBrowser/gtk/main.c:-36
> > -    WKStringRef bundlePath = WKStringCreateWithUTF8CString("Libraries/.libs/libMiniBrowserWebBundle.so");
> 
> That change looks unrelated, altough it totally make sense I think it should be split out from this patch.
Done. Moved the change bug https://bugs.webkit.org/show_bug.cgi?id=68553

[Gustavo Noronha (kov)]

(In reply to comment #19)
> One thing we can do is to make this the default behavior. Later we can add signals and this will transition to be the default signal handler. Thoughts?

That works for me!

[Ravi Phaneendra Kasibhatla]

Created attachment 108317 [details]
Initial implementation of WebInspector for GTK - revision 3

Addressing all review comments.

[Ravi Phaneendra Kasibhatla]

Created attachment 108318 [details]
App changes for WebInspector implementation - revision 2

Addressing review comments.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #16)
> (From update of attachment 107856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107856&action=review
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:90
> > +    gtk_window_set_title(GTK_WINDOW(m_inspectorWindow), "Web Inspector GTK");
> 
> missing l10n call wrapping the string.
Done.
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:118
> > +    GOwnPtr<gchar> title(g_strconcat("Web Inspector GTK - ", url.utf8().data(), NULL));
> 
> Ditto
Done.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #17)
> (From update of attachment 107856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107856&action=review
> 
> >> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:90
> >> +    gtk_window_set_title(GTK_WINDOW(m_inspectorWindow), "Web Inspector GTK");
> > 
> > missing l10n call wrapping the string.
> 
> Bike shed moment: Probably should just title this  "Web Inspector" Users don't really care what toolkit they're using as long as it deliver to them the kittens.
Done.
> 
> >> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:118
> >> +    GOwnPtr<gchar> title(g_strconcat("Web Inspector GTK - ", url.utf8().data(), NULL));
> > 
> > Ditto
> 
> Ditto.
Done.

[Martin Robinson]

Comment on attachment 108318 [details]
App changes for WebInspector implementation - revision 2

View in context: https://bugs.webkit.org/attachment.cgi?id=108318&action=review

> Tools/MiniBrowser/gtk/BrowserWindow.c:193
> +    if (g_getenv("WEBKIT_ENABLE_WEB_INSPECTOR")) {
> +        WKPageGroupRef groupRef = WKPageGetPageGroup(WKViewGetPage(window->webView));
> +        WKPreferencesRef preferencesRef = WKPageGroupGetPreferences(groupRef);
> +        WKPreferencesSetDeveloperExtrasEnabled(preferencesRef, true);
> +    }

Why not just enable it always?

[Martin Robinson]

Comment on attachment 108317 [details]
Initial implementation of WebInspector for GTK - revision 3

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?

[Ravi Phaneendra Kasibhatla]

Created attachment 108375 [details]
Initial implementation of WebInspector for GTK - revision 4

Addressed comments.

[Ravi Phaneendra Kasibhatla]

Created attachment 108376 [details]
App changes for WebInspector implementation - revision 3

Addressed comments.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #26)
> (From update of attachment 108318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108318&action=review
> 
> > Tools/MiniBrowser/gtk/BrowserWindow.c:193
> > +    if (g_getenv("WEBKIT_ENABLE_WEB_INSPECTOR")) {
> > +        WKPageGroupRef groupRef = WKPageGetPageGroup(WKViewGetPage(window->webView));
> > +        WKPreferencesRef preferencesRef = WKPageGroupGetPreferences(groupRef);
> > +        WKPreferencesSetDeveloperExtrasEnabled(preferencesRef, true);
> > +    }
> 
> Why not just enable it always?
Done.

[Ravi Phaneendra Kasibhatla]

(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.

[Martin Robinson]

Comment on attachment 108375 [details]
Initial implementation of WebInspector for GTK - revision 4

View in context: https://bugs.webkit.org/attachment.cgi?id=108375&action=review

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:116
> +    GOwnPtr<gchar> filePath(g_build_filename("file://", DATA_DIR, "webkitgtk-", WEBKITGTK_API_VERSION_STRING, "webinspector", "inspector.html", NULL));

You are still creating this path at runtime when I asked if you would make it a static string contructed at build time.

[Ravi Phaneendra Kasibhatla]

Created attachment 108466 [details]
Initial implementation of WebInspector for GTK - revision 5

As per discussion removed the changes in WebPreferencesStore.h. Ensured that shouldOpenAttached() returns false so we don't go into docked mode.
Also addressed 1 comment which I missed out in earlier patch.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #32)
> (From update of attachment 108375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108375&action=review
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:116
> > +    GOwnPtr<gchar> filePath(g_build_filename("file://", DATA_DIR, "webkitgtk-", WEBKITGTK_API_VERSION_STRING, "webinspector", "inspector.html", NULL));
> 
> You are still creating this path at runtime when I asked if you would make it a static string contructed at build time.
Sorry missed out in last patch. Done in latest patch.

[Ravi Phaneendra Kasibhatla]

martin,kov,philip:
Can you please review the reworked patches?

[Martin Robinson]

Comment on attachment 108466 [details]
Initial implementation of WebInspector for GTK - revision 5

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)

[Ravi Phaneendra Kasibhatla]

Created attachment 109148 [details]
Initial implementation of WebInspector for WebKit2 GTK - revision 6

Address last set of comments from Martin. Also rebased to trunk HEAD.

[Ravi Phaneendra Kasibhatla]

Created attachment 109150 [details]
App changes for WebInspector implementation - revision 4

Rebased to the trunk HEAD

[Ravi Phaneendra Kasibhatla]

(In reply to comment #37)
> Created an attachment (id=109148) [details]
> Initial implementation of WebInspector for WebKit2 GTK - revision 6
> 
> Address last set of comments from Martin. Also rebased to trunk HEAD.
In the new patch, apart from addressing the last comments, there is one more change:
- Implementation of the WebInspectorProxy::inspectorBaseURL() which was introduced in trunk HEAD. 
- This returns the base location from where inspector resources are read.
- The function is important because in WebInspectorProxy::createInspectorPage(), it calls for inspectorBaseURL() and stores the folder location path into a map of WebProcessProxy with status as "allow read access" (assumeReadAccessToBaseURL()).
- Failure to doing so fails to load the inspector and crashes in debug builds since WebProcess are allowed to read file system for files.

[Martin Robinson]

Comment on attachment 109148 [details]
Initial implementation of WebInspector for WebKit2 GTK - revision 6

Looks good to me.  I think this is a good default behavior for the library to have.

[Martin Robinson]

r- because gtk_widget_set_size_request seems to have creeped back into your patch. :)

[Carlos Garcia Campos]

Comment on attachment 109148 [details]
Initial implementation of WebInspector for WebKit2 GTK - revision 6

View in context: https://bugs.webkit.org/attachment.cgi?id=109148&action=review

> Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:92
> +    gtk_widget_set_size_request(m_inspectorWindow, minimumWindowWidth, minimumWindowHeight);

You adeed this again, is it really needed? does it allow you to make the window smaller? or is it on purpose?

[WebKit Review Bot]

Comment on attachment 109150 [details]
App changes for WebInspector implementation - revision 4

Clearing flags on attachment: 109150

Committed r96334: <http://trac.webkit.org/changeset/96334>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ravi Phaneendra Kasibhatla]

Created attachment 109170 [details]
Initial implementation of WebInspector for WebKit2 GTK - revision 7

Oops sorry. Wrong merge while rebasing the patch to the HEAD. gtk_widget_set_size_request is not required. :)

[Ravi Phaneendra Kasibhatla]

(In reply to comment #42)
> (From update of attachment 109148 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109148&action=review
> 
> > Source/WebKit2/UIProcess/gtk/WebInspectorGtk.cpp:92
> > +    gtk_widget_set_size_request(m_inspectorWindow, minimumWindowWidth, minimumWindowHeight);
> 
> You adeed this again, is it really needed? does it allow you to make the window smaller? or is it on purpose?

Nope it was wrong merge while rebasing the patch to HEAD. Thanks for catching it. :)

[Martin Robinson]

The bug commit queue is not landing the last patch. I'll reopen the bug and try flipping the cq bit. Please try to use one patch per bug in the future as that keeps the tools happy.

[WebKit Review Bot]

Comment on attachment 109170 [details]
Initial implementation of WebInspector for WebKit2 GTK - revision 7

Clearing flags on attachment: 109170

Committed r96383: <http://trac.webkit.org/changeset/96383>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.