Move flash clipboard to the new proxy and add custom format support

Move flash clipboard to the new proxy and add custom format support

This moves the flash clipboard interface to the new pepper proxy. It also adds support for copying/pasting data of custom data formats (which is needed for flash). 


BUG=154497
TEST=Added new pepper tests


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

[raymes, 2012-10-24 18:26:56]

This looks scary but most of it is just a few lines changed in each file.


dcheng PTAL at the following:
Mostly simple changes to the chrome clipboard code:
webkit/tools/test_shell/simple_clipboard_impl.*
webkit/glue/scoped_clipboard_writer_glue.*
webkit/glue/clipboard_client.h
ui/base/clipboard/scoped_clipboard_writer.*
content/renderer/renderer_clipboard_client.*
content/common/clipboard_messages.h 
content/browser/renderer_host/clipboard_message_filter.*

Also how we generate the format name in:
ppapi/proxy/flash_clipboard_format_registry.cc

You can also (if you like) take a look at the public interface in:
ppapi/api/private/ppb_flash_clipboard.idl 

And how the chrome interface is used in:
content/renderer/pepper/pepper_flash_clipboard_host.cc

yzshen for the rest.

[raymes, 2012-10-25 22:59:34]

Updated, PTAL. Note that I couldn't use the custom_data_helper.* code directly
because it deals with string16s rather than strings and fixing this requires
some moderate refactoring work. I've noted this in TODOs in
pepper_flash_clipboard_host.cc. If you like a can file a bug to track this.

[yzshen1, 2012-10-29 16:55:46]

