PPAPI: Force async callback invocation option.

Add a flag field to PP_CompletionCallback to control if the callback should
always be invoked asynchronously on success or error or skipped if the
operation can complete synchronously without blocking. Keep the default
behavior as-is  until clients update their code. Bump revisions of all
interfaces that take callbacks as args. Update browser interface function
implementations and C++ layer to force callbacks if sync option is not set.
Change ppapi/tests to run tests involving callbacks with both flag options.

BUG=79376
TEST=ppapi_tests + bots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91205

[polina, 2011-04-25 07:37:50]

Here is the prototype of the feature with URLLoader only for now.

[darin (slow to review), 2011-04-25 18:31:06]

http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
File ppapi/c/pp_completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
ppapi/c/pp_completion_callback.h:70: PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1
<< 0  // or SYNCOK?
it looks like you are inverting the sense of this flag from what we previously
discussed.  can you say more about this?

http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:143: return PP_ERROR_BADRESOURCE;
what about this case?

http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:153: return PP_ERROR_BADRESOURCE;
what about this case?  and the others like this...

http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:255: return
CompletionResult(callback, rv);
I think it would be better to put the callback fixup code into the entry points
(that ppb_urlloader points to) so that the implementation classes will not need
to change.

(For "Open" you have some of the checks in PPB_URLLoader_Impl::Open and others
in Open.)

Eventually, we might be able to auto-generate the entry point stubs, and this
callback fixup could easily be auto-generated.

[polina, 2011-04-25 19:44:44]

http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
File ppapi/c/pp_completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
ppapi/c/pp_completion_callback.h:70: PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1
<< 0  // or SYNCOK?
On 2011/04/25 18:31:06, darin wrote:
> it looks like you are inverting the sense of this flag from what we previously
> discussed.  can you say more about this?

Yes, I have. As I was writing the comments, I realized that it would make more
sense to make forceasync the default and create a special flag for the other,
non-default option. This way I could also add a "default" enum value.

http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:143: return PP_ERROR_BADRESOURCE;
I could not get ppapi_tests for url_loader get passed an Open failure for bad
request, so I left this code as is until I could test it. I must have something
wrong with my test set-up.

http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:153: return PP_ERROR_BADRESOURCE;
On 2011/04/25 18:31:06, darin wrote:
> what about this case?  and the others like this...

same reply as above.

http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:255: return
CompletionResult(callback, rv);
On 2011/04/25 18:31:06, darin wrote:
> I think it would be better to put the callback fixup code into the entry
points
> (that ppb_urlloader points to) so that the implementation classes will not
need
> to change.
> 
> (For "Open" you have some of the checks in PPB_URLLoader_Impl::Open and others
> in Open.)
> 
> Eventually, we might be able to auto-generate the entry point stubs, and this
> callback fixup could easily be auto-generated.

Do you mean put this in outer Open where it has:
 return loader->Open(request, callback);?

[brettw, 2011-04-25 19:55:46]

http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
File ppapi/c/pp_completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
ppapi/c/pp_completion_callback.h:70: PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1
<< 0  // or SYNCOK?
I wasn't at the meeting and haven't done any thinking about it, but if we have
the flag with this meaning, but "no force not synchronous" is almost impossible
to understand. ALLOW_SYNC would be better.

[darin (slow to review), 2011-04-25 21:07:12]

On Mon, Apr 25, 2011 at 12:55 PM, <brettw@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
> File ppapi/c/pp_completion_callback.h (right):
>
>
>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
> ppapi/c/pp_completion_callback.h:70:
> PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1 << 0  // or SYNCOK?
> I wasn't at the meeting and haven't done any thinking about it, but if
> we have the flag with this meaning, but "no force not synchronous" is
> almost impossible to understand. ALLOW_SYNC would be better.


Yeah, I had a similar thought.  Even ALLOW_SYNC is a little unclear, as it
could indicate
that the callbacks run synchronously, when what it really means is that the
callbacks do
not run at all, but that the result is provided via return value.

Does PP_COMPLETIONCALLBACK_FLAG_ALLOWSYNC really best capture that meaning?

-Darin



>
>
> http://codereview.chromium.org/6899055/
>

[darin (slow to review), 2011-04-25 21:10:11]

On Mon, Apr 25, 2011 at 12:44 PM, <polina@google.com> wrote:

>
>
>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
> File ppapi/c/pp_completion_callback.h (right):
>
>
>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
> ppapi/c/pp_completion_callback.h:70:
> PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1 << 0  // or SYNCOK?
> On 2011/04/25 18:31:06, darin wrote:
>
>> it looks like you are inverting the sense of this flag from what we
>>
> previously
>
>> discussed.  can you say more about this?
>>
>
> Yes, I have. As I was writing the comments, I realized that it would
> make more sense to make forceasync the default and create a special flag
> for the other, non-default option. This way I could also add a "default"
> enum value.


OK...

We should think about the implications this will have on migrating to the
new behavior.  Maybe we should start out with the bits set to allow a sync
completion until we have migrated all of the consumers?



>
>
>
>
http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
> File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):
>
>
>
http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
> webkit/plugins/ppapi/ppb_url_loader_impl.cc:143: return
> PP_ERROR_BADRESOURCE;
> I could not get ppapi_tests for url_loader get passed an Open failure
> for bad request, so I left this code as is until I could test it. I must
> have something wrong with my test set-up.
>
>
I don't think it is necessary to prove that these branches may be taken.  I
think it
is enough to patch by inspection here.



>
>
>
http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
> webkit/plugins/ppapi/ppb_url_loader_impl.cc:153: return
> PP_ERROR_BADRESOURCE;
> On 2011/04/25 18:31:06, darin wrote:
>
>> what about this case?  and the others like this...
>>
>
> same reply as above.
>
>
>
>
http://codereview.chromium.org/6899055/diff/3005/webkit/plugins/ppapi/ppb_url...
> webkit/plugins/ppapi/ppb_url_loader_impl.cc:255: return
> CompletionResult(callback, rv);
> On 2011/04/25 18:31:06, darin wrote:
>
>> I think it would be better to put the callback fixup code into the
>>
> entry points
>
>> (that ppb_urlloader points to) so that the implementation classes will
>>
> not need
>
>> to change.
>>
>
>  (For "Open" you have some of the checks in PPB_URLLoader_Impl::Open
>>
> and others
>
>> in Open.)
>>
>
>  Eventually, we might be able to auto-generate the entry point stubs,
>>
> and this
>
>> callback fixup could easily be auto-generated.
>>
>
> Do you mean put this in outer Open where it has:
>  return loader->Open(request, callback);?


Yes.



>
>
> http://codereview.chromium.org/6899055/
>

[brettw, 2011-04-25 21:17:41]

