[Webkit-unassigned] [Bug 69753] [GTK] Initial UI client implementation for WebKit2 GTK +API
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 11 00:09:48 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=69753
--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com> 2011-10-11 00:09:48 PST ---
(In reply to comment #4)
> (From update of attachment 110349 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110349&action=review
>
> I'd prefer us to separate these into a Client class. Is there a reason it's impossible? I'm not opposed to having these three signals on the WebView, but they can be fired via an exposed private API from the WebView if you don't want to use g_signal_emit_by_name.
I think web view is actually the ui client, that's why I haven't used a separate client for this. I you want the ui client separated from the page, we should probably add a page class like Gustavo suggested.
> I believe that the ::ready signal should be called ::show to match the underlying API.
I find "show" very confusing.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:66
> > +static GtkWidget* webkitWebViewCreate(WebKitWebView*)
> > +{
> > + return 0;
> > +}
>
> This seems to be dead code?
No, it's the default implementation, it's only called when the signal is not handled.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:76
> > +static WKPageRef createNewPage(WKPageRef page, WKURLRequestRef request, WKDictionaryRef features, WKEventModifiers modifiers, WKEventMouseButton button, const void* clientInfo)
> > +{
> > + WebKitWebView* newWebView;
> > + g_signal_emit(WEBKIT_WEB_VIEW(clientInfo), signals[CREATE], 0, &newWebView);
> > + if (!newWebView)
> > + return 0;
> > +
> > + return static_cast<WKPageRef>(WKRetain(toAPI(webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(newWebView)))));
> > +}
>
> It's important to send all those arguments along. A hash table of features is probably fine here, but if you like you can create a WebKitWebViewFeatures object ala the WK1 API. This is one thing that the WK1 port didn't do quite right.
I haven't added ViewFeatures in this patch on purpose, to keep it as small as possible, I though you were going to ask me to split it if I added the view features part.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:86
> > +static void showPage(WKPageRef page, const void *clientInfo)
> > +{
> > + g_signal_emit(WEBKIT_WEB_VIEW(clientInfo), signals[READY], 0, NULL);
> > +}
> > +
> > +static void closePage(WKPageRef page, const void *clientInfo)
> > +{
> > + g_signal_emit(WEBKIT_WEB_VIEW(clientInfo), signals[CLOSE], 0, NULL);
> > +}
>
> The asterisks on clientInfo are in the wrong place.
Ok.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:108
> > + webView,/* clientInfo */
> > + 0, /* createNewPage_deprecatedForUseWithV0 */
> > + showPage,
> > + closePage,
> > + 0, /* takeFocus */
> > + 0, /* focus */
> > + 0, /* unfocus */
> > + 0, /* runJavaScriptAlert */
> > + 0, /* runJavaScriptConfirm */
> > + 0, /* runJavaScriptPrompt */
> > + 0, /* setStatusText */
> > + 0, /* mouseDidMoveOverElement_deprecatedForUseWithV0 */
> > + 0, /* missingPluginButtonClicked */
> > + 0, /* didNotHandleKeyEvent */
> > + 0, /* didNotHandleWheelEvent */
> > + 0, /* toolbarsAreVisible */
> > + 0, /* setToolbarsAreVisible */
>
> You should use C++ style comments here and they should be one space away from the 0,.
Didn't you ask me to line this up before?
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:136
> > + WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> > + WKPageSetPageUIClient(toAPI(page), &uiClient);
>
> This can be one line.
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:148
> > priv->loaderClient = adoptGRef(WEBKIT_WEB_LOADER_CLIENT(g_object_new(WEBKIT_TYPE_WEB_LOADER_CLIENT, "web-view", webView, NULL)));
> > +
> > + webkitWebViewUIClientInit(webView);
>
> Again, I think a WebKitUIClient would be a lot cleaner here.
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:229
> > + * Emitted when the creation of a new view is requested.
>
> We should probably say #WebKitWebView here instead of "view" as it's less ambiguous.
Ok.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:239
> > + * The signal handlers should not try to deal with the reference count for
> > + * the new #WebKitWebView. The widget to which the widget is added will
> > + * handle that.
> > + *
>
> Should the user wait until ::ready to add the widget to a parent widget? If so you should probably mention that explicitly.
No, you can add the view to a parent in the create handler.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:243
> > + signals[CREATE] =
> > + g_signal_new("create",
>
> This can be one line.
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:259
> > + * Emitted after #WebKitWebView::create on the newly created #WebKitWebView
> > + * when it should be displayed to the user. When this signal is emitted
> > + * all the information about how the window should look, including
> > + * size, position, whether the location, status and scroll bars
> > + * should be displayed, is already set.
>
> scroll bars -> scrollbars
Ok.
> I'm not sure I understand exactly how you mean the information is already set. All that information should be passed as signal arguments to ::create.
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:262
> > + signals[READY] =
> > + g_signal_new("ready",
>
> This can be one line.
>
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:277
> > + * delete the web view, if necessary.
>
> web view -> #WebKitWebView.
Ok.
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:280
> > + signals[CLOSE] =
> > + g_signal_new("close",
>
> Ditto.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list