Refactor bookmark clipboard code to be cross platform.

GTK: implement bookmark manager copy/paste.

- Refactor bookmark util clipboard code to be cross platform.
- Add a CBF_DATA format for writing arbitrary data to the clipboard.
- Change MenuGtk so that showing a menu only makes the menu items update asynchronously. This way we can show the menu immediately and we won't get in trouble if the delegate's IsCommandEnabled() runs a nested message loop.

I did not do any mac-side implementation of the new clipboard stuff. The implementation should be easy when they get around to writing the bookmark manager.


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

[Evan Stade, 2009-08-03 22:59:18]

I haven't tested this yet on Windows. It's been taking me all day to get a
working build there.

[sky, 2009-08-03 23:14:53]

I hate to see two separate clipboard/dnd systems (OSExchangeData and Clipboard).
On the views side we use OSExchangeData, but on Linux (not sure about Mac) we
seem to use Clipboard. Do we plan on supporting drag and drop of bookmarks on
linux from the bookmark bar/bookmark manager? If so, what is the plan for
getting both copy and paste and drag and drop to work? I would hate to see
another API for drag and drop.

http://codereview.chromium.org/159815/diff/1020/47
File base/clipboard_win.cc (right):

http://codereview.chromium.org/159815/diff/1020/47#newcode378
Line 378: CLIPFORMAT clip_format = ::RegisterClipboardFormat(format);
This likely needs to convert to wide.

[Evan Stade, 2009-08-03 23:23:17]

On 2009/08/03 23:14:53, sky wrote:
> I hate to see two separate clipboard/dnd systems (OSExchangeData and
Clipboard).
> On the views side we use OSExchangeData, but on Linux (not sure about Mac) we
> seem to use Clipboard. Do we plan on supporting drag and drop of bookmarks on
> linux from the bookmark bar/bookmark manager? If so, what is the plan for
> getting both copy and paste and drag and drop to work? I would hate to see
> another API for drag and drop.

Linux already has drag and drop support in most of the UI, including full
support in the bookmark manager and bookmark bar, tabs, render view, and omnibox
(down to bugs). There is little to no code shared with Windows for DnD. The code
for DnD is too platform-specific to be able to share almost anything. For
example, there is no sane way to refactor tab_contents/web_dr*. With regards to
bookmarks, we do share the pickling code between windows and linux, and between
Clipboard and DnD.

This patch leaves drag and drop alone on linux and windows but makes the
clipboard code shared. As Clipboard is already cross-platform, this patch does
not add any new APIs.

> http://codereview.chromium.org/159815/diff/1020/47
> File base/clipboard_win.cc (right):
> 
> http://codereview.chromium.org/159815/diff/1020/47#newcode378
> Line 378: CLIPFORMAT clip_format = ::RegisterClipboardFormat(format);
> This likely needs to convert to wide.

oh right.

[sky, 2009-08-03 23:43:58]

Thanks for the clarification on drag and drop. It looks like you are adding the
third; OSExchangeData, Pickle and now Clipboard. But the Pickle one is an
internal format so it isn't really another format. I feel better, although I'm
still am not happy that we have both OSExchangeData and Clipboard, but that's a
separate argument.

http://codereview.chromium.org/159815/diff/1020/40
File base/clipboard.h (right):

http://codereview.chromium.org/159815/diff/1020/40#newcode44
Line 44: CBF_DATA,
Can you add a description here. In particular mention this can be used for
arbitrary types.

http://codereview.chromium.org/159815/diff/1020/40#newcode97
Line 97: bool IsFormatAvailableByString(const std::string& format) const;
Can you add test coverage of these new methods?

