Bug 76998 - [GTK] Add cut, copy and paste methods to WebKit2 GTK+ API
: [GTK] Add cut, copy and paste methods to WebKit2 GTK+ API
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Gtk
:
:
  Show dependency treegraph
 
Reported: 2012-01-25 02:54 PST by
Modified: 2012-02-07 10:57 PST (History)


Attachments
Patch (22.07 KB, patch)
2012-01-25 02:59 PST, Carlos Garcia Campos
no flags Review Patch | Details | Formatted Diff | Diff
New patch (25.34 KB, patch)
2012-02-02 04:13 PST, Carlos Garcia Campos
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch according to review comments (31.63 KB, patch)
2012-02-07 02:03 PST, Carlos Garcia Campos
mrobinson: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-25 02:54:15 PST
And API required to query whether it's possible to cut, copy and paste, too.
------- Comment #1 From 2012-01-25 02:59:57 PST -------
Created an attachment (id=123908) [details]
Patch
------- Comment #2 From 2012-01-25 03:03:28 PST -------
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 #3 From 2012-01-25 08:10:07 PST -------
(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
------- Comment #4 From 2012-01-25 08:19:51 PST -------
(In reply to comment #3)
> (From update of attachment 123908 [details] [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! :-)
------- Comment #5 From 2012-01-25 08:28:54 PST -------
(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
------- Comment #6 From 2012-02-02 04:13:55 PST -------
Created an attachment (id=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 #7 From 2012-02-02 09:08:03 PST -------
(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?

> 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?
------- Comment #8 From 2012-02-02 09:30:40 PST -------
(In reply to comment #7)
> (From update of attachment 125107 [details] [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
------- Comment #9 From 2012-02-07 01:05:41 PST -------
(In reply to comment #7)
> (From update of attachment 125107 [details] [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
------- Comment #10 From 2012-02-07 02:03:38 PST -------
Created an attachment (id=125789) [details]
Updated patch according to review comments

Fixed all review comments
------- Comment #11 From 2012-02-07 10:57:29 PST -------
Committed r106961: <http://trac.webkit.org/changeset/106961>