Make Clipboard::FormatType an opaque handle type.

Make Clipboard::FormatType an opaque handle type.

BUG=106523
TEST=ui_unittests --gtest_filter=ClipboardTest.* and manual testing

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

[dcheng, 2011-12-06 10:08:31]

+jam for content and IPC changes, tony/sky to review the general idea behind the
patch.

[tony, 2011-12-06 18:58:29]

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

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.h:215: static FormatType RegisterFormatType(const
std::string& format_string);
Nit: I would perhaps name this GetFormatType since it feels like registration is
a side effect of getting the format type rather than getting the format type a
side effect of registering.

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.h:221: static FormatType StringToFormatType(const
std::string& format_string);
Why not just call these methods SerializeFormatType and DeserializeFormatType. 
You could even put these methods on FormatType.

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:170: if (GetPlainTextFormatType().data()
== format.data())
Would it be worth adding an Equals method to FormatType?

Also, it looks like we just use .data() in the platform specific files.  It
might be clearer to just make methods for each OS (ToString, ToGtkAtom,
ToNSString, etc). This is like what we do in ui/gfx/rect.h.

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:684:
::RegisterClipboardFormat(ASCIIToWide(format_string).c_str()));
Do we need to check the return value of RegisterClipboardFormat?

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:690:
*reinterpret_cast<UINT*>(string_as_array(&result)) = type.data();
This seems unnecessarily clever/hard to read.  Can we just use StringPrintf and
UintToString for serializing/deserializing?

Alternately, you could typedef the serialization format and just use UINT on
Windows.

[dcheng, 2011-12-06 22:57:02]

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

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.h:215: static FormatType RegisterFormatType(const
std::string& format_string);
On 2011/12/06 18:58:29, tony wrote:
> Nit: I would perhaps name this GetFormatType since it feels like registration
is
> a side effect of getting the format type rather than getting the format type a
> side effect of registering.

Done.

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard....
ui/base/clipboard/clipboard.h:221: static FormatType StringToFormatType(const
std::string& format_string);
On 2011/12/06 18:58:29, tony wrote:
> Why not just call these methods SerializeFormatType and DeserializeFormatType.

> You could even put these methods on FormatType.

Done.

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:170: if (GetPlainTextFormatType().data()
== format.data())
On 2011/12/06 18:58:29, tony wrote:
> Would it be worth adding an Equals method to FormatType?
> 
> Also, it looks like we just use .data() in the platform specific files.  It
> might be clearer to just make methods for each OS (ToString, ToGtkAtom,
> ToNSString, etc). This is like what we do in ui/gfx/rect.h.

Done (for both).

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:684:
::RegisterClipboardFormat(ASCIIToWide(format_string).c_str()));
On 2011/12/06 18:58:29, tony wrote:
> Do we need to check the return value of RegisterClipboardFormat?

We didn't check it before. If it fails, the other clipboard reading functions
will return failure.

http://codereview.chromium.org/8801038/diff/5029/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:690:
*reinterpret_cast<UINT*>(string_as_array(&result)) = type.data();
On 2011/12/06 18:58:29, tony wrote:
> This seems unnecessarily clever/hard to read.  Can we just use StringPrintf
and
> UintToString for serializing/deserializing?
> 
> Alternately, you could typedef the serialization format and just use UINT on
> Windows.

Done. I was afraid someone would say that =)

It seems kind of dumb to itoa something only to atoi it back later.
Unfortunately, there's no real alternative at the moment because of the way
Clipboard::DispatchObjects() is implemented.

[tony, 2011-12-06 23:57:20]

LG, just some questions for things that aren't clear to me.

http://codereview.chromium.org/8801038/diff/17001/content/common/clipboard_me...
File content/common/clipboard_messages.h (right):

http://codereview.chromium.org/8801038/diff/17001/content/common/clipboard_me...
content/common/clipboard_messages.h:19: #define
CONTENT_COMMON_CLIPBOARD_MESSAGES_H_
I don't understand this, so I'll let John review it.

http://codereview.chromium.org/8801038/diff/17001/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_mac.mm (right):

http://codereview.chromium.org/8801038/diff/17001/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_mac.mm:62: const FormatType& other) {
Why does mac need this?

http://codereview.chromium.org/8801038/diff/17001/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_mac.mm:82: }
Where's Equals?  Maybe we don't need it?  Wouldn't hurt to define it even if we
don't use it.

[sky, 2011-12-06 23:58:28]

Since Tony (the clipboard expert) reviewed this, rubber stamp LGTM from me.

[dcheng, 2011-12-07 00:09:01]

http://codereview.chromium.org/8801038/diff/17001/content/common/clipboard_me...
File content/common/clipboard_messages.h (right):

http://codereview.chromium.org/8801038/diff/17001/content/common/clipboard_me...
content/common/clipboard_messages.h:19: #define
CONTENT_COMMON_CLIPBOARD_MESSAGES_H_
On 2011/12/06 23:57:20, tony wrote:
> I don't understand this, so I'll let John review it.

This file normally gets included multiple times to emit the proper definitions
for various things related to ParamTraits<T> macros and IPC message goop. Since
we provide the definitions/declarations for this specialization, I need to put a
guard around on it so the multiple-include magic doesn't cause compile errors.

http://codereview.chromium.org/8801038/diff/17001/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_mac.mm (right):

http://codereview.chromium.org/8801038/diff/17001/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_mac.mm:62: const FormatType& other) {
On 2011/12/06 23:57:20, tony wrote:
> Why does mac need this?

It's there for other platforms as well, it's just using the implicit ones. It's
so FormatType can be a drop-in replacement for std::string.

http://codereview.chromium.org/8801038/diff/17001/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_mac.mm:82: }
On 2011/12/06 23:57:21, tony wrote:
> Where's Equals?  Maybe we don't need it?  Wouldn't hurt to define it even if
we
> don't use it.

I intentionally left it out since there's no one that uses it on Mac/Win. It'd
be easy to add it if it ever changed in the future.

[jam, 2011-12-07 09:40:03]

content lgtm

[commit-bot: I haz the power, 2011-12-07 12:27:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8801038/17001

[commit-bot: I haz the power, 2011-12-07 12:27:42]

Presubmit check for 8801038-17001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: webkit/glue/webclipboard_impl.cc

Presubmit checks took 2.1s to calculate.

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

[tony, 2011-12-07 18:06:28]

LGTM

[commit-bot: I haz the power, 2011-12-07 18:08:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8801038/17001

[commit-bot: I haz the power, 2011-12-07 20:26:59]

Try job failure for 8801038-17001 (retry) on mac_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...