Bug 68235 - [GTK][WK2] Initial implementation of WebInspector
Summary: [GTK][WK2] Initial implementation of WebInspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-16 02:24 PDT by Ravi Phaneendra Kasibhatla
Modified: 2011-09-29 19:29 PDT (History)
10 users (show)

See Also:


Attachments
WebKit2 GTK changes for WebInspector (11.38 KB, patch)
2011-09-16 09:18 PDT, Ravi Phaneendra Kasibhatla
mrobinson: review-
Details | Formatted Diff | Diff
Minor changes in MiniBrowser for enabling WebInspector in WebKit2 GTK (2.71 KB, patch)
2011-09-16 09:19 PDT, Ravi Phaneendra Kasibhatla
mrobinson: review-
Details | Formatted Diff | Diff
Initial implementation of WebInspector for GTK - revision 1 (9.02 KB, patch)
2011-09-19 04:09 PDT, Ravi Phaneendra Kasibhatla
no flags Details | Formatted Diff | Diff
App changes for WebInspector implementation - revision 1 (3.16 KB, patch)
2011-09-19 04:10 PDT, Ravi Phaneendra Kasibhatla
no flags Details | Formatted Diff | Diff
Initial implementation of WebInspector for GTK - revision 2 (8.82 KB, patch)
2011-09-19 08:13 PDT, Ravi Phaneendra Kasibhatla
no flags Details | Formatted Diff | Diff
Initial implementation of WebInspector for GTK - revision 3 (8.84 KB, patch)
2011-09-22 05:39 PDT, Ravi Phaneendra Kasibhatla
mrobinson: review-
Details | Formatted Diff | Diff
App changes for WebInspector implementation - revision 2 (1.42 KB, patch)
2011-09-22 05:40 PDT, Ravi Phaneendra Kasibhatla
mrobinson: review-
Details | Formatted Diff | Diff
Initial implementation of WebInspector for GTK - revision 4 (8.34 KB, patch)
2011-09-22 11:57 PDT, Ravi Phaneendra Kasibhatla
no flags Details | Formatted Diff | Diff
App changes for WebInspector implementation - revision 3 (1.37 KB, patch)
2011-09-22 11:58 PDT, Ravi Phaneendra Kasibhatla
no flags Details | Formatted Diff | Diff
Initial implementation of WebInspector for GTK - revision 5 (7.01 KB, patch)
2011-09-23 05:53 PDT, Ravi Phaneendra Kasibhatla
mrobinson: review-
Details | Formatted Diff | Diff
Initial implementation of WebInspector for WebKit2 GTK - revision 6 (7.86 KB, patch)
2011-09-29 04:21 PDT, Ravi Phaneendra Kasibhatla
mrobinson: review-
mrobinson: commit-queue+
Details | Formatted Diff | Diff
App changes for WebInspector implementation - revision 4 (1.31 KB, patch)
2011-09-29 04:21 PDT, Ravi Phaneendra Kasibhatla
no flags Details | Formatted Diff | Diff
Initial implementation of WebInspector for WebKit2 GTK - revision 7 (7.83 KB, patch)
2011-09-29 09:51 PDT, Ravi Phaneendra Kasibhatla
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ravi Phaneendra Kasibhatla 2011-09-16 02:24:48 PDT
First cut implementation of WebInspector for WebKit2 GTK port. 
Further enhancements:
- Docking inspector window with main window
Comment 1 Ravi Phaneendra Kasibhatla 2011-09-16 09:18:14 PDT
Created attachment 107661 [details]
WebKit2 GTK changes for WebInspector
Comment 2 Ravi Phaneendra Kasibhatla 2011-09-16 09:19:22 PDT
Created attachment 107662 [details]
Minor changes in MiniBrowser for enabling WebInspector in WebKit2 GTK
Comment 3 Adam Roben (:aroben) 2011-09-16 09:29:32 PDT
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?
Comment 4 Martin Robinson 2011-09-16 14:30:01 PDT
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.
Comment 5 Martin Robinson 2011-09-16 14:30:19 PDT
(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.
Comment 6 Carlos Garcia Campos 2011-09-18 23:57:50 PDT
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
Comment 7 Ravi Phaneendra Kasibhatla 2011-09-19 04:09:44 PDT
Created attachment 107830 [details]
Initial implementation of WebInspector for GTK - revision 1

Address comments.
Comment 8 Ravi Phaneendra Kasibhatla 2011-09-19 04:10:27 PDT
Created attachment 107831 [details]
App changes for WebInspector implementation - revision 1

Address comments.
Comment 9 Ravi Phaneendra Kasibhatla 2011-09-19 04:11:19 PDT
(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.
Comment 10 Ravi Phaneendra Kasibhatla 2011-09-19 04:12:39 PDT
(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.
Comment 11 Ravi Phaneendra Kasibhatla 2011-09-19 04:24:40 PDT
(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.
Comment 12 Carlos Garcia Campos 2011-09-19 04:45:22 PDT
(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?
Comment 13 Ravi Phaneendra Kasibhatla 2011-09-19 08:13:50 PDT
Created attachment 107856 [details]
Initial implementation of WebInspector for GTK - revision 2

Address comments from carlos.
Comment 14 Ravi Phaneendra Kasibhatla 2011-09-19 08:18:14 PDT
(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.
Comment 15 Philippe Normand 2011-09-21 08:31:36 PDT
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.
Comment 16 Philippe Normand 2011-09-21 08:43:38 PDT
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
Comment 17 Martin Robinson 2011-09-21 09:54:28 PDT
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.
Comment 18 Gustavo Noronha (kov) 2011-09-21 10:19:08 PDT
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?
Comment 19 Martin Robinson 2011-09-21 10:54:51 PDT
(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?
Comment 20 Ravi Phaneendra Kasibhatla 2011-09-21 23:36:23 PDT
(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
Comment 21 Gustavo Noronha (kov) 2011-09-22 04:17:36 PDT
(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!
Comment 22 Ravi Phaneendra Kasibhatla 2011-09-22 05:39:53 PDT
Created attachment 108317 [details]
Initial implementation of WebInspector for GTK - revision 3

Addressing all review comments.
Comment 23 Ravi Phaneendra Kasibhatla 2011-09-22 05:40:31 PDT
Created attachment 108318 [details]
App changes for WebInspector implementation - revision 2

Addressing review comments.
Comment 24 Ravi Phaneendra Kasibhatla 2011-09-22 05:42:48 PDT
(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.
Comment 25 Ravi Phaneendra Kasibhatla 2011-09-22 05:43:19 PDT
(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.
Comment 26 Martin Robinson 2011-09-22 10:10:55 PDT
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?
Comment 27 Martin Robinson 2011-09-22 10:36:08 PDT
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?
Comment 28 Ravi Phaneendra Kasibhatla 2011-09-22 11:57:30 PDT
Created attachment 108375 [details]
Initial implementation of WebInspector for GTK - revision 4

Addressed comments.
Comment 29 Ravi Phaneendra Kasibhatla 2011-09-22 11:58:19 PDT
Created attachment 108376 [details]
App changes for WebInspector implementation - revision 3

Addressed comments.
Comment 30 Ravi Phaneendra Kasibhatla 2011-09-22 11:59:32 PDT
(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.
Comment 31 Ravi Phaneendra Kasibhatla 2011-09-22 12:03:40 PDT
(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.
Comment 32 Martin Robinson 2011-09-22 12:05:38 PDT
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.
Comment 33 Ravi Phaneendra Kasibhatla 2011-09-23 05:53:08 PDT
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.
Comment 34 Ravi Phaneendra Kasibhatla 2011-09-23 05:55:36 PDT
(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.
Comment 35 Ravi Phaneendra Kasibhatla 2011-09-27 11:49:16 PDT
martin,kov,philip:
Can you please review the reworked patches?
Comment 36 Martin Robinson 2011-09-28 09:49:09 PDT
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)
Comment 37 Ravi Phaneendra Kasibhatla 2011-09-29 04:21:14 PDT
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.
Comment 38 Ravi Phaneendra Kasibhatla 2011-09-29 04:21:53 PDT
Created attachment 109150 [details]
App changes for WebInspector implementation - revision 4

Rebased to the trunk HEAD
Comment 39 Ravi Phaneendra Kasibhatla 2011-09-29 07:28:38 PDT
(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.
Comment 40 Martin Robinson 2011-09-29 08:25:30 PDT
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.
Comment 41 Martin Robinson 2011-09-29 08:32:35 PDT
r- because gtk_widget_set_size_request seems to have creeped back into your patch. :)
Comment 42 Carlos Garcia Campos 2011-09-29 08:33:41 PDT
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?
Comment 43 WebKit Review Bot 2011-09-29 09:31:55 PDT
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>
Comment 44 WebKit Review Bot 2011-09-29 09:32:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 45 Ravi Phaneendra Kasibhatla 2011-09-29 09:51:41 PDT
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. :)
Comment 46 Ravi Phaneendra Kasibhatla 2011-09-29 09:53:35 PDT
(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. :)
Comment 47 Martin Robinson 2011-09-29 19:17:59 PDT
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.
Comment 48 WebKit Review Bot 2011-09-29 19:29:21 PDT
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>
Comment 49 WebKit Review Bot 2011-09-29 19:29:28 PDT
All reviewed patches have been landed.  Closing bug.