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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76998





--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-02-02 09:30:40 PST ---
(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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.