http://codereview.chromium.org/159815/diff/1020/40#newcode173
Line 173: void WriteData(const char* format_data,
Could you describe format_data a bit better. Perhaps it should be named
format_name or format_type.

http://codereview.chromium.org/159815/diff/1020/37
File base/scoped_clipboard_writer.cc (right):

http://codereview.chromium.org/159815/diff/1020/37#newcode151
Line 151: for (int i = 0; i < pickle.size(); ++i)
This seems pretty inefficient, can't you set an initial size of data then memcpy
or something?

http://codereview.chromium.org/159815/diff/1020/45
File chrome/browser/bookmarks/bookmark_drag_data.cc (right):

http://codereview.chromium.org/159815/diff/1020/45#newcode103
Line 103: scw.WriteBookmark(WideToUTF16Hack(elements[0].title),
Do you need to special case javascript urls as is done in
BookmarkDragData::Write

http://codereview.chromium.org/159815/diff/1020/45#newcode119
Line 119: ReadFromPickle(&pickle);
This should match the code in ::Read(

http://codereview.chromium.org/159815/diff/1020/44
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/159815/diff/1020/44#newcode11
Line 11: #include "app/os_exchange_data.h"
Do we need this include anymore?

http://codereview.chromium.org/159815/diff/1020/44#newcode384
Line 384: bookmark_data.ReadFromClipboard();
You should check the return value before proceeding.

http://codereview.chromium.org/159815/diff/1020/42
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/159815/diff/1020/42#newcode25
Line 25: factory_(this) {
Don't you need ALLOW_THIS_IN_INITIALIZER_LIST?

http://codereview.chromium.org/159815/diff/1020/42#newcode34
Line 34: factory_(this) {
Don't you need ALLOW_THIS_IN_INITIALIZER_LIST?

[Ben Goodger (Google), 2009-08-03 23:49:06]

FYI, the original intent of OSExchangeData was that it be an opaque
blob that had platform-specific converters for clipboard operations
and D&D.

Because clipboard operations tend to be a superset of D&D,
OSExchangeData has the richest API.

Since its inception, some Win32 stuff may have been added but that
could be easily cleaned up.

-Ben

On Mon, Aug 3, 2009 at 4:43 PM, <sky@chromium.org> wrote:
> Thanks for the clarification on drag and drop. It looks like you are
> adding the third; OSExchangeData, Pickle and now Clipboard. But the
> Pickle one is an internal format so it isn't really another format. I
> feel better, although I'm still am not happy that we have both
> OSExchangeData and Clipboard, but that's a separate argument.
>
>
> http://codereview.chromium.org/159815/diff/1020/40
> File base/clipboard.h (right):
>
> http://codereview.chromium.org/159815/diff/1020/40#newcode44
> Line 44: CBF_DATA,
> Can you add a description here. In particular mention this can be used
> for arbitrary types.
>
> http://codereview.chromium.org/159815/diff/1020/40#newcode97
> Line 97: bool IsFormatAvailableByString(const std::string& format)
> const;
> Can you add test coverage of these new methods?
>
> http://codereview.chromium.org/159815/diff/1020/40#newcode173
> Line 173: void WriteData(const char* format_data,
> Could you describe format_data a bit better. Perhaps it should be named
> format_name or format_type.
>
> http://codereview.chromium.org/159815/diff/1020/37
> File base/scoped_clipboard_writer.cc (right):
>
> http://codereview.chromium.org/159815/diff/1020/37#newcode151
> Line 151: for (int i = 0; i < pickle.size(); ++i)
> This seems pretty inefficient, can't you set an initial size of data
> then memcpy or something?
>
> http://codereview.chromium.org/159815/diff/1020/45
> File chrome/browser/bookmarks/bookmark_drag_data.cc (right):
>
> http://codereview.chromium.org/159815/diff/1020/45#newcode103
> Line 103: scw.WriteBookmark(WideToUTF16Hack(elements[0].title),
> Do you need to special case javascript urls as is done in
> BookmarkDragData::Write
>
> http://codereview.chromium.org/159815/diff/1020/45#newcode119
> Line 119: ReadFromPickle(&pickle);
> This should match the code in ::Read(
>
> http://codereview.chromium.org/159815/diff/1020/44
> File chrome/browser/bookmarks/bookmark_utils.cc (right):
>
> http://codereview.chromium.org/159815/diff/1020/44#newcode11
> Line 11: #include "app/os_exchange_data.h"
> Do we need this include anymore?
>
> http://codereview.chromium.org/159815/diff/1020/44#newcode384
> Line 384: bookmark_data.ReadFromClipboard();
> You should check the return value before proceeding.
>
> http://codereview.chromium.org/159815/diff/1020/42
> File chrome/browser/gtk/menu_gtk.cc (right):
>
> http://codereview.chromium.org/159815/diff/1020/42#newcode25
> Line 25: factory_(this) {
> Don't you need ALLOW_THIS_IN_INITIALIZER_LIST?
>
> http://codereview.chromium.org/159815/diff/1020/42#newcode34
> Line 34: factory_(this) {
> Don't you need ALLOW_THIS_IN_INITIALIZER_LIST?
>
> http://codereview.chromium.org/159815
>

[Evan Stade, 2009-08-04 00:59:59]

updated. Works on Windows.

http://codereview.chromium.org/159815/diff/1020/40
File base/clipboard.h (right):

http://codereview.chromium.org/159815/diff/1020/40#newcode44
Line 44: CBF_DATA,
On 2009/08/03 23:43:58, sky wrote:
> Can you add a description here. In particular mention this can be used for
> arbitrary types.

Done.

http://codereview.chromium.org/159815/diff/1020/40#newcode97
Line 97: bool IsFormatAvailableByString(const std::string& format) const;
On 2009/08/03 23:43:58, sky wrote:
> Can you add test coverage of these new methods?

Done.

http://codereview.chromium.org/159815/diff/1020/40#newcode173
Line 173: void WriteData(const char* format_data,
On 2009/08/03 23:43:58, sky wrote:
> Could you describe format_data a bit better. Perhaps it should be named
> format_name or format_type.

Done.

http://codereview.chromium.org/159815/diff/1020/47
File base/clipboard_win.cc (right):

http://codereview.chromium.org/159815/diff/1020/47#newcode378
Line 378: CLIPFORMAT clip_format = ::RegisterClipboardFormat(format);
On 2009/08/03 23:14:53, sky wrote:
> This likely needs to convert to wide.

Done.

http://codereview.chromium.org/159815/diff/1020/37
File base/scoped_clipboard_writer.cc (right):

http://codereview.chromium.org/159815/diff/1020/37#newcode151
Line 151: for (int i = 0; i < pickle.size(); ++i)
On 2009/08/03 23:43:58, sky wrote:
> This seems pretty inefficient, can't you set an initial size of data then
memcpy
> or something?

Done.

http://codereview.chromium.org/159815/diff/1020/45
File chrome/browser/bookmarks/bookmark_drag_data.cc (right):

http://codereview.chromium.org/159815/diff/1020/45#newcode103
Line 103: scw.WriteBookmark(WideToUTF16Hack(elements[0].title),
On 2009/08/03 23:43:58, sky wrote:
> Do you need to special case javascript urls as is done in
> BookmarkDragData::Write

I don't know why it is special-cased there, I suspect some Windows-specific
reason, and think that if we want it, it should be pushed into the logic of
WriteBookmark.

http://codereview.chromium.org/159815/diff/1020/45#newcode119
Line 119: ReadFromPickle(&pickle);
On 2009/08/03 23:43:58, sky wrote:
> This should match the code in ::Read(

updated to check return value (although it still tries to fall back if reading
the pickled data fails)

http://codereview.chromium.org/159815/diff/1020/44
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/159815/diff/1020/44#newcode11
Line 11: #include "app/os_exchange_data.h"
On 2009/08/03 23:43:58, sky wrote:
> Do we need this include anymore?

no, removed

http://codereview.chromium.org/159815/diff/1020/44#newcode384
Line 384: bookmark_data.ReadFromClipboard();
On 2009/08/03 23:43:58, sky wrote:
> You should check the return value before proceeding.

Done.

http://codereview.chromium.org/159815/diff/1020/42
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/159815/diff/1020/42#newcode25
Line 25: factory_(this) {
On 2009/08/03 23:43:58, sky wrote:
> Don't you need ALLOW_THIS_IN_INITIALIZER_LIST?

no, that's only for windows

[sky, 2009-08-04 16:14:28]

LGTM with the following changes.

http://codereview.chromium.org/159815/diff/1068/1080
File base/clipboard_unittest.cc (right):

http://codereview.chromium.org/159815/diff/1068/1080#newcode63
Line 63:
EXPECT_TRUE(clipboard.IsFormatAvailable(Clipboard::GetPlainTextWFormatType()));
nit > 80 chars

http://codereview.chromium.org/159815/diff/1068/1079
File base/clipboard_win.cc (right):

http://codereview.chromium.org/159815/diff/1068/1079#newcode403
Line 403: bool Clipboard::IsFormatAvailableByString(const std::string&
ascii_format) const {
nit: > 80