Bug 72056 - [chromium] Fix plumbing for differentiating between clipboard/selection pastes.
Summary: [chromium] Fix plumbing for differentiating between clipboard/selection pastes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Cheng
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-10 13:59 PST by Daniel Cheng
Modified: 2011-11-22 16:57 PST (History)
2 users (show)

See Also:


Attachments
Patch (13.40 KB, patch)
2011-11-10 14:15 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.74 KB, patch)
2011-11-21 16:11 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Cheng 2011-11-10 13:59:32 PST
[chromium] Fix plumbing for differentiating between clipboard/selection pastes.
Comment 1 Daniel Cheng 2011-11-10 14:15:18 PST
Created attachment 114570 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-10 14:17:08 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-11-14 14:44:01 PST
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?
Comment 4 Daniel Cheng 2011-11-14 14:48:43 PST
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?
Comment 5 Daniel Cheng 2011-11-21 16:11:29 PST
Created attachment 116154 [details]
Patch
Comment 6 Daniel Cheng 2011-11-21 16:11:50 PST
(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.
Comment 7 Tony Chang 2011-11-21 16:16:04 PST
Comment on attachment 116154 [details]
Patch

LGTM, I defer to fishd due to the API change.
Comment 8 Tony Chang 2011-11-21 16:16:53 PST
Comment on attachment 116154 [details]
Patch

Oh, is it possible to write a test for this?
Comment 9 Daniel Cheng 2011-11-21 16:19:35 PST
(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.
Comment 10 Darin Fisher (:fishd, Google) 2011-11-21 17:18:41 PST
Comment on attachment 116154 [details]
Patch

WebKit API change LGTM
Comment 11 Daniel Cheng 2011-11-22 16:39:38 PST
Committed r101031: <http://trac.webkit.org/changeset/101031>