Bug 161907 – [GTK] Get rid of DataObjectGtk::forClipboard and cleanup pasteboard code

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

[Carlos Garcia Campos]

We don't really need to keep a DataObjectGtk for every clipboard, we could simply pass the DataObjectGtk to read and write methods of PasteboardHelper.

[Carlos Garcia Campos]

Created attachment 288687 [details]
Patch

[Michael Catanzaro]

Comment on attachment 288687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288687&action=review

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:99
> -DataObjectGtk* Pasteboard::dataObject() const
> +const DataObjectGtk& Pasteboard::dataObject() const
>  {
> -    return m_dataObject.get();
> +    return *m_dataObject;
>  }

You should also change m_dataObject to be a (non-const) reference, since it's always initialized in the constructor initializer lists and there are also ASSERTs to ensure it's not null. Then you can get rid of the ASSERTS, and don't have to dereference it throughout the file anymore.

> Source/WebCore/platform/gtk/PasteboardHelper.cpp:311
> +            // When gtk_clipboard_set_with_data fails the callbacks are ignored, so we need to leak the data we were passing to clearClipboardContentsCallback.
> +            data.release();

This confused me because I assumed this was on the failure path, because your comment starts with "when it fails," but in fact this is the success case. I see that you need to leak it only on success, so the code is right, just the comment is confusing.

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp:69
> +    DragData dragData(const_cast<DataObjectGtk*>(&dataObject), clientPosition, globalPosition, dataTransfer.sourceOperation());

Wouldn't it be better to provide a non-const accessor as well, to avoid the const_cast here?

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 288687 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288687&action=review
> 
> > Source/WebCore/platform/gtk/PasteboardGtk.cpp:99
> > -DataObjectGtk* Pasteboard::dataObject() const
> > +const DataObjectGtk& Pasteboard::dataObject() const
> >  {
> > -    return m_dataObject.get();
> > +    return *m_dataObject;
> >  }
> 
> You should also change m_dataObject to be a (non-const) reference, since
> it's always initialized in the constructor initializer lists and there are
> also ASSERTs to ensure it's not null. Then you can get rid of the ASSERTS,
> and don't have to dereference it throughout the file anymore.

Yes, I tried that, but it's not that easy because of the way DragData works, I plan to do another cleanup after the clipboard is moved to the UI process, to not delay more that patch.

> > Source/WebCore/platform/gtk/PasteboardHelper.cpp:311
> > +            // When gtk_clipboard_set_with_data fails the callbacks are ignored, so we need to leak the data we were passing to clearClipboardContentsCallback.
> > +            data.release();
> 
> This confused me because I assumed this was on the failure path, because
> your comment starts with "when it fails," but in fact this is the success
> case. I see that you need to leak it only on success, so the code is right,
> just the comment is confusing.

Agree, I modified the existing comment that was in the failure path, and it's not clear enough now, I'll rephrase it.

> > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp:69
> > +    DragData dragData(const_cast<DataObjectGtk*>(&dataObject), clientPosition, globalPosition, dataTransfer.sourceOperation());
> 
> Wouldn't it be better to provide a non-const accessor as well, to avoid the
> const_cast here?

This is temporal, I'll get rid of all const_casts in the second cleanup.

[Carlos Garcia Campos]

Committed r205860: <http://trac.webkit.org/changeset/205860>

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 288687 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=288687&action=review
> > 
> > > Source/WebCore/platform/gtk/PasteboardGtk.cpp:99
> > > -DataObjectGtk* Pasteboard::dataObject() const
> > > +const DataObjectGtk& Pasteboard::dataObject() const
> > >  {
> > > -    return m_dataObject.get();
> > > +    return *m_dataObject;
> > >  }
> > 
> > You should also change m_dataObject to be a (non-const) reference, since
> > it's always initialized in the constructor initializer lists and there are
> > also ASSERTs to ensure it's not null. Then you can get rid of the ASSERTS,
> > and don't have to dereference it throughout the file anymore.
> 
> Yes, I tried that, but it's not that easy because of the way DragData works,
> I plan to do another cleanup after the clipboard is moved to the UI process,
> to not delay more that patch.
> 

The cleanup
https://bugs.webkit.org/show_bug.cgi?id=162267

The next step is renaming DataObjectGtk to something like SelectionData.