Modified the flash cipboard interface to add html clipboard support.

Modified the flash cipboard interface to add html clipboard support.

This changes the way the interface works. To write data, the user now passes an array of data items, one for each format they want to write to the clipboard. When reading data, they specify the format they want to read.

BUG=110796
TEST=./ui_tests --gtest_filter=*PPAPITest.*Clipboard* passes


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

[viettrungluu, 2012-01-25 00:01:55]

Note: You'll also need to properly version this interface and maintain backwards
compatibility with the old version. (Look through ppapi/api/*.idl for examples
of versioning/deprecation.)

https://chromiumcodereview.appspot.com/9212066/diff/1/ppapi/api/private/ppb_f...
File ppapi/api/private/ppb_flash_clipboard.idl (right):

https://chromiumcodereview.appspot.com/9212066/diff/1/ppapi/api/private/ppb_f...
ppapi/api/private/ppb_flash_clipboard.idl:68: PP_Var ReadPlainText(
Maybe we should have:

PP_Var ReadData([in] PP_Instance instance_id,
                [in] PP_Flash_Clipboard_Type clipboard_type,
                [in] PP_Flash_Clipboard_Format format);

and

int32_t AddData([in] PP_Instance instance_id,
                [in] PP_Flash_Clipboard_Type clipboard_type,
                [in] PP_Var data);

(see below for "Add" versus "Write").

https://chromiumcodereview.appspot.com/9212066/diff/1/ppapi/api/private/ppb_f...
ppapi/api/private/ppb_flash_clipboard.idl:77: int32_t WritePlainText(
Since "write" no longer parallels "read" directly (since the former now requires
"commit"), I think we should call the former "add" or maybe "set".

https://chromiumcodereview.appspot.com/9212066/diff/1/ppapi/api/private/ppb_f...
ppapi/api/private/ppb_flash_clipboard.idl:102: void Clear(
Is this instantaneous? Or does it also require commit?

[raymes1, 2012-01-25 01:33:11]

https://chromiumcodereview.appspot.com/9212066/diff/1/ppapi/api/private/ppb_f...
File ppapi/api/private/ppb_flash_clipboard.idl (right):

https://chromiumcodereview.appspot.com/9212066/diff/1/ppapi/api/private/ppb_f...
ppapi/api/private/ppb_flash_clipboard.idl:68: PP_Var ReadPlainText(
On 2012/01/25 00:01:55, viettrungluu wrote:
> Maybe we should have:
> 
> PP_Var ReadData([in] PP_Instance instance_id,
>                 [in] PP_Flash_Clipboard_Type clipboard_type,
>                 [in] PP_Flash_Clipboard_Format format);
> 
> and
> 
> int32_t AddData([in] PP_Instance instance_id,
>                 [in] PP_Flash_Clipboard_Type clipboard_type,
>                 [in] PP_Var data);
> 
> (see below for "Add" versus "Write").

Sounds good, I was debating whether or not to do this.

https://chromiumcodereview.appspot.com/9212066/diff/1/ppapi/api/private/ppb_f...
ppapi/api/private/ppb_flash_clipboard.idl:77: int32_t WritePlainText(
On 2012/01/25 00:01:55, viettrungluu wrote:
> Since "write" no longer parallels "read" directly (since the former now
requires
> "commit"), I think we should call the former "add" or maybe "set".

Done.

https://chromiumcodereview.appspot.com/9212066/diff/1/ppapi/api/private/ppb_f...
ppapi/api/private/ppb_flash_clipboard.idl:102: void Clear(
On 2012/01/25 00:01:55, viettrungluu wrote:
> Is this instantaneous? Or does it also require commit?

This requires a commit. I updated the docs to make that clear.

[viettrungluu, 2012-01-26 18:26:18]

Looks good (with updated docs).

(Don't commit this without the implementation, obviously.)

http://codereview.chromium.org/9212066/diff/3006/ppapi/api/private/ppb_flash_...
File ppapi/api/private/ppb_flash_clipboard.idl (right):

http://codereview.chromium.org/9212066/diff/3006/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_clipboard.idl:88: */
Probably you should add that it returns a null PP_Var on error (e.g., if the
given format is not available from the given clipboard).

http://codereview.chromium.org/9212066/diff/3006/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_clipboard.idl:97: * Any existing clipboard data is
erased before writing this data.
Any existing clipboard data in any format, I presume?

http://codereview.chromium.org/9212066/diff/3006/ppapi/api/private/ppb_flash_...
ppapi/api/private/ppb_flash_clipboard.idl:98: */
You should probably clarify what happens on error. If one of the items is too
big, will it write the others? What would it return in that case?

(Also, what if two items have the same type?)

[dcheng, 2012-01-27 19:55:32]

https://chromiumcodereview.appspot.com/9212066/diff/14001/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/14001/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:170: ScopedClipboardWriterGlue
&scw) {
We should probably use a pointer here instead of a non-const reference to
conform to Chromium style.

https://chromiumcodereview.appspot.com/9212066/diff/14001/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:217: scw.WriteText(string16());
Does this work? WriteText doesn't push anything into objects_ if given an empty
string, and ~ScopedClipboardWriterGlue becomes a no-op if objects_ is empty.

[raymes, 2012-01-30 17:50:30]

Thanks for the comments.

On 2012/01/27 19:55:32, dcheng wrote:
>
https://chromiumcodereview.appspot.com/9212066/diff/14001/webkit/plugins/ppap...
> File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9212066/diff/14001/webkit/plugins/ppap...
> webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:170:
ScopedClipboardWriterGlue
> &scw) {
> We should probably use a pointer here instead of a non-const reference to
> conform to Chromium style.
> 
>
https://chromiumcodereview.appspot.com/9212066/diff/14001/webkit/plugins/ppap...
> webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:217:
scw.WriteText(string16());
> Does this work? WriteText doesn't push anything into objects_ if given an
empty
> string, and ~ScopedClipboardWriterGlue becomes a no-op if objects_ is empty.

No, it actually doesn't. I will probably have to implement a Clear() function in
the clipboard interface to make things work nicely.

Also, as discussed, it is a problem that the RendererClipboardClient is included
in the webkit/plugins implementation. I'm investigating using plugin_delegate to
get around this.

[raymes, 2012-01-30 21:11:10]

This new patch uses plugin_delegate to provide access to the ClipboardClient
which plugins can use to access the clipboard. Now passes checkdeps.py and also
passes the existing pepper clipboard test.

This CL still needs:
-Properly convert the C++ API
-Implement Clear() in the clipboard
-Add more tests

[raymes, 2012-02-06 19:12:00]

This is ready for a look.

The only thing that I haven't implemented is functionality to clear the
clipboard. This can probably be done in a separate CL.

[viettrungluu, 2012-02-06 21:29:29]

Some initial comments.

http://codereview.chromium.org/9212066/diff/32008/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9212066/diff/32008/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2122:
PepperPluginDelegateImpl::CreateClipboardClient() const {
Style nit: Apparently, the style in this file is to not indent this line. Or to
double-indent it (this file isn't consistent). (Normal style in Chromium is to
double-indent, though sometimes not-indenting is also used; a single-indent is
never used in this situation.)

http://codereview.chromium.org/9212066/diff/32008/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/32008/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:19: return PPB_FLASH_CLIPBOARD_INTERFACE;
For ease of updating Flapper (in conjunction with Chrome), please make this able
to handle both version 3.0 and 4.0 (with the HTML methods failing). See
ppapi/cpp/private/flash.cc for how to do this.

http://codereview.chromium.org/9212066/diff/32008/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.h (left):

http://codereview.chromium.org/9212066/diff/32008/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.h:35: static bool WritePlainText(Instance*
instance,
You should keep this (and also ReadPlainText()) temporarily for backwards
compatibility, and indicate that it's deprecated.

http://codereview.chromium.org/9212066/diff/32008/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.h (right):

http://codereview.chromium.org/9212066/diff/32008/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.h:18: class ClipboardData {
This seems like overkill to me. I'd just use an
std::vector<PP_Flash_Clipboard_Data_Item>.

http://codereview.chromium.org/9212066/diff/32008/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.h:52: // Returns true on success, in which
case |html_out| will be filled with html
I'd just implement a ReadData() that parallels the C interface.

http://codereview.chromium.org/9212066/diff/32008/ppapi/proxy/ppb_flash_clipb...
File ppapi/proxy/ppb_flash_clipboard_proxy.h (right):

http://codereview.chromium.org/9212066/diff/32008/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.h:59: void OnMsgReadPlainText(PP_Instance
instance,
I don't think you need these anymore. You should be able to implement
ReadPlainText() and WritePlainText() in terms of the new primitives.

[raymes, 2012-02-07 00:02:25]

https://chromiumcodereview.appspot.com/9212066/diff/32008/content/renderer/pe...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/32008/content/renderer/pe...
content/renderer/pepper_plugin_delegate_impl.cc:2122:
PepperPluginDelegateImpl::CreateClipboardClient() const {
On 2012/02/06 21:29:29, viettrungluu wrote:
> Style nit: Apparently, the style in this file is to not indent this line. Or
to
> double-indent it (this file isn't consistent). (Normal style in Chromium is to
> double-indent, though sometimes not-indenting is also used; a single-indent is
> never used in this situation.)

Done.

https://chromiumcodereview.appspot.com/9212066/diff/32008/ppapi/cpp/private/f...
File ppapi/cpp/private/flash_clipboard.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/32008/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.cc:19: return PPB_FLASH_CLIPBOARD_INTERFACE;
On 2012/02/06 21:29:29, viettrungluu wrote:
> For ease of updating Flapper (in conjunction with Chrome), please make this
able
> to handle both version 3.0 and 4.0 (with the HTML methods failing). See
> ppapi/cpp/private/flash.cc for how to do this.

Done.

https://chromiumcodereview.appspot.com/9212066/diff/32008/ppapi/cpp/private/f...
File ppapi/cpp/private/flash_clipboard.h (left):

https://chromiumcodereview.appspot.com/9212066/diff/32008/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.h:35: static bool WritePlainText(Instance*
instance,
On 2012/02/06 21:29:29, viettrungluu wrote:
> You should keep this (and also ReadPlainText()) temporarily for backwards
> compatibility, and indicate that it's deprecated.

Done.

https://chromiumcodereview.appspot.com/9212066/diff/32008/ppapi/cpp/private/f...
File ppapi/cpp/private/flash_clipboard.h (right):

https://chromiumcodereview.appspot.com/9212066/diff/32008/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.h:18: class ClipboardData {
On 2012/02/06 21:29:29, viettrungluu wrote:
> This seems like overkill to me. I'd just use an
> std::vector<PP_Flash_Clipboard_Data_Item>.

Done.

https://chromiumcodereview.appspot.com/9212066/diff/32008/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.h:52: // Returns true on success, in which
case |html_out| will be filled with html
On 2012/02/06 21:29:29, viettrungluu wrote:
> I'd just implement a ReadData() that parallels the C interface.

Done.

https://chromiumcodereview.appspot.com/9212066/diff/32008/ppapi/proxy/ppb_fla...
File ppapi/proxy/ppb_flash_clipboard_proxy.h (right):

https://chromiumcodereview.appspot.com/9212066/diff/32008/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_clipboard_proxy.h:59: void OnMsgReadPlainText(PP_Instance
instance,
On 2012/02/06 21:29:29, viettrungluu wrote:
> I don't think you need these anymore. You should be able to implement
> ReadPlainText() and WritePlainText() in terms of the new primitives.

Done.

[dcheng, 2012-02-07 23:30:19]

https://chromiumcodereview.appspot.com/9212066/diff/32008/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/32008/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:49: client_(0) {
I personally prefer to see NULL instead of 0.

https://chromiumcodereview.appspot.com/9212066/diff/32008/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:66: delete client_;
Nit: Should we store client_ as a scoped_ptr instead?

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

https://chromiumcodereview.appspot.com/9212066/diff/46029/ui/base/clipboard/s...
ui/base/clipboard/scoped_clipboard_writer.h:67: void Clear();
I wonder if we should rename this method. Clear() sounds like it might clear the
clipboard. Unfortunately I don't really have a suggestion for an alternate name.

[raymes, 2012-02-08 19:02:25]

https://chromiumcodereview.appspot.com/9212066/diff/32008/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/32008/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:49: client_(0) {
On 2012/02/07 23:30:19, dcheng wrote:
> I personally prefer to see NULL instead of 0.

Done.

https://chromiumcodereview.appspot.com/9212066/diff/32008/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:66: delete client_;
On 2012/02/07 23:30:19, dcheng wrote:
> Nit: Should we store client_ as a scoped_ptr instead?

Done.

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

https://chromiumcodereview.appspot.com/9212066/diff/46029/ui/base/clipboard/s...
ui/base/clipboard/scoped_clipboard_writer.h:67: void Clear();
On 2012/02/07 23:30:19, dcheng wrote:
> I wonder if we should rename this method. Clear() sounds like it might clear
the
> clipboard. Unfortunately I don't really have a suggestion for an alternate
name.

How about Reset()?

[viettrungluu, 2012-02-08 22:25:01]

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:50: bool Clipboard::ReadData(Instance*
instance,
nit: Move "Instance* instance," to the next line (and align with the others).

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:54: if
(has_interface<PPB_Flash_Clipboard>() ||
This function is just wrong.

I'd prefer, in any case, if you did:

bool rv = false;
if (has_interface<PPB_Flash_Clipboard>()) {
  rv = ...
} else if (has_interface<PPB_Flash_Clipboard_3_0() &&
           clipboard_format == ...) {
  rv = ...
}
return rv;

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:75: if
(has_interface<PPB_Flash_Clipboard>() ||
Again, I'd prefer the above completed-separated way of doing things (it makes
removing old code easier later). This function is also wrong, in any case.

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:93: rv =
(get_interface<PPB_Flash_Clipboard>()->WriteData(
E.g., you'd need to use PPB_Flash_Clipboard_3_0 for the 3.0 case.

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.h (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.h:37: static bool ReadData(Instance* instance,
nit: I'd prefer that "Read" precede "Write". ;)

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
File ppapi/proxy/ppb_flash_clipboard_proxy.cc (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:78:
{PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT, text};
style nit: indentation is off (and I think we want spaces inside the braces, but
I'm not sure about that)

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:139: OnMsgReadData)
nit: alignment

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:191: DCHECK(formats_size == data_size);
This may as well be a CHECK, imo.

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:193: struct
PP_Flash_Clipboard_Data_Item data_items[formats_size];
nit: no struct

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
File ppapi/proxy/ppb_flash_clipboard_proxy.h (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.h:34: virtual PP_Var
ReadPlainText(PP_Instance instance,
Do Read/WritePlainText still need to be in PPB_Flash_Clipboard_FunctionAPI?

http://codereview.chromium.org/9212066/diff/54002/ppapi/tests/test_flash_clip...
File ppapi/tests/test_flash_clipboard.h (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.h:16: static const int kIntervalMs;
no need for these to be in the header

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:99: default:
eh?
I'd do a |break;| for the PP_FLASH_CLIPBOARD_FORMAT_INVALID case, leave out the
|default:|, and put the NOTREACHED(); return PP_FALSE; outside.

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:169: break;
no break

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:172: default: {
This one makes more sense (though the braces are unneeded), but I'd still do
what I said previously. (Leaving out the "default:" means that some compilers
will warn about unhandled cases, if/when more formats are added.)

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:209: default: {
"

[raymes, 2012-02-09 00:20:27]

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:50: bool Clipboard::ReadData(Instance*
instance,
On 2012/02/08 22:25:01, viettrungluu wrote:
> nit: Move "Instance* instance," to the next line (and align with the others).

Done.

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:54: if
(has_interface<PPB_Flash_Clipboard>() ||
On 2012/02/08 22:25:01, viettrungluu wrote:
> This function is just wrong.
> 
> I'd prefer, in any case, if you did:
> 
> bool rv = false;
> if (has_interface<PPB_Flash_Clipboard>()) {
>   rv = ...
> } else if (has_interface<PPB_Flash_Clipboard_3_0() &&
>            clipboard_format == ...) {
>   rv = ...
> }
> return rv;

Umm yes...don't know what I was thinking when I changed this file. Done.

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:75: if
(has_interface<PPB_Flash_Clipboard>() ||
On 2012/02/08 22:25:01, viettrungluu wrote:
> Again, I'd prefer the above completed-separated way of doing things (it makes
> removing old code easier later). This function is also wrong, in any case.

Done.

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:93: rv =
(get_interface<PPB_Flash_Clipboard>()->WriteData(
On 2012/02/08 22:25:01, viettrungluu wrote:
> E.g., you'd need to use PPB_Flash_Clipboard_3_0 for the 3.0 case.

Done.

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.h (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.h:37: static bool ReadData(Instance* instance,
On 2012/02/08 22:25:01, viettrungluu wrote:
> nit: I'd prefer that "Read" precede "Write". ;)

Done.

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
File ppapi/proxy/ppb_flash_clipboard_proxy.cc (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:78:
{PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT, text};
On 2012/02/08 22:25:01, viettrungluu wrote:
> style nit: indentation is off (and I think we want spaces inside the braces,
but
> I'm not sure about that)

Done. Spaces inside the braces are optional according to the style.

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:139: OnMsgReadData)
On 2012/02/08 22:25:01, viettrungluu wrote:
> nit: alignment

Done.

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:191: DCHECK(formats_size == data_size);
On 2012/02/08 22:25:01, viettrungluu wrote:
> This may as well be a CHECK, imo.

Done.

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:193: struct
PP_Flash_Clipboard_Data_Item data_items[formats_size];
On 2012/02/08 22:25:01, viettrungluu wrote:
> nit: no struct

Done.

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
File ppapi/proxy/ppb_flash_clipboard_proxy.h (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.h:34: virtual PP_Var
ReadPlainText(PP_Instance instance,
On 2012/02/08 22:25:01, viettrungluu wrote:
> Do Read/WritePlainText still need to be in PPB_Flash_Clipboard_FunctionAPI?

Nope. I thought they did but you're right. Done.

http://codereview.chromium.org/9212066/diff/54002/ppapi/tests/test_flash_clip...
File ppapi/tests/test_flash_clipboard.h (right):

http://codereview.chromium.org/9212066/diff/54002/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.h:16: static const int kIntervalMs;
On 2012/02/08 22:25:01, viettrungluu wrote:
> no need for these to be in the header

Done.

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:99: default:
On 2012/02/08 22:25:01, viettrungluu wrote:
> eh?
> I'd do a |break;| for the PP_FLASH_CLIPBOARD_FORMAT_INVALID case, leave out
the
> |default:|, and put the NOTREACHED(); return PP_FALSE; outside.

Done.

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:169: break;
On 2012/02/08 22:25:01, viettrungluu wrote:
> no break

Done.

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:172: default: {
Done. The default style was copied from the existing switch statement in
ConvertClipboardFormat(). But you're right that it's safer not to include.

On 2012/02/08 22:25:01, viettrungluu wrote:
> This one makes more sense (though the braces are unneeded), but I'd still do
> what I said previously. (Leaving out the "default:" means that some compilers
> will warn about unhandled cases, if/when more formats are added.)

Done.

http://codereview.chromium.org/9212066/diff/54002/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:209: default: {
On 2012/02/08 22:25:01, viettrungluu wrote:
> "

Done.

[viettrungluu, 2012-02-09 00:47:36]

a few more nits, but almost there

http://codereview.chromium.org/9212066/diff/57001/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/57001/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:58: *out =
get_interface<PPB_Flash_Clipboard>()->ReadData(
nit: indentation

http://codereview.chromium.org/9212066/diff/57001/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:64: else if
(has_interface<PPB_Flash_Clipboard_3_0>() &&
nit (here and elsewhere): the '}' should be on the same line as the 'else'

http://codereview.chromium.org/9212066/diff/57001/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:82:
scoped_array<PP_Flash_Clipboard_Data_Item> data_items_array(
Not necessary; std::vectors are actually stored as arrays.

http://codereview.chromium.org/9212066/diff/57001/ui/base/clipboard/scoped_cl...
File ui/base/clipboard/scoped_clipboard_writer.cc (right):

http://codereview.chromium.org/9212066/diff/57001/ui/base/clipboard/scoped_cl...
ui/base/clipboard/scoped_clipboard_writer.cc:142: void
ScopedClipboardWriter::Reset() {
nit: probably keep the implementations in this file in the same order as in the
header (if they currently are in the same order)

http://codereview.chromium.org/9212066/diff/57001/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

http://codereview.chromium.org/9212066/diff/57001/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:36: break;
no break

http://codereview.chromium.org/9212066/diff/57001/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:145: unsigned fragment_start;
-> uint32, and get rid of the casts

also, look into if we should actually use these

http://codereview.chromium.org/9212066/diff/57001/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:210: // TODO: implement clear()
-> TODO(raymes), to assign responsibility ;)

[raymes, 2012-02-09 17:28:22]

http://codereview.chromium.org/9212066/diff/57001/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/57001/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:58: *out =
get_interface<PPB_Flash_Clipboard>()->ReadData(
On 2012/02/09 00:47:37, viettrungluu wrote:
> nit: indentation

Done.

http://codereview.chromium.org/9212066/diff/57001/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:64: else if
(has_interface<PPB_Flash_Clipboard_3_0>() &&
On 2012/02/09 00:47:37, viettrungluu wrote:
> nit (here and elsewhere): the '}' should be on the same line as the 'else'

Done.

http://codereview.chromium.org/9212066/diff/57001/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:82:
scoped_array<PP_Flash_Clipboard_Data_Item> data_items_array(
On 2012/02/09 00:47:37, viettrungluu wrote:
> Not necessary; std::vectors are actually stored as arrays.

Done.

http://codereview.chromium.org/9212066/diff/57001/ui/base/clipboard/scoped_cl...
File ui/base/clipboard/scoped_clipboard_writer.cc (right):

http://codereview.chromium.org/9212066/diff/57001/ui/base/clipboard/scoped_cl...
ui/base/clipboard/scoped_clipboard_writer.cc:142: void
ScopedClipboardWriter::Reset() {
On 2012/02/09 00:47:37, viettrungluu wrote:
> nit: probably keep the implementations in this file in the same order as in
the
> header (if they currently are in the same order)

Done.

http://codereview.chromium.org/9212066/diff/57001/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

http://codereview.chromium.org/9212066/diff/57001/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:145: unsigned fragment_start;
The method comment says the following:
// Reads HTML from the clipboard, if available. If the HTML fragment requires
  // context to parse, |fragment_start| and |fragment_end| are indexes into
  // markup indicating the beginning and end of the actual fragment. Otherwise,
  // they will contain 0 and markup->size().

In a few of the clipboard implementations, it always returns [0, markup->size()]
so I'm confident that it isn't important.

On 2012/02/09 00:47:37, viettrungluu wrote:
> -> uint32, and get rid of the casts
> 
> also, look into if we should actually use these

http://codereview.chromium.org/9212066/diff/57001/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:210: // TODO: implement clear()
On 2012/02/09 00:47:37, viettrungluu wrote:
> -> TODO(raymes), to assign responsibility ;)

haha. Done.

[viettrungluu, 2012-02-09 18:43:31]

LGTM. You'll need to get OWNERS approvals (and green try runs, probably).

[raymes, 2012-02-22 22:47:14]

+dmichael for ppapi/ approval
+jam for content/renderer and webkit/plugins approval
+dcheng for ui/base/clipboard approval

[dcheng, 2012-02-23 00:27:43]

lgtm

[jam, 2012-02-23 00:58:09]

content lgtm

[dmichael (off chromium), 2012-02-23 18:13:25]

https://chromiumcodereview.appspot.com/9212066/diff/79072/content/renderer/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/79072/content/renderer/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:43: #include
"content/renderer/renderer_clipboard_client.h"
nit: include order

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/c/private/ppb...
File ppapi/c/private/ppb_flash_clipboard.h (right):

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/c/private/ppb...
ppapi/c/private/ppb_flash_clipboard.h:70: struct PP_Flash_Clipboard_Data_Item {
This should probably be PP_FlashClipboardDataItem (though I see this is
consistent with the other names already in here). Might be worth filing a bug or
something to clean up the names, especially if we think this might ever become a
public API.

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/c/private/ppb...
ppapi/c/private/ppb_flash_clipboard.h:97: * <code>PP_Var</code> is returned if
there is an error in reading the
I think returning an undefined Var on error is more consistent with other
places.

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/c/private/ppb...
ppapi/c/private/ppb_flash_clipboard.h:100: struct PP_Var (*ReadData)(PP_Instance
instance_id,
So this requires a synchronous message through the proxy. Would it be practical
to instead use a PP_Var out-param and a completion callback?

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/cpp/private/f...
File ppapi/cpp/private/flash_clipboard.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.cc:51: Instance* instance,
brettw's been working on a change to replace Instance* in the C++ layer with
InstanceHandle. Assuming he lands first, you should adapt to that new style:
http://codereview.chromium.org/9381010/

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.cc:66: clipboard_type);
Do you really need backwards compatibility here? This would only help if you're
building a new Flash plugin on the new API and pushing it to older versions of
Chrome that don't support the old API. It's up to you guys, but if you don't
need it, leave it out.

In any case, as soon as you're not using the old interface anymore, you should
probably get rid of any and all of the backwards compat code for the API.
Backwards compat is a pain in the neck, and we should only do it when we have to
(mostly only public interfaces or to prevent breakages on DEPS rolls). For
private interfaces, we can often avoid it.

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.cc:80: new
PP_Flash_Clipboard_Data_Item[data_items.size()];
What is this line doing? Looks like a blatant leak from an unused expression...

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.cc:85: &data_items[0]) == PP_OK);
probably best to return early if data_items.empty() to avoid indexing out of
bounds here.

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.cc:87: // Take the last plaintext item and
write it according to the API.
I think this backwards-compat part should probably go away. But if I'm wrong
about that, it would probably better to send write them all, wouldn't it? It
also looks like you're writing the first one, not the last one.

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/cpp/private/f...
File ppapi/cpp/private/flash_clipboard.h (right):

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.h:34: PP_Var* out);
why not use pp::Var?

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/cpp/private/f...
ppapi/cpp/private/flash_clipboard.h:40: const
std::vector<PP_Flash_Clipboard_Data_Item>& data);
Similarly, it might be better to use pp::Var so the reference counting is harder
to screw up. That might require separating PP_Flash_Clipboard_Data_Item up in to
two parameters instead of using the struct. It's probably not worth the trouble
to try to wrap PP_Flash_Clipboard_Data_Item in a C++ class.

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/proxy/ppb_fla...
File ppapi/proxy/ppb_flash_clipboard_proxy.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:85: PP_Flash_Clipboard_Data_Item
data_items[]) {
nit: shouldn't this be on line 84?

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:95: formats[i] =
PP_MakeInt32(data_items[i].format);
Can't you just send a vector of int32? Why turn them in to vars?

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
File ppapi/tests/test_flash_clipboard.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
ppapi/tests/test_flash_clipboard.cc:14: // WriteData() causes an async request
sent to the browser process.
nit: request "to be" sent?
Or just say "sends an async request to".

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
ppapi/tests/test_flash_clipboard.cc:35: RUN_TEST(ReadWriteMultipleFormats,
filter);
Don't you also need to change ppapi_uitest.cc to get these to run?

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
ppapi/tests/test_flash_clipboard.cc:57: pp::Var result_var(pp::Var::PassRef(),
style nit: I'd usually put the first param on the next line in this case.

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
ppapi/tests/test_flash_clipboard.cc:61: 
style nit: unnecessary blank line. Blank line on 65 is kind of unnecessary too.

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
ppapi/tests/test_flash_clipboard.cc:64: }
style nit: maybe no {} here to be consistent with 46,47 (and elsewhere...  we
mostly leave them out when they're unnecessary in ppapi code)

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
ppapi/tests/test_flash_clipboard.cc:72: PP_Flash_Clipboard_Data_Item item =
{format, input_var.pp_var()};
style nit: space after { and before }

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
ppapi/tests/test_flash_clipboard.cc:77: &item);
Does WriteData expect to be passed a reference count for the var in item? If so,
I think the reference counting here is wrong, since input_var has 1 ref count
which it will release at the end. So if WriteData is "given" a ref-count, then
we increase it once (input_var constructor) and decrease it twice (input_var
destructor and WriteData when it's finished with it).

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
ppapi/tests/test_flash_clipboard.cc:129: pp::Var html_var(html);
nit: I think you could do each of these in one line, e.g.
pp::Var html_var("html");

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/tests/test_fl...
ppapi/tests/test_flash_clipboard.cc:137: 2,
nit: best practice would probably be to compute the number of elements...  e.g.:
sizeof(items)/sizeof(*items)
(note in regular chromium code you'd use arraysize, but we can't use that here)

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/thunk/ppb_fla...
File ppapi/thunk/ppb_flash_clipboard_api.h (right):

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/thunk/ppb_fla...
ppapi/thunk/ppb_flash_clipboard_api.h:28: PP_Flash_Clipboard_Data_Item
data_items[]) = 0;
style nit: if you can't fit the parameter at this indent level, indent them all
4 spaces. the stuff on line 28 should not be separated from the stuff on 27.

Also...  in C++ code, you don't need to provide "struct" with the type name.

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/thunk/ppb_fla...
File ppapi/thunk/ppb_flash_clipboard_thunk.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/79072/ppapi/thunk/ppb_fla...
ppapi/thunk/ppb_flash_clipboard_thunk.cc:61: const PPB_Flash_Clipboard_3_0
g_ppb_flash_clipboard_thunk_3_0 = {
As noted elsewhere, only provide the backwards compatibility if you really need
it. E.g., if you really need to avoid breaking the flash plugin build when you
roll DEPS. If you can change the code that depends on the API quickly, you might
just leave out the backwards compat since this is a private interface.

https://chromiumcodereview.appspot.com/9212066/diff/79072/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

https://chromiumcodereview.appspot.com/9212066/diff/79072/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:139: return PP_MakeNull();
Does it ever make sense for the clipboard to simply be empty and return an empty
string? If this is an error condition, then I think PP_MakeUndefined is the way
to go. But it's not clear to me that an empty clipboard shouldn't just return an
empty string.

[raymes, 2012-02-24 07:28:28]

David, thanks for the helpful review! I've fixed up the CL. A couple of things
to note:
-I changed the interface such that it simply takes an array of formats and an
array of data items. This simplified the implementation and made sense given
your suggestion about the C++ api.
-According to Trung we unfortunately do need the backward compat code for flash.
Probably best to talk to him for all the details.

http://codereview.chromium.org/9212066/diff/79072/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9212066/diff/79072/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:43: #include
"content/renderer/renderer_clipboard_client.h"
On 2012/02/23 18:13:25, dmichael wrote:
> nit: include order

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/c/private/ppb_flash_c...
File ppapi/c/private/ppb_flash_clipboard.h (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/c/private/ppb_flash_c...
ppapi/c/private/ppb_flash_clipboard.h:70: struct PP_Flash_Clipboard_Data_Item {
I removed this struct. I'll leave the other names as-is for now and can fix them
in a subsequent CL. Happy to file a bug, but the naming scheme is unclear to me,
even by looking at other examples. Some interfaces use pure camel case, while
others use underscores to separate components, e.g. PP_InputEvent_Key. Is it
documented somewhere?

On 2012/02/23 18:13:25, dmichael wrote:
> This should probably be PP_FlashClipboardDataItem (though I see this is
> consistent with the other names already in here). Might be worth filing a bug
or
> something to clean up the names, especially if we think this might ever become
a
> public API.

http://codereview.chromium.org/9212066/diff/79072/ppapi/c/private/ppb_flash_c...
ppapi/c/private/ppb_flash_clipboard.h:97: * <code>PP_Var</code> is returned if
there is an error in reading the
On 2012/02/23 18:13:25, dmichael wrote:
> I think returning an undefined Var on error is more consistent with other
> places.

Done. Undefined for an error and Null for an empty value.

http://codereview.chromium.org/9212066/diff/79072/ppapi/c/private/ppb_flash_c...
ppapi/c/private/ppb_flash_clipboard.h:100: struct PP_Var (*ReadData)(PP_Instance
instance_id,
Spoke to vtl and according to him this is not practical because the flash API
expects a synchronous response. If we were to make this a public API then this
is something we would have to consider.

On 2012/02/23 18:13:25, dmichael wrote:
> So this requires a synchronous message through the proxy. Would it be
practical
> to instead use a PP_Var out-param and a completion callback?

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:51: Instance* instance,
Ok

On 2012/02/23 18:13:25, dmichael wrote:
> brettw's been working on a change to replace Instance* in the C++ layer with
> InstanceHandle. Assuming he lands first, you should adapt to that new style:
> http://codereview.chromium.org/9381010/

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:66: clipboard_type);
Spoke to vtl and he says that the backward compat code is necessary for handling
multiple versions of flash.

On 2012/02/23 18:13:25, dmichael wrote:
> Do you really need backwards compatibility here? This would only help if
you're
> building a new Flash plugin on the new API and pushing it to older versions of
> Chrome that don't support the old API. It's up to you guys, but if you don't
> need it, leave it out.
> 
> In any case, as soon as you're not using the old interface anymore, you should
> probably get rid of any and all of the backwards compat code for the API.
> Backwards compat is a pain in the neck, and we should only do it when we have
to
> (mostly only public interfaces or to prevent breakages on DEPS rolls). For
> private interfaces, we can often avoid it.

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:80: new
PP_Flash_Clipboard_Data_Item[data_items.size()];
On 2012/02/23 18:13:25, dmichael wrote:
> What is this line doing? Looks like a blatant leak from an unused
expression...
=( undeleted line from an older patchset. Good catch.
Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:85: &data_items[0]) == PP_OK);
On 2012/02/23 18:13:25, dmichael wrote:
> probably best to return early if data_items.empty() to avoid indexing out of
> bounds here.

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:87: // Take the last plaintext item and
write it according to the API.
The backward compat code is apparently necessary. Why do you suggest that it
would be better to write all of them? I'm pretty sure it's correctly writing the
last one, not the first. (Iterating from rbegin()). This code changed slightly
in the latest patchset anyhow.

On 2012/02/23 18:13:25, dmichael wrote:
> I think this backwards-compat part should probably go away. But if I'm wrong
> about that, it would probably better to send write them all, wouldn't it? It
> also looks like you're writing the first one, not the last one.

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.h (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.h:34: PP_Var* out);
On 2012/02/23 18:13:25, dmichael wrote:
> why not use pp::Var?

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.h:40: const
std::vector<PP_Flash_Clipboard_Data_Item>& data);
On 2012/02/23 18:13:25, dmichael wrote:
> Similarly, it might be better to use pp::Var so the reference counting is
harder
> to screw up. That might require separating PP_Flash_Clipboard_Data_Item up in
to
> two parameters instead of using the struct. It's probably not worth the
trouble
> to try to wrap PP_Flash_Clipboard_Data_Item in a C++ class.

Thanks, I understand the ref counting better now. Based on this, I decided that
it simplified things a lot to just have 2 arrays everywhere (one for formats and
one for data) so I've updated the CL with this approach.

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/proxy/ppb_flash_clipb...
File ppapi/proxy/ppb_flash_clipboard_proxy.cc (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:85: PP_Flash_Clipboard_Data_Item
data_items[]) {
On 2012/02/23 18:13:25, dmichael wrote:
> nit: shouldn't this be on line 84?

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/proxy/ppb_flash_clipb...
ppapi/proxy/ppb_flash_clipboard_proxy.cc:95: formats[i] =
PP_MakeInt32(data_items[i].format);
Ah I didn't know about this. Done.

On 2012/02/23 18:13:25, dmichael wrote:
> Can't you just send a vector of int32? Why turn them in to vars?

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
File ppapi/tests/test_flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:14: // WriteData() causes an async request
sent to the browser process.
On 2012/02/23 18:13:25, dmichael wrote:
> nit: request "to be" sent?
> Or just say "sends an async request to".

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:35: RUN_TEST(ReadWriteMultipleFormats,
filter);
I don't know about that, they seem to be running already (I've seen them fail on
trybots, for example). ppapi_uitest already contains:
TEST_PPAPI_IN_PROCESS(FlashClipboard)
TEST_PPAPI_OUT_OF_PROCESS(FlashClipboard)

Does something else need to be done?

On 2012/02/23 18:13:25, dmichael wrote:
> Don't you also need to change ppapi_uitest.cc to get these to run?

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:57: pp::Var result_var(pp::Var::PassRef(),
On 2012/02/23 18:13:25, dmichael wrote:
> style nit: I'd usually put the first param on the next line in this case.

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:61: 
On 2012/02/23 18:13:25, dmichael wrote:
> style nit: unnecessary blank line. Blank line on 65 is kind of unnecessary
too.

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:64: }
On 2012/02/23 18:13:25, dmichael wrote:
> style nit: maybe no {} here to be consistent with 46,47 (and elsewhere...  we
> mostly leave them out when they're unnecessary in ppapi code)

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:72: PP_Flash_Clipboard_Data_Item item =
{format, input_var.pp_var()};
On 2012/02/23 18:13:25, dmichael wrote:
> style nit: space after { and before }

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:77: &item);
Not sure what you mean exactly. WriteData finishes immediately and AFAIK does
not decrement the reference count. 

On 2012/02/23 18:13:25, dmichael wrote:
> Does WriteData expect to be passed a reference count for the var in item? If
so,
> I think the reference counting here is wrong, since input_var has 1 ref count
> which it will release at the end. So if WriteData is "given" a ref-count, then
> we increase it once (input_var constructor) and decrease it twice (input_var
> destructor and WriteData when it's finished with it).

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:129: pp::Var html_var(html);
On 2012/02/23 18:13:25, dmichael wrote:
> nit: I think you could do each of these in one line, e.g.
> pp::Var html_var("html");

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:137: 2,
On 2012/02/23 18:13:25, dmichael wrote:
> nit: best practice would probably be to compute the number of elements... 
e.g.:
> sizeof(items)/sizeof(*items)
> (note in regular chromium code you'd use arraysize, but we can't use that
here)

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/thunk/ppb_flash_clipb...
File ppapi/thunk/ppb_flash_clipboard_api.h (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/thunk/ppb_flash_clipb...
ppapi/thunk/ppb_flash_clipboard_api.h:28: PP_Flash_Clipboard_Data_Item
data_items[]) = 0;
On 2012/02/23 18:13:25, dmichael wrote:
> style nit: if you can't fit the parameter at this indent level, indent them
all
> 4 spaces. the stuff on line 28 should not be separated from the stuff on 27.
> 
> Also...  in C++ code, you don't need to provide "struct" with the type name.

Done.

http://codereview.chromium.org/9212066/diff/79072/ppapi/thunk/ppb_flash_clipb...
File ppapi/thunk/ppb_flash_clipboard_thunk.cc (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/thunk/ppb_flash_clipb...
ppapi/thunk/ppb_flash_clipboard_thunk.cc:61: const PPB_Flash_Clipboard_3_0
g_ppb_flash_clipboard_thunk_3_0 = {
It's needed according to vtl.

On 2012/02/23 18:13:25, dmichael wrote:
> As noted elsewhere, only provide the backwards compatibility if you really
need
> it. E.g., if you really need to avoid breaking the flash plugin build when you
> roll DEPS. If you can change the code that depends on the API quickly, you
might
> just leave out the backwards compat since this is a private interface.

http://codereview.chromium.org/9212066/diff/79072/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

http://codereview.chromium.org/9212066/diff/79072/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:139: return PP_MakeNull();
I think an empty clipboard should just return PP_Null as this would naturally
extend to other clipboard formats (e.g. images). The caller can interpret the
null as an empty string if it wishes.

On 2012/02/23 18:13:25, dmichael wrote:
> Does it ever make sense for the clipboard to simply be empty and return an
empty
> string? If this is an error condition, then I think PP_MakeUndefined is the
way
> to go. But it's not clear to me that an empty clipboard shouldn't just return
an
> empty string.

[dmichael (off chromium), 2012-02-24 19:35:44]

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:66: clipboard_type);
On 2012/02/24 07:28:28, raymes wrote:
> Spoke to vtl and he says that the backward compat code is necessary for
handling
> multiple versions of flash.
> 
> On 2012/02/23 18:13:25, dmichael wrote:
> > Do you really need backwards compatibility here? This would only help if
> you're
> > building a new Flash plugin on the new API and pushing it to older versions
of
> > Chrome that don't support the old API. It's up to you guys, but if you don't
> > need it, leave it out.
> > 
> > In any case, as soon as you're not using the old interface anymore, you
should
> > probably get rid of any and all of the backwards compat code for the API.
> > Backwards compat is a pain in the neck, and we should only do it when we
have
> to
> > (mostly only public interfaces or to prevent breakages on DEPS rolls). For
> > private interfaces, we can often avoid it.
> 
Okay, vtl understands what you need for Flash more than I do. But bear in mind,
you're implementing two distinct forms  of compatibility.
1) Making new Chrome support older versions of the plugin.
2) Making new plugins support older versions of Chrome.
In this case, you're doing (2), because this code gets built in to the plugin
itself. I've done this before for a change to PPB_Var, but it turned out to not
really help much because we weren't doing it consistently everywhere in the C++
layer...  IIRC, plugins using the C++ layer and targeting (say) 18 don't work
with Chrome 16 because of other interface changes.

So I'd bet that you want the backwards-compat code that's elsewhere in this CL,
but you *might* not need it here. Again, vtl understands the needs for Flash
better than I do.

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:87: // Take the last plaintext item and
write it according to the API.
On 2012/02/24 07:28:28, raymes wrote:
> The backward compat code is apparently necessary. Why do you suggest that it
> would be better to write all of them?
It might be that I don't understand how the clipboard works well enough. But
imagine you're the client of this C++ wrapper, and you Write 10 things. If your
plugin is running on a newer version of Chrome, all 10 things get written to the
clipboard. If your plugin is running on an older version of Chrome, then only
the last item gets written. It seems to me that it would be less surprising for
the author of the plugin if the behavior was the same in both cases, if we can
make it so.
> I'm pretty sure it's correctly writing the
> last one, not the first. (Iterating from rbegin()). This code changed slightly
> in the latest patchset anyhow.
> 
> On 2012/02/23 18:13:25, dmichael wrote:
> > I think this backwards-compat part should probably go away. But if I'm wrong
> > about that, it would probably better to send write them all, wouldn't it? It
> > also looks like you're writing the first one, not the last one.
> 
Ah, didn't catch it was a reverse iterator. My bad.

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
File ppapi/tests/test_flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:35: RUN_TEST(ReadWriteMultipleFormats,
filter);
On 2012/02/24 07:28:28, raymes wrote:
> I don't know about that, they seem to be running already (I've seen them fail
on
> trybots, for example). ppapi_uitest already contains:
> TEST_PPAPI_IN_PROCESS(FlashClipboard)
> TEST_PPAPI_OUT_OF_PROCESS(FlashClipboard)
> 
> Does something else need to be done?
> 
> On 2012/02/23 18:13:25, dmichael wrote:
> > Don't you also need to change ppapi_uitest.cc to get these to run?
> 
You're right. There are two ways to run tests in ppapi_uitest.cc. You can either
do it this way:
TEST_PPAPI_IN_PROCESS(FlashClipboard)
...which runs all the tests listed in RunTests. Or you can do them individually:
TEST_PPAPI_IN_PROCESS(FlashClipboard_ReadWritePlainText)
TEST_PPAPI_IN_PROCESS(FlashClipboard_ReadWriteHTML)
TEST_PPAPI_IN_PROCESS(FlashClipboard_ReadWriteMultipleFormats)

The latter has the advantage that you can enable/disable individual tests. I'd
like to eventually get all the tests running that way, but I haven't been
pushing for it since it makes the test suite slower overall. What you have is
fine.

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:77: &item);
On 2012/02/24 07:28:28, raymes wrote:
> Not sure what you mean exactly. WriteData finishes immediately and AFAIK does
> not decrement the reference count. 
> 
> On 2012/02/23 18:13:25, dmichael wrote:
> > Does WriteData expect to be passed a reference count for the var in item? If
> so,
> > I think the reference counting here is wrong, since input_var has 1 ref
count
> > which it will release at the end. So if WriteData is "given" a ref-count,
then
> > we increase it once (input_var constructor) and decrease it twice (input_var
> > destructor and WriteData when it's finished with it).
> 
I guess I wanted to make sure you had thought about how the reference counting
worked for your API, but I do think it's right. Sorry for muddying the waters.

http://codereview.chromium.org/9212066/diff/79072/webkit/plugins/ppapi/ppb_fl...
File webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc (right):

http://codereview.chromium.org/9212066/diff/79072/webkit/plugins/ppapi/ppb_fl...
webkit/plugins/ppapi/ppb_flash_clipboard_impl.cc:139: return PP_MakeNull();
On 2012/02/24 07:28:28, raymes wrote:
> I think an empty clipboard should just return PP_Null as this would naturally
> extend to other clipboard formats (e.g. images). The caller can interpret the
> null as an empty string if it wishes.
> 
> On 2012/02/23 18:13:25, dmichael wrote:
> > Does it ever make sense for the clipboard to simply be empty and return an
> empty
> > string? If this is an error condition, then I think PP_MakeUndefined is the
> way
> > to go. But it's not clear to me that an empty clipboard shouldn't just
return
> an
> > empty string.
> 
Makes sense, thanks for explaining.

http://codereview.chromium.org/9212066/diff/89008/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9212066/diff/89008/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:38: #include
"content/renderer/renderer_clipboard_client.h"
I think it should go somewhere down around 51? gcl lint or something should tell
you. In any case, it should probably be consistent with the other things that
start "content/renderer/render...".

http://codereview.chromium.org/9212066/diff/89008/ppapi/tests/test_flash_clip...
File ppapi/tests/test_flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/89008/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:127: PP_FLASH_CLIPBOARD_FORMAT_HTML };
style nits: I would probably break the line just after "=". 127 should be
4-space indented, so I think you can get both items on line 127.

[raymes, 2012-02-24 21:38:16]

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:66: clipboard_type);
According to vtl, both are needed.
On 2012/02/24 19:35:44, dmichael wrote:
> On 2012/02/24 07:28:28, raymes wrote:
> > Spoke to vtl and he says that the backward compat code is necessary for
> handling
> > multiple versions of flash.
> > 
> > On 2012/02/23 18:13:25, dmichael wrote:
> > > Do you really need backwards compatibility here? This would only help if
> > you're
> > > building a new Flash plugin on the new API and pushing it to older
versions
> of
> > > Chrome that don't support the old API. It's up to you guys, but if you
don't
> > > need it, leave it out.
> > > 
> > > In any case, as soon as you're not using the old interface anymore, you
> should
> > > probably get rid of any and all of the backwards compat code for the API.
> > > Backwards compat is a pain in the neck, and we should only do it when we
> have
> > to
> > > (mostly only public interfaces or to prevent breakages on DEPS rolls). For
> > > private interfaces, we can often avoid it.
> > 
> Okay, vtl understands what you need for Flash more than I do. But bear in
mind,
> you're implementing two distinct forms  of compatibility.
> 1) Making new Chrome support older versions of the plugin.
> 2) Making new plugins support older versions of Chrome.
> In this case, you're doing (2), because this code gets built in to the plugin
> itself. I've done this before for a change to PPB_Var, but it turned out to
not
> really help much because we weren't doing it consistently everywhere in the
C++
> layer...  IIRC, plugins using the C++ layer and targeting (say) 18 don't work
> with Chrome 16 because of other interface changes.
> 
> So I'd bet that you want the backwards-compat code that's elsewhere in this
CL,
> but you *might* not need it here. Again, vtl understands the needs for Flash
> better than I do.

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:87: // Take the last plaintext item and
write it according to the API.
Ah I see what you're saying. The thing is that each time an item of the same
format is written, the prior item is overwritten. We are only writing plain text
items here, so writing the last has the same effect as writing them all.

On 2012/02/24 19:35:44, dmichael wrote:
> On 2012/02/24 07:28:28, raymes wrote:
> > The backward compat code is apparently necessary. Why do you suggest that it
> > would be better to write all of them?
> It might be that I don't understand how the clipboard works well enough. But
> imagine you're the client of this C++ wrapper, and you Write 10 things. If
your
> plugin is running on a newer version of Chrome, all 10 things get written to
the
> clipboard. If your plugin is running on an older version of Chrome, then only
> the last item gets written. It seems to me that it would be less surprising
for
> the author of the plugin if the behavior was the same in both cases, if we can
> make it so.
> > I'm pretty sure it's correctly writing the
> > last one, not the first. (Iterating from rbegin()). This code changed
slightly
> > in the latest patchset anyhow.
> > 
> > On 2012/02/23 18:13:25, dmichael wrote:
> > > I think this backwards-compat part should probably go away. But if I'm
wrong
> > > about that, it would probably better to send write them all, wouldn't it?
It
> > > also looks like you're writing the first one, not the last one.
> > 
> Ah, didn't catch it was a reverse iterator. My bad.

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
File ppapi/tests/test_flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:35: RUN_TEST(ReadWriteMultipleFormats,
filter);
Ah I see, thanks for the explanation.

On 2012/02/24 19:35:44, dmichael wrote:
> On 2012/02/24 07:28:28, raymes wrote:
> > I don't know about that, they seem to be running already (I've seen them
fail
> on
> > trybots, for example). ppapi_uitest already contains:
> > TEST_PPAPI_IN_PROCESS(FlashClipboard)
> > TEST_PPAPI_OUT_OF_PROCESS(FlashClipboard)
> > 
> > Does something else need to be done?
> > 
> > On 2012/02/23 18:13:25, dmichael wrote:
> > > Don't you also need to change ppapi_uitest.cc to get these to run?
> > 
> You're right. There are two ways to run tests in ppapi_uitest.cc. You can
either
> do it this way:
> TEST_PPAPI_IN_PROCESS(FlashClipboard)
> ...which runs all the tests listed in RunTests. Or you can do them
individually:
> TEST_PPAPI_IN_PROCESS(FlashClipboard_ReadWritePlainText)
> TEST_PPAPI_IN_PROCESS(FlashClipboard_ReadWriteHTML)
> TEST_PPAPI_IN_PROCESS(FlashClipboard_ReadWriteMultipleFormats)
> 
> The latter has the advantage that you can enable/disable individual tests. I'd
> like to eventually get all the tests running that way, but I haven't been
> pushing for it since it makes the test suite slower overall. What you have is
> fine.

http://codereview.chromium.org/9212066/diff/89008/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9212066/diff/89008/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:38: #include
"content/renderer/renderer_clipboard_client.h"
Hmm, cpplint.py didn't complain but I fixed this.

On 2012/02/24 19:35:44, dmichael wrote:
> I think it should go somewhere down around 51? gcl lint or something should
tell
> you. In any case, it should probably be consistent with the other things that
> start "content/renderer/render...".

http://codereview.chromium.org/9212066/diff/89008/ppapi/tests/test_flash_clip...
File ppapi/tests/test_flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/89008/ppapi/tests/test_flash_clip...
ppapi/tests/test_flash_clipboard.cc:127: PP_FLASH_CLIPBOARD_FORMAT_HTML };
On 2012/02/24 19:35:44, dmichael wrote:
> style nits: I would probably break the line just after "=". 127 should be
> 4-space indented, so I think you can get both items on line 127.

Done.

[dmichael (off chromium), 2012-02-24 21:49:36]

lgtm

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/9212066/diff/79072/ppapi/cpp/private/flash_cli...
ppapi/cpp/private/flash_clipboard.cc:87: // Take the last plaintext item and
write it according to the API.
On 2012/02/24 21:38:17, raymes wrote:
> Ah I see what you're saying. The thing is that each time an item of the same
> format is written, the prior item is overwritten. We are only writing plain
text
> items here, so writing the last has the same effect as writing them all.
That makes sense, thanks for explaining. I think a comment here to that effect
would be helpful.
> 
> On 2012/02/24 19:35:44, dmichael wrote:
> > On 2012/02/24 07:28:28, raymes wrote:
> > > The backward compat code is apparently necessary. Why do you suggest that
it
> > > would be better to write all of them?
> > It might be that I don't understand how the clipboard works well enough. But
> > imagine you're the client of this C++ wrapper, and you Write 10 things. If
> your
> > plugin is running on a newer version of Chrome, all 10 things get written to
> the
> > clipboard. If your plugin is running on an older version of Chrome, then
only
> > the last item gets written. It seems to me that it would be less surprising
> for
> > the author of the plugin if the behavior was the same in both cases, if we
can
> > make it so.
> > > I'm pretty sure it's correctly writing the
> > > last one, not the first. (Iterating from rbegin()). This code changed
> slightly
> > > in the latest patchset anyhow.
> > > 
> > > On 2012/02/23 18:13:25, dmichael wrote:
> > > > I think this backwards-compat part should probably go away. But if I'm
> wrong
> > > > about that, it would probably better to send write them all, wouldn't
it?
> It
> > > > also looks like you're writing the first one, not the last one.
> > > 
> > Ah, didn't catch it was a reverse iterator. My bad.
>

[commit-bot: I haz the power, 2012-02-24 21:54:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9212066/103001

[commit-bot: I haz the power, 2012-02-24 23:15:03]

Change committed as 123581