On 2011/04/25 21:07:12, darin wrote:
> On Mon, Apr 25, 2011 at 12:55 PM, <mailto:brettw@chromium.org> wrote:
>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
> > File ppapi/c/pp_completion_callback.h (right):
> >
> >
> >
>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
> > ppapi/c/pp_completion_callback.h:70:
> > PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1 << 0  // or SYNCOK?
> > I wasn't at the meeting and haven't done any thinking about it, but if
> > we have the flag with this meaning, but "no force not synchronous" is
> > almost impossible to understand. ALLOW_SYNC would be better.
> 
> 
> Yeah, I had a similar thought.  Even ALLOW_SYNC is a little unclear, as it
> could indicate
> that the callbacks run synchronously, when what it really means is that the
> callbacks do
> not run at all, but that the result is provided via return value.
> 
> Does PP_COMPLETIONCALLBACK_FLAG_ALLOWSYNC really best capture that meaning?

I see what you mean and I agree that my suggestion isn't accurate. Unless we can
come up with a better name, I'd like to find a way to make FORCE_ASYNC work.

[darin (slow to review), 2011-04-25 22:27:54]

On Mon, Apr 25, 2011 at 2:17 PM, <brettw@chromium.org> wrote:

> On 2011/04/25 21:07:12, darin wrote:
>
>  On Mon, Apr 25, 2011 at 12:55 PM, <mailto:brettw@chromium.org> wrote:
>>
>
>
>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
>
>> > File ppapi/c/pp_completion_callback.h (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
>
>> > ppapi/c/pp_completion_callback.h:70:
>> > PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1 << 0  // or SYNCOK?
>> > I wasn't at the meeting and haven't done any thinking about it, but if
>> > we have the flag with this meaning, but "no force not synchronous" is
>> > almost impossible to understand. ALLOW_SYNC would be better.
>>
>
>
>  Yeah, I had a similar thought.  Even ALLOW_SYNC is a little unclear, as it
>> could indicate
>> that the callbacks run synchronously, when what it really means is that
>> the
>> callbacks do
>> not run at all, but that the result is provided via return value.
>>
>
>  Does PP_COMPLETIONCALLBACK_FLAG_ALLOWSYNC really best capture that
>> meaning?
>>
>
> I see what you mean and I agree that my suggestion isn't accurate. Unless
> we can
> come up with a better name, I'd like to find a way to make FORCE_ASYNC
> work.
>
>
Right... that's why I originally proposed FORCE_ASYNC as the default.  I had
trouble
coming up with a succinct form of:

PP_COMPLETIONCALLBACK_FLAG_ALLOWEDTORETURNTHERESULTOFTHISFUNCTIONINSTEADOFRUNNINGTHECALLBACK

-Darin




>
> http://codereview.chromium.org/6899055/
>

[polina, 2011-04-26 00:01:25]

On Mon, Apr 25, 2011 at 3:27 PM, Darin Fisher <darin@chromium.org> wrote:

> On Mon, Apr 25, 2011 at 2:17 PM, <brettw@chromium.org> wrote:
>
>> On 2011/04/25 21:07:12, darin wrote:
>>
>>  On Mon, Apr 25, 2011 at 12:55 PM, <mailto:brettw@chromium.org> wrote:
>>>
>>
>>
>>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
>>
>>> > File ppapi/c/pp_completion_callback.h (right):
>>> >
>>> >
>>> >
>>>
>>
>>
>>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
>>
>>> > ppapi/c/pp_completion_callback.h:70:
>>> > PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1 << 0  // or SYNCOK?
>>> > I wasn't at the meeting and haven't done any thinking about it, but if
>>> > we have the flag with this meaning, but "no force not synchronous" is
>>> > almost impossible to understand. ALLOW_SYNC would be better.
>>>
>>
>>
>>  Yeah, I had a similar thought.  Even ALLOW_SYNC is a little unclear, as
>>> it
>>> could indicate
>>> that the callbacks run synchronously, when what it really means is that
>>> the
>>> callbacks do
>>> not run at all, but that the result is provided via return value.
>>>
>>
>>  Does PP_COMPLETIONCALLBACK_FLAG_ALLOWSYNC really best capture that
>>> meaning?
>>>
>>
>> I see what you mean and I agree that my suggestion isn't accurate. Unless
>> we can
>> come up with a better name, I'd like to find a way to make FORCE_ASYNC
>> work.
>>
>>
> Right... that's why I originally proposed FORCE_ASYNC as the default.  I
> had trouble
> coming up with a succinct form of:
>
>
>
PP_COMPLETIONCALLBACK_FLAG_ALLOWEDTORETURNTHERESULTOFTHISFUNCTIONINSTEADOFRUNNINGTHECALLBACK
>

The part where we return the result synchronously and skip the callback is
confusing and unintuitive in any case. I remember how tricky it was for NaCl
to "warm up" to the idea.
In either case, we have to explain it. And I think that's much easier to do
if it is associated with a name. ForceAsync, on the other hand, is very
intuitive and expected to an outsider, not one of us, who are used to the
PPAPI ways. It makes a perfect default, and defaults don't usually need
special names.

In addition, we have to extend this beyond just a bool option. The default
means more than just forcing callbacks since we are going to add more
specialized flags in the future (e.g. for running off the main thread). And
it is common for the default to be 0 when dealing with bit-wise flags.

Hence, there are a couple of things I wanted to address by introducing
NOFORCEASYNC / ALLOWSYNC / SYNCOK flag instead of FORCEASYNC:
- Clear flag semantics: flags are meant to alter "normal" callback behavior
- Easy usage: PP_MakeCompletionCallback sets flags to the default of 0, user
can turn on specific bits (on is easier than off)
- Intuitive default semantics: callbacks always run by default, flags modify
this behavior

So you have:

PP_CompletionCallback cc = PP_MakeCompletionCallback(fun, user_data);
cc.flags = cc.flags | PP_COMPLETIONCALLBACK_FLAG_FOO;
cc.flags = cc.flags | PP_COMPLETIONCALLBACK_FLAG_ALLOWSYNCRETURN;
// Or PP_SetCompletionCallbackFlag(cc, PP_COMPLETIONCALLBACK_FLAG_XYZ);

If you use FORCEASYNC instead, you will have to worry about having to turn
off one bit while turning on others:

PP_CompletionCallback cc = PP_MakeCompletionCallback(func, user_data);
cc.flags = 0;  // What does this mean? What did we just disable? Not clear
from the code.
cc.flags = PP_COMPLETIONCALLBACK_FLAG_NONE;  // Still not very clear what to
expect.
cc.flags = cc.flags | PP_COMPLETIONCALLBACK_FLAG_FOO;
cc.flags = cc.flags & !PP_COMPLETIONCALLBACK_FLAG_FORCEASYNC;  // Turn off
force-async, which means the callback might not run and the result will be
returned synchronously, but you would never guess this by reading this line
of code :)

I can add PP_CompletionCallbackFlagOn() and PP_CompletionCallbackFlagOff()
and PP_CompletionCallbackFlagsClear(), but the code would still be less
readable because you will be saying what you won't be doing in the special
case, not what you will be doing.

[darin (slow to review), 2011-04-26 15:50:37]

On Mon, Apr 25, 2011 at 5:01 PM, Polina Sokolova <polina@google.com> wrote:

