WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
161907
[GTK] Get rid of DataObjectGtk::forClipboard and cleanup pasteboard code
https://bugs.webkit.org/show_bug.cgi?id=161907
Summary
[GTK] Get rid of DataObjectGtk::forClipboard and cleanup pasteboard code
Carlos Garcia Campos
Reported
2016-09-13 05:10:07 PDT
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.
Attachments
Patch
(32.55 KB, patch)
2016-09-13 05:19 PDT
,
Carlos Garcia Campos
mcatanzaro
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1
2016-09-13 05:19:52 PDT
Created
attachment 288687
[details]
Patch
Michael Catanzaro
Comment 2
2016-09-13 09:04:32 PDT
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
Comment 3
2016-09-13 09:10:33 PDT
(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
Comment 4
2016-09-13 09:16:00 PDT
Committed
r205860
: <
http://trac.webkit.org/changeset/205860
>
Carlos Garcia Campos
Comment 5
2016-09-20 06:38:52 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug