Move clipboard-related webkit_glue embedder functions into a ClipboardClient interface.

Move clipboard-related webkit_glue embedder functions into a ClipboardClient interface.

This moves the functions into a pattern that is component-friendly.

BUG=98755
TEST=Chrome, DRT, test_shell

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

[piman, 2011-11-19 03:21:47]

dpranke: please review
tony: webkit/ OWNERS
sky: ui/base OWNERS (it's a trivial fix).

[sky, 2011-11-21 15:13:17]

Tony should really review this one in depth.

http://codereview.chromium.org/8591030/diff/6001/content/renderer/renderer_cl...
File content/renderer/renderer_clipboard_client.cc (right):

http://codereview.chromium.org/8591030/diff/6001/content/renderer/renderer_cl...
content/renderer/renderer_clipboard_client.cc:25: class
RendererClipboardWriteContext:
nit: space between 't' and ':'

http://codereview.chromium.org/8591030/diff/6001/content/renderer/renderer_cl...
File content/renderer/renderer_clipboard_client.h (right):

http://codereview.chromium.org/8591030/diff/6001/content/renderer/renderer_cl...
content/renderer/renderer_clipboard_client.h:33: #endif //
CONTENT_RENDERER_RENDERER_CLIPBOARD_CLIENT_H_
nit: two spaces after endif.

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

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/webclipboard_imp...
webkit/glue/webclipboard_impl.h:23: WebClipboardImpl(ClipboardClient* client);
explicit

http://codereview.chromium.org/8591030/diff/6001/webkit/tools/test_shell/simp...
File webkit/tools/test_shell/simple_clipboard_impl.h (right):

http://codereview.chromium.org/8591030/diff/6001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_clipboard_impl.h:32: 
nit: only one empty line.

[Dirk Pranke, 2011-11-21 20:00:54]

change looks basically fine to me w/ the changes suggested by the other
reviewers.

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/clipboard_client.h
File webkit/glue/clipboard_client.h (right):

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/clipboard_client...
webkit/glue/clipboard_client.h:19: virtual ~WriteContext() { }
we might want to de-inline these when we need to mark everything for export. I
don't think de-inlining is strictly necessary, but I don't think we've reached a
consistent convention for this yet.

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/scoped_clipboard...
File webkit/glue/scoped_clipboard_writer_glue.cc (right):

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/scoped_clipboard...
webkit/glue/scoped_clipboard_writer_glue.cc:4: webkit_glue::ClipboardClient*
client)
I realize this is the pre-existing design, but I'm a little fuzzy on why we have
both the context and the client. I'm wondering if we can simplify this now that
we have a proper clipboardclient interface. That could be done as a separate
change, though.

[piman, 2011-11-21 20:27:13]

http://codereview.chromium.org/8591030/diff/6001/content/renderer/renderer_cl...
File content/renderer/renderer_clipboard_client.cc (right):

http://codereview.chromium.org/8591030/diff/6001/content/renderer/renderer_cl...
content/renderer/renderer_clipboard_client.cc:25: class
RendererClipboardWriteContext:
On 2011/11/21 15:13:17, sky wrote:
> nit: space between 't' and ':'

Done.

http://codereview.chromium.org/8591030/diff/6001/content/renderer/renderer_cl...
File content/renderer/renderer_clipboard_client.h (right):

http://codereview.chromium.org/8591030/diff/6001/content/renderer/renderer_cl...
content/renderer/renderer_clipboard_client.h:33: #endif //
CONTENT_RENDERER_RENDERER_CLIPBOARD_CLIENT_H_
On 2011/11/21 15:13:17, sky wrote:
> nit: two spaces after endif.

Done.

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/clipboard_client.h
File webkit/glue/clipboard_client.h (right):

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/clipboard_client...
webkit/glue/clipboard_client.h:19: virtual ~WriteContext() { }
On 2011/11/21 20:00:54, Dirk Pranke wrote:
> we might want to de-inline these when we need to mark everything for export. I
> don't think de-inlining is strictly necessary, but I don't think we've reached
a
> consistent convention for this yet.

