Various fixes to make ppapi_unittests pass again.

Various fixes to make ppapi_unittests pass again.

The following fixes are contained in this patch:
-Construction/destruction of ppapi globals has been moved to the thread
on which those globals are used (to prevent thread-safety CHECKs firing).
-Some tests for the state of the var tracker in the plugin have been moved
onto the plugin thread (to prevent thread-safety CHECKs firing).
-Fixed a crash in ppp_instance_private_proxy_unittest.cc which was due
to not passing a |PPP_Class_Deprecated| to |PPB_Var_Deprecated->CreateObject|
which is required when deleting a var on the plugin side.
-Set up a |PluginProxyDelegate| in |PluginGlobals| so that a channel to the
browser can be correctly obtained for unittests.

BUG=none
TEST=Ran ppapi_unittests


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

[raymes, 2012-09-13 22:43:22]

No hurry for reviewing this.

[dmichael (off chromium), 2012-09-17 15:27:29]

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_tes...
File ppapi/proxy/ppapi_proxy_test.cc (right):

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_tes...
ppapi/proxy/ppapi_proxy_test.cc:158: return plugin_globals_.get();
What about explicitly setting it in SetUpHarness and SetUpHarnessWithChannel?
Those run on the correct thread. That will make it more certain that we do it in
the right place, I think.

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_tes...
ppapi/proxy/ppapi_proxy_test.cc:176: // |PluginPepperDelegate::GetBrowserSender|
which is used
PluginProxyDelegate

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_test.h
File ppapi/proxy/ppapi_proxy_test.h (right):

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_tes...
ppapi/proxy/ppapi_proxy_test.h:133: // PluginPepperDelegate implementation.
Can you change this to say PluginProxyDelegate, since you're here?

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppp_instance_pr...
File ppapi/proxy/ppp_instance_private_proxy_unittest.cc (right):

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppp_instance_pr...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:189: EXPECT_EQ(1, ref_count);
This is fine, but I'd also be okay with just disabling this test entirely for
builds that don't have ENABLE_PEPPER_THREADING. It won't be too long, and then
we won't need the extra gyrations that are really only there to appease the
ThreadChecker; there's no real race condition even without the locking.

WDYT?

[raymes, 2012-09-18 21:29:15]

Thanks for the review

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_tes...
File ppapi/proxy/ppapi_proxy_test.cc (right):

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_tes...
ppapi/proxy/ppapi_proxy_test.cc:158: return plugin_globals_.get();
On 2012/09/17 15:27:29, dmichael wrote:
> What about explicitly setting it in SetUpHarness and SetUpHarnessWithChannel?
> Those run on the correct thread. That will make it more certain that we do it
in
> the right place, I think.

Done.

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_tes...
ppapi/proxy/ppapi_proxy_test.cc:176: // |PluginPepperDelegate::GetBrowserSender|
which is used
On 2012/09/17 15:27:29, dmichael wrote:
> PluginProxyDelegate

Done.

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_test.h
File ppapi/proxy/ppapi_proxy_test.h (right):

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppapi_proxy_tes...
ppapi/proxy/ppapi_proxy_test.h:133: // PluginPepperDelegate implementation.
On 2012/09/17 15:27:29, dmichael wrote:
> Can you change this to say PluginProxyDelegate, since you're here?

Done.

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppp_instance_pr...
File ppapi/proxy/ppp_instance_private_proxy_unittest.cc (right):

http://codereview.chromium.org/10913258/diff/5002/ppapi/proxy/ppp_instance_pr...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:189: EXPECT_EQ(1, ref_count);
On 2012/09/17 15:27:29, dmichael wrote:
> This is fine, but I'd also be okay with just disabling this test entirely for
> builds that don't have ENABLE_PEPPER_THREADING. It won't be too long, and then
> we won't need the extra gyrations that are really only there to appease the
> ThreadChecker; there's no real race condition even without the locking.
> 
> WDYT?

Done.

[dmichael (off chromium), 2012-09-18 21:56:53]

lgtm

[commit-bot: I haz the power, 2012-09-18 22:02:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/10913258/12001

[commit-bot: I haz the power, 2012-09-19 00:55:43]

Change committed as 157469