Bug 76998 – [GTK] Add cut, copy and paste methods to WebKit2 GTK+ API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

And API required to query whether it's possible to cut, copy and paste, too.

[Carlos Garcia Campos]

Created attachment 123908 [details]
Patch

[WebKit Review Bot]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

(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! :-)

[Martin Robinson]

(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

[Carlos Garcia Campos]

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.

[Martin Robinson]

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?

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

Created attachment 125789 [details]
Updated patch according to review comments

Fixed all review comments

[Carlos Garcia Campos]

Committed r106961: <http://trac.webkit.org/changeset/106961>