Fix unit test to allow repeated successful runs by avoiding a function static variable in tests.

Fix unit test to allow repeated successful runs by avoiding a function static variable in tests.

BUG=137145
TEST=run unit_tests with --gtest_filter=ComponentInstallerTest.PepperFlashCheck --gtest_repeat=2 successfully.


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

Reverted as http://src.chromium.org/viewvc/chrome?view=rev&revision=146672

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

[Scott Byer, 2012-07-12 22:53:02]

I ran across this while trying to solidify up some other tests. The test was
failing when using --gtest_repeat, seemed like something that should be fixed.

[brettw, 2012-07-12 23:11:40]

David: you added the
CR_DEFINE_STATIC_LOCAL(scoped_refptr<base::MessageLoopProxy>,

in ppapi_globals.cc. Is there any reason this couldn't just be a member of the
PpapiGlobals class? I think this would be a better way of fixing this problem.

http://codereview.chromium.org/10702188/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10702188/diff/1/chrome/chrome_tests.gypi#newco...
chrome/chrome_tests.gypi:2027: '../ppapi/shared_impl/test_globals.cc',
If we're going to end up with this solution, can you just add a project
dependency on ppapi_internal.gyp:ppapi_unittest_shared instead? This will be a
more maintainable solution than randomly including files in other directory's
projects.

[dmichael (off chromium), 2012-07-12 23:53:29]

On Jul 12, 2012 5:11 PM, <brettw@chromium.org> wrote:
>
> David: you added the
> CR_DEFINE_STATIC_LOCAL(scoped_refptr<base::MessageLoopProxy>,
>
> in ppapi_globals.cc. Is there any reason this couldn't just be a member
of the
> PpapiGlobals class? I think this would be a better way of fixing this
problem.
I'm on my phone so can't look in detail. But it's not a class static to
avoid introducing a static initializer. There are probably other ways to do
that, but this was cleanest at the time.

>
>
> http://codereview.chromium.org/10702188/diff/1/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
>
>
http://codereview.chromium.org/10702188/diff/1/chrome/chrome_tests.gypi#newco...
> chrome/chrome_tests.gypi:2027: '../ppapi/shared_impl/test_globals.cc',
> If we're going to end up with this solution, can you just add a project
> dependency on ppapi_internal.gyp:ppapi_unittest_shared instead? This
> will be a more maintainable solution than randomly including files in
> other directory's projects.
>
> http://codereview.chromium.org/10702188/

[brettw, 2012-07-12 23:57:56]

I was thinking it could just be a class member variable (not static at all).

Brett


On Thu, Jul 12, 2012 at 4:53 PM, David Michael <dmichael@chromium.org>wrote:

>
> On Jul 12, 2012 5:11 PM, <brettw@chromium.org> wrote:
> >
> > David: you added the
> > CR_DEFINE_STATIC_LOCAL(scoped_refptr<base::MessageLoopProxy>,
> >
> > in ppapi_globals.cc. Is there any reason this couldn't just be a member
> of the
> > PpapiGlobals class? I think this would be a better way of fixing this
> problem.
> I'm on my phone so can't look in detail. But it's not a class static to
> avoid introducing a static initializer. There are probably other ways to do
> that, but this was cleanest at the time.
>
> >
> >
> > http://codereview.chromium.org/10702188/diff/1/chrome/chrome_tests.gypi
> > File chrome/chrome_tests.gypi (right):
> >
> >
>
http://codereview.chromium.org/10702188/diff/1/chrome/chrome_tests.gypi#newco...
> > chrome/chrome_tests.gypi:2027: '../ppapi/shared_impl/test_globals.cc',
> > If we're going to end up with this solution, can you just add a project
> > dependency on ppapi_internal.gyp:ppapi_unittest_shared instead? This
> > will be a more maintainable solution than randomly including files in
> > other directory's projects.
> >
> > http://codereview.chromium.org/10702188/
>

[Scott Byer, 2012-07-13 16:18:43]

+cpu for the component_updater part.

Went with a member variable; it does simplify things.

http://codereview.chromium.org/10702188/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10702188/diff/1/chrome/chrome_tests.gypi#newco...
chrome/chrome_tests.gypi:2027: '../ppapi/shared_impl/test_globals.cc',
On 2012/07/12 23:11:40, brettw wrote:
> If we're going to end up with this solution, can you just add a project
> dependency on ppapi_internal.gyp:ppapi_unittest_shared instead? This will be a
> more maintainable solution than randomly including files in other directory's
> projects.

Done. I had tried this originally, but my gyp-fu isn't quite there yet.

[brettw, 2012-07-13 16:36:22]

lgtm

http://codereview.chromium.org/10702188/diff/8001/chrome/browser/component_up...
File chrome/browser/component_updater/test/component_installers_unittest.cc
(right):

http://codereview.chromium.org/10702188/diff/8001/chrome/browser/component_up...
chrome/browser/component_updater/test/component_installers_unittest.cc:43:
scoped_ptr<ppapi::TestGlobals> for_test(
I think you can just make this on the stack rather than bothering with the
scoped_refptr/new junk.

http://codereview.chromium.org/10702188/diff/8001/ppapi/shared_impl/ppapi_glo...
File ppapi/shared_impl/ppapi_globals.cc (right):

http://codereview.chromium.org/10702188/diff/8001/ppapi/shared_impl/ppapi_glo...
ppapi/shared_impl/ppapi_globals.cc:47: message_loop_proxy_ =
base::MessageLoopProxy::current();
I think you should just set this in the constructor rather than trying to do it
lazily here.

http://codereview.chromium.org/10702188/diff/8001/ppapi/shared_impl/ppapi_glo...
File ppapi/shared_impl/ppapi_globals.h (right):

http://codereview.chromium.org/10702188/diff/8001/ppapi/shared_impl/ppapi_glo...
ppapi/shared_impl/ppapi_globals.h:109: // Virtual for testing.
I think you can revert this part.

[dmichael (off chromium), 2012-07-13 16:44:58]

ppapi_globals LGTM too, with Brett's comments.

http://codereview.chromium.org/10702188/diff/8001/ppapi/shared_impl/ppapi_glo...
File ppapi/shared_impl/ppapi_globals.cc (right):

http://codereview.chromium.org/10702188/diff/8001/ppapi/shared_impl/ppapi_glo...
ppapi/shared_impl/ppapi_globals.cc:47: message_loop_proxy_ =
base::MessageLoopProxy::current();
On 2012/07/13 16:36:22, brettw wrote:
> I think you should just set this in the constructor rather than trying to do
it
> lazily here.
Agree. The important thing is that base::MessageLoopProxy::current is called on
the main thread. Doing it in the constructor will do that nicely. And then we
can probably get rid of a "special" call to GetMainThreadMessageLoop:
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/plugins/ppapi/plugi...

[cpu_(ooo_6.6-7.5), 2012-07-13 17:56:44]

rubber stamp lgtm of the CU files.

[commit-bot: I haz the power, 2012-07-13 19:32:27]

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

[commit-bot: I haz the power, 2012-07-13 20:41:08]

Try job failure for 10702188-12001 (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-07-13 20:58:55]

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

[commit-bot: I haz the power, 2012-07-13 22:20:00]

Change committed as 146670

[commit-bot: I haz the power, 2012-07-13 22:46:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottbyer@chromium.org/10702188/22001

[commit-bot: I haz the power, 2012-07-14 00:05:12]

Change committed as 146690