Bug 27028 – [gtk] Pasteboard/GtkClipboard can't handle the "text/html" target.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jiahua Huang]

when I select some html content, and Copy them use Ctrl-C,
the Pasteboard/GtkClipboard can't handle the "text/html" target.

I found that the "text/html" target 
only in X clipboard (GDK_SELECTION_CLIPBOARD),
but not gtk Desktop clipboard (GDK_SELECTION_PRIMARY).

So we can't use Ctrl-C to Copy rich text from WebkitGtk.

[Jiahua Huang]

Created attachment 32376 [details]
Fix improper set of enum WebKitWebViewTargetInfo

I use this Change:
> Index: WebCore/platform/gtk/PasteboardGtk.cpp
> ===================================================================
> --- WebCore/platform/gtk/PasteboardGtk.cpp	(revision 45546)
> +++ WebCore/platform/gtk/PasteboardGtk.cpp	(working copy)
> @@ -37,8 +38,8 @@ namespace WebCore {
>  /* FIXME: we must get rid of this and use the enum in webkitwebview.h someway */
>  typedef enum
>  {
> -    WEBKIT_WEB_VIEW_TARGET_INFO_HTML = - 1,
> -    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT = - 2
> +    WEBKIT_WEB_VIEW_TARGET_INFO_HTML,
> +    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT
>  } WebKitWebViewTargetInfo;
> 

it works.
it can use Ctrl-C to Copy rich text from WebkitGtk page now.

[Jiahua Huang]

(In reply to comment #1)
> it works.

a manual-test 
WebCore/manual-tests/gtk/copy-htmltext.html has been added.

[Jan Alonzo]

(In reply to comment #1)
> Created an attachment (id=32376) [details]
> Fix improper set of enum WebKitWebViewTargetInfo
> 
> I use this Change:
> > Index: WebCore/platform/gtk/PasteboardGtk.cpp
> > ===================================================================
> > --- WebCore/platform/gtk/PasteboardGtk.cpp	(revision 45546)
> > +++ WebCore/platform/gtk/PasteboardGtk.cpp	(working copy)
> > @@ -37,8 +38,8 @@ namespace WebCore {
> >  /* FIXME: we must get rid of this and use the enum in webkitwebview.h someway */
> >  typedef enum
> >  {
> > -    WEBKIT_WEB_VIEW_TARGET_INFO_HTML = - 1,
> > -    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT = - 2
> > +    WEBKIT_WEB_VIEW_TARGET_INFO_HTML,
> > +    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT
> >  } WebKitWebViewTargetInfo;

Maybe we can remove this altogether by grabbing the target infos via m_helper?

[Jiahua Huang]

(In reply to comment #3)
> Maybe we can remove this altogether by grabbing the target infos via m_helper?

How about it:
> Index: WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp
> ===================================================================
> --- WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp	(revision 45595)
> +++ WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp	(working copy)
> @@ -57,8 +57,10 @@ GtkClipboard* PasteboardHelperGtk::getPr
>  
>  GtkTargetList* PasteboardHelperGtk::getCopyTargetList(Frame* frame) const
>  {
> -    WebKitWebView* webView = webkit_web_frame_get_web_view(kit(frame));
> -    return webkit_web_view_get_copy_target_list(webView);
> +    GtkTargetList* target_list = gtk_target_list_new(NULL, 0);
> +    gtk_target_list_add(target_list, gdk_atom_intern_static_string("text/html"), 0, WEBKIT_WEB_VIEW_TARGET_INFO_HTML);
> +    gtk_target_list_add_text_targets(target_list, WEBKIT_WEB_VIEW_TARGET_INFO_TEXT);    
> +    return target_list;
>  }
>  
>  GtkTargetList* PasteboardHelperGtk::getPasteTargetList(Frame* frame) const

[Jiahua Huang]

(In reply to comment #4)
> How about it:

err, no, that is it:

> Index: WebCore/platform/gtk/PasteboardGtk.cpp
> ===================================================================
> --- WebCore/platform/gtk/PasteboardGtk.cpp	(revision 45595)
> +++ WebCore/platform/gtk/PasteboardGtk.cpp	(working copy)
> @@ -108,7 +108,10 @@ void Pasteboard::writeSelection(Range* s
>      PasteboardSelectionData* data = new PasteboardSelectionData(text, markup);
>  
>      gint n_targets;
> -    GtkTargetEntry* targets = gtk_target_table_new_from_list(m_helper->getCopyTargetList(frame), &n_targets);
> +    GtkTargetList* target_list = gtk_target_list_new(NULL, 0);
> +    gtk_target_list_add(target_list, gdk_atom_intern_static_string("text/html"), 0, WEBKIT_WEB_VIEW_TARGET_INFO_HTML);
> +    gtk_target_list_add_text_targets(target_list, WEBKIT_WEB_VIEW_TARGET_INFO_TEXT); 
> +    GtkTargetEntry* targets = gtk_target_table_new_from_list(target_list, &n_targets);      
>      gtk_clipboard_set_with_data(clipboard, targets, n_targets,
>                                  clipboard_get_contents_cb, clipboard_clear_contents_cb, data);
>      gtk_target_table_free(targets, n_targets);

[Jan Alonzo]

(In reply to comment #5)
> (In reply to comment #4)
> > How about it:
> 
> err, no, that is it:
> 
> > -    GtkTargetEntry* targets = gtk_target_table_new_from_list(m_helper->getCopyTargetList(frame), &n_targets);
> > +    GtkTargetList* target_list = gtk_target_list_new(NULL, 0);
> > +    gtk_target_list_add(target_list, gdk_atom_intern_static_string("text/html"), 0, WEBKIT_WEB_VIEW_TARGET_INFO_HTML);
> > +    gtk_target_list_add_text_targets(target_list, WEBKIT_WEB_VIEW_TARGET_INFO_TEXT); 
> > +    GtkTargetEntry* targets = gtk_target_table_new_from_list(target_list, &n_targets);      
> >      gtk_clipboard_set_with_data(clipboard, targets, n_targets,
> >                                  clipboard_get_contents_cb, clipboard_clear_contents_cb, data);
> >      gtk_target_table_free(targets, n_targets);

I think a proper fix for this is to add a function in WebKit/gtk/WebCoreSuppoer/PasteboardHelperGtk.cpp that will return the value of WEBKIT_WEB_VIEW_TARGET_INFO_HTML, and use that in clipboard_get_contents_cb.

[Jiahua Huang]

(In reply to comment #6)
> I think a proper fix for this is to add a function in
> WebKit/gtk/WebCoreSuppoer/PasteboardHelperGtk.cpp that will return the value of
> WEBKIT_WEB_VIEW_TARGET_INFO_HTML, and use that in clipboard_get_contents_cb.

well, I see.

I'm trying to do it like that.

[Jiahua Huang]

(In reply to comment #6)
> I think a proper fix for this is to add a function in
> WebKit/gtk/WebCoreSuppoer/PasteboardHelperGtk.cpp that will return the value of
> WEBKIT_WEB_VIEW_TARGET_INFO_HTML, and use that in clipboard_get_contents_cb.


I agree.

How about it:
> Index: WebCore/platform/gtk/PasteboardGtk.cpp
> ===================================================================
> --- WebCore/platform/gtk/PasteboardGtk.cpp	(revision 45658)
> +++ WebCore/platform/gtk/PasteboardGtk.cpp	(working copy)
> @@ -35,13 +35,6 @@
>  
>  namespace WebCore {
>  
> -/* FIXME: we must get rid of this and use the enum in webkitwebview.h someway */
> -typedef enum
> -{
> -    WEBKIT_WEB_VIEW_TARGET_INFO_HTML = - 1,
> -    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT = - 2
> -} WebKitWebViewTargetInfo;
> -
>  class PasteboardSelectionData {
>  public:
>      PasteboardSelectionData(gchar* text, gchar* markup)
> @@ -65,7 +58,7 @@ static void clipboard_get_contents_cb(Gt
>                                        guint info, gpointer data) {
>      PasteboardSelectionData* clipboardData = reinterpret_cast<PasteboardSelectionData*>(data);
>      ASSERT(clipboardData);
> -    if ((gint)info == WEBKIT_WEB_VIEW_TARGET_INFO_HTML) {
> +    if ((gint)info == m_helper->PasteboardHelperGtk()) {
>          gtk_selection_data_set(selection_data, selection_data->target, 8,
>                                 reinterpret_cast<const guchar*>(clipboardData->markup()),
>                                 g_utf8_strlen(clipboardData->markup(), -1));
> Index: WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp
> ===================================================================
> --- WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp	(revision 45658)
> +++ WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp	(working copy)
> @@ -67,4 +67,14 @@ GtkTargetList* PasteboardHelperGtk::getP
>      return webkit_web_view_get_paste_target_list(webView);
>  }
>  
> +gint PasteboardHelperGtk::getWebViewTargetInfoHtml() const
> +{
> +    return WEBKIT_WEB_VIEW_TARGET_INFO_HTML;
> +}
> +
> +gint PasteboardHelperGtk::getWebViewTargetInfoText() const
> +{
> +    return WEBKIT_WEB_VIEW_TARGET_INFO_TEXT;
> +}
> +
>  }


I'm waiting for the rebuild process to complete.

[Jiahua Huang]

Created attachment 32558 [details]
grabbing the WEBKIT_WEB_VIEW_TARGET_INFO_HTML via m_helper

(In reply to comment #6)
> I think a proper fix for this is to add a function in
> WebKit/gtk/WebCoreSuppoer/PasteboardHelperGtk.cpp that will return the value of
> WEBKIT_WEB_VIEW_TARGET_INFO_HTML, and use that in clipboard_get_contents_cb.

Done,
it works =)

[Jiahua Huang]

Created attachment 32559 [details]
update patch to svn r45707

[Jiahua Huang]

Created attachment 32560 [details]
cleanup.  update patch to svn r45707

[Jan Alonzo]

Comment on attachment 32560 [details]
cleanup.  update patch to svn r45707

> Index: WebCore/platform/Pasteboard.h
> ===================================================================
> --- WebCore/platform/Pasteboard.h	(revision 45707)
> +++ WebCore/platform/Pasteboard.h	(working copy)
> @@ -102,6 +102,7 @@ public:
>  
>  #if PLATFORM(GTK)
>      void setHelper(PasteboardHelper*);
> +    PasteboardHelper* m_helper;
>  #endif

Is there any reason for this change?

> -    if ((gint)info == WEBKIT_WEB_VIEW_TARGET_INFO_HTML) {
> +    if ((gint)info == Pasteboard::generalPasteboard()->m_helper->getWebViewTargetInfoHtml()) {

I think calling generalPasteboard() is unnecessary here.

> +    virtual gint getWebViewTargetInfoHtml() const = 0;

What's this for?

Looks fine otherwise. r- for now as the patch needs to be revised.

[Jiahua Huang]

(In reply to comment #12)
> Looks fine otherwise. r- for now as the patch needs to be revised.

use the enum in webkitwebview.h

> Index: GNUmakefile.am
> ===================================================================
> --- GNUmakefile.am	(revision 45707)
> +++ GNUmakefile.am	(working copy)
> @@ -193,6 +193,9 @@ libWebCore_la_SOURCES = \
>  	$(webcore_sources) \
>  	$(webcoregtk_sources)
>  
> +webcore_cppflags += \
> +	-I$(srcdir)/WebKit/gtk/
> +
>  libWebCore_la_CXXFLAGS = \
>  	$(global_cxxflags) \
>  	$(corekit_cflags)
> Index: WebCore/platform/gtk/PasteboardGtk.cpp
> ===================================================================
> --- WebCore/platform/gtk/PasteboardGtk.cpp	(revision 45707)
> +++ WebCore/platform/gtk/PasteboardGtk.cpp	(working copy)
> @@ -30,18 +30,12 @@
>  #include "RenderImage.h"
>  #include "KURL.h"
>  #include "markup.h"
> +#include "webkit/webkitwebview.h"
>  
>  #include <gtk/gtk.h>
>  
>  namespace WebCore {
>  
> -/* FIXME: we must get rid of this and use the enum in webkitwebview.h someway */
> -typedef enum
> -{
> -    WEBKIT_WEB_VIEW_TARGET_INFO_HTML = - 1,
> -    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT = - 2
> -} WebKitWebViewTargetInfo;
> -
>  class PasteboardSelectionData {
>  public:
>      PasteboardSelectionData(gchar* text, gchar* markup)

[Jiahua Huang]

Created attachment 33043 [details]
use the enum in webkitwebview.h

(In reply to comment #12)
> Looks fine otherwise. r- for now as the patch needs to be revised.

Remove the improper set of enum WebKitWebViewTargetInfo
in platform/gtk/PasteboardGtk.cpp,
and use the enum in webkitwebview.h

[Jan Alonzo]

Comment on attachment 33043 [details]
use the enum in webkitwebview.h

> +webcore_cppflags += \
> +	-I$(srcdir)/WebKit/gtk/
> +

This is a layering violation. WebCore should not depend on WebKit. It should go through the *Client interface, or in the case of Pasteboard, should go through PasteboardHelper. The previous patch was already doing that. Why the change?

r- because of the layering violation.

[Jiahua Huang]

(In reply to comment #15)

Dear Alonzo ,
  I'm sorry, I'm so busy these days.
  Cound you mend it for me, please?

  Thank you very much.

Best regards.

[Jan Alonzo]

Comment on attachment 32560 [details]
cleanup.  update patch to svn r45707

> -    if ((gint)info == WEBKIT_WEB_VIEW_TARGET_INFO_HTML) {
> +    if ((gint)info == Pasteboard::generalPasteboard()->m_helper->getWebViewTargetInfoHtml()) {
>          gtk_selection_data_set(selection_data, selection_data->target, 8,
>                                 reinterpret_cast<const guchar*>(clipboardData->markup()),
>                                 g_utf8_strlen(clipboardData->markup(), -1));

The braces should be removed as there's only one statement here.

I've looked into this again and seems I overlooked a few things in my review. This patch is fine. r=me.

[Jan Alonzo]

Landed as http://trac.webkit.org/changeset/47310

[Jiahua Huang]

(In reply to comment #18)

Hi Alonzo,
  Many thanks for your review and Landing~

Best regards.