Most of the pure interfaces I've seen (and written) follow this pattern, having
an empty virtual destructor that's inline.
So far, I don't think it causes problems with the exports, see e.g. on top of my
head: ui/gfx/compositor/layer_delegate.h ,
ui/gfx/compositor/layer_animation_delegate.h etc.

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/scoped_clipboard...
File webkit/glue/scoped_clipboard_writer_glue.cc (right):

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/scoped_clipboard...
webkit/glue/scoped_clipboard_writer_glue.cc:4: webkit_glue::ClipboardClient*
client)
On 2011/11/21 20:00:54, Dirk Pranke wrote:
> I realize this is the pre-existing design, but I'm a little fuzzy on why we
have
> both the context and the client. I'm wondering if we can simplify this now
that
> we have a proper clipboardclient interface. That could be done as a separate
> change, though.

There is a single client per webkit instance (i.e. essentially 1 per process),
but you need one "context" per ScopedClipboardWriterGlue, which is where you can
store per-write & per-client data.
I agree that the code is a bit awkward in the combination of this and
ScopedClipboardWriter. What is needed I think for a clean design is to separate
the "gather data and format it" part from the "write it" part from the "scoped"
part.

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

http://codereview.chromium.org/8591030/diff/6001/webkit/glue/webclipboard_imp...
webkit/glue/webclipboard_impl.h:23: WebClipboardImpl(ClipboardClient* client);
On 2011/11/21 15:13:17, sky wrote:
> explicit

Done.

http://codereview.chromium.org/8591030/diff/6001/webkit/tools/test_shell/simp...
File webkit/tools/test_shell/simple_clipboard_impl.h (right):

http://codereview.chromium.org/8591030/diff/6001/webkit/tools/test_shell/simp...
webkit/tools/test_shell/simple_clipboard_impl.h:32: 
On 2011/11/21 15:13:17, sky wrote:
> nit: only one empty line.

Done.

[piman, 2011-11-21 22:26:41]

+darin for webkit/ OWNERS since tony looks like he's out.

[tony, 2011-11-21 22:42:53]

LGTM, just some nits.

Also cc'ing dcheng since he's been working in this area.

http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.h:99:
scoped_ptr<RendererClipboardClient> clipboard_client_;
Should we just stack allocate this?

http://codereview.chromium.org/8591030/diff/14001/webkit/glue/scoped_clipboar...
File webkit/glue/scoped_clipboard_writer_glue.cc (right):

http://codereview.chromium.org/8591030/diff/14001/webkit/glue/scoped_clipboar...
webkit/glue/scoped_clipboard_writer_glue.cc:1: #include
"webkit/glue/scoped_clipboard_writer_glue.h"
Missing copyright

