Have ScopedClipboardWriter and Clipboard::WriteObjects take a buffer parameter.

Have ScopedClipboardWriter and Clipboard::WriteObjects take a buffer parameter.

GTK: add support for writing to primary selection (BUFFER_SELECTION).

BUG=none
TEST=ui_unittests
TBR=ananta

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122972

[dcheng, 2012-02-01 19:03:24]

Two questions:
1) Is there a crbug for this?
2) What are you trying to fix? I see you added the ability to write to the
primary selection, but it doesn't look like it's actually used outside the unit
test.

[peter1, 2012-02-02 22:49:40]

On Wed, Feb 01, 2012 at 07:03:24PM +0000, dcheng@chromium.org wrote:
> Two questions:
> 1) Is there a crbug for this?

No, would you like me to create one?

> 2) What are you trying to fix? I see you added the ability to write to the
> primary selection, but it doesn't look like it's actually used outside 
> the unit
> test.

I'd like the primary selection to provide a text/html target.
The attached patch will do that, but it won't work correctly until
this WebKit patch [1] goes in and WebKit rolls, so I haven't submitted
it for review yet.

Have I submitted the WebKit patch correctly?

Thanks,
-- 
Peter

[1] https://bugs.webkit.org/show_bug.cgi?id=77307

[dcheng, 2012-02-03 21:53:57]

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard.h
File ui/base/clipboard/clipboard.h (right):

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.h:167: void DidWriteURL(Buffer buffer, const
std::string& utf8_text) {}
It seems like we don't need the buffer parameter since it only applies when we
write to the standard selection (e.g. the pasteboard).

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_unittest.cc (right):

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_unittest.cc:137: ScopedClipboardWriter
clipboard_writer(&clipboard,
Should we write different strings to the primary selection and the clipboard
selection? This might catch cases where we clobber one clipboard by accident (or
intentionally)

[dcheng, 2012-02-03 21:55:19]

Also, don't hesitate to ping me if I haven't looked at your change in a timely
manner--IRC might be best for this. I apologize for the latency on this review.

[peter1, 2012-02-04 00:54:46]

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard.h
File ui/base/clipboard/clipboard.h (right):

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.h:167: void DidWriteURL(Buffer buffer, const
std::string& utf8_text) {}
On 2012/02/03 21:53:58, dcheng wrote:
> It seems like we don't need the buffer parameter since it only applies when we
> write to the standard selection (e.g. the pasteboard).

We need to check in DidWriteURL that the write was to the clipboard to avoid
overwriting the just-written (and possibly multi-format) primary selection with
a plain text URL, and for that we need the buffer parameter.

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_unittest.cc (right):

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_unittest.cc:137: ScopedClipboardWriter
clipboard_writer(&clipboard,
On 2012/02/03 21:53:58, dcheng wrote:
> Should we write different strings to the primary selection and the clipboard
> selection? This might catch cases where we clobber one clipboard by accident
(or
> intentionally)

Well, that is essentially what this test is doing.  It loads a text string to
clipboard and a different HTML string to primary then checks that they are
preserved that way.

[dcheng, 2012-02-04 01:11:05]

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard.h
File ui/base/clipboard/clipboard.h (right):

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.h:167: void DidWriteURL(Buffer buffer, const
std::string& utf8_text) {}
On 2012/02/04 00:54:46, peter1 wrote:
> On 2012/02/03 21:53:58, dcheng wrote:
> > It seems like we don't need the buffer parameter since it only applies when
we
> > write to the standard selection (e.g. the pasteboard).
> 
> We need to check in DidWriteURL that the write was to the clipboard to avoid
> overwriting the just-written (and possibly multi-format) primary selection
with
> a plain text URL, and for that we need the buffer parameter.

Right, but this is only called from ScopedClipboardWriter (and not
ScopedClipboardWriterGlue). ScopedClipboardWriter already knows which buffer
it's writing to, so it can skip the call there can't it?

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_unittest.cc (right):

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_unittest.cc:137: ScopedClipboardWriter
clipboard_writer(&clipboard,
On 2012/02/04 00:54:46, peter1 wrote:
> On 2012/02/03 21:53:58, dcheng wrote:
> > Should we write different strings to the primary selection and the clipboard
> > selection? This might catch cases where we clobber one clipboard by accident
> (or
> > intentionally)
> 
> Well, that is essentially what this test is doing.  It loads a text string to
> clipboard and a different HTML string to primary then checks that they are
> preserved that way.

Do we have a test that DidWriteURL is correctly causing URL to be set in the
primary selection when a URL is written to the standard selection though? I
don't see that.

[peter1, 2012-02-04 02:05:43]

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard.h
File ui/base/clipboard/clipboard.h (right):

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.h:167: void DidWriteURL(Buffer buffer, const
std::string& utf8_text) {}
On 2012/02/04 01:11:05, dcheng wrote:
> On 2012/02/04 00:54:46, peter1 wrote:
> > On 2012/02/03 21:53:58, dcheng wrote:
> > > It seems like we don't need the buffer parameter since it only applies
when
> we
> > > write to the standard selection (e.g. the pasteboard).
> > 
> > We need to check in DidWriteURL that the write was to the clipboard to avoid
> > overwriting the just-written (and possibly multi-format) primary selection
> with
> > a plain text URL, and for that we need the buffer parameter.
> 
> Right, but this is only called from ScopedClipboardWriter (and not
> ScopedClipboardWriterGlue). ScopedClipboardWriter already knows which buffer
> it's writing to, so it can skip the call there can't it?

I see.  Well, I don't think it matters too much where we do this test, so I'll
go ahead and move it.

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_unittest.cc (right):

http://codereview.chromium.org/9232075/diff/5001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_unittest.cc:137: ScopedClipboardWriter
clipboard_writer(&clipboard,
On 2012/02/04 01:11:05, dcheng wrote:
> On 2012/02/04 00:54:46, peter1 wrote:
> > On 2012/02/03 21:53:58, dcheng wrote:
> > > Should we write different strings to the primary selection and the
clipboard
> > > selection? This might catch cases where we clobber one clipboard by
accident
> > (or
> > > intentionally)
> > 
> > Well, that is essentially what this test is doing.  It loads a text string
to
> > clipboard and a different HTML string to primary then checks that they are
> > preserved that way.
> 
> Do we have a test that DidWriteURL is correctly causing URL to be set in the
> primary selection when a URL is written to the standard selection though? I
> don't see that.

No, and it makes sense to add one.  I'll do that.

[peter1, 2012-02-04 02:10:44]

> > Do we have a test that DidWriteURL is correctly causing URL to be set in the
> > primary selection when a URL is written to the standard selection though? I
> > don't see that.
> 
> No, and it makes sense to add one.  I'll do that.

Hmm, actually we do already have a test for this -- ClipboardTest.URLTest.

[peter1, 2012-02-15 01:09:13]

Ping.

[dcheng, 2012-02-15 21:18:16]

lgtm

[commit-bot: I haz the power, 2012-02-15 21:30:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@pcc.me.uk/9232075/16001

[commit-bot: I haz the power, 2012-02-15 21:30:47]

Presubmit check for 9232075-16001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
ui/base/clipboard/clipboard_aurax11.cc,ui/base/clipboard/clipboard_win.cc,ash/drag_drop/drag_drop_controller_unittest.cc,chrome/browser/net/browser_url_util.cc,ui/base/clipboard/scoped_clipboard_writer.h,chrome/browser/ui/pdf/pdf_browsertest.cc,ui/views/controls/textfield/native_textfield_views_unittest.cc,chrome/browser/ui/views/omnibox/omnibox_view_win.cc,ui/base/clipboard/clipboard_unittest.cc,ui/views/controls/textfield/native_textfield_win.cc,chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm,ui/base/clipboard/scoped_clipboard_writer.cc,ui/base/clipboard/clipboard_gtk.cc,ui/views/controls/message_box_view.cc,ui/base/dragdrop/os_exchange_data_provider_aura.cc,ui/base/clipboard/clipboard_mac.mm,ui/base/clipboard/clipboard.h,content/browser/renderer_host/clipboard_message_filter.cc,webkit/glue/scoped_clipboard_writer_glue.cc,chrome/browser/bookmarks/bookmark_utils_unittest.cc,chrome/browser/bookmarks/bookmark_node_data.cc,ui/views/controls/textfield/textfield_views_model.cc,chrome/browser/ui/views/omnibox/omnibox_view_views.cc

Presubmit checks took 3.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[battre, 2012-02-15 23:00:02]

LGTM for chrome/browser/net/browser_url_util.cc

[willchan no longer on Chromium, 2012-02-15 23:00:33]

lgtm

[eroman, 2012-02-15 23:01:03]

LGTM for chrome/browser/net/*

[sky, 2012-02-16 01:39:03]

LGTM

https://chromiumcodereview.appspot.com/9232075/diff/16001/ui/base/clipboard/s...
File ui/base/clipboard/scoped_clipboard_writer.cc (right):

https://chromiumcodereview.appspot.com/9232075/diff/16001/ui/base/clipboard/s...
ui/base/clipboard/scoped_clipboard_writer.cc:19: : clipboard_(clipboard),
buffer_(buffer) {
each param on its own line.

https://chromiumcodereview.appspot.com/9232075/diff/16001/ui/base/clipboard/s...
File ui/base/clipboard/scoped_clipboard_writer.h (right):

https://chromiumcodereview.appspot.com/9232075/diff/16001/ui/base/clipboard/s...
ui/base/clipboard/scoped_clipboard_writer.h:32: explicit
ScopedClipboardWriter(Clipboard* clipboard,
no explicit

[sky, 2012-02-16 01:39:50]

Can you also get evan or erg to review this?

[Elliot Glaysher, 2012-02-16 20:16:54]

lgtm

[peter1, 2012-02-17 18:45:41]

https://chromiumcodereview.appspot.com/9232075/diff/16001/ui/base/clipboard/s...
File ui/base/clipboard/scoped_clipboard_writer.cc (right):

https://chromiumcodereview.appspot.com/9232075/diff/16001/ui/base/clipboard/s...
ui/base/clipboard/scoped_clipboard_writer.cc:19: : clipboard_(clipboard),
buffer_(buffer) {
On 2012/02/16 01:39:03, sky wrote:
> each param on its own line.

Done.

https://chromiumcodereview.appspot.com/9232075/diff/16001/ui/base/clipboard/s...
File ui/base/clipboard/scoped_clipboard_writer.h (right):

https://chromiumcodereview.appspot.com/9232075/diff/16001/ui/base/clipboard/s...
ui/base/clipboard/scoped_clipboard_writer.h:32: explicit
ScopedClipboardWriter(Clipboard* clipboard,
On 2012/02/16 01:39:03, sky wrote:
> no explicit

Done.

[peter1, 2012-02-17 21:06:51]

tony: please look at 
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm
ananta: please look at chrome_frame/test/chrome_frame_test_utils.cc
jeremy: please look at webkit/glue/scoped_clipboard_writer_glue.cc

These should only be API updates for your code.

[jeremy, 2012-02-19 11:24:06]

LGTM

[tony, 2012-02-21 19:23:59]

webkit LGTM

[commit-bot: I haz the power, 2012-02-22 01:10:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@pcc.me.uk/9232075/25001

[commit-bot: I haz the power, 2012-02-22 02:14:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@pcc.me.uk/9232075/33003

[commit-bot: I haz the power, 2012-02-22 04:54:29]

Change committed as 122972