Add GetSequenceNumber function to PPB_Flash_Clipboard

Add GetSequenceNumber function to PPB_Flash_Clipboard

This provides access to a sequence number which identifies the clipboard state.

BUG=333094
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247383

[raymes, 2014-01-13 04:18:02]

Hey dcheng, estade, ptal. I couldn't get this working on gtk. The reason is that
Clipboard::GetSequenceNumber was always returning 0. For some reason when
writing to the clipboard using the pepper API, the selection clipboard sequence
number was being updated (this path:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...)

Do you have any idea why that would be?

[dcheng, 2014-01-13 04:34:52]

On 2014/01/13 04:18:02, raymes wrote:
> Hey dcheng, estade, ptal. I couldn't get this working on gtk. The reason is
that
> Clipboard::GetSequenceNumber was always returning 0. For some reason when
> writing to the clipboard using the pepper API, the selection clipboard
sequence
> number was being updated (this path:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...)
> 
> Do you have any idea why that would be?

I'm guessing it's probably been broken for awhile... I've added a unit test for
it, but haven't yet fixed it in https://codereview.chromium.org/136193002... I'm
sending it to the tryservers to see what other platforms it's broken on =(

[dcheng, 2014-01-13 04:59:52]

On 2014/01/13 04:34:52, dcheng wrote:
> On 2014/01/13 04:18:02, raymes wrote:
> > Hey dcheng, estade, ptal. I couldn't get this working on gtk. The reason is
> that
> > Clipboard::GetSequenceNumber was always returning 0. For some reason when
> > writing to the clipboard using the pepper API, the selection clipboard
> sequence
> > number was being updated (this path:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...)
> > 
> > Do you have any idea why that would be?
> 
> I'm guessing it's probably been broken for awhile... I've added a unit test
for
> it, but haven't yet fixed it in https://codereview.chromium.org/136193002...
I'm
> sending it to the tryservers to see what other platforms it's broken on =(

Actually, it looks like it's working to me. Since the sequence number is updated
in an XEvent callback, we needed to pump the message loop (see the updated
test). Where are you seeing situations when it doesn't work?

[raymes, 2014-01-13 05:50:20]

Ahh that was the problem. I forgot that writing was async in pepper too. That
fixes it. Thanks! Feel free not to review the rest of the CL, yuzhu will take a
look but let me know if you have any other comments.

Thanks!

[raymes, 2014-01-14 03:01:39]

-yzshen
+teravest

to spread review load. Thanks!

[raymes, 2014-01-20 00:42:13]

On 2014/01/14 03:01:39, raymes wrote:
> -yzshen
> +teravest
> 
> to spread review load. Thanks!

friendly ping
Thanks!

[teravest, 2014-01-21 14:31:55]

Looks reasonable to me, just a couple minor things.

Sorry about the delay in reviewing this one, I totally missed it. Feel free to
bug me if a code review ever sits around for 24 hours without a response. I'll
pay more attention to the code review dashboard in the future.

https://codereview.chromium.org/136183002/diff/170001/ppapi/api/private/ppb_f...
File ppapi/api/private/ppb_flash_clipboard.idl (right):

https://codereview.chromium.org/136183002/diff/170001/ppapi/api/private/ppb_f...
ppapi/api/private/ppb_flash_clipboard.idl:148: * Returns a sequence number which
uniquely identifies clipboard state. This
Nit: This doesn't actually return the sequence number. I'd replace "returns"
with "gets", but this is pretty minor.

https://codereview.chromium.org/136183002/diff/170001/ppapi/cpp/private/flash...
File ppapi/cpp/private/flash_clipboard.cc (right):

https://codereview.chromium.org/136183002/diff/170001/ppapi/cpp/private/flash...
ppapi/cpp/private/flash_clipboard.cc:58: if
(has_interface<PPB_Flash_Clipboard_5_0>()) {
For all the existing methods, I think you need to add a
has_interface/get_interface sections for PPB_Flash_Clipboard_5_1 as well.

The argument I saw for this in the past was that adding a stanza for new
versions would make it more possible for us to remove old versions in the
future.

[raymes, 2014-01-22 23:23:18]

Thanks Justin!

https://codereview.chromium.org/136183002/diff/170001/ppapi/api/private/ppb_f...
File ppapi/api/private/ppb_flash_clipboard.idl (right):

https://codereview.chromium.org/136183002/diff/170001/ppapi/api/private/ppb_f...
ppapi/api/private/ppb_flash_clipboard.idl:148: * Returns a sequence number which
uniquely identifies clipboard state. This
On 2014/01/21 14:31:55, teravest wrote:
> Nit: This doesn't actually return the sequence number. I'd replace "returns"
> with "gets", but this is pretty minor.

Done.

https://codereview.chromium.org/136183002/diff/170001/ppapi/cpp/private/flash...
File ppapi/cpp/private/flash_clipboard.cc (right):

https://codereview.chromium.org/136183002/diff/170001/ppapi/cpp/private/flash...
ppapi/cpp/private/flash_clipboard.cc:58: if
(has_interface<PPB_Flash_Clipboard_5_0>()) {
On 2014/01/21 14:31:55, teravest wrote:
> For all the existing methods, I think you need to add a
> has_interface/get_interface sections for PPB_Flash_Clipboard_5_1 as well.
> 
> The argument I saw for this in the past was that adding a stanza for new
> versions would make it more possible for us to remove old versions in the
> future.

Done.

[teravest, 2014-01-23 15:10:07]

lgtm

[commit-bot: I haz the power, 2014-01-24 00:43:42]

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

[commit-bot: I haz the power, 2014-01-24 01:06:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[raymes, 2014-01-24 01:59:24]

dcheng: could you ptal at ppapi_messages for OWNERS? Thanks!

[dcheng, 2014-01-27 19:47:20]

Sorry, looking at this now.

[dcheng, 2014-01-27 19:55:17]

ppapi/proxy/ppapi_messages.h LGTM

[commit-bot: I haz the power, 2014-01-28 00:04:25]

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

[commit-bot: I haz the power, 2014-01-28 01:45:41]

Change committed as 247383