| Summary: | [GTK] Initial UI client implementation for WebKit2 GTK +API | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||
| Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | gustavo, mrobinson, pnormand, webkit.review.bot, xan.lopez | ||||||||||||
| Priority: | P2 | Keywords: | Gtk | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 71362, 71447 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Carlos Garcia Campos
2011-10-10 04:14:39 PDT
Created attachment 110348 [details]
Patch
Attachment 110348 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:247: Use 0 instead of NULL. [readability/null] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:266: Use 0 instead of NULL. [readability/null] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:284: Use 0 instead of NULL. [readability/null] [5]
Total errors found: 3 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 110349 [details]
Patch updated to fix coding style issues
Comment on attachment 110349 [details] Patch updated to fix coding style issues 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 believe that the ::ready signal should be called ::show to match the underlying API. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:66 > +static GtkWidget* webkitWebViewCreate(WebKitWebView*) > +{ > + return 0; > +} This seems to be dead code? > 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. > 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. > 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,. > 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. > 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. > 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 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. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:280 > + signals[CLOSE] = > + g_signal_new("close", Ditto. (In reply to comment #4) > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:243 > > + signals[CREATE] = > > + g_signal_new("create", > > This can be one line. I'm implementing the policy client now and I think I agree with you that this is better as two lines. Please disregard this comment! (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. (In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 110349 [details] [details]) > 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. The WebKitWebView is the actual widget, but the UIClient controls everything surrounding the WebView. I think it makes sense to split them into two things. For instance, setMenuBarIsVisible has no relation to the WebView widget at all. There are some delegates that I associate with the widget itself such as focus and unfocus -- maybe we can consider having the UIClient just fire signals on the WebView. What do you think? Do you mind expanding a bit on how the existence of a page client would change things? > > > I believe that the ::ready signal should be called ::show to match the underlying API. > > I find "show" very confusing. Do you mind elaborating a bit on this? > > > 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. Ah, I see it now! Thanks. > > > 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? If I did, I apologize. I noticed that the style bot was complaining about them recently. (In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > (From update of attachment 110349 [details] [details] [details]) > > > 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. > > The WebKitWebView is the actual widget, but the UIClient controls everything surrounding the WebView. I think it makes sense to split them into two things. For instance, setMenuBarIsVisible has no relation to the WebView widget at all. There are some delegates that I associate with the widget itself such as focus and unfocus -- maybe we can consider having the UIClient just fire signals on the WebView. What do you think? If we are talking about internal code separation, it's fine with me, what I don't see is a WebKitWebUIClient object like the loader client or the policy client. > Do you mind expanding a bit on how the existence of a page client would change things? The web view would be the UI client, while the page object would do the other things the view currently does. > > > > > I believe that the ::ready signal should be called ::show to match the underlying API. > > > > I find "show" very confusing. > > Do you mind elaborating a bit on this? Show makes me think you have to show the view or something, while you don't have to. The first time I saw it I thought it was called everytime you have to show the view for some reason. But it's called only once, to notify that the new view is *ready*. (In reply to comment #8) > > The WebKitWebView is the actual widget, but the UIClient controls everything surrounding the WebView. I think it makes sense to split them into two things. For instance, setMenuBarIsVisible has no relation to the WebView widget at all. There are some delegates that I associate with the widget itself such as focus and unfocus -- maybe we can consider having the UIClient just fire signals on the WebView. What do you think? > > If we are talking about internal code separation, it's fine with me, what I don't see is a WebKitWebUIClient object like the loader client or the policy client. Still, it seems odd for the widget to have signals that are about controlling and getting information about its parent window. I think the more conservative aproach is to make UIClient and then later go back and move signals to the WebView if they make sense there. Another approach is to just move some signals there now. The three that you have make sense as WebView signals for instance. > > Do you mind expanding a bit on how the existence of a page client would change things? > The web view would be the UI client, while the page object would do the other things the view currently does. I think of the WebView as the widget that contains the page and the UIClient as controlling the things around it. > Show makes me think you have to show the view or something, while you don't have to. The first time I saw it I thought it was called everytime you have to show the view for some reason. But it's called only once, to notify that the new view is *ready*. My undertanding of this API is that the embedder should not actually show the WebView until the show calback has been called. I don't know if the "ready" captures that. Would it be clearer to you as "ready-to-show" ? Created attachment 111801 [details]
New patch
Move UI client implementation to its own class (private for the moment). Renamed ready signal as ready-to-show as suggested by martin.
Created attachment 113038 [details]
Patch updated to current git master
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Created attachment 113313 [details] Updated patch to fix unit tests Tests for this patch were broken since r98845, because now the web view has default settings applied and javascript-can-open-windows-automatically is FALSE by default. Comment on attachment 113313 [details] Updated patch to fix unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=113313&action=review Looks good to me. I have a couple documentation nits below as well as a request to use skip the soup server, if possible. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:163 > +static gboolean webkitWebViewAccumulatorObjectHandled(GSignalInvocationHint*, GValue* returnValue, const GValue* handlerReturn, gpointer) > +{ > + gpointer object = g_value_get_object(handlerReturn); > + if (object) > + g_value_set_object(returnValue, object); > + > + return !object; I prefer void* to gpointer. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:236 > + * 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. I find this a bit confusing still. From what I understand, the user will add the widget to a container. The container will handle the reference count. The user is still responsible for adding the widget to the container. They can do this at any time during the process of create -> ready-to-show -> close, right? I think the documentation should just explain that the user should add the WebKitWebView to a container as usual. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:258 > + * size, position, whether the location, status and scrollbars > + * should be displayed, is already set. Later we should come back and explain where it's set. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:276 > + * delete the #WebKitWebView, if necessary. delete -> destroy > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:99 > + static const char* htmlOnLoadFormat = "<html><body onLoad=\"%s\"></body></html>"; > + > + if (message->method != SOUP_METHOD_GET) { > + soup_message_set_status(message, SOUP_STATUS_NOT_IMPLEMENTED); > + return; > + } > + > + soup_message_set_status(message, SOUP_STATUS_OK); > + > + if (g_str_equal(path, "/create_new_view")) { > + GOwnPtr<char> windowOpenString(g_strdup_printf("window.open('%s')", kServer->getURIForPath("/new_window").data())); > + char* htmlString = g_strdup_printf(htmlOnLoadFormat, windowOpenString.get()); > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, htmlString, strlen(htmlString)); > + } else if (g_str_equal(path, "/new_window")) { > + char* htmlString = g_strdup_printf(htmlOnLoadFormat, "self.close();"); > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, htmlString, strlen(htmlString)); > + } Can you load a string now instead of using Soup here? (In reply to comment #14) > (From update of attachment 113313 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113313&action=review > > Looks good to me. I have a couple documentation nits below as well as a request to use skip the soup server, if possible. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:163 > > +static gboolean webkitWebViewAccumulatorObjectHandled(GSignalInvocationHint*, GValue* returnValue, const GValue* handlerReturn, gpointer) > > +{ > > + gpointer object = g_value_get_object(handlerReturn); > > + if (object) > > + g_value_set_object(returnValue, object); > > + > > + return !object; > > I prefer void* to gpointer. Ok > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:236 > > + * 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. > > I find this a bit confusing still. From what I understand, the user will add the widget to a container. The container will handle the reference count. The user is still responsible for adding the widget to the container. They can do this at any time during the process of create -> ready-to-show -> close, right? I think the documentation should just explain that the user should add the WebKitWebView to a container as usual. I agree, the fact that we are adding that comment makes me think that it's a special situation, while it isn't. It's like any other widget creation, the widget contains a floating reference, so I think we should just remove the comment. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:258 > > + * size, position, whether the location, status and scrollbars > > + * should be displayed, is already set. > > Later we should come back and explain where it's set. Yes. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:276 > > + * delete the #WebKitWebView, if necessary. > > delete -> destroy Ok. > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:99 > > + static const char* htmlOnLoadFormat = "<html><body onLoad=\"%s\"></body></html>"; > > + > > + if (message->method != SOUP_METHOD_GET) { > > + soup_message_set_status(message, SOUP_STATUS_NOT_IMPLEMENTED); > > + return; > > + } > > + > > + soup_message_set_status(message, SOUP_STATUS_OK); > > + > > + if (g_str_equal(path, "/create_new_view")) { > > + GOwnPtr<char> windowOpenString(g_strdup_printf("window.open('%s')", kServer->getURIForPath("/new_window").data())); > > + char* htmlString = g_strdup_printf(htmlOnLoadFormat, windowOpenString.get()); > > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, htmlString, strlen(htmlString)); > > + } else if (g_str_equal(path, "/new_window")) { > > + char* htmlString = g_strdup_printf(htmlOnLoadFormat, "self.close();"); > > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, htmlString, strlen(htmlString)); > > + } > > Can you load a string now instead of using Soup here? Yes, I'll rework the tests to use html. Committed r102673: <http://trac.webkit.org/changeset/102673> |