> On Mon, Apr 25, 2011 at 3:27 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> On Mon, Apr 25, 2011 at 2:17 PM, <brettw@chromium.org> wrote:
>>
>>> On 2011/04/25 21:07:12, darin wrote:
>>>
>>>  On Mon, Apr 25, 2011 at 12:55 PM, <mailto:brettw@chromium.org> wrote:
>>>>
>>>
>>>
>>>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
>>>
>>>> > File ppapi/c/pp_completion_callback.h (right):
>>>> >
>>>> >
>>>> >
>>>>
>>>
>>>
>>>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
>>>
>>>> > ppapi/c/pp_completion_callback.h:70:
>>>> > PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1 << 0  // or SYNCOK?
>>>> > I wasn't at the meeting and haven't done any thinking about it, but if
>>>> > we have the flag with this meaning, but "no force not synchronous" is
>>>> > almost impossible to understand. ALLOW_SYNC would be better.
>>>>
>>>
>>>
>>>  Yeah, I had a similar thought.  Even ALLOW_SYNC is a little unclear, as
>>>> it
>>>> could indicate
>>>> that the callbacks run synchronously, when what it really means is that
>>>> the
>>>> callbacks do
>>>> not run at all, but that the result is provided via return value.
>>>>
>>>
>>>  Does PP_COMPLETIONCALLBACK_FLAG_ALLOWSYNC really best capture that
>>>> meaning?
>>>>
>>>
>>> I see what you mean and I agree that my suggestion isn't accurate. Unless
>>> we can
>>> come up with a better name, I'd like to find a way to make FORCE_ASYNC
>>> work.
>>>
>>>
>> Right... that's why I originally proposed FORCE_ASYNC as the default.  I
>> had trouble
>> coming up with a succinct form of:
>>
>>
>>
PP_COMPLETIONCALLBACK_FLAG_ALLOWEDTORETURNTHERESULTOFTHISFUNCTIONINSTEADOFRUNNINGTHECALLBACK
>>
>
> The part where we return the result synchronously and skip the callback is
> confusing and unintuitive in any case. I remember how tricky it was for NaCl
> to "warm up" to the idea.
> In either case, we have to explain it. And I think that's much easier to do
> if it is associated with a name. ForceAsync, on the other hand, is very
> intuitive and expected to an outsider, not one of us, who are used to the
> PPAPI ways. It makes a perfect default, and defaults don't usually need
> special names.
>
> In addition, we have to extend this beyond just a bool option. The default
> means more than just forcing callbacks since we are going to add more
> specialized flags in the future (e.g. for running off the main thread). And
> it is common for the default to be 0 when dealing with bit-wise flags.
>
> Hence, there are a couple of things I wanted to address by introducing
> NOFORCEASYNC / ALLOWSYNC / SYNCOK flag instead of FORCEASYNC:
> - Clear flag semantics: flags are meant to alter "normal" callback behavior
> - Easy usage: PP_MakeCompletionCallback sets flags to the default of 0,
> user can turn on specific bits (on is easier than off)
> - Intuitive default semantics: callbacks always run by default, flags
> modify this behavior
>
> So you have:
>
> PP_CompletionCallback cc = PP_MakeCompletionCallback(fun, user_data);
> cc.flags = cc.flags | PP_COMPLETIONCALLBACK_FLAG_FOO;
> cc.flags = cc.flags | PP_COMPLETIONCALLBACK_FLAG_ALLOWSYNCRETURN;
> // Or PP_SetCompletionCallbackFlag(cc, PP_COMPLETIONCALLBACK_FLAG_XYZ);
>
> If you use FORCEASYNC instead, you will have to worry about having to turn
> off one bit while turning on others:
>
> PP_CompletionCallback cc = PP_MakeCompletionCallback(func, user_data);
> cc.flags = 0;  // What does this mean? What did we just disable? Not clear
> from the code.
> cc.flags = PP_COMPLETIONCALLBACK_FLAG_NONE;  // Still not very clear what
> to expect.
> cc.flags = cc.flags | PP_COMPLETIONCALLBACK_FLAG_FOO;
> cc.flags = cc.flags & !PP_COMPLETIONCALLBACK_FLAG_FORCEASYNC;  // Turn off
> force-async, which means the callback might not run and the result will be
> returned synchronously, but you would never guess this by reading this line
> of code :)
>
> I can add PP_CompletionCallbackFlagOn() and PP_CompletionCallbackFlagOff()
> and PP_CompletionCallbackFlagsClear(), but the code would still be less
> readable because you will be saying what you won't be doing in the special
> case, not what you will be doing.
>


I completely agree with the above.  I'm just struggling to find a good name
for the option.

One idea came to me this morning.  How about:

PP_COMPLETIONCALLBACK_FLAG_OPTIONAL

It reads like the "completion callback is optional," which seems fairly
descriptive.  The callback
is optional from the perspective of the guy receiving the
PP_CompletionCallback object.

-Darin

[polina, 2011-04-26 18:19:40]

