And API required to query whether it's possible to cut, copy and paste, too.
Created attachment 123908 [details] Patch
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
Comment on attachment 123908 [details] Patch Perhaps it's better to add the more generic command API. This is necessary to use WebKit in things like email clients. I'm imagining an API like: webkit_web_view_validate_editing_command webkit_web_view_execute_editing_command
(In reply to comment #3) > (From update of attachment 123908 [details]) > Perhaps it's better to add the more generic command API. This is necessary to use WebKit in things like email clients. I'm imagining an API like: > > webkit_web_view_validate_editing_command > webkit_web_view_execute_editing_command And defines for command names WEBKIT_COMMAND_CUT, WEBKIT_COMMAND_COPY, etc. I like the idea! :-)
(In reply to comment #4) > And defines for command names WEBKIT_COMMAND_CUT, WEBKIT_COMMAND_COPY, etc. > I like the idea! :-) That's nice too. Here's the DOM API this calls: http://www.quirksmode.org/dom/execCommand.html
Created attachment 125107 [details] New patch New patch adding a generic API for execute and validate editing commands as suggested by Martin. This adds support for any editing command, but for now it only includes predefined names for Cut, Copy and Paste commands.
Comment on attachment 125107 [details] New patch View in context: https://bugs.webkit.org/attachment.cgi?id=125107&action=review Looks great. I have just a few minor nits. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1296 > + reinterpret_cast<gpointer>(webkit_web_view_can_execute_editing_command)); I'm surprised that a function pointer needs to be cast to a gpointer. What sort of error do you get here when you omit this cast? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:52 > +#define WEBKIT_TYPE_WEB_VIEW (webkit_web_view_get_type()) > +#define WEBKIT_WEB_VIEW(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEB_VIEW, WebKitWebView)) > +#define WEBKIT_WEB_VIEW_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST((klass), WEBKIT_TYPE_WEB_VIEW, WebKitWebViewClass)) > +#define WEBKIT_IS_WEB_VIEW(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj), WEBKIT_TYPE_WEB_VIEW)) > +#define WEBKIT_IS_WEB_VIEW_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass), WEBKIT_TYPE_WEB_VIEW)) > +#define WEBKIT_WEB_VIEW_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS((obj), WEBKIT_TYPE_WEB_VIEW, WebKitWebViewClass)) > + > +typedef struct _WebKitWebView WebKitWebView; > +typedef struct _WebKitWebViewClass WebKitWebViewClass; > +typedef struct _WebKitWebViewPrivate WebKitWebViewPrivate; > + This change seems unrelated? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:119 > +#define WEBKIT_EDITING_COMMAND_CUT "Cut" We may end up adding a lot of commands here in the future, because WebCore supports a lot of them. Having these values aligned is going to be incredibly tedious to maintain. This is because there is documentation between them, so you cannot use your editor's block select to update them all. Do you mind just using one space here to avoid this situation? I think it will really decrease the time spend adjusting spacing in this file. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebViewEditor.cpp:69 > + g_timeout_add_seconds(1, reinterpret_cast<GSourceFunc>(waitForClipboardText), this); One second is quite a long long time. Why not query every 50 milliseconds? It might be good to make the time between waiting and the amount of time to wait before timing out constants in the class or the file. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebViewEditor.cpp:100 > + g_signal_connect(test->m_webView, "load-changed", G_CALLBACK(webViewLoadChanged), test); > + test->loadHtml(selectedSpanHTML, 0); > + g_main_loop_run(test->m_mainLoop); I think I'd rather see you have the fixture extend loading the fixture or suck the logic for waiting until the load is done into the WebViewTest. This is a fairly common operation in tests and we shouldn't need to write it over and over again. I think this is not necessarily the correct time though. In WebKit1 we would wait until window-object-cleared, because that means the DOM is ready. Is there a similar moment in WebKit2?
(In reply to comment #7) > (From update of attachment 125107 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125107&action=review > > Looks great. I have just a few minor nits. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1296 > > + reinterpret_cast<gpointer>(webkit_web_view_can_execute_editing_command)); > > I'm surprised that a function pointer needs to be cast to a gpointer. What sort of error do you get here when you omit this cast? I don't remember, I'll check it again, but I don't usually use a C++ cast if it's not needed :-P > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:52 > > +#define WEBKIT_TYPE_WEB_VIEW (webkit_web_view_get_type()) > > +#define WEBKIT_WEB_VIEW(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEB_VIEW, WebKitWebView)) > > +#define WEBKIT_WEB_VIEW_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST((klass), WEBKIT_TYPE_WEB_VIEW, WebKitWebViewClass)) > > +#define WEBKIT_IS_WEB_VIEW(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj), WEBKIT_TYPE_WEB_VIEW)) > > +#define WEBKIT_IS_WEB_VIEW_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass), WEBKIT_TYPE_WEB_VIEW)) > > +#define WEBKIT_WEB_VIEW_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS((obj), WEBKIT_TYPE_WEB_VIEW, WebKitWebViewClass)) > > + > > +typedef struct _WebKitWebView WebKitWebView; > > +typedef struct _WebKitWebViewClass WebKitWebViewClass; > > +typedef struct _WebKitWebViewPrivate WebKitWebViewPrivate; > > + > > This change seems unrelated? Yes, I was going to add editing command and I realized the standard macros block was between two enum declarations. This block is usually the first thing you expect in a GObject header, I can do it in a different commit if you want. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:119 > > +#define WEBKIT_EDITING_COMMAND_CUT "Cut" > > We may end up adding a lot of commands here in the future, because WebCore supports a lot of them. Having these values aligned is going to be incredibly tedious to maintain. This is because there is documentation between them, so you cannot use your editor's block select to update them all. Do you mind just using one space here to avoid this situation? I think it will really decrease the time spend adjusting spacing in this file. Sure, no problem. > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebViewEditor.cpp:69 > > + g_timeout_add_seconds(1, reinterpret_cast<GSourceFunc>(waitForClipboardText), this); > > One second is quite a long long time. Why not query every 50 milliseconds? Yes, I can try. > It might be good to make the time between waiting and the amount of time to wait before timing out constants in the class or the file. Ok. > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebViewEditor.cpp:100 > > + g_signal_connect(test->m_webView, "load-changed", G_CALLBACK(webViewLoadChanged), test); > > + test->loadHtml(selectedSpanHTML, 0); > > + g_main_loop_run(test->m_mainLoop); > > I think I'd rather see you have the fixture extend loading the fixture or suck the logic for waiting until the load is done into the WebViewTest. This is a fairly common operation in tests and we shouldn't need to write it over and over again. Agree. > I think this is not necessarily the correct time though. In WebKit1 we would wait until window-object-cleared, because that means the DOM is ready. Is there a similar moment in WebKit2? I have no idea :-P
(In reply to comment #7) > (From update of attachment 125107 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125107&action=review > > Looks great. I have just a few minor nits. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1296 > > + reinterpret_cast<gpointer>(webkit_web_view_can_execute_editing_command)); > > I'm surprised that a function pointer needs to be cast to a gpointer. What sort of error do you get here when you omit this cast? CXX Source/WebKit2/UIProcess/API/gtk/libwebkit2gtk_3_0_la-WebKitWebView.lo ../Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp: In function 'void webkit_web_view_can_execute_editing_command(WebKitWebView*, const char*, GAsyncReadyCallback, gpointer)': ../Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1296:142: error: invalid conversion from 'void (*)(WebKitWebView*, const char*, GAsyncReadyCallback, gpointer) {aka void (*)(_WebKitWebView*, const char*, void (*)(_GObject*, _GAsyncResult*, void*), void*)}' to 'gpointer {aka void*}' [-fpermissive] /home/cgarcia/gnome/include/glib-2.0/gio/gsimpleasyncresult.h:51:21: error: initializing argument 4 of 'GSimpleAsyncResult* g_simple_async_result_new(GObject*, GAsyncReadyCallback, gpointer, gpointer)' [-fpermissive] make[1]: *** [Source/WebKit2/UIProcess/API/gtk/libwebkit2gtk_3_0_la-WebKitWebView.lo] Error 1
Created attachment 125789 [details] Updated patch according to review comments Fixed all review comments
Committed r106961: <http://trac.webkit.org/changeset/106961>