Pepper: Use base::Callback in RequestNexeFd.

Pepper: Use base::Callback in RequestNexeFd.

GetNexeFd() in ppb_nacl_private_impl will change soon to be an internal method,
not one exposed through the PPB_NaCl_Private interface. From inside Chromium,
base::Callback is much easier to use than PP_CompletionCallback or
TrackedCallback.

This is one of a series of changes to refactor the pexe downloading logic in
the trusted plugin.

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

[dmichael (off chromium), 2014-07-09 17:38:31]

https://codereview.chromium.org/380773002/diff/20001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/380773002/diff/20001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:661:
base::Bind(&GetNexeFdContinuation, callback, is_hit, handle));
Can you use TrackedCallback instead of PP_CompletionCallback here? I.e., pass
enter.callback() instead of just callback, and change the type in
GetNexeFdContinuation?

TrackedCallback normally expects that once it's created, it's the only thing
that invokes the callback. I think it happens to work in this case, but if the
Callback had a Resource associated with it, it would get confused not knowing
that the callback was invoked outside of it, and would end up calling the
callback with PP_ERROR_ABORTED eventually when the resource was destroyed.
That's what makes it "Tracked". In theory, we should  maybe be aborting at
instance shutdown for ones like this, but we don't right now.

You could even use TrackedCallback::set_completion_task if you want, then just
Bind to TrackedCallback::Run here. It's useful for real APIs to make sure that
out-params are set on the correct thread just before firing the callback, which
helps with some weird race conditions. Arguably overkill here. But in real APIs,
it's the "right" way.

If you strongly prefer not using TrackedCallback at all, you could instead go
back to your other approach of not using EnterInstance.

[teravest, 2014-07-09 17:55:52]

On Wed, Jul 9, 2014 at 11:38 AM,  <dmichael@chromium.org> wrote:
>
>
https://codereview.chromium.org/380773002/diff/20001/components/nacl/renderer...
> File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
>
>
https://codereview.chromium.org/380773002/diff/20001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:661:
>
> base::Bind(&GetNexeFdContinuation, callback, is_hit, handle));
> Can you use TrackedCallback instead of PP_CompletionCallback here? I.e.,
> pass enter.callback() instead of just callback, and change the type in
> GetNexeFdContinuation?

Changed to use TrackedCallback. I don't like it, but I guess it's fine for now.
This is going to change to be a base::Callback soon anyway. I just
want to tidy this up so I can call GetNexeFd() reasonably from inside
Chromium.

>
> TrackedCallback normally expects that once it's created, it's the only
> thing that invokes the callback. I think it happens to work in this
> case, but if the Callback had a Resource associated with it, it would
> get confused not knowing that the callback was invoked outside of it,
> and would end up calling the callback with PP_ERROR_ABORTED eventually
> when the resource was destroyed. That's what makes it "Tracked". In
> theory, we should  maybe be aborting at instance shutdown for ones like
> this, but we don't right now.
>
> You could even use TrackedCallback::set_completion_task if you want,
> then just Bind to TrackedCallback::Run here. It's useful for real APIs
> to make sure that out-params are set on the correct thread just before
> firing the callback, which helps with some weird race conditions.
> Arguably overkill here. But in real APIs, it's the "right" way.
>
> If you strongly prefer not using TrackedCallback at all, you could
> instead go back to your other approach of not using EnterInstance.
>
> https://codereview.chromium.org/380773002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-07-09 18:03:16]

lgtm

[teravest, 2014-07-09 18:03:52]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-07-09 18:04:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/380773002/40001

[commit-bot: I haz the power, 2014-07-09 20:33:13]

Change committed as 282112