On Tue, Apr 26, 2011 at 8:50 AM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Mon, Apr 25, 2011 at 5:01 PM, Polina Sokolova <polina@google.com>wrote:
>
>> On Mon, Apr 25, 2011 at 3:27 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>>> On Mon, Apr 25, 2011 at 2:17 PM, <brettw@chromium.org> wrote:
>>>
>>>> On 2011/04/25 21:07:12, darin wrote:
>>>>
>>>>  On Mon, Apr 25, 2011 at 12:55 PM, <mailto:brettw@chromium.org> wrote:
>>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
>>>>
>>>>> > File ppapi/c/pp_completion_callback.h (right):
>>>>> >
>>>>> >
>>>>> >
>>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6899055/diff/3005/ppapi/c/pp_completion_callba...
>>>>
>>>>> > ppapi/c/pp_completion_callback.h:70:
>>>>> > PP_COMPLETIONCALLBACK_FLAG_NOFORCEASYNC = 1 << 0  // or SYNCOK?
>>>>> > I wasn't at the meeting and haven't done any thinking about it, but
>>>>> if
>>>>> > we have the flag with this meaning, but "no force not synchronous" is
>>>>> > almost impossible to understand. ALLOW_SYNC would be better.
>>>>>
>>>>
>>>>
>>>>  Yeah, I had a similar thought.  Even ALLOW_SYNC is a little unclear, as
>>>>> it
>>>>> could indicate
>>>>> that the callbacks run synchronously, when what it really means is that
>>>>> the
>>>>> callbacks do
>>>>> not run at all, but that the result is provided via return value.
>>>>>
>>>>
>>>>  Does PP_COMPLETIONCALLBACK_FLAG_ALLOWSYNC really best capture that
>>>>> meaning?
>>>>>
>>>>
>>>> I see what you mean and I agree that my suggestion isn't accurate.
>>>> Unless we can
>>>> come up with a better name, I'd like to find a way to make FORCE_ASYNC
>>>> work.
>>>>
>>>>
>>> Right... that's why I originally proposed FORCE_ASYNC as the default.  I
>>> had trouble
>>> coming up with a succinct form of:
>>>
>>>
>>>
PP_COMPLETIONCALLBACK_FLAG_ALLOWEDTORETURNTHERESULTOFTHISFUNCTIONINSTEADOFRUNNINGTHECALLBACK
>>>
>>
>> The part where we return the result synchronously and skip the callback is
>> confusing and unintuitive in any case. I remember how tricky it was for NaCl
>> to "warm up" to the idea.
>> In either case, we have to explain it. And I think that's much easier to
>> do if it is associated with a name. ForceAsync, on the other hand, is very
>> intuitive and expected to an outsider, not one of us, who are used to the
>> PPAPI ways. It makes a perfect default, and defaults don't usually need
>> special names.
>>
>> In addition, we have to extend this beyond just a bool option. The default
>> means more than just forcing callbacks since we are going to add more
>> specialized flags in the future (e.g. for running off the main thread). And
>> it is common for the default to be 0 when dealing with bit-wise flags.
>>
>> Hence, there are a couple of things I wanted to address by introducing
>> NOFORCEASYNC / ALLOWSYNC / SYNCOK flag instead of FORCEASYNC:
>> - Clear flag semantics: flags are meant to alter "normal" callback
>> behavior
>> - Easy usage: PP_MakeCompletionCallback sets flags to the default of 0,
>> user can turn on specific bits (on is easier than off)
>> - Intuitive default semantics: callbacks always run by default, flags
>> modify this behavior
>>
>> So you have:
>>
>> PP_CompletionCallback cc = PP_MakeCompletionCallback(fun, user_data);
>> cc.flags = cc.flags | PP_COMPLETIONCALLBACK_FLAG_FOO;
>> cc.flags = cc.flags | PP_COMPLETIONCALLBACK_FLAG_ALLOWSYNCRETURN;
>> // Or PP_SetCompletionCallbackFlag(cc, PP_COMPLETIONCALLBACK_FLAG_XYZ);
>>
>> If you use FORCEASYNC instead, you will have to worry about having to turn
>> off one bit while turning on others:
>>
>> PP_CompletionCallback cc = PP_MakeCompletionCallback(func, user_data);
>> cc.flags = 0;  // What does this mean? What did we just disable? Not clear
>> from the code.
>> cc.flags = PP_COMPLETIONCALLBACK_FLAG_NONE;  // Still not very clear what
>> to expect.
>> cc.flags = cc.flags | PP_COMPLETIONCALLBACK_FLAG_FOO;
>> cc.flags = cc.flags & !PP_COMPLETIONCALLBACK_FLAG_FORCEASYNC;  // Turn off
>> force-async, which means the callback might not run and the result will be
>> returned synchronously, but you would never guess this by reading this line
>> of code :)
>>
>> I can add PP_CompletionCallbackFlagOn() and PP_CompletionCallbackFlagOff()
>> and PP_CompletionCallbackFlagsClear(), but the code would still be less
>> readable because you will be saying what you won't be doing in the special
>> case, not what you will be doing.
>>
>
>
> I completely agree with the above.  I'm just struggling to find a good name
> for the option.
>
> One idea came to me this morning.  How about:
>
> PP_COMPLETIONCALLBACK_FLAG_OPTIONAL
>
> It reads like the "completion callback is optional," which seems fairly
> descriptive.  The callback
> is optional from the perspective of the guy receiving the
> PP_CompletionCallback object.
>
>
Sounds good!



> -Darin
>

[polina, 2011-05-28 03:22:35]

I have uploaded all impl files and corresponding tests that are similar in
structure to ppb_url_loader.

I am still working on ppb_graphics_2d, ppb_font_list_dev and ppb_audio_trusted.

Thanks,
P.

[polina, 2011-06-03 03:01:47]

On 2011/05/28 03:22:35, polina wrote:
> I have uploaded all impl files and corresponding tests that are similar in
> structure to ppb_url_loader.
> 
> I am still working on ppb_graphics_2d, ppb_font_list_dev and
ppb_audio_trusted.
> 
> Thanks,
> P.

Please take another look.  I have modified all files except for
ppb_layer_compositer_dev and ppb_video_decoder_dev, which are waiting for
http://codereview.chromium.org/6975053/.

Thanks, 
P.

[piman, 2011-06-07 17:32:14]

stuff in ppapi/proxy will need to be changed as well.

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
File ppapi/c/pp_completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
ppapi/c/pp_completion_callback.h:91: int32_t flags;
Because you're changing the size (and semantics) of PP_CompletionCallback, you
need to rev all or the interfaces that uses a PP_CompletionCallback

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
ppapi/c/pp_completion_callback.h:123: cc.flags =
PP_COMPLETIONCALLBACK_FLAG_NONE;
I thought we were not going to change the default behavior. Shouldn't this be
PP_COMPLETIONCALLBACK_FLAG_OPTIONAL ?

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
File ppapi/cpp/completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:49: return  (cc_.func == NULL ||
nit: only 1 space after return

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:50: (cc_.flags &
PP_COMPLETIONCALLBACK_FLAG_OPTIONAL) != 0);
nit: Indent looks wrong

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/directory_rea...
File ppapi/cpp/dev/directory_reader_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/directory_rea...
ppapi/cpp/dev/directory_reader_dev.cc:38: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
Why the need for static_cast ? It should cast implicitly.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_chooser_...
File ppapi/cpp/dev/file_chooser_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_chooser_...
ppapi/cpp/dev/file_chooser_dev.cc:39: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ?

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_io_dev.cc
File ppapi/cpp/dev/file_io_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_io_dev.c...
ppapi/cpp/dev/file_io_dev.cc:44: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ? Here an below.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_ref_dev.cc
File ppapi/cpp/dev/file_ref_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_ref_dev....
ppapi/cpp/dev/file_ref_dev.cc:72: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ? here and below

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_system_d...
File ppapi/cpp/dev/file_system_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_system_d...
ppapi/cpp/dev/file_system_dev.cc:35: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ?

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/surface_3d_de...
File ppapi/cpp/dev/surface_3d_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/surface_3d_de...
ppapi/cpp/dev/surface_3d_dev.cc:43: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ?

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/transport_dev.cc
File ppapi/cpp/dev/transport_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/transport_dev...
ppapi/cpp/dev/transport_dev.cc:41: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ? here and below

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/video_decoder...
File ppapi/cpp/dev/video_decoder_dev.cc (left):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/video_decoder...
ppapi/cpp/dev/video_decoder_dev.cc:76: if (!pp_resource())
Why removing the check for pp_resource ?

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/video_decoder...
File ppapi/cpp/dev/video_decoder_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/video_decoder...
ppapi/cpp/dev/video_decoder_dev.cc:39: return
callback.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ? here and below

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/graphics_2d.cc
File ppapi/cpp/graphics_2d.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/graphics_2d.cc#ne...
ppapi/cpp/graphics_2d.cc:103: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ? here and below

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/private/flash_men...
File ppapi/cpp/private/flash_menu.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/private/flash_men...
ppapi/cpp/private/flash_menu.cc:39: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ?

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/private/flash_net...
File ppapi/cpp/private/flash_net_connector.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/private/flash_net...
ppapi/cpp/private/flash_net_connector.cc:41: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ? here and below

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/url_loader.cc
File ppapi/cpp/url_loader.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/url_loader.cc#new...
ppapi/cpp/url_loader.cc:52: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
no need for static_cast ? here and below

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_surface_3d_...
File ppapi/proxy/ppb_surface_3d_proxy.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_surface_3d_...
ppapi/proxy/ppb_surface_3d_proxy.cc:91: if (object->is_flush_pending())
Add braces around the next 2 lines

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_url_loader_...
File ppapi/proxy/ppb_url_loader_proxy.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_url_loader_...
ppapi/proxy/ppb_url_loader_proxy.cc:252: if (!buffer || bytes_to_read <= 0)
add braces around the next 2 lines

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_url_loader_...
ppapi/proxy/ppb_url_loader_proxy.cc:255: if
(object->current_read_callback_.func)
add braces around the next 2 lines