http://codereview.chromium.org/8591030/diff/14001/webkit/glue/scoped_clipboar...
webkit/glue/scoped_clipboard_writer_glue.cc:13: void
ScopedClipboardWriterGlue::WriteBitmapFromPixels(const void* pixels,
Nit: blank line between functions

[piman, 2011-11-22 00:27:20]

http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.h:99:
scoped_ptr<RendererClipboardClient> clipboard_client_;
On 2011/11/21 22:42:54, tony wrote:
> Should we just stack allocate this?

I'm not sure I understand what you're suggesting here... When do you suggest we
allocate it on the stack?

http://codereview.chromium.org/8591030/diff/14001/webkit/glue/scoped_clipboar...
File webkit/glue/scoped_clipboard_writer_glue.cc (right):

http://codereview.chromium.org/8591030/diff/14001/webkit/glue/scoped_clipboar...
webkit/glue/scoped_clipboard_writer_glue.cc:1: #include
"webkit/glue/scoped_clipboard_writer_glue.h"
On 2011/11/21 22:42:54, tony wrote:
> Missing copyright

Done.

http://codereview.chromium.org/8591030/diff/14001/webkit/glue/scoped_clipboar...
webkit/glue/scoped_clipboard_writer_glue.cc:13: void
ScopedClipboardWriterGlue::WriteBitmapFromPixels(const void* pixels,
On 2011/11/21 22:42:54, tony wrote:
> Nit: blank line between functions

Done.

[tony, 2011-11-22 00:46:23]

http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.h:99:
scoped_ptr<RendererClipboardClient> clipboard_client_;
On 2011/11/22 00:27:20, piman wrote:
> On 2011/11/21 22:42:54, tony wrote:
> > Should we just stack allocate this?
> 
> I'm not sure I understand what you're suggesting here... When do you suggest
we
> allocate it on the stack?

Could this be RendererClipboardClient clipboard_client_ and in the initializer
list:

 clipboard_(new webkit_glue::WebClipboardImpl(&clipboard_client_)),

[piman, 2011-11-22 00:58:48]

On Mon, Nov 21, 2011 at 4:46 PM, <tony@chromium.org> wrote:

>
> http://codereview.chromium.**org/8591030/diff/14001/**
>
content/renderer/renderer_**webkitplatformsupport_impl.h<http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_webkitplatformsupport_impl.h>
> File content/renderer/renderer_**webkitplatformsupport_impl.h (right):
>
> http://codereview.chromium.**org/8591030/diff/14001/**
>
content/renderer/renderer_**webkitplatformsupport_impl.h#**newcode99<http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_webkitplatformsupport_impl.h#newcode99>
> content/renderer/renderer_**webkitplatformsupport_impl.h:**99:
> scoped_ptr<**RendererClipboardClient> clipboard_client_;
> On 2011/11/22 00:27:20, piman wrote:
>
>> On 2011/11/21 22:42:54, tony wrote:
>> > Should we just stack allocate this?
>>
>
>  I'm not sure I understand what you're suggesting here... When do you
>>
> suggest we
>
>> allocate it on the stack?
>>
>
> Could this be RendererClipboardClient clipboard_client_ and in the
> initializer list:
>
>  clipboard_(new webkit_glue::WebClipboardImpl(**&clipboard_client_)),
>

We can, though that pulls an extra include in the header. I'm fine either
way.


>
http://codereview.chromium.**org/8591030/<http://codereview.chromium.org/8591...
>

[piman, 2011-11-22 01:00:29]

On Mon, Nov 21, 2011 at 4:58 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Nov 21, 2011 at 4:46 PM, <tony@chromium.org> wrote:
>
>>
>> http://codereview.chromium.**org/8591030/diff/14001/**
>>
content/renderer/renderer_**webkitplatformsupport_impl.h<http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_webkitplatformsupport_impl.h>
>> File content/renderer/renderer_**webkitplatformsupport_impl.h (right):
>>
>> http://codereview.chromium.**org/8591030/diff/14001/**
>>
content/renderer/renderer_**webkitplatformsupport_impl.h#**newcode99<http://codereview.chromium.org/8591030/diff/14001/content/renderer/renderer_webkitplatformsupport_impl.h#newcode99>
>> content/renderer/renderer_**webkitplatformsupport_impl.h:**99:
>> scoped_ptr<**RendererClipboardClient> clipboard_client_;
>> On 2011/11/22 00:27:20, piman wrote:
>>
>>> On 2011/11/21 22:42:54, tony wrote:
>>> > Should we just stack allocate this?
>>>
>>
>>  I'm not sure I understand what you're suggesting here... When do you
>>>
>> suggest we
>>
>>> allocate it on the stack?
>>>
>>
>> Could this be RendererClipboardClient clipboard_client_ and in the
>> initializer list:
>>
>>  clipboard_(new webkit_glue::WebClipboardImpl(**&clipboard_client_)),
>>
>
> We can, though that pulls an extra include in the header. I'm fine either
> way.
>

Also, all the other similar members in this class are done with a
scoped_ptr too.


>
>
>>
http://codereview.chromium.**org/8591030/<http://codereview.chromium.org/8591...
>>
>
>

[tony, 2011-11-22 01:01:27]

On 2011/11/22 00:58:48, piman wrote:
> We can, though that pulls an extra include in the header. I'm fine either
> way.

Either way is fine with me too.  I would probably pick the (slightly) slower
compile for the (slightly) faster runtime.  *shrug*

Anyway, LGTM.

[commit-bot: I haz the power, 2011-11-22 01:06:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8591030/24002

[commit-bot: I haz the power, 2011-11-22 01:06:20]

Presubmit check for 8591030-24002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: ui/base/clipboard/scoped_clipboard_writer.cc

Presubmit checks took 3.5s to calculate.

[sky, 2011-11-22 01:09:13]

LGTM

[commit-bot: I haz the power, 2011-11-22 01:17:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8591030/24002

[commit-bot: I haz the power, 2011-11-22 03:48:54]

Change committed as 111097