[webkit-reviews] review denied: [Bug 30623] [GTK] Enable DOM clipboard and drag-and-drop access : [Attachment 44975] Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY (rev3)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 29 11:01:09 PST 2009


Gustavo Noronha (kov) <gns at gnome.org> has denied Martin Robinson
<martin.james.robinson at gmail.com>'s request for review:
Bug 30623: [GTK] Enable DOM clipboard and drag-and-drop access
https://bugs.webkit.org/show_bug.cgi?id=30623

Attachment 44975: Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY (rev3)
https://bugs.webkit.org/attachment.cgi?id=44975&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
 10	    No new tests. (OOPS!)

Aren't there pasteboard tests we could enable? It looks like in this case we
are not really changing the current behavior at all, right?

 28 DataObjectGtk::DataObjectGtk()
 29 {
 30 
 31 }

Empty line.

 94 /*static*/
 95 DataObjectGtk* DataObjectGtk::forClipboard(GtkClipboard* clipboard)

Why the commented out static here?

 85	if (info == WEBKIT_WEB_VIEW_TARGET_INFO_TEXT)
 86	    gtk_selection_data_set_text(selectionData,
dataObject->text().utf8().data(), -1);
 87 
 88	else if (info == WEBKIT_WEB_VIEW_TARGET_INFO_HTML) {

This empty line makes the if/else if blocks relationship confusing =).

 152 
 153	 } else

Unnecessary empty line.

There are also a couple complaints by the style bot. The overall change in code
looks pretty good to me, though. I'll r- just so you can fix these nits.


More information about the webkit-reviews mailing list