http://codereview.chromium.org/6899055/diff/44001/ppapi/tests/test_case.h
File ppapi/tests/test_case.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/tests/test_case.h#new...
ppapi/tests/test_case.h:128: instance_->LogTest(#name, Test##name()); \
Wouldn't it be useful to log different strings in the async vs sync case ?

http://codereview.chromium.org/6899055/diff/44001/webkit/plugins/ppapi/common.h
File webkit/plugins/ppapi/common.h (right):

http://codereview.chromium.org/6899055/diff/44001/webkit/plugins/ppapi/common...
webkit/plugins/ppapi/common.h:26: int32_t MayForceCallback(PP_CompletionCallback
callback, int32_t result);
Can you re-use the one in ppapi/thunk instead of duplicating ? All the
interfaces will be converted to it eventually anyway.

http://codereview.chromium.org/6899055/diff/44001/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/6899055/diff/44001/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:64: int32_t result
=decoder->Initialize(decoder_config, callback);
nit: space after =

[polina, 2011-06-09 23:53:51]

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
File ppapi/c/pp_completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
ppapi/c/pp_completion_callback.h:91: int32_t flags;
On 2011/06/07 17:32:14, piman wrote:
> Because you're changing the size (and semantics) of PP_CompletionCallback, you
> need to rev all or the interfaces that uses a PP_CompletionCallback

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
ppapi/c/pp_completion_callback.h:123: cc.flags =
PP_COMPLETIONCALLBACK_FLAG_NONE;
On 2011/06/07 17:32:14, piman wrote:
> I thought we were not going to change the default behavior. Shouldn't this be
> PP_COMPLETIONCALLBACK_FLAG_OPTIONAL 

You have it backwards. The idea was to have the new default behavior - always
forcing the callbacks. See the bug by darin (linked from the CL description).

There was a discussion about the specifics and naming of the enum fields. I
reflected the resolution of the most recent discussion here.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
File ppapi/cpp/completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:49: return  (cc_.func == NULL ||
On 2011/06/07 17:32:14, piman wrote:
> nit: only 1 space after return

good eye! Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:50: (cc_.flags &
PP_COMPLETIONCALLBACK_FLAG_OPTIONAL) != 0);
On 2011/06/07 17:32:14, piman wrote:
> nit: Indent looks wrong

could you be more specific please?

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/directory_rea...
File ppapi/cpp/dev/directory_reader_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/directory_rea...
ppapi/cpp/dev/directory_reader_dev.cc:38: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> Why the need for static_cast ? It should cast implicitly.

I added them after getting compiler errors, but I don't get them anymore... Not
sure what happened. I took out all the casts now.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_chooser_...
File ppapi/cpp/dev/file_chooser_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_chooser_...
ppapi/cpp/dev/file_chooser_dev.cc:39: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ?

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_io_dev.cc
File ppapi/cpp/dev/file_io_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_io_dev.c...
ppapi/cpp/dev/file_io_dev.cc:44: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ? Here an below.

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_ref_dev.cc
File ppapi/cpp/dev/file_ref_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_ref_dev....
ppapi/cpp/dev/file_ref_dev.cc:72: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ? here and below

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_system_d...
File ppapi/cpp/dev/file_system_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/file_system_d...
ppapi/cpp/dev/file_system_dev.cc:35: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ?

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/surface_3d_de...
File ppapi/cpp/dev/surface_3d_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/surface_3d_de...
ppapi/cpp/dev/surface_3d_dev.cc:43: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ?

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/transport_dev.cc
File ppapi/cpp/dev/transport_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/transport_dev...
ppapi/cpp/dev/transport_dev.cc:41: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ? here and below

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/video_decoder...
File ppapi/cpp/dev/video_decoder_dev.cc (left):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/video_decoder...
ppapi/cpp/dev/video_decoder_dev.cc:76: if (!pp_resource())
On 2011/06/07 17:32:14, piman wrote:
> Why removing the check for pp_resource ?

This check must be and is performed by the C function (which is why no other C++
wrapper does it).

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/video_decoder...
File ppapi/cpp/dev/video_decoder_dev.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/dev/video_decoder...
ppapi/cpp/dev/video_decoder_dev.cc:39: return
callback.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ? here and below

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/graphics_2d.cc
File ppapi/cpp/graphics_2d.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/graphics_2d.cc#ne...
ppapi/cpp/graphics_2d.cc:103: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ? here and below

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/private/flash_men...
File ppapi/cpp/private/flash_menu.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/private/flash_men...
ppapi/cpp/private/flash_menu.cc:39: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ?

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/private/flash_net...
File ppapi/cpp/private/flash_net_connector.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/private/flash_net...
ppapi/cpp/private/flash_net_connector.cc:41: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ? here and below

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/url_loader.cc
File ppapi/cpp/url_loader.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/url_loader.cc#new...
ppapi/cpp/url_loader.cc:52: return
cc.MayForce(static_cast<int32_t>(PP_ERROR_NOINTERFACE));
On 2011/06/07 17:32:14, piman wrote:
> no need for static_cast ? here and below

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_surface_3d_...
File ppapi/proxy/ppb_surface_3d_proxy.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_surface_3d_...
ppapi/proxy/ppb_surface_3d_proxy.cc:91: if (object->is_flush_pending())
On 2011/06/07 17:32:14, piman wrote:
> Add braces around the next 2 lines

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_url_loader_...
File ppapi/proxy/ppb_url_loader_proxy.cc (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_url_loader_...
ppapi/proxy/ppb_url_loader_proxy.cc:252: if (!buffer || bytes_to_read <= 0)
On 2011/06/07 17:32:14, piman wrote:
> add braces around the next 2 lines

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/proxy/ppb_url_loader_...
ppapi/proxy/ppb_url_loader_proxy.cc:255: if
(object->current_read_callback_.func)
On 2011/06/07 17:32:14, piman wrote:
> add braces around the next 2 lines

Done.

http://codereview.chromium.org/6899055/diff/44001/ppapi/tests/test_case.h
File ppapi/tests/test_case.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/tests/test_case.h#new...
ppapi/tests/test_case.h:128: instance_->LogTest(#name, Test##name()); \
On 2011/06/07 17:32:14, piman wrote:
> Wouldn't it be useful to log different strings in the async vs sync case ?

Done.

http://codereview.chromium.org/6899055/diff/44001/webkit/plugins/ppapi/common.h
File webkit/plugins/ppapi/common.h (right):

http://codereview.chromium.org/6899055/diff/44001/webkit/plugins/ppapi/common...
webkit/plugins/ppapi/common.h:26: int32_t MayForceCallback(PP_CompletionCallback
callback, int32_t result);
On 2011/06/07 17:32:14, piman wrote:
> Can you re-use the one in ppapi/thunk instead of duplicating ? All the
> interfaces will be converted to it eventually anyway.

Done.

http://codereview.chromium.org/6899055/diff/44001/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/6899055/diff/44001/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:64: int32_t result
=decoder->Initialize(decoder_config, callback);
On 2011/06/07 17:32:14, piman wrote:
> nit: space after =

Done.

[polina, 2011-06-09 23:55:21]

What did you mean by "stuff in ppapi/proxy will need to be changed as well"?
There are already ppapi/proxy files in the CL. Are there some files in
ppapi/proxy that I missed?

Thanks,
P.

[piman, 2011-06-10 00:51:19]

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
File ppapi/c/pp_completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
ppapi/c/pp_completion_callback.h:123: cc.flags =
PP_COMPLETIONCALLBACK_FLAG_NONE;
On 2011/06/09 23:53:51, polina wrote:
> On 2011/06/07 17:32:14, piman wrote:
> > I thought we were not going to change the default behavior. Shouldn't this
be
> > PP_COMPLETIONCALLBACK_FLAG_OPTIONAL 
> 
> You have it backwards. The idea was to have the new default behavior - always
> forcing the callbacks. See the bug by darin (linked from the CL description).
> 
> There was a discussion about the specifics and naming of the enum fields. I
> reflected the resolution of the most recent discussion here.

But that breaks existing plugins.... Can we do a 2-step process, first we add
the flag, then we make it the default ?

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
File ppapi/cpp/completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:50: (cc_.flags &
PP_COMPLETIONCALLBACK_FLAG_OPTIONAL) != 0);
On 2011/06/09 23:53:51, polina wrote:
> On 2011/06/07 17:32:14, piman wrote:
> > nit: Indent looks wrong
> 
> could you be more specific please?

4 spaces for continuations ?

[piman, 2011-06-10 00:57:24]

On Thu, Jun 9, 2011 at 4:55 PM, <polina@google.com> wrote:

> What did you mean by "stuff in ppapi/proxy will need to be changed as
> well"?
> There are already ppapi/proxy files in the CL. Are there some files in
> ppapi/proxy that I missed?
>

Well, there's several places where we instantiate callbacks, with the
expectation that it isn't run if we have a synchronous result. This CL
changes them to be forced asynchronous. Either the former behavior needs to
be restored for these callbacks, or (if appropriate) the code needs to be
fixed to use the forced-asynchronous callback.

Antoine


>
> Thanks,
> P.
>
>
>
http://codereview.chromium.**org/6899055/<http://codereview.chromium.org/6899...
>

[polina, 2011-06-10 19:33:18]

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
File ppapi/c/pp_completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callb...
ppapi/c/pp_completion_callback.h:123: cc.flags =
PP_COMPLETIONCALLBACK_FLAG_NONE;
On 2011/06/10 00:51:20, piman wrote:
> On 2011/06/09 23:53:51, polina wrote:
> > On 2011/06/07 17:32:14, piman wrote:
> > > I thought we were not going to change the default behavior. Shouldn't this
> be
> > > PP_COMPLETIONCALLBACK_FLAG_OPTIONAL 
> > 
> > You have it backwards. The idea was to have the new default behavior -
always
> > forcing the callbacks. See the bug by darin (linked from the CL
description).
> > 
> > There was a discussion about the specifics and naming of the enum fields. I
> > reflected the resolution of the most recent discussion here.
> 
> But that breaks existing plugins.... Can we do a 2-step process, first we add
> the flag, then we make it the default ?

This should not break any existing plugins that properly handle return codes
from functions taking callbacks. They should have the logic for
PP_OK_COMPLETIONPENDING (which is still applicable as-is) and the logic for
other values (which will now be dead code).

Do you have a specific case in mind that will suffer from this? The only thing I
can think of is tests that assert that PP_OK_COMPLETIONPENDING is not returned.

Btw, if I make this a 2-step process, will I have to rev the interfaces twice?

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
File ppapi/cpp/completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:50: (cc_.flags &
PP_COMPLETIONCALLBACK_FLAG_OPTIONAL) != 0);
On 2011/06/10 00:51:20, piman wrote:
> On 2011/06/09 23:53:51, polina wrote:
> > On 2011/06/07 17:32:14, piman wrote:
> > > nit: Indent looks wrong
> > 
> > could you be more specific please?
> 
> 4 spaces for continuations ?

Not in this case because we have () and break after an operator.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Return_Values

[piman, 2011-06-10 19:49:02]

On Fri, Jun 10, 2011 at 12:33 PM, <polina@google.com> wrote:

>
> http://codereview.chromium.**org/6899055/diff/44001/ppapi/**
>
c/pp_completion_callback.h<http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callback.h>
> File ppapi/c/pp_completion_**callback.h (right):
>
> http://codereview.chromium.**org/6899055/diff/44001/ppapi/**
>
c/pp_completion_callback.h#**newcode123<http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callback.h#newcode123>
> ppapi/c/pp_completion_**callback.h:123: cc.flags =
> PP_COMPLETIONCALLBACK_FLAG_**NONE;
> On 2011/06/10 00:51:20, piman wrote:
>
>> On 2011/06/09 23:53:51, polina wrote:
>> > On 2011/06/07 17:32:14, piman wrote:
>> > > I thought we were not going to change the default behavior.
>>
> Shouldn't this
>
>> be
>> > > PP_COMPLETIONCALLBACK_FLAG_**OPTIONAL
>> >
>> > You have it backwards. The idea was to have the new default behavior
>>
> - always
>
>> > forcing the callbacks. See the bug by darin (linked from the CL
>>
> description).
>
>> >
>> > There was a discussion about the specifics and naming of the enum
>>
> fields. I
>
>> > reflected the resolution of the most recent discussion here.
>>
>
>  But that breaks existing plugins.... Can we do a 2-step process, first
>>
> we add
>
>> the flag, then we make it the default ?
>>
>
> This should not break any existing plugins that properly handle return
> codes from functions taking callbacks. They should have the logic for
> PP_OK_COMPLETIONPENDING (which is still applicable as-is) and the logic
> for other values (which will now be dead code).
>
> Do you have a specific case in mind that will suffer from this? The only
> thing I can think of is tests that assert that PP_OK_COMPLETIONPENDING
> is not returned.
>

I have at least one case in mind. When we use PPB_URLLoader, we call
ReadResponseBody that may return immediately if the bytes are there. We keep
calling it until it returns PP_OK_COMPLETIONPENDING to get as many bytes as
possible. Otherwise we are limited to one call per message loop cycle,
making it essentially one call per frame, leading to very slow streaming
bandwidth, making some videos not play.
So while it will not crash, it will not work either.


> Btw, if I make this a 2-step process, will I have to rev the interfaces
> twice?


I think we wouldn't need to, but we need to make sure we properly notify
pepper-team when the change goes in.


> http://codereview.chromium.**org/6899055/diff/44001/ppapi/**
>
cpp/completion_callback.h<http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callback.h>
> File ppapi/cpp/completion_callback.**h (right):
>
> http://codereview.chromium.**org/6899055/diff/44001/ppapi/**
>
cpp/completion_callback.h#**newcode50<http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callback.h#newcode50>
> ppapi/cpp/completion_callback.**h:50: (cc_.flags &
> PP_COMPLETIONCALLBACK_FLAG_**OPTIONAL) != 0);
> On 2011/06/10 00:51:20, piman wrote:
>
>> On 2011/06/09 23:53:51, polina wrote:
>> > On 2011/06/07 17:32:14, piman wrote:
>> > > nit: Indent looks wrong
>> >
>> > could you be more specific please?
>>
>
>  4 spaces for continuations ?
>>
>
> Not in this case because we have () and break after an operator.
> http://google-styleguide.**googlecode.com/svn/trunk/**
>
cppguide.xml#Return_Values<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Return_Values>
>
> I see, fair enough.


