|
Description
Ravi Phaneendra Kasibhatla
2011-09-16 02:24:48 PDT
Created attachment 107661 [details]
WebKit2 GTK changes for WebInspector
Created attachment 107662 [details]
Minor changes in MiniBrowser for enabling WebInspector in WebKit2 GTK
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 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. (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 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 Created attachment 107830 [details]
Initial implementation of WebInspector for GTK - revision 1
Address comments.
Created attachment 107831 [details]
App changes for WebInspector implementation - revision 1
Address comments.
(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. (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. (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. (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? Created attachment 107856 [details]
Initial implementation of WebInspector for GTK - revision 2
Address comments from carlos.
(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 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 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 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 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? (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? (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 (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! Created attachment 108317 [details]
Initial implementation of WebInspector for GTK - revision 3
Addressing all review comments.
Created attachment 108318 [details]
App changes for WebInspector implementation - revision 2
Addressing review comments.
(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. (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 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 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? Created attachment 108375 [details]
Initial implementation of WebInspector for GTK - revision 4
Addressed comments.
Created attachment 108376 [details]
App changes for WebInspector implementation - revision 3
Addressed comments.
(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. (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 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. 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.
(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. martin,kov,philip: Can you please review the reworked patches? 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) 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.
Created attachment 109150 [details]
App changes for WebInspector implementation - revision 4
Rebased to the trunk HEAD
(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 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.
r- because gtk_widget_set_size_request seems to have creeped back into your patch. :) 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 on attachment 109150 [details] App changes for WebInspector implementation - revision 4 Clearing flags on attachment: 109150 Committed r96334: <http://trac.webkit.org/changeset/96334> All reviewed patches have been landed. Closing bug. 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. :)
(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. :) 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 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> All reviewed patches have been landed. Closing bug. |