Change the ChromiumPasteboard to have a notion of an alternate clipboard...

Change the ChromoimPasteboard to have a notion of an alternate clipboard and use that for pasting the primary selection.  Requires lots of plumbing.

Depends on WebKit change r48168

BUG=18792
TEST=layout:middle-click-onpaste or middle click in a field with an onpaste handler (see bug)

committed: http://code.google.com/p/chromium/source/detail?r=25669

[vandebo (ex-Chrome), 2009-08-25 00:20:29]

I would appreciate feedback on this approach to bug 18792. It solves the problem
but requires a lot of plumbing.  I also need to remove the plumbing that goes
along with the previous hacky solution.

[agl, 2009-08-27 00:25:28]

Darin is, indeed, probably the right reviewer for this. It's a shame that you
had to touch so many files, but the change looks sane to me.

[vandebo (ex-Chrome), 2009-08-28 21:43:42]

This is ready for a full review.

[vandebo (ex-Chrome), 2009-09-02 16:43:20]

I've updated this change to mesh with changes in the WebKit patch, which has
gotten the OK modulo aesthetic changes.

[darin (slow to review), 2009-09-02 22:15:53]

http://codereview.chromium.org/174367/diff/6001/6014
File base/clipboard.h (right):

http://codereview.chromium.org/174367/diff/6001/6014#newcode78
Line 78: enum TargetClipboard {
i think it is sufficient to just name this enum Target since it is already in
the Clipboard "namespace"

Clipboard::Target seems sufficient.  then it would be good to name the values
accordingly:

  TARGET_STANDARD
  TARGET_SELECTION

by the way, i think it would also be good to use the same term here for the
system clipboard.  so if you call this TARGET_STANDARD, then perhaps the other
similar enums should use "standard" as the term instead of "system" as I had
suggested.

it would be good to have a comment here about how the selection target only
supports reading text.

http://codereview.chromium.org/174367/diff/6001/6014#newcode79
Line 79: StandardClipboard,
please use MACRO_STYLE for naming enums in chromium code.

http://codereview.chromium.org/174367/diff/6001/6013
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/174367/diff/6001/6013#newcode210
Line 210: GtkClipboard* clipboard;
this looks like it should be a subroutine since you repeat this case-switch down
below.

http://codereview.chromium.org/174367/diff/6001/6025
File base/clipboard_mac.mm (right):

http://codereview.chromium.org/174367/diff/6001/6025#newcode139
Line 139: Clipboard::TargetClipboard target) const {
please assert that target is only the standard target

http://codereview.chromium.org/174367/diff/6001/6025#newcode148
Line 148: void Clipboard::ReadText(Clipboard::TargetClipboard target,
please assert that target is only the standard target

http://codereview.chromium.org/174367/diff/6001/6025#newcode158
Line 158: void Clipboard::ReadAsciiText(Clipboard::TargetClipboard target,
please assert that target is only the standard target

http://codereview.chromium.org/174367/diff/6001/6012
File base/clipboard_win.cc (right):

http://codereview.chromium.org/174367/diff/6001/6012#newcode402
Line 402: return ::IsClipboardFormatAvailable(StringToInt(format)) != FALSE;
please assert that target is only the standard target

http://codereview.chromium.org/174367/diff/6001/6012#newcode406
Line 406: const std::string& ascii_format, Clipboard::TargetClipboard target)
const {
please assert that target is only the standard target

http://codereview.chromium.org/174367/diff/6001/6012#newcode412
Line 412: void Clipboard::ReadText(Clipboard::TargetClipboard target,
please assert that target is only the standard target

http://codereview.chromium.org/174367/diff/6001/6012#newcode434
Line 434: void Clipboard::ReadAsciiText(Clipboard::TargetClipboard target,
please assert that target is only the standard target

http://codereview.chromium.org/174367/diff/6001/6029
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (left):

http://codereview.chromium.org/174367/diff/6001/6029#oldcode624
Line 624: host_view->host_->Send(new
ViewMsg_InsertText(host_view->host_->routing_id(),
can we remove the code for ViewMsg_InsertText now?

http://codereview.chromium.org/174367/diff/6001/6017
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/174367/diff/6001/6017#newcode635
Line 635: static_cast<Clipboard::TargetClipboard>(target_num);
minor nit: its chromium convention to indent line continuations by 4 spaces

http://codereview.chromium.org/174367/diff/6001/6015
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/174367/diff/6001/6015#newcode177
Line 177: void OnClipboardReadText(int target, IPC::Message* reply);
please use the real enum type for target to preserve type safety.

http://codereview.chromium.org/174367/diff/6001/6019
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/174367/diff/6001/6019#newcode1047
Line 1047: int /* target Clipboard::TargetClipboard enum*/,
please pass the enum type instead of int here.  no reason to lose type safety. 
add a ParamTraits for the enum type.

http://codereview.chromium.org/174367/diff/6001/6010
File webkit/api/public/WebClipboard.h (right):

http://codereview.chromium.org/174367/diff/6001/6010#newcode50
Line 50: Clipboard,
it seems strange that we have an interface for the clipboard that might not be
the clipboard.  hmm...

maybe it would be better to use different terms:

  enum Target {
      TargetSystem,
      TargetSelection
  };

In the above, TargetSystem refers to the system clipboard.  TargetSelection
refers to the clipboard for the current selection.

http://codereview.chromium.org/174367/diff/6001/6010#newcode57
Line 57: virtual WebString readHTML(WebURL*) = 0;
why doesn't readHTML have a Target parameter?  is that because you can only read
HTML from the clipboard?

the asymmetry here is a bit concerning...

http://codereview.chromium.org/174367/diff/6001/6009
File webkit/api/src/ChromiumBridge.cpp (right):

http://codereview.chromium.org/174367/diff/6001/6009#newcode90
Line 90: static_cast<WebClipboard::Target>(target));
please add COMPILE_ASSERTs to ensure that these enums stay in sync.  see the
group of COMPILE_ASSERTs above.

http://codereview.chromium.org/174367/diff/6001/6003
File webkit/glue/webclipboard_impl.h (right):

http://codereview.chromium.org/174367/diff/6001/6003#newcode24
Line 24: virtual bool isFormatAvailable(WebKit::WebClipboard::Format,
it turns out that the scoping for both Format and Target is not necessary here
since WebClipboardImpl extends from WebKit::WebClipboard.

[vandebo (ex-Chrome), 2009-09-03 22:28:53]

Address all of Darin's comments with a few specific notes.

http://codereview.chromium.org/174367/diff/6001/6029
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (left):

http://codereview.chromium.org/174367/diff/6001/6029#oldcode624
Line 624: host_view->host_->Send(new
ViewMsg_InsertText(host_view->host_->routing_id(),
> can we remove the code for ViewMsg_InsertText now?
Looks like yes. Done.

http://codereview.chromium.org/174367/diff/6001/6010
File webkit/api/public/WebClipboard.h (right):

http://codereview.chromium.org/174367/diff/6001/6010#newcode50
Line 50: Clipboard,
On 2009/09/02 22:15:53, darin wrote:
> it seems strange that we have an interface for the clipboard that might not be
> the clipboard.  hmm...
> 
> maybe it would be better to use different terms:
> 
>   enum Target {
>       TargetSystem,
>       TargetSelection
>   };

I agree that a "non-clipboard" target is odd.  I've tried to keep the naming of
these enums consistent (there are three that all do the same thing at three
different levels of abstraction).  ClipboardTarget seems a bit odd, like the
destination of the clipboard operation.  I've settled on ClipboardBuffer/Buffer
with StandardBuffer and SelectionBuffer as the types.  If you strongly prefer
Target, I can change it to that.

http://codereview.chromium.org/174367/diff/6001/6010#newcode57
Line 57: virtual WebString readHTML(WebURL*) = 0;
On 2009/09/02 22:15:53, darin wrote:
> why doesn't readHTML have a Target parameter?  is that because you can only
read
> HTML from the clipboard?
> the asymmetry here is a bit concerning...

Currently only the plain text variants are used with the "primary selection," so
I had only changed those.  readHTML being the only read interface that doesn't
support selecting which buffer to use is a bit odd, so I've updated its
interface as well.  The fact that we don't currently use readHTML for the
"primary selection" probably indicates a bug, but one that should be addressed
in a different issue.

[darin (slow to review), 2009-09-04 16:59:24]

i haven't had a chance to fully review this... but looking better...

http://codereview.chromium.org/174367/diff/11001/11014
File base/clipboard.h (right):

http://codereview.chromium.org/174367/diff/11001/11014#newcode81
Line 81: STANDARD_BUFFER,
i think BUFFER_{STANDARD,SELECTION} would be better just as you did for
WebClipboard

http://codereview.chromium.org/174367/diff/11001/11014#newcode87
Line 87: static bool ValidBuffer(int32 buffer) {
nit: IsValidBuffer <- use a verb

http://codereview.chromium.org/174367/diff/11001/11010
File webkit/api/public/WebClipboard.h (right):

http://codereview.chromium.org/174367/diff/11001/11010#newcode34
Line 34: #include "build/build_config.h"
you cannot include this header here.  webkit/api is destined to live in
svn.webkit.org where build_config.h doesn't exist.

http://codereview.chromium.org/174367/diff/11001/11010#newcode53
Line 53: BufferSelection
instead of excluding this on some platforms, just add a comment saying that a
selection buffer may not be available on all platforms.  e.g., this is intended
to support the X11 selection buffer but may be useful on other platforms.

http://codereview.chromium.org/174367/diff/11001/11004
File webkit/glue/webclipboard_impl.cc (right):

http://codereview.chromium.org/174367/diff/11001/11004#newcode80
Line 80: return false;
nit: indentation is supposed to be 2 white spaces.

there are more errors like this.  please fix.

[vandebo (ex-Chrome), 2009-09-04 22:06:43]

http://codereview.chromium.org/174367/diff/11001/11014
File base/clipboard.h (right):

http://codereview.chromium.org/174367/diff/11001/11014#newcode81
Line 81: STANDARD_BUFFER,
On 2009/09/04 16:59:25, darin wrote:
> i think BUFFER_{STANDARD,SELECTION} would be better
Sounds good. Done.

http://codereview.chromium.org/174367/diff/11001/11014#newcode87
Line 87: static bool ValidBuffer(int32 buffer) {
On 2009/09/04 16:59:25, darin wrote:
> nit: IsValidBuffer <- use a verb
Done. (Had just copied ParamTraits<FilterPolicy::Type>)

http://codereview.chromium.org/174367/diff/11001/11004
File webkit/glue/webclipboard_impl.cc (right):

http://codereview.chromium.org/174367/diff/11001/11004#newcode80
Line 80: return false;
On 2009/09/04 16:59:25, darin wrote:
> nit: indentation is supposed to be 2 white spaces.
> there are more errors like this.  please fix.

Sorry contended with WebKit style and chromium style simultaneously is error
prone.  I think I got them all.

[darin (slow to review), 2009-09-04 22:38:06]

Looks great...

http://codereview.chromium.org/174367/diff/18002/19009
File webkit/api/public/WebClipboard.h (right):

http://codereview.chromium.org/174367/diff/18002/19009#newcode51
Line 51: // Only used on platforms that use the X Window System
This comment is fine, but my suggestion was actually to word this a little more
openly since this could be usable by other ports provided they have a concept
similar to the primary selection of X.  From the point-of-view of the API, it is
not required that the window system be X in order for BufferSelection to be
usable.

http://codereview.chromium.org/174367/diff/18002/19006
File webkit/tools/test_shell/mock_webclipboard_impl.h (right):

http://codereview.chromium.org/174367/diff/18002/19006#newcode19
Line 19: WebKit::WebClipboard::Buffer);
nit: please wrap params similar to how they are done for writeHTML, writeURL,
and writeImage.

[vandebo (ex-Chrome), 2009-09-04 22:52:24]

Fixed up those two points.

[darin (slow to review), 2009-09-04 23:28:07]

LGTM