[webkit-reviews] review denied: [Bug 76998] [GTK] Add cut, copy and paste methods to WebKit2 GTK+ API : [Attachment 125107] New patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 2 09:08:02 PST 2012


Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 76998: [GTK] Add cut, copy and paste methods to WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=76998

Attachment 125107: New patch
https://bugs.webkit.org/attachment.cgi?id=125107&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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?


More information about the webkit-reviews mailing list