Add test to make sure if PPB_Audio_Shared::StartThread() works.

Add test to make sure if PPB_Audio_Shared::StartThread() works.

To NaCl in either SFI or non-SFI mode, PPB_Audio_Shraed::StartThread requires 
ppapi_register_thread_creator invoked in advance.
Currently no test about it. This CL adds the test.

Tests are guarded by #ifdef __native_client__, because it
won't work and is meaningless for trusted plugins.

TEST=Ran browser_tests --gtest_filter=*AudioThreadCreation locally, and ran trybots.
BUG=359710
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264974

[Mark Seaborn, 2014-04-16 20:00:00]

https://codereview.chromium.org/240523002/diff/1/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/240523002/diff/1/ppapi/tests/test_audio.cc#ne...
ppapi/tests/test_audio.cc:36: Elf32_auxv_t* FindAuxv(void) {
You should be able to use nacl_interface_query() instead.  It's provided by
libc, in irt.h.  Then you don't need to find and read the auxv yourself.

[hidehiko, 2014-04-17 14:05:14]

Could you take a look?

Thanks,
- hidehiko

https://codereview.chromium.org/240523002/diff/1/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/240523002/diff/1/ppapi/tests/test_audio.cc#ne...
ppapi/tests/test_audio.cc:36: Elf32_auxv_t* FindAuxv(void) {
On 2014/04/16 20:00:00, Mark Seaborn wrote:
> You should be able to use nacl_interface_query() instead.  It's provided by
> libc, in irt.h.  Then you don't need to find and read the auxv yourself.

Great to know. Fixed.

https://codereview.chromium.org/240523002/diff/1/ppapi/tests/test_audio.cc#ne...
ppapi/tests/test_audio.cc:101:
DISALLOW_COPY_AND_ASSIGN(ScopedThreadFunctionsResetter);
Removed DISALLOW_COPY_AND_ASSIGN here, because, according to DEPS, it'd be
better to avoid depending on base/ libraries in this test. base/macros.h is just
for this macro, and as this is testee code, we don't have huge merit by this
macro IMHO. So I just removed.

[dmichael (off chromium), 2014-04-17 21:30:07]

bbudge@, would you be willing to look at this?

[bbudge, 2014-04-17 21:33:47]

On 2014/04/17 21:30:07, dmichael wrote:
> bbudge@, would you be willing to look at this?

Sure, no problem.

[bbudge, 2014-04-17 21:44:27]

https://codereview.chromium.org/240523002/diff/60001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/240523002/diff/60001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.cc:90: playing_ = false;
This makes the comment above false. It seems like setting playing_ false here
will cause audio to not start in this case.

[hidehiko, 2014-04-17 23:03:36]

Thank you for review. Could you take another look?

https://codereview.chromium.org/240523002/diff/60001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/240523002/diff/60001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.cc:90: playing_ = false;
On 2014/04/17 21:44:27, bbudge wrote:
> This makes the comment above false. It seems like setting playing_ false here
> will cause audio to not start in this case.

The comment above is true. StartThread() returns false only if thread creator
is not yet set. Added comment.

[bbudge, 2014-04-18 13:27:24]

https://codereview.chromium.org/240523002/diff/60001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/240523002/diff/60001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.cc:90: playing_ = false;
On 2014/04/17 23:03:37, hidehiko wrote:
> On 2014/04/17 21:44:27, bbudge wrote:
> > This makes the comment above false. It seems like setting playing_ false
here
> > will cause audio to not start in this case.
> 
> The comment above is true. StartThread() returns false only if thread creator
> is not yet set. Added comment.

Ah, I see now.

https://codereview.chromium.org/240523002/diff/80001/ppapi/proxy/ppb_audio_pr...
File ppapi/proxy/ppb_audio_proxy.cc (right):

https://codereview.chromium.org/240523002/diff/80001/ppapi/proxy/ppb_audio_pr...
ppapi/proxy/ppb_audio_proxy.cc:99: return PP_FALSE;
Could you check the thread creation hooks here instead of in
SetStartPlaybackState. I think that could be a lot simpler.

[bbudge, 2014-04-18 13:34:40]

https://codereview.chromium.org/240523002/diff/80001/ppapi/proxy/ppb_audio_pr...
File ppapi/proxy/ppb_audio_proxy.cc (right):

https://codereview.chromium.org/240523002/diff/80001/ppapi/proxy/ppb_audio_pr...
ppapi/proxy/ppb_audio_proxy.cc:99: return PP_FALSE;
On 2014/04/18 13:27:25, bbudge wrote:
> Could you check the thread creation hooks here instead of in
> SetStartPlaybackState. I think that could be a lot simpler.

Never mind, I see that those hooks are part of the shared object, so you have to
test them there.

https://codereview.chromium.org/240523002/diff/80001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/240523002/diff/80001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.cc:91: // Note that StartThread returns true
even if socket doesn't exist as it is
I think this last sentence just repeats the comment above and can be eliminated.

[bbudge, 2014-04-18 15:08:07]

https://codereview.chromium.org/240523002/diff/80001/ppapi/proxy/ppb_audio_pr...
File ppapi/proxy/ppb_audio_proxy.cc (right):

https://codereview.chromium.org/240523002/diff/80001/ppapi/proxy/ppb_audio_pr...
ppapi/proxy/ppb_audio_proxy.cc:99: return PP_FALSE;
On 2014/04/18 13:34:40, bbudge wrote:
> On 2014/04/18 13:27:25, bbudge wrote:
> > Could you check the thread creation hooks here instead of in
> > SetStartPlaybackState. I think that could be a lot simpler.
> 
> Never mind, I see that those hooks are part of the shared object, so you have
to
> test them there.

Another thought: add a protected method to the PPB_AudioShared that tests
whether the hooks were set. You could call it here. The SetStartPlaybackState
and StartThread methods are already complicated, so I'd like to avoid this
return value. WDYT?

[Mark Seaborn, 2014-04-18 15:28:20]

https://codereview.chromium.org/240523002/diff/80001/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/240523002/diff/80001/ppapi/tests/test_audio.c...
ppapi/tests/test_audio.cc:40: int g_num_thread_create_called = 0;
Could you also check that thread_join() is called?  The old PPAPI proxy had a
bug in which the thread wasn't joined properly, which was non-memory-safe.  It
would be nice to check for that by checking that thread_join() is called.

[hidehiko, 2014-04-18 18:16:29]

Thank you for review. Could you take another look?

https://codereview.chromium.org/240523002/diff/80001/ppapi/proxy/ppb_audio_pr...
File ppapi/proxy/ppb_audio_proxy.cc (right):

https://codereview.chromium.org/240523002/diff/80001/ppapi/proxy/ppb_audio_pr...
ppapi/proxy/ppb_audio_proxy.cc:99: return PP_FALSE;
I like your idea. How about this?


On 2014/04/18 15:08:08, bbudge wrote:
> On 2014/04/18 13:34:40, bbudge wrote:
> > On 2014/04/18 13:27:25, bbudge wrote:
> > > Could you check the thread creation hooks here instead of in
> > > SetStartPlaybackState. I think that could be a lot simpler.
> > 
> > Never mind, I see that those hooks are part of the shared object, so you
have
> to
> > test them there.
> 
> Another thought: add a protected method to the PPB_AudioShared that tests
> whether the hooks were set. You could call it here. The SetStartPlaybackState
> and StartThread methods are already complicated, so I'd like to avoid this
> return value. WDYT?

https://codereview.chromium.org/240523002/diff/80001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/240523002/diff/80001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.cc:91: // Note that StartThread returns true
even if socket doesn't exist as it is
On 2014/04/18 13:34:40, bbudge wrote:
> I think this last sentence just repeats the comment above and can be
eliminated.

Acknowledged.

https://codereview.chromium.org/240523002/diff/80001/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/240523002/diff/80001/ppapi/tests/test_audio.c...
ppapi/tests/test_audio.cc:40: int g_num_thread_create_called = 0;
On 2014/04/18 15:28:20, Mark Seaborn wrote:
> Could you also check that thread_join() is called?  The old PPAPI proxy had a
> bug in which the thread wasn't joined properly, which was non-memory-safe.  It
> would be nice to check for that by checking that thread_join() is called.

Done.

[bbudge, 2014-04-18 18:44:13]

Thanks. LGTM

https://codereview.chromium.org/240523002/diff/120001/ppapi/shared_impl/ppb_a...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/240523002/diff/120001/ppapi/shared_impl/ppb_a...
ppapi/shared_impl/ppb_audio_shared.h:85: // is invoked properly.
s/is/was

or 'has been'

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:404: // Makes sure the behavior of the thread_create
functions.
s/Makes sure/Tests

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:405: // To work PPB_Audio_Shared properly, the user
code must call
s/To work PPB_Audio_Shared properly/For PPB_Audio_Shared to work properly

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:408: // it checks whether the thread functions which
is passed from the user code
s/is/are

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:417: // by user code.
// First of all, set the thread functions to NULLs to emulate
// the situation where ppapi_register_thread_creator() is not called
// by user code.

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:428: // First, StartPlayback() fails, because no
thread creating funciton
sp function

[hidehiko, 2014-04-18 19:01:37]

Thank you for review, Bill. Mark, could you take another look, too?

https://codereview.chromium.org/240523002/diff/120001/ppapi/shared_impl/ppb_a...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/240523002/diff/120001/ppapi/shared_impl/ppb_a...
ppapi/shared_impl/ppb_audio_shared.h:85: // is invoked properly.
On 2014/04/18 18:44:14, bbudge wrote:
> s/is/was
> 
> or 'has been'

Done.

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:404: // Makes sure the behavior of the thread_create
functions.
On 2014/04/18 18:44:14, bbudge wrote:
> s/Makes sure/Tests

Done.

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:405: // To work PPB_Audio_Shared properly, the user
code must call
On 2014/04/18 18:44:14, bbudge wrote:
> s/To work PPB_Audio_Shared properly/For PPB_Audio_Shared to work properly

Done.

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:408: // it checks whether the thread functions which
is passed from the user code
On 2014/04/18 18:44:14, bbudge wrote:
> s/is/are

Done.

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:417: // by user code.
On 2014/04/18 18:44:14, bbudge wrote:
> // First of all, set the thread functions to NULLs to emulate
> // the situation where ppapi_register_thread_creator() is not called
> // by user code.

Done.

https://codereview.chromium.org/240523002/diff/120001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:428: // First, StartPlayback() fails, because no
thread creating funciton
On 2014/04/18 18:44:14, bbudge wrote:
> sp function

Done.

[Mark Seaborn, 2014-04-18 19:59:19]

LGTM with a suggestion.

https://codereview.chromium.org/240523002/diff/140001/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/240523002/diff/140001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:431: 
You might split this function, here, into two test cases:
TestAudioThreadCreatorIsRequired()
TestAudioThreadCreatorIsCalled()
-- just to make it clearer that these are separate properties being tested for.

[hidehiko, 2014-04-21 04:16:04]

The tests passed. Sending to CQ.

https://codereview.chromium.org/240523002/diff/140001/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/240523002/diff/140001/ppapi/tests/test_audio....
ppapi/tests/test_audio.cc:431: 
On 2014/04/18 19:59:19, Mark Seaborn wrote:
> You might split this function, here, into two test cases:
> TestAudioThreadCreatorIsRequired()
> TestAudioThreadCreatorIsCalled()
> -- just to make it clearer that these are separate properties being tested
for.

Done.

[hidehiko, 2014-04-21 04:16:34]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-21 04:16:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/240523002/160001

[commit-bot: I haz the power, 2014-04-21 10:36:23]

Change committed as 264974