Fix flaky PPAPITest.FlashClipboard.

Fix flaky PPAPITest.FlashClipboard.

BUG=None
TEST=PPAPITest.FlashClipboard passes.


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

[yzshen1, 2011-11-10 22:19:46]

Hi, David.

Would you please take a look? Thanks!

[dmichael (off chromium), 2011-11-10 22:57:07]

lgtm

[commit-bot: I haz the power, 2011-11-11 21:06:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/8515014/3001

[commit-bot: I haz the power, 2011-11-11 23:22:53]

Change committed as 109724

[Paweł Hajdan Jr., 2011-11-14 12:32:48]

Drive-by, FYI.

http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipb...
File ppapi/tests/test_flash_clipboard.cc (right):

http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipb...
ppapi/tests/test_flash_clipboard.cc:38: // or ReadPlainText() immediately. We
need to wait and retry.
I'm not sure how well this will work. If you really want to fix flakiness,
please make it wait for an event instead of sleeping in a loop.

[dmichael (off chromium), 2011-11-14 15:48:15]

http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipb...
File ppapi/tests/test_flash_clipboard.cc (right):

http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipb...
ppapi/tests/test_flash_clipboard.cc:38: // or ReadPlainText() immediately. We
need to wait and retry.
On 2011/11/14 12:32:48, Paweł Hajdan Jr. wrote:
> I'm not sure how well this will work. If you really want to fix flakiness,
> please make it wait for an event instead of sleeping in a loop.
Oops, he is right. I wasn't looking carefully enough. You need to use
PPB_Testing_Dev.RunMessageLoop below instead of sleeping. I'm kind of surprised
the OOP test is working.

[yzshen1, 2011-11-14 18:40:46]

On 2011/11/14 15:48:15, dmichael wrote:
>
http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipb...
> File ppapi/tests/test_flash_clipboard.cc (right):
> 
>
http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipb...
> ppapi/tests/test_flash_clipboard.cc:38: // or ReadPlainText() immediately. We
> need to wait and retry.
> On 2011/11/14 12:32:48, Paweł Hajdan Jr. wrote:
> > I'm not sure how well this will work. If you really want to fix flakiness,
> > please make it wait for an event instead of sleeping in a loop.
This operation sends an asyc request to the browser process, which is completed
there and doesn't send back any ACK.
So there is no event to wait for, unless we added ACK from browser to renderer
(and from renderer to plugin, in the OOP case).

> Oops, he is right. I wasn't looking carefully enough. You need to use
> PPB_Testing_Dev.RunMessageLoop below instead of sleeping. I'm kind of
surprised
> the OOP test is working.
Would you please explain why?
I thought the completion of this operation doesn't rely on any incoming message,
so it is okay to sleep instead of running the message loop.

[dmichael(do not use this one), 2011-11-14 18:59:53]

On Mon, Nov 14, 2011 at 11:40 AM, <yzshen@chromium.org> wrote:

> On 2011/11/14 15:48:15, dmichael wrote:
>
> http://codereview.chromium.**org/8515014/diff/3001/ppapi/**
>
tests/test_flash_clipboard.cc<http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipboard.cc>
>
>> File ppapi/tests/test_flash_**clipboard.cc (right):
>>
>
>
> http://codereview.chromium.**org/8515014/diff/3001/ppapi/**
>
tests/test_flash_clipboard.cc#**newcode38<http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipboard.cc#newcode38>
>
>> ppapi/tests/test_flash_**clipboard.cc:38: // or ReadPlainText()
>> immediately. We
>> need to wait and retry.
>> On 2011/11/14 12:32:48, Paweł Hajdan Jr. wrote:
>> > I'm not sure how well this will work. If you really want to fix
>> flakiness,
>> > please make it wait for an event instead of sleeping in a loop.
>>
> This operation sends an asyc request to the browser process, which is
> completed
> there and doesn't send back any ACK.
> So there is no event to wait for, unless we added ACK from browser to
> renderer
> (and from renderer to plugin, in the OOP case).
>
>
>  Oops, he is right. I wasn't looking carefully enough. You need to use
>> PPB_Testing_Dev.RunMessageLoop below instead of sleeping. I'm kind of
>>
> surprised
>
>> the OOP test is working.
>>
> Would you please explain why?
> I thought the completion of this operation doesn't rely on any incoming
> message,
> so it is okay to sleep instead of running the message loop.
>
>  I see, I was assuming a different implementation than what's really
there. Since you're using a Sync IPC from plugin to host for ReadPlainText,
what you have should work.

>
>
http://codereview.chromium.**org/8515014/<http://codereview.chromium.org/8515...
>

[yzshen1, 2011-11-14 19:05:09]

On 2011/11/14 18:59:53, dmichael(do not use this one) wrote:
> On Mon, Nov 14, 2011 at 11:40 AM, <mailto:yzshen@chromium.org> wrote:
> 
> > On 2011/11/14 15:48:15, dmichael wrote:
> >
> > http://codereview.chromium.**org/8515014/diff/3001/ppapi/**
> >
>
tests/test_flash_clipboard.cc<http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipboard.cc>
> >
> >> File ppapi/tests/test_flash_**clipboard.cc (right):
> >>
> >
> >
> > http://codereview.chromium.**org/8515014/diff/3001/ppapi/**
> >
>
tests/test_flash_clipboard.cc#**newcode38<http://codereview.chromium.org/8515014/diff/3001/ppapi/tests/test_flash_clipboard.cc#newcode38>
> >
> >> ppapi/tests/test_flash_**clipboard.cc:38: // or ReadPlainText()
> >> immediately. We
> >> need to wait and retry.
> >> On 2011/11/14 12:32:48, Paweł Hajdan Jr. wrote:
> >> > I'm not sure how well this will work. If you really want to fix
> >> flakiness,
> >> > please make it wait for an event instead of sleeping in a loop.
> >>
> > This operation sends an asyc request to the browser process, which is
> > completed
> > there and doesn't send back any ACK.
> > So there is no event to wait for, unless we added ACK from browser to
> > renderer
> > (and from renderer to plugin, in the OOP case).
> >
> >
> >  Oops, he is right. I wasn't looking carefully enough. You need to use
> >> PPB_Testing_Dev.RunMessageLoop below instead of sleeping. I'm kind of
> >>
> > surprised
> >
> >> the OOP test is working.
> >>
> > Would you please explain why?
> > I thought the completion of this operation doesn't rely on any incoming
> > message,
> > so it is okay to sleep instead of running the message loop.
> >
> >  I see, I was assuming a different implementation than what's really
> there. Since you're using a Sync IPC from plugin to host for ReadPlainText,
> what you have should work.

Thanks for explanation, David!

> 
> >
> >
>
http://codereview.chromium.**org/8515014/%3Chttp://codereview.chromium.org/85...>
> >