Convert to new enter method

Convert most users in thunk that used MayForceCallback to use the new Enter... format that does callback validation and forcing in its destructor.

This adds the same support for callback tracking for EnterFunction... as my previous patch did for EnterResource.

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

[brettw, 2012-01-25 00:44:21]

This is the follow-on to this code review:
https://chromiumcodereview.appspot.com/9235003
to convert most of the rest of the stuff. It should be very mechanical.

The tricky part, which I double checked but would be worth triple-checking) is
that any place I changed, I added the callback parameter to the constructor of
the Enter object. Otherwise, it won't know there's a callback and won't do the
new version of "MayForceCallback" in its destructor.

The more obvious things which I was less likely to screw up is that the
functions should return enter.retval() on error if the function returns an
int32_t. The enter object will soon be able to make different return values for
different things like thread usage, etc. so we'll need to propagate this. Also,
any changed function that takes a callback should use enter SetResult which will
make sure the callback will get the return value.

[yzshen1, 2012-01-25 19:11:00]

LGTM

with a few small issues:

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_dire...
File ppapi/thunk/ppb_directory_reader_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_dire...
ppapi/thunk/ppb_directory_reader_thunk.cc:33:
EnterResource<PPB_DirectoryReader_API> enter(directory_reader, true);
Yeah, find one! :)

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_file...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_file...
ppapi/thunk/ppb_file_ref_thunk.cc:58: return enter.retval();
This should return 0?

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_file...
File ppapi/thunk/ppb_file_system_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_file...
ppapi/thunk/ppb_file_system_thunk.cc:21: EnterFunction<ResourceCreationAPI>
enter(instance, true);
EnterResourceCreation?

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_flas...
File ppapi/thunk/ppb_flash_menu_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_flas...
ppapi/thunk/ppb_flash_menu_thunk.cc:19: EnterFunction<ResourceCreationAPI>
enter(instance, true);
Some places are changed to EnterResourceCreation, while others are unchanged.

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
File ppapi/thunk/ppb_layer_compositor_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
ppapi/thunk/ppb_layer_compositor_thunk.cc:48: return PP_ERROR_NOINTERFACE;
This breaks the convention of how |callback| is used.
I think it is better to still use the new enter as everyone else.

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_tran...
File ppapi/thunk/ppb_transport_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_tran...
ppapi/thunk/ppb_transport_thunk.cc:79: EnterTransport enter(transport, true);
callback, please.

[brettw, 2012-01-25 19:54:12]

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
File ppapi/thunk/ppb_layer_compositor_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
ppapi/thunk/ppb_layer_compositor_thunk.cc:48: return PP_ERROR_NOINTERFACE;
The problem is that since there's no implementation for this interface, there is
no "API" object to request, so there is nothing to "enter".

I'm OK leaking a callback if you use an interface we haven't implemented yet.

[yzshen1, 2012-01-25 20:06:26]

LGTM

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
File ppapi/thunk/ppb_layer_compositor_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
ppapi/thunk/ppb_layer_compositor_thunk.cc:48: return PP_ERROR_NOINTERFACE;
Okay. I am okay to leave it unchanged.

I just thought that even if there is no implementation, EnterResource will fail
gracefully (right?). And we can then use return
enter.SetResult(PP_ERROR_NOINTERFACE).


On 2012/01/25 19:54:12, brettw wrote:
> The problem is that since there's no implementation for this interface, there
is
> no "API" object to request, so there is nothing to "enter".
> 
> I'm OK leaking a callback if you use an interface we haven't implemented yet.

[brettw, 2012-01-26 05:17:39]

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
File ppapi/thunk/ppb_layer_compositor_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
ppapi/thunk/ppb_layer_compositor_thunk.cc:48: return PP_ERROR_NOINTERFACE;
On 2012/01/25 20:06:26, yzshen1 wrote:
> Okay. I am okay to leave it unchanged.
> 
> I just thought that even if there is no implementation, EnterResource will
fail
> gracefully (right?). And we can then use return
> enter.SetResult(PP_ERROR_NOINTERFACE).

Yeah, but EnterResource is a template, so I have to say EnterResource of
something. I don't want to say some random other resource class. I think
probably EnterResource<anything> would work, but that also seems slightly
misleading.

[brettw, 2012-01-26 05:22:00]

Thanks for the great review.

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_flas...
File ppapi/thunk/ppb_flash_menu_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_flas...
ppapi/thunk/ppb_flash_menu_thunk.cc:19: EnterFunction<ResourceCreationAPI>
enter(instance, true);
Yeah, I changed them when I saw them but I'm not trying to convert everything.
I'm planning on doing that in the future (there are a whole lot of places).

[yzshen1, 2012-01-27 18:25:47]

LGTM again. :)

[Not sure whether you are waiting for my response.]

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_flas...
File ppapi/thunk/ppb_flash_menu_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_flas...
ppapi/thunk/ppb_flash_menu_thunk.cc:19: EnterFunction<ResourceCreationAPI>
enter(instance, true);
Sounds good.

On 2012/01/26 05:22:00, brettw wrote:
> Yeah, I changed them when I saw them but I'm not trying to convert everything.
> I'm planning on doing that in the future (there are a whole lot of places).

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
File ppapi/thunk/ppb_layer_compositor_thunk.cc (right):

https://chromiumcodereview.appspot.com/9113044/diff/3021/ppapi/thunk/ppb_laye...
ppapi/thunk/ppb_layer_compositor_thunk.cc:48: return PP_ERROR_NOINTERFACE;
[Again, I am fine with the current approach.]
I thought PPB_LayerCompositor_API could be used in this case. Am I missing
anything?

On 2012/01/26 05:17:40, brettw wrote:
> On 2012/01/25 20:06:26, yzshen1 wrote:
> > Okay. I am okay to leave it unchanged.
> > 
> > I just thought that even if there is no implementation, EnterResource will
> fail
> > gracefully (right?). And we can then use return
> > enter.SetResult(PP_ERROR_NOINTERFACE).
> 
> Yeah, but EnterResource is a template, so I have to say EnterResource of
> something. I don't want to say some random other resource class. I think
> probably EnterResource<anything> would work, but that also seems slightly
> misleading.

[commit-bot: I haz the power, 2012-02-04 17:22:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/9113044/13001

[commit-bot: I haz the power, 2012-02-04 17:43:42]

Try job failure for 9113044-13001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-02-04 22:36:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/9113044/12003

[commit-bot: I haz the power, 2012-02-05 03:07:30]

Change committed as 120499