http://codereview.chromium.org/11225021/diff/42057/ppapi/host/ppapi_host.cc
File ppapi/host/ppapi_host.cc (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/host/ppapi_host.cc#n...
ppapi/host/ppapi_host.cc:146: << "'Post' message handler failed to complete
successfully.";
wrong indent.

http://codereview.chromium.org/11225021/diff/42057/ppapi/ppapi_proxy.gypi
File ppapi/ppapi_proxy.gypi (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/ppapi_proxy.gypi#new...
ppapi/ppapi_proxy.gypi:35: 'proxy/flash_clipboard_format_registry.cc',
sort, please.

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
File ppapi/proxy/flash_clipboard_format_registry.cc (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_format_registry.cc:15: const
PP_Flash_Clipboard_Format kPredefinedFormats[] = {
Is it better to define something like PP_FLASH_CLIPBOARD_FORMAT_COUNT in
PP_Flash_Clipboard_Format enum?

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
File ppapi/proxy/flash_clipboard_format_registry.h (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_format_registry.h:13: #include
"ppapi/c/private/ppb_flash_clipboard.h"
sort, please.

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_format_registry.h:16: namespace proxy {
Shall we put this in shared_impl?

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_format_registry.h:26: virtual
~FlashClipboardFormatRegistry();
Don't make it virtual if this is not a base class with virtual methods.

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_format_registry.h:61: // structure will be reserved
for predefined formats.
Does it make sense to let index + predefined_format_number =  custom_format_id?
Then we don't need to have some empty entries in the custom_formats_ vector.

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
File ppapi/proxy/flash_clipboard_resource.cc (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_resource.cc:9: #include
"ppapi/proxy/dispatch_reply_message.h"
Do you need this?

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_resource.cc:11: #include
"ppapi/shared_impl/array_writer.h"
I don't think you need it.

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_resource.cc:14: #include "ppapi/thunk/enter.h"
Do you need this?

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_resource.cc:22: bool PP_VarToClipboardString(int32_t
format,
nit: better to follow the style of "StringVar::FromPPVar" to remove the '_' in
the name. (Same for the next function definition.)

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
File ppapi/proxy/flash_clipboard_resource.h (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_resource.h:8: #include <set>
You don't need 'set' here.

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_resource.h:13: #include
"ppapi/proxy/ppapi_proxy_export.h"
I think we don't need to export it, right?

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_resource.h:16: struct PP_ArrayOutput;
You don't need this forward declaration.

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboar...
ppapi/proxy/flash_clipboard_resource.h:31: // PPB_Flash_Clipboard_API overrides.
nit: use 'implementation' instead of 'override' here.

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/plugin_dispatc...
File ppapi/proxy/plugin_dispatcher.h (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/plugin_dispatc...
ppapi/proxy/plugin_dispatcher.h:38: class FlashClipboardResource;
sort.

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1723: // Flash clipboard.
nit, optional: sort according to the name (Flash clipboard, flash font file,
flash functions)

[yzshen1, 2012-10-29 17:58:04]

I reviewed all .*ppapi.* and .*pepper.* files.

http://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_clipboard_host.cc (right):

http://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.cc:21: using
ppapi::thunk::PPB_Flash_Clipboard_API;
you don't need this.

http://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.cc:46: // by changing that
code (and it's uses) to read/write custom data as arbitrary
and *its* uses?

http://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.cc:77: return result;
return empty string?

http://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.cc:101:
clipboard_client_.reset(new RendererClipboardClient);
why not doing it in the initialization list?

http://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_clipboard_host.h (right):

http://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.h:40: : public
ppapi::host::ResourceHost {
no need to be on a new line.

http://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (left):

http://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.h:397: virtual
webkit_glue::ClipboardClient* CreateClipboardClient() const OVERRIDE;
change mock_plugin_delegate accordingly, please.

http://codereview.chromium.org/11225021/diff/42057/ppapi/c/private/ppb_flash_...
File ppapi/c/private/ppb_flash_clipboard.h (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/c/private/ppb_flash_...
ppapi/c/private/ppb_flash_clipboard.h:83: * <code>str_t</code>. An id
identifying the format will be returned if the
better to use 'string' instead of '<code>str_t</code>' here, because the c
interface has translated 'str_t' to 'const char*'.

http://codereview.chromium.org/11225021/diff/42057/ppapi/cpp/private/flash_cl...
File ppapi/cpp/private/flash_clipboard.cc (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/cpp/private/flash_cl...
ppapi/cpp/private/flash_clipboard.cc:43: instance.pp_instance(),
format_name.c_str());
wrong indent. (You worked on Flapper code for too long. ;))

http://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cli...
File ppapi/tests/test_flash_clipboard.cc (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cli...
ppapi/tests/test_flash_clipboard.cc:169: char *bytes =
static_cast<char*>(array_buffer.Map());
nit: char* instead of "char *". (and line 194)

http://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cli...
ppapi/tests/test_flash_clipboard.cc:170: std::copy(custom_data.data(),
custom_data.data() + custom_data.size(), bytes);
Why not using custom_data.begin() and custom_data.end()?

http://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cli...
ppapi/tests/test_flash_clipboard.cc:187: instance_,
wrong indent.

http://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cli...
ppapi/tests/test_flash_clipboard.cc:253:
pp::flash::Clipboard::RegisterCustomFormat(instance_, "");
- wrong indent.
- why don't you verify the resulting |format_id|?

http://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/interfaces_ppb...
File ppapi/thunk/interfaces_ppb_private_flash.h (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/interfaces_ppb...
ppapi/thunk/interfaces_ppb_private_flash.h:42: PROXIED_IFACE(PPB_Flash,
I thought it should be 'NoAPIName'?

http://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_clip...
File ppapi/thunk/ppb_flash_clipboard_api.h (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_clip...
ppapi/thunk/ppb_flash_clipboard_api.h:14: class PPAPI_THUNK_EXPORT
PPB_Flash_Clipboard_API {
Do you need to export it?

http://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_clip...
ppapi/thunk/ppb_flash_clipboard_api.h:20: PP_Instance instance,
wrong indent. (The whole class)

http://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_clip...
File ppapi/thunk/ppb_flash_clipboard_thunk.cc (right):

http://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_clip...
ppapi/thunk/ppb_flash_clipboard_thunk.cc:9: #include "ppapi/thunk/thunk.h"
sort.

http://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_clip...
ppapi/thunk/ppb_flash_clipboard_thunk.cc:76: scoped_array<uint32_t>
new_formats(new uint32_t[data_item_count]);
[Haven't tried myself]
will new uint32_t[0] run correctly?

http://codereview.chromium.org/11225021/diff/42057/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/11225021/diff/42057/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:405: PP_Instance instance)
OVERRIDE;
Wrong indent!

[raymes, 2012-10-29 18:44:56]

Thanks.

dcheng, PTAL when you have a chance.

https://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/p...
File content/renderer/pepper/pepper_flash_clipboard_host.cc (right):

https://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.cc:21: using
ppapi::thunk::PPB_Flash_Clipboard_API;
On 2012/10/29 17:58:04, yzshen1 wrote:
> you don't need this.

Done.

https://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.cc:46: // by changing that
code (and it's uses) to read/write custom data as arbitrary
On 2012/10/29 17:58:04, yzshen1 wrote:
> and *its* uses?
See I told you your english is better than mine =)

https://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.cc:77: return result;
On 2012/10/29 17:58:04, yzshen1 wrote:
> return empty string?

Done.

https://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.cc:101:
clipboard_client_.reset(new RendererClipboardClient);
On 2012/10/29 17:58:04, yzshen1 wrote:
> why not doing it in the initialization list?

Done.

https://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/p...
File content/renderer/pepper/pepper_flash_clipboard_host.h (right):

https://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.h:40: : public
ppapi::host::ResourceHost {
On 2012/10/29 17:58:04, yzshen1 wrote:
> no need to be on a new line.

Done.

https://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/p...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (left):

https://codereview.chromium.org/11225021/diff/42057/content/renderer/pepper/p...
content/renderer/pepper/pepper_plugin_delegate_impl.h:397: virtual
webkit_glue::ClipboardClient* CreateClipboardClient() const OVERRIDE;
On 2012/10/29 17:58:04, yzshen1 wrote:
> change mock_plugin_delegate accordingly, please.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/c/private/ppb_flash...
File ppapi/c/private/ppb_flash_clipboard.h (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/c/private/ppb_flash...
ppapi/c/private/ppb_flash_clipboard.h:83: * <code>str_t</code>. An id
identifying the format will be returned if the
On 2012/10/29 17:58:04, yzshen1 wrote:
> better to use 'string' instead of '<code>str_t</code>' here, because the c
> interface has translated 'str_t' to 'const char*'.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/cpp/private/flash_c...
File ppapi/cpp/private/flash_clipboard.cc (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/cpp/private/flash_c...
ppapi/cpp/private/flash_clipboard.cc:43: instance.pp_instance(),
format_name.c_str());
On 2012/10/29 17:58:04, yzshen1 wrote:
> wrong indent. (You worked on Flapper code for too long. ;))

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/host/ppapi_host.cc
File ppapi/host/ppapi_host.cc (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/host/ppapi_host.cc#...
ppapi/host/ppapi_host.cc:146: << "'Post' message handler failed to complete
successfully.";
On 2012/10/29 16:55:46, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/ppapi_proxy.gypi
File ppapi/ppapi_proxy.gypi (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/ppapi_proxy.gypi#ne...
ppapi/ppapi_proxy.gypi:35: 'proxy/flash_clipboard_format_registry.cc',
On 2012/10/29 16:55:46, yzshen1 wrote:
> sort, please.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
File ppapi/proxy/flash_clipboard_format_registry.cc (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_format_registry.cc:15: const
PP_Flash_Clipboard_Format kPredefinedFormats[] = {
On 2012/10/29 16:55:46, yzshen1 wrote:
> Is it better to define something like PP_FLASH_CLIPBOARD_FORMAT_COUNT in
> PP_Flash_Clipboard_Format enum?

As discussed, this could have problems if more values were added to the enum.
Tests should catch the case when this list does not match the enum.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
File ppapi/proxy/flash_clipboard_format_registry.h (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_format_registry.h:13: #include
"ppapi/c/private/ppb_flash_clipboard.h"
On 2012/10/29 16:55:46, yzshen1 wrote:
> sort, please.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_format_registry.h:16: namespace proxy {
On 2012/10/29 16:55:46, yzshen1 wrote:
> Shall we put this in shared_impl?

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_format_registry.h:26: virtual
~FlashClipboardFormatRegistry();
On 2012/10/29 16:55:46, yzshen1 wrote:
> Don't make it virtual if this is not a base class with virtual methods.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_format_registry.h:61: // structure will be reserved
for predefined formats.
On 2012/10/29 16:55:46, yzshen1 wrote:
> Does it make sense to let index + predefined_format_number = 
custom_format_id?
> Then we don't need to have some empty entries in the custom_formats_ vector.

This just simplifies the code. As discussed it is easy to change later if we
want but right now the overhead is low so I'll keep the simpler code.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
File ppapi/proxy/flash_clipboard_resource.cc (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_resource.cc:9: #include
"ppapi/proxy/dispatch_reply_message.h"
On 2012/10/29 16:55:46, yzshen1 wrote:
> Do you need this?

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_resource.cc:11: #include
"ppapi/shared_impl/array_writer.h"
On 2012/10/29 16:55:46, yzshen1 wrote:
> I don't think you need it.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_resource.cc:14: #include "ppapi/thunk/enter.h"
On 2012/10/29 16:55:46, yzshen1 wrote:
> Do you need this?

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_resource.cc:22: bool PP_VarToClipboardString(int32_t
format,
On 2012/10/29 16:55:46, yzshen1 wrote:
> nit: better to follow the style of "StringVar::FromPPVar" to remove the '_' in
> the name. (Same for the next function definition.)

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
File ppapi/proxy/flash_clipboard_resource.h (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_resource.h:8: #include <set>
On 2012/10/29 16:55:46, yzshen1 wrote:
> You don't need 'set' here.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_resource.h:13: #include
"ppapi/proxy/ppapi_proxy_export.h"
On 2012/10/29 16:55:46, yzshen1 wrote:
> I think we don't need to export it, right?

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_resource.h:16: struct PP_ArrayOutput;
On 2012/10/29 16:55:46, yzshen1 wrote:
> You don't need this forward declaration.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/flash_clipboa...
ppapi/proxy/flash_clipboard_resource.h:31: // PPB_Flash_Clipboard_API overrides.
On 2012/10/29 16:55:46, yzshen1 wrote:
> nit: use 'implementation' instead of 'override' here.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/plugin_dispat...
File ppapi/proxy/plugin_dispatcher.h (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/plugin_dispat...
ppapi/proxy/plugin_dispatcher.h:38: class FlashClipboardResource;
On 2012/10/29 16:55:46, yzshen1 wrote:
> sort.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1723: // Flash clipboard.
On 2012/10/29 16:55:46, yzshen1 wrote:
> nit, optional: sort according to the name (Flash clipboard, flash font file,
> flash functions)

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cl...
File ppapi/tests/test_flash_clipboard.cc (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cl...
ppapi/tests/test_flash_clipboard.cc:169: char *bytes =
static_cast<char*>(array_buffer.Map());
On 2012/10/29 17:58:04, yzshen1 wrote:
> nit: char* instead of "char *". (and line 194)

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cl...
ppapi/tests/test_flash_clipboard.cc:170: std::copy(custom_data.data(),
custom_data.data() + custom_data.size(), bytes);
On 2012/10/29 17:58:04, yzshen1 wrote:
> Why not using custom_data.begin() and custom_data.end()?

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cl...
ppapi/tests/test_flash_clipboard.cc:187: instance_,
On 2012/10/29 17:58:04, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/tests/test_flash_cl...
ppapi/tests/test_flash_clipboard.cc:253:
pp::flash::Clipboard::RegisterCustomFormat(instance_, "");
On 2012/10/29 17:58:04, yzshen1 wrote:
> - wrong indent.
> - why don't you verify the resulting |format_id|?

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/interfaces_pp...
File ppapi/thunk/interfaces_ppb_private_flash.h (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/interfaces_pp...
ppapi/thunk/interfaces_ppb_private_flash.h:42: PROXIED_IFACE(PPB_Flash,
On 2012/10/29 17:58:04, yzshen1 wrote:
> I thought it should be 'NoAPIName'?

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_cli...
File ppapi/thunk/ppb_flash_clipboard_api.h (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_cli...
ppapi/thunk/ppb_flash_clipboard_api.h:14: class PPAPI_THUNK_EXPORT
PPB_Flash_Clipboard_API {
On 2012/10/29 17:58:04, yzshen1 wrote:
> Do you need to export it?

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_cli...
ppapi/thunk/ppb_flash_clipboard_api.h:20: PP_Instance instance,
On 2012/10/29 17:58:04, yzshen1 wrote:
> wrong indent. (The whole class)

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_cli...
File ppapi/thunk/ppb_flash_clipboard_thunk.cc (right):

https://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_cli...
ppapi/thunk/ppb_flash_clipboard_thunk.cc:9: #include "ppapi/thunk/thunk.h"
On 2012/10/29 17:58:04, yzshen1 wrote:
> sort.

Done.

https://codereview.chromium.org/11225021/diff/42057/ppapi/thunk/ppb_flash_cli...
ppapi/thunk/ppb_flash_clipboard_thunk.cc:76: scoped_array<uint32_t>
new_formats(new uint32_t[data_item_count]);
On 2012/10/29 17:58:04, yzshen1 wrote:
> [Haven't tried myself]
> will new uint32_t[0] run correctly?

Yes, but we can't dereference the pointer. If the size is 0 then we wouldn't do
that.

https://codereview.chromium.org/11225021/diff/42057/webkit/plugins/ppapi/ppap...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

https://codereview.chromium.org/11225021/diff/42057/webkit/plugins/ppapi/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.h:405: PP_Instance instance)
OVERRIDE;
On 2012/10/29 17:58:04, yzshen1 wrote:
> Wrong indent!

Done.

[yzshen1, 2012-10-29 18:57:33]

LGTM for the pepper changes, with a few nits.

http://codereview.chromium.org/11225021/diff/50003/ppapi/api/private/ppb_flas...
File ppapi/api/private/ppb_flash_clipboard.idl (right):

http://codereview.chromium.org/11225021/diff/50003/ppapi/api/private/ppb_flas...
ppapi/api/private/ppb_flash_clipboard.idl:87: * string. An id identifying the
format will be returned if the
nit: adjust the text so that every line is as close to 80 chars as possible.
(There is some script in VS can do that; you might be able to find/write similar
things for your editor.)

http://codereview.chromium.org/11225021/diff/50003/ppapi/tests/test_flash_cli...
File ppapi/tests/test_flash_clipboard.cc (right):

http://codereview.chromium.org/11225021/diff/50003/ppapi/tests/test_flash_cli...
ppapi/tests/test_flash_clipboard.cc:187: instance_,
wrong indent.

http://codereview.chromium.org/11225021/diff/50003/ppapi/tests/test_flash_cli...
ppapi/tests/test_flash_clipboard.cc:253:
pp::flash::Clipboard::RegisterCustomFormat(instance_, "");
wrong indent.

http://codereview.chromium.org/11225021/diff/50003/ppapi/thunk/ppb_flash_clip...
File ppapi/thunk/ppb_flash_clipboard_api.h (right):

http://codereview.chromium.org/11225021/diff/50003/ppapi/thunk/ppb_flash_clip...
ppapi/thunk/ppb_flash_clipboard_api.h:19: PP_Instance instance,
still wrong indent. (the whole class.)

http://codereview.chromium.org/11225021/diff/50003/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/11225021/diff/50003/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:411: PP_Instance instance)
OVERRIDE;
wrong indent.

[raymes, 2012-10-29 20:04:48]

https://codereview.chromium.org/11225021/diff/50003/ppapi/api/private/ppb_fla...
File ppapi/api/private/ppb_flash_clipboard.idl (right):

https://codereview.chromium.org/11225021/diff/50003/ppapi/api/private/ppb_fla...
ppapi/api/private/ppb_flash_clipboard.idl:87: * string. An id identifying the
format will be returned if the
On 2012/10/29 18:57:33, yzshen1 wrote:
> nit: adjust the text so that every line is as close to 80 chars as possible.
> (There is some script in VS can do that; you might be able to find/write
similar
> things for your editor.)

Done.

https://codereview.chromium.org/11225021/diff/50003/ppapi/tests/test_flash_cl...
File ppapi/tests/test_flash_clipboard.cc (right):

https://codereview.chromium.org/11225021/diff/50003/ppapi/tests/test_flash_cl...
ppapi/tests/test_flash_clipboard.cc:187: instance_,
On 2012/10/29 18:57:33, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/11225021/diff/50003/ppapi/tests/test_flash_cl...
ppapi/tests/test_flash_clipboard.cc:253:
pp::flash::Clipboard::RegisterCustomFormat(instance_, "");
On 2012/10/29 18:57:33, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/11225021/diff/50003/ppapi/thunk/ppb_flash_cli...
File ppapi/thunk/ppb_flash_clipboard_api.h (right):

https://codereview.chromium.org/11225021/diff/50003/ppapi/thunk/ppb_flash_cli...
ppapi/thunk/ppb_flash_clipboard_api.h:19: PP_Instance instance,
On 2012/10/29 18:57:33, yzshen1 wrote:
> still wrong indent. (the whole class.)

Done.

https://codereview.chromium.org/11225021/diff/50003/webkit/plugins/ppapi/ppap...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

https://codereview.chromium.org/11225021/diff/50003/webkit/plugins/ppapi/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.h:411: PP_Instance instance)
OVERRIDE;
On 2012/10/29 18:57:33, yzshen1 wrote:
> wrong indent.

Done.

[dcheng, 2012-10-29 20:26:29]

http://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_clipboard_host.cc (right):

http://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.cc:41: // TODO(raymes): This
code is very similar to the code in
We should probably address this sooner rather than later. This code expects
valid UTF-8, while the custom data code assumes UTF-16 right? If we mix and
match, there would be a lot of unexpected results.

It'd be better if we just decided the serialization format should always be
UTF-8 or UTF-16 and convert where necessary (whether that's here or in the
ui/base/clipboard code)

http://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.cc:75: return "";
std::string() is preferred over "".

http://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_clipboard_host.h (right):

http://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.h:78: 
Nit: extra newline.

http://codereview.chromium.org/11225021/diff/49013/content/renderer/renderer_...
File content/renderer/renderer_clipboard_client.h (right):

http://codereview.chromium.org/11225021/diff/49013/content/renderer/renderer_...
content/renderer/renderer_clipboard_client.h:8: #include <string>
How come we need this #include? Shouldn't it be included through the interface
that it's overriding?

http://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_cl...
File ppapi/shared_impl/flash_clipboard_format_registry.cc (right):

http://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:24:
sizeof(kPredefinedFormats) / sizeof(kPredefinedFormats[0]);
You can just use the arraysize macro in base/basictypes.h for this.

http://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_cl...
File ppapi/shared_impl/flash_clipboard_format_registry.h (right):

http://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.h:21: class
PPAPI_SHARED_EXPORT FlashClipboardFormatRegistry {
With the custom web data changes, do you still need this? I don't see any
references to registering a custom format in ActionScript (I'm looking at
http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/desk...).
Could we just use plain old strings instead of requiring custom formats to be
registered?

http://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.h:60: std::vector<std::string>
custom_formats_;
Should this class be copyable/assignable?

[raymes, 2012-10-29 23:30:00]

https://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/p...
File content/renderer/pepper/pepper_flash_clipboard_host.cc (right):

https://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.cc:41: // TODO(raymes): This
code is very similar to the code in
I filed a bug and added a TODO. As discussed, I've used a different native
format for now.

On 2012/10/29 20:26:29, dcheng wrote:
> We should probably address this sooner rather than later. This code expects
> valid UTF-8, while the custom data code assumes UTF-16 right? If we mix and
> match, there would be a lot of unexpected results.
> 
> It'd be better if we just decided the serialization format should always be
> UTF-8 or UTF-16 and convert where necessary (whether that's here or in the
> ui/base/clipboard code)

https://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.cc:75: return "";
On 2012/10/29 20:26:29, dcheng wrote:
> std::string() is preferred over "".

Done.

https://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/p...
File content/renderer/pepper/pepper_flash_clipboard_host.h (right):

https://codereview.chromium.org/11225021/diff/49013/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.h:78: 
On 2012/10/29 20:26:29, dcheng wrote:
> Nit: extra newline.

Done.

https://codereview.chromium.org/11225021/diff/49013/content/renderer/renderer...
File content/renderer/renderer_clipboard_client.h (right):

https://codereview.chromium.org/11225021/diff/49013/content/renderer/renderer...
content/renderer/renderer_clipboard_client.h:8: #include <string>
On 2012/10/29 20:26:29, dcheng wrote:
> How come we need this #include? Shouldn't it be included through the interface
> that it's overriding?

Done.

https://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_c...
File ppapi/shared_impl/flash_clipboard_format_registry.cc (right):

https://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.cc:24:
sizeof(kPredefinedFormats) / sizeof(kPredefinedFormats[0]);
Thanks didn't know about this.

Done

On 2012/10/29 20:26:29, dcheng wrote:
> You can just use the arraysize macro in base/basictypes.h for this.

https://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_c...
File ppapi/shared_impl/flash_clipboard_format_registry.h (right):

https://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.h:21: class
PPAPI_SHARED_EXPORT FlashClipboardFormatRegistry {
As discussed, this is still needed because of the way the pepper interface is
designed.

On 2012/10/29 20:26:29, dcheng wrote:
> With the custom web data changes, do you still need this? I don't see any
> references to registering a custom format in ActionScript (I'm looking at
>
http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/desk...).
> Could we just use plain old strings instead of requiring custom formats to be
> registered?

https://codereview.chromium.org/11225021/diff/49013/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.h:60: std::vector<std::string>
custom_formats_;
On 2012/10/29 20:26:29, dcheng wrote:
> Should this class be copyable/assignable?

Done.

[dcheng, 2012-10-30 00:52:33]

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
File ppapi/shared_impl/flash_clipboard_format_registry.cc (right):

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:27: if
(format_name.length() == 0 || format_name.length() > kMaxFormatNameLength)
Nit: it's slightly more canonical to say format_name.empty() here.

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:71: for (size_t i =
kFirstCustomFormat; i < custom_formats_.size(); ++i) {
Nit: this should be indented two spaces.

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:79: bool
FlashClipboardFormatRegistry::IsValidClipboardType(
This validation function feels out of place. It has nothing to do with the rest
of the class right?

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
File ppapi/shared_impl/flash_clipboard_format_registry.h (right):

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.h:61: std::vector<std::string>
custom_formats_;
My general feedback is this vector makes the class feel very awkward. We
allocate <x> empty slots at the beginning that we never use.

Instead of doing that, how about making this a std::map<uint32_t, std::string>
format_map_?
When we want to allocate a new format, we can just allocate a format at
kNumberOfBuiltInFormats + format_map_.size(). And SetRegisteredFormat becomes
simpler, since we don't need to call std::vector::resize().

[raymes, 2012-10-30 01:57:16]

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
File ppapi/shared_impl/flash_clipboard_format_registry.cc (right):

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:27: if
(format_name.length() == 0 || format_name.length() > kMaxFormatNameLength)
On 2012/10/30 00:52:33, dcheng wrote:
> Nit: it's slightly more canonical to say format_name.empty() here.

Done.

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:71: for (size_t i =
kFirstCustomFormat; i < custom_formats_.size(); ++i) {
On 2012/10/30 00:52:33, dcheng wrote:
> Nit: this should be indented two spaces.

Done.

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:79: bool
FlashClipboardFormatRegistry::IsValidClipboardType(
Yes, it is slightly out of place. It's actually only used in one place so I
moved the function.

On 2012/10/30 00:52:33, dcheng wrote:
> This validation function feels out of place. It has nothing to do with the
rest
> of the class right?

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
File ppapi/shared_impl/flash_clipboard_format_registry.h (right):

http://codereview.chromium.org/11225021/diff/56011/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.h:61: std::vector<std::string>
custom_formats_;
On 2012/10/30 00:52:33, dcheng wrote:
> My general feedback is this vector makes the class feel very awkward. We
> allocate <x> empty slots at the beginning that we never use.
> 
> Instead of doing that, how about making this a std::map<uint32_t, std::string>
> format_map_?
> When we want to allocate a new format, we can just allocate a format at
> kNumberOfBuiltInFormats + format_map_.size(). And SetRegisteredFormat becomes
> simpler, since we don't need to call std::vector::resize().

Done.

[dcheng, 2012-10-30 02:21:13]

LGTM with some const nits.

http://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_cl...
File ppapi/shared_impl/flash_clipboard_format_registry.cc (right):

http://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:57: bool
FlashClipboardFormatRegistry::IsFormatRegistered(uint32_t format) {
const

http://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:58: return
custom_formats_.count(format) == 1;
custom_formats_.find(format) != custom_formats.end() is slightly more efficient.
Or just use ContainsKey from base/stl_util.h.

http://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:61: std::string
FlashClipboardFormatRegistry::GetFormatName(uint32_t format) {
const

http://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:62: FormatMap::iterator it
= custom_formats_.find(format);
const_iterator

http://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:69: const std::string&
format_name) {
This method can be const.

http://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:70: for
(FormatMap::iterator it = custom_formats_.begin();
const_iterator

[raymes, 2012-10-30 02:53:03]

Thanks for this!

https://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_c...
File ppapi/shared_impl/flash_clipboard_format_registry.cc (right):

https://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.cc:57: bool
FlashClipboardFormatRegistry::IsFormatRegistered(uint32_t format) {
On 2012/10/30 02:21:14, dcheng wrote:
> const

Done.

https://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.cc:58: return
custom_formats_.count(format) == 1;
On 2012/10/30 02:21:14, dcheng wrote:
> custom_formats_.find(format) != custom_formats.end() is slightly more
efficient.
> Or just use ContainsKey from base/stl_util.h.

Done.

https://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.cc:61: std::string
FlashClipboardFormatRegistry::GetFormatName(uint32_t format) {
On 2012/10/30 02:21:14, dcheng wrote:
> const

Done.

https://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.cc:62: FormatMap::iterator it
= custom_formats_.find(format);
On 2012/10/30 02:21:14, dcheng wrote:
> const_iterator

Done.

https://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.cc:69: const std::string&
format_name) {
On 2012/10/30 02:21:14, dcheng wrote:
> This method can be const.

Done.

https://codereview.chromium.org/11225021/diff/60078/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.cc:70: for
(FormatMap::iterator it = custom_formats_.begin();
On 2012/10/30 02:21:14, dcheng wrote:
> const_iterator

Done.

[commit-bot: I haz the power, 2012-10-31 16:05:36]

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

[commit-bot: I haz the power, 2012-10-31 16:06:00]

Presubmit check for 11225021-75001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  ppapi/shared_impl/flash_clipboard_format_registry.cc

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit
    content
    content/common
    content/browser/renderer_host
    content/renderer

Presubmit checks took 5.5s to calculate.

[raymes, 2012-10-31 16:07:49]

brettw for OWNERS

[brettw, 2012-10-31 18:05:53]

owners LGTM. I only did a quick once over. Noted a few style nits, but looks
good otherwise.

http://codereview.chromium.org/11225021/diff/40066/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_clipboard_host.cc (right):

http://codereview.chromium.org/11225021/diff/40066/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.cc:28: uint32_t type) {
Can be on prev line.

http://codereview.chromium.org/11225021/diff/40066/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_clipboard_host.h (right):

http://codereview.chromium.org/11225021/diff/40066/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_clipboard_host.h:50: int32_t
OnMsgRegisterCustomFormat(
Can these messages be in the private section? I usually put all the IPC handlers
in a block with no blank lines to conceptually group them (and because there's
no point in writing comments for IPC handlers).

http://codereview.chromium.org/11225021/diff/40066/ppapi/shared_impl/flash_cl...
File ppapi/shared_impl/flash_clipboard_format_registry.cc (right):

http://codereview.chromium.org/11225021/diff/40066/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:11: namespace {
Nit: blank line after this.

http://codereview.chromium.org/11225021/diff/40066/ppapi/shared_impl/flash_cl...
ppapi/shared_impl/flash_clipboard_format_registry.cc:90: }  // ppapi
Nit: need "namespace"

[raymes, 2012-11-01 19:57:54]

Thanks!

https://codereview.chromium.org/11225021/diff/40066/content/renderer/pepper/p...
File content/renderer/pepper/pepper_flash_clipboard_host.cc (right):

https://codereview.chromium.org/11225021/diff/40066/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.cc:28: uint32_t type) {
On 2012/10/31 18:05:53, brettw wrote:
> Can be on prev line.

Done.

https://codereview.chromium.org/11225021/diff/40066/content/renderer/pepper/p...
File content/renderer/pepper/pepper_flash_clipboard_host.h (right):

https://codereview.chromium.org/11225021/diff/40066/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_clipboard_host.h:50: int32_t
OnMsgRegisterCustomFormat(
On 2012/10/31 18:05:53, brettw wrote:
> Can these messages be in the private section? I usually put all the IPC
handlers
> in a block with no blank lines to conceptually group them (and because there's
> no point in writing comments for IPC handlers).

Done.

https://codereview.chromium.org/11225021/diff/40066/ppapi/shared_impl/flash_c...
File ppapi/shared_impl/flash_clipboard_format_registry.cc (right):

https://codereview.chromium.org/11225021/diff/40066/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.cc:11: namespace {
On 2012/10/31 18:05:53, brettw wrote:
> Nit: blank line after this.

Done.

https://codereview.chromium.org/11225021/diff/40066/ppapi/shared_impl/flash_c...
ppapi/shared_impl/flash_clipboard_format_registry.cc:90: }  // ppapi
On 2012/10/31 18:05:53, brettw wrote:
> Nit: need "namespace"

Done.

[commit-bot: I haz the power, 2012-11-01 19:59:24]

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

[commit-bot: I haz the power, 2012-11-01 20:21:32]

Retried try job too often for step(s) compile

[commit-bot: I haz the power, 2012-11-01 21:41:38]

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

[commit-bot: I haz the power, 2012-11-01 23:39:37]

Change committed as 165548