Bug 72056 – [chromium] Fix plumbing for differentiating between clipboard/selection pastes.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Daniel Cheng]

[chromium] Fix plumbing for differentiating between clipboard/selection pastes.

[Daniel Cheng]

Created attachment 114570 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 114570 [details]
Patch

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

> Source/WebKit/chromium/src/PlatformSupport.cpp:153
> +uint64_t PlatformSupport::clipboardGetSequenceNumber(PasteboardPrivate::ClipboardBuffer buffer)

if you are going to rename WebClipboard::getSequenceNumber to WebClipboard::sequenceNumber, then
it follows that you should also rename clipboardGetSequenceNumber to clipboardSequenceNumber, right?

[Daniel Cheng]

Comment on attachment 114570 [details]
Patch

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

>> Source/WebKit/chromium/src/PlatformSupport.cpp:153
>> +uint64_t PlatformSupport::clipboardGetSequenceNumber(PasteboardPrivate::ClipboardBuffer buffer)
> 
> if you are going to rename WebClipboard::getSequenceNumber to WebClipboard::sequenceNumber, then
> it follows that you should also rename clipboardGetSequenceNumber to clipboardSequenceNumber, right?

Hm. I wasn't sure.
I remember hearing somewhere that in WebKit, Class::getX() is usually something that returns a mutable X, so I thought WebClipboard::sequenceNumber would be more appropriate. But it seems like renaming the general convention in PlatformSupport is prefixVerbObject, which clipboardSequenceNumber wouldn't follow. Do you have a preference here?

[Daniel Cheng]

Created attachment 116154 [details]
Patch

[Daniel Cheng]

(In reply to comment #3)
> (From update of attachment 114570 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114570&action=review
> 
> > Source/WebKit/chromium/src/PlatformSupport.cpp:153
> > +uint64_t PlatformSupport::clipboardGetSequenceNumber(PasteboardPrivate::ClipboardBuffer buffer)
> 
> if you are going to rename WebClipboard::getSequenceNumber to WebClipboard::sequenceNumber, then
> it follows that you should also rename clipboardGetSequenceNumber to clipboardSequenceNumber, right?

I've updated the patch to make the naming consistent in WebKit.

[Tony Chang]

Comment on attachment 116154 [details]
Patch

LGTM, I defer to fishd due to the API change.

[Tony Chang]

Comment on attachment 116154 [details]
Patch

Oh, is it possible to write a test for this?

[Daniel Cheng]

(In reply to comment #8)
> (From update of attachment 116154 [details])
> Oh, is it possible to write a test for this?

It might be possible, by calling execCommand("copy"), then calling execCommand("paste") in the copy handler, then verifying that reads fail as expected. But the logic for exposing a clipboard sequence number is currently not implemented in the mock clipboard used by DRT, so it would require another 2-sided patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 116154 [details]
Patch

WebKit API change LGTM

[Daniel Cheng]

Committed r101031: <http://trac.webkit.org/changeset/101031>