PPAPI: Refactor ppapi test callbacks to ease testing blocking callbacks.

PPAPI: Refactor ppapi test callbacks to ease testing blocking callbacks.

There are really 3 changes to test_utils:
- Change "force_async_" to "callback_type_" so that we can represent REQUIRED, OPTIONAL, _and_ BLOCKING. (I left backwards compatibility with force_async_ and will follow up to change them all after this CL).
- Add a new form of WaitForResult and a new CHECK_CALLBACK_BEHAVIOR macro. This simplifies checking that the callback ran as expected (i.e., asynchronous or not). test_url_loader.cc in this CL is a good example of the new form. (My intent is to remove the existing WaitForResult in the aforementioned follow-up CL).
- Add TestCompletionCallback::GetCallback. This is a minor thing, but it means we can get rid of a bunch of ugly "static_cast<pp::CompletionCallback>" in the tests (see test_websocket.cc for an example of that).

BUG=92909
TEST=


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

[dmichael (off chromium), 2012-04-02 21:15:02]

bbudge: primary
brettw: FYI

http://codereview.chromium.org/9937001/diff/7013/ppapi/proxy/ppb_network_moni...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9937001/diff/7013/ppapi/proxy/ppb_network_moni...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:101:
MessageLoop::current()->PostTask(FROM_HERE, RunWhileLocked(base::Bind(
somewhat unrelated, but causes problems running the tests locally for me, since
I'm building with enable_pepper_threading=1.

[bbudge, 2012-04-04 19:03:00]

This looks like it nicely simplifies a bunch of testing code. Just some
questions and naming comment. Otherwise, LGTM.

http://codereview.chromium.org/9937001/diff/7013/ppapi/proxy/ppb_network_moni...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9937001/diff/7013/ppapi/proxy/ppb_network_moni...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:101:
MessageLoop::current()->PostTask(FROM_HERE, RunWhileLocked(base::Bind(
Does it also cause problems in normal use? Is there a performance cost for
locking?

http://codereview.chromium.org/9937001/diff/7013/ppapi/tests/test_file_system.cc
File ppapi/tests/test_file_system.cc (right):

http://codereview.chromium.org/9937001/diff/7013/ppapi/tests/test_file_system...
ppapi/tests/test_file_system.cc:48: // completed or not.
s/no matter whether/whether or not?

http://codereview.chromium.org/9937001/diff/7013/ppapi/tests/test_utils.h
File ppapi/tests/test_utils.h (right):

http://codereview.chromium.org/9937001/diff/7013/ppapi/tests/test_utils.h#new...
ppapi/tests/test_utils.h:24: // Used for waiting for a nested event, such as a
callback on a PPP interface.
This comment makes me wonder about the name of the class. However, I can't think
of anything that isn't significantly longer than 'NestedEvent'.

[dmichael (off chromium), 2012-04-04 19:50:47]

http://codereview.chromium.org/9937001/diff/7013/ppapi/proxy/ppb_network_moni...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9937001/diff/7013/ppapi/proxy/ppb_network_moni...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:101:
MessageLoop::current()->PostTask(FROM_HERE, RunWhileLocked(base::Bind(
On 2012/04/04 19:03:00, bbudge1 wrote:
> Does it also cause problems in normal use? Is there a performance cost for
> locking?
Yes, but locking is not enabled yet, so RunWhileLocked ultimately just adds a
little function-call overhead. We'll have to do some benchmarking or something
to make sure locking is fast enough, and Graphics3D is going to need some help
to avoid too much locking. In this case, the cost of locking is negligible,
since NetworkLists aren't going to change very frequently.

The real problem here is one of correctness, when locking is enabled. The design
is to always hold the lock while in proxy code. Note that in
OnNetworkListReceived, we unlock the mutex before calling out to the plugin. The
code before this patch means we enter the proxy when the task is run and invokes
OnNetworkListReceived, and we do not have the lock. This is bad because (a) we
probably will have concurrency problems, and (b) we try to _unlock_ the lock in
OnNetworkListReceived, which fails if we don't own the lock.

http://codereview.chromium.org/9937001/diff/7013/ppapi/tests/test_file_system.cc
File ppapi/tests/test_file_system.cc (right):

http://codereview.chromium.org/9937001/diff/7013/ppapi/tests/test_file_system...
ppapi/tests/test_file_system.cc:48: // completed or not.
On 2012/04/04 19:03:00, bbudge1 wrote:
> s/no matter whether/whether or not?
changed to "regardless of whether or not the first open has completed".

http://codereview.chromium.org/9937001/diff/7013/ppapi/tests/test_utils.h
File ppapi/tests/test_utils.h (right):

http://codereview.chromium.org/9937001/diff/7013/ppapi/tests/test_utils.h#new...
ppapi/tests/test_utils.h:24: // Used for waiting for a nested event, such as a
callback on a PPP interface.
On 2012/04/04 19:03:00, bbudge1 wrote:
> This comment makes me wonder about the name of the class. However, I can't
think
> of anything that isn't significantly longer than 'NestedEvent'.
Agreed...  I don't love the name but can't think of anything better. As
discussed on IM, I tried to improve the documentation.

[dmichael (off chromium), 2012-04-06 18:37:24]

ping?

[bbudge, 2012-04-06 19:17:53]

still LGTM.

[commit-bot: I haz the power, 2012-04-06 19:23:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/9937001/12002

[commit-bot: I haz the power, 2012-04-06 21:58:39]

Try job failure for 9937001-12002 (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-06 22:10:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/9937001/12002

[commit-bot: I haz the power, 2012-04-07 00:09:12]

Change committed as 131215