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

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Ravi Phaneendra Kasibhatla <ravi.kasibhatla at motorola.com>  2011-09-19 04:24:40 PST ---
(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.

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