>
>
http://codereview.chromium.**org/6899055/<http://codereview.chromium.org/6899...
>

[polina, 2011-06-21 21:55:39]

On 2011/06/10 19:49:02, piman wrote:
> On Fri, Jun 10, 2011 at 12:33 PM, <mailto:polina@google.com> wrote:
> 
> >
> > http://codereview.chromium.**org/6899055/diff/44001/ppapi/**
> >
>
c/pp_completion_callback.h<http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callback.h>
> > File ppapi/c/pp_completion_**callback.h (right):
> >
> > http://codereview.chromium.**org/6899055/diff/44001/ppapi/**
> >
>
c/pp_completion_callback.h#**newcode123<http://codereview.chromium.org/6899055/diff/44001/ppapi/c/pp_completion_callback.h#newcode123>
> > ppapi/c/pp_completion_**callback.h:123: cc.flags =
> > PP_COMPLETIONCALLBACK_FLAG_**NONE;
> > On 2011/06/10 00:51:20, piman wrote:
> >
> >> On 2011/06/09 23:53:51, polina wrote:
> >> > On 2011/06/07 17:32:14, piman wrote:
> >> > > I thought we were not going to change the default behavior.
> >>
> > Shouldn't this
> >
> >> be
> >> > > PP_COMPLETIONCALLBACK_FLAG_**OPTIONAL
> >> >
> >> > You have it backwards. The idea was to have the new default behavior
> >>
> > - always
> >
> >> > forcing the callbacks. See the bug by darin (linked from the CL
> >>
> > description).
> >
> >> >
> >> > There was a discussion about the specifics and naming of the enum
> >>
> > fields. I
> >
> >> > reflected the resolution of the most recent discussion here.
> >>
> >
> >  But that breaks existing plugins.... Can we do a 2-step process, first
> >>
> > we add
> >
> >> the flag, then we make it the default ?
> >>
> >
> > This should not break any existing plugins that properly handle return
> > codes from functions taking callbacks. They should have the logic for
> > PP_OK_COMPLETIONPENDING (which is still applicable as-is) and the logic
> > for other values (which will now be dead code).
> >
> > Do you have a specific case in mind that will suffer from this? The only
> > thing I can think of is tests that assert that PP_OK_COMPLETIONPENDING
> > is not returned.
> >
> 
> I have at least one case in mind. When we use PPB_URLLoader, we call
> ReadResponseBody that may return immediately if the bytes are there. We keep
> calling it until it returns PP_OK_COMPLETIONPENDING to get as many bytes as
> possible. Otherwise we are limited to one call per message loop cycle,
> making it essentially one call per frame, leading to very slow streaming
> bandwidth, making some videos not play.
> So while it will not crash, it will not work either.
> 
> 
> > Btw, if I make this a 2-step process, will I have to rev the interfaces
> > twice?
> 
> 
> I think we wouldn't need to, but we need to make sure we properly notify
> pepper-team when the change goes in.
> 
> 
> > http://codereview.chromium.**org/6899055/diff/44001/ppapi/**
> >
>
cpp/completion_callback.h<http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callback.h>
> > File ppapi/cpp/completion_callback.**h (right):
> >
> > http://codereview.chromium.**org/6899055/diff/44001/ppapi/**
> >
>
cpp/completion_callback.h#**newcode50<http://codereview.chromium.org/6899055/diff/44001/ppapi/cpp/completion_callback.h#newcode50>
> > ppapi/cpp/completion_callback.**h:50: (cc_.flags &
> > PP_COMPLETIONCALLBACK_FLAG_**OPTIONAL) != 0);
> > On 2011/06/10 00:51:20, piman wrote:
> >
> >> On 2011/06/09 23:53:51, polina wrote:
> >> > On 2011/06/07 17:32:14, piman wrote:
> >> > > nit: Indent looks wrong
> >> >
> >> > could you be more specific please?
> >>
> >
> >  4 spaces for continuations ?
> >>
> >
> > Not in this case because we have () and break after an operator.
> > http://google-styleguide.**googlecode.com/svn/trunk/**
> >
>
cppguide.xml#Return_Values<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Return_Values>
> >
> > I see, fair enough.
> 
> 
> >
> >
>
http://codereview.chromium.**org/6899055/%3Chttp://codereview.chromium.org/68...>
> >

PTAL

[polina, 2011-06-21 22:05:08]

Last message is looking messy in my Inbox. So just in case:

PTAL

[piman, 2011-06-21 22:40:45]

LGTM, thanks !

[brettw, 2011-06-21 22:56:41]

LGTM, just some nits, don't need to wait for another pass from me.

http://codereview.chromium.org/6899055/diff/70001/ppapi/c/pp_completion_callb...
File ppapi/c/pp_completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/70001/ppapi/c/pp_completion_callb...
ppapi/c/pp_completion_callback.h:82: /* Any method that takes a
PP_CompletionCallback can complete asynchronously.
You lost the ** here which I assume is needed for doxygen.

http://codereview.chromium.org/6899055/diff/70001/ppapi/cpp/completion_callba...
File ppapi/cpp/completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/70001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:56: int32_t MayForce(int32_t result) const {
Can you provide a comment for this? It's a bit hard to see what the intended
usage is looking at the code.

http://codereview.chromium.org/6899055/diff/70001/ppapi/tests/test_directory_...
File ppapi/tests/test_directory_reader.cc (right):

http://codereview.chromium.org/6899055/diff/70001/ppapi/tests/test_directory_...
ppapi/tests/test_directory_reader.cc:155: if (force_async_ && rv !=
PP_OK_COMPLETIONPENDING)
Nit: needs {} according to Chrome style since it's >1 line.

http://codereview.chromium.org/6899055/diff/70001/ppapi/tests/test_directory_...
ppapi/tests/test_directory_reader.cc:202: if (force_async_ && rv !=
PP_OK_COMPLETIONPENDING)
Ditto

[polina, 2011-06-21 23:40:01]

Thanks for the super speedy replies!

Waiting for the trybots to finish and then committing.

http://codereview.chromium.org/6899055/diff/70001/ppapi/c/pp_completion_callb...
File ppapi/c/pp_completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/70001/ppapi/c/pp_completion_callb...
ppapi/c/pp_completion_callback.h:82: /* Any method that takes a
PP_CompletionCallback can complete asynchronously.
On 2011/06/21 22:56:41, brettw wrote:
> You lost the ** here which I assume is needed for doxygen.

Done.

http://codereview.chromium.org/6899055/diff/70001/ppapi/cpp/completion_callba...
File ppapi/cpp/completion_callback.h (right):

http://codereview.chromium.org/6899055/diff/70001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:56: int32_t MayForce(int32_t result) const {
On 2011/06/21 22:56:41, brettw wrote:
> Can you provide a comment for this? It's a bit hard to see what the intended
> usage is looking at the code.

Done.

http://codereview.chromium.org/6899055/diff/70001/ppapi/tests/test_directory_...
File ppapi/tests/test_directory_reader.cc (right):

http://codereview.chromium.org/6899055/diff/70001/ppapi/tests/test_directory_...
ppapi/tests/test_directory_reader.cc:155: if (force_async_ && rv !=
PP_OK_COMPLETIONPENDING)
On 2011/06/21 22:56:41, brettw wrote:
> Nit: needs {} according to Chrome style since it's >1 line.

Done.

http://codereview.chromium.org/6899055/diff/70001/ppapi/tests/test_directory_...
ppapi/tests/test_directory_reader.cc:202: if (force_async_ && rv !=
PP_OK_COMPLETIONPENDING)
On 2011/06/21 22:56:41, brettw wrote:
> Ditto

Done.

[brettw, 2011-06-27 23:59:03]

http://codereview.chromium.org/6899055/diff/78029/ppapi/tests/test_utils.cc
File ppapi/tests/test_utils.cc (right):

http://codereview.chromium.org/6899055/diff/78029/ppapi/tests/test_utils.cc#n...
ppapi/tests/test_utils.cc:40: : result_(PP_OK_COMPLETIONPENDING),
You forgot to initialize have_result. This caused a lot of failures for me and I
spent some time tracking it down to this.

[polina, 2011-06-28 00:11:26]

http://codereview.chromium.org/6899055/diff/78029/ppapi/tests/test_utils.cc
File ppapi/tests/test_utils.cc (right):

http://codereview.chromium.org/6899055/diff/78029/ppapi/tests/test_utils.cc#n...
ppapi/tests/test_utils.cc:40: : result_(PP_OK_COMPLETIONPENDING),
On 2011/06/27 23:59:03, brettw wrote:
> You forgot to initialize have_result. This caused a lot of failures for me and
I
> spent some time tracking it down to this.

Lost somebody else's new field during a sync. Thanks for catching this!

[brettw, 2011-06-29 15:41:09]

On Mon, Jun 27, 2011 at 5:11 PM,  <polina@google.com> wrote:
>
> http://codereview.chromium.org/6899055/diff/78029/ppapi/tests/test_utils.cc
> File ppapi/tests/test_utils.cc (right):
>
>
http://codereview.chromium.org/6899055/diff/78029/ppapi/tests/test_utils.cc#n...
> ppapi/tests/test_utils.cc:40: : result_(PP_OK_COMPLETIONPENDING),
> On 2011/06/27 23:59:03, brettw wrote:
>>
>> You forgot to initialize have_result. This caused a lot of failures
>
> for me and I
>>
>> spent some time tracking it down to this.
>
> Lost somebody else's new field during a sync. Thanks for catching this!

I did some more testing on Windows and I can't figure out why it's
slow sometimes. When I run the URLLoader tests in either sync or async
mode it's fast, but sometimes the StreamToFile one is super slow when
run back-to-back.

I would prefer not to mark this as failing on Windows since then we'd
lose test coverage. If we're still seeing timeouts, I'd prefer to just
do the URLLoader ones as either sync or async for now. Probably the
simple handling of the sync flag is the least likely thing to break in
the URLLoader at this point.

Brett

[polina, 2011-06-29 20:16:44]

Last trybot run had no timeouts on Linux or Windows. Only Transport timed
out on Mac.
I will do a couple more runs today to get more deterministic results.
I am also happy to change some tests to only be sync and async.

P.

On Wed, Jun 29, 2011 at 8:41 AM, Brett Wilson <brettw@chromium.org> wrote:

> On Mon, Jun 27, 2011 at 5:11 PM,  <polina@google.com> wrote:
> >
> >
> http://codereview.chromium.org/6899055/diff/78029/ppapi/tests/test_utils.cc
> > File ppapi/tests/test_utils.cc (right):
> >
> >
>
http://codereview.chromium.org/6899055/diff/78029/ppapi/tests/test_utils.cc#n...
> > ppapi/tests/test_utils.cc:40: : result_(PP_OK_COMPLETIONPENDING),
> > On 2011/06/27 23:59:03, brettw wrote:
> >>
> >> You forgot to initialize have_result. This caused a lot of failures
> >
> > for me and I
> >>
> >> spent some time tracking it down to this.
> >
> > Lost somebody else's new field during a sync. Thanks for catching this!
>
> I did some more testing on Windows and I can't figure out why it's
> slow sometimes. When I run the URLLoader tests in either sync or async
> mode it's fast, but sometimes the StreamToFile one is super slow when
> run back-to-back.
>
> I would prefer not to mark this as failing on Windows since then we'd
> lose test coverage. If we're still seeing timeouts, I'd prefer to just
> do the URLLoader ones as either sync or async for now. Probably the
> simple handling of the sync flag is the least likely thing to break in
> the URLLoader at this point.
>
> Brett
>

[brettw, 2011-06-29 20:21:42]

On Wed, Jun 29, 2011 at 1:16 PM, Polina Sokolova <polina@google.com> wrote:
> Last trybot run had no timeouts on Linux or Windows. Only Transport timed
> out on Mac.
> I will do a couple more runs today to get more deterministic results.
> I am also happy to change some tests to only be sync and async.

Yeah, transport was already kind of slow, so I can imagine that making
it twice as slow could go over the timeout. Let's just make that
either sync or async and not worry about it.

Brett

[polina, 2011-06-30 21:43:44]

On 2011/06/29 20:21:42, brettw wrote:
> On Wed, Jun 29, 2011 at 1:16 PM, Polina Sokolova <mailto:polina@google.com>
wrote:
> > Last trybot run had no timeouts on Linux or Windows. Only Transport timed
> > out on Mac.
> > I will do a couple more runs today to get more deterministic results.
> > I am also happy to change some tests to only be sync and async.
> 
> Yeah, transport was already kind of slow, so I can imagine that making
> it twice as slow could go over the timeout. Let's just make that
> either sync or async and not worry about it.
> 
> Brett

Committed revision 91205.