Reland the CL 8162015.

Reland the CL 8162015.

CL 8162015 failed the trybot and was reverted.
But I could not reproduce the issue in my local machine, so I just make a new CL and test it on trybot again.

BUG=
TEST=


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

[no longer working on chromium, 2011-10-21 16:12:33]

Hi Andrew, my CL 8162015(http://codereview.chromium.org/8162015) failed the
trybot and was reverted. But the issue is not reproducable in my local machine,
so I just made this new CL and test it on trybot. If I pass the trybots this
time, I need a new lgt* from you to land it.

Tommi pointed out that I should use const std::string& in the AudioDeviceName()
constructor, I have already made the changes, but added him as a reviewer to see
he has any more input.

Thanks for all the help,

BR,
/SX

[scherkus (not reviewing), 2011-10-21 19:38:13]

In your the reverted patch it says the following:

"Reverted because this CL caused the Linux/Windows (shared) builds to fail."

http://crrev.com/106517

Here's how to build with shared libraries:
http://code.google.com/p/chromium/wiki/LinuxFasterBuilds#Shared_libraries_(co...

You're probably missing a MEDIA_EXPORT somewhere.

[no longer working on chromium, 2011-10-21 20:46:03]

On 2011/10/21 19:38:13, scherkus wrote:
> In your the reverted patch it says the following:
> 
> "Reverted because this CL caused the Linux/Windows (shared) builds to fail."
> 
> http://crrev.com/106517
> 
> Here's how to build with shared libraries:
>
http://code.google.com/p/chromium/wiki/LinuxFasterBuilds#Shared_libraries_%28...)
> 
> You're probably missing a MEDIA_EXPORT somewhere.

Great, it is. MEDIA_EXPORT was missing in the AudioDeviceName. 
I reproduced the problem, and now it is fixed.

Thanks a lot.

[tommi (sloooow) - chröme, 2011-10-24 07:05:00]

lgtm.  a few minor suggestions below.

http://codereview.chromium.org/8361031/diff/6001/media/audio/audio_device_name.h
File media/audio/audio_device_name.h (right):

http://codereview.chromium.org/8361031/diff/6001/media/audio/audio_device_nam...
media/audio/audio_device_name.h:17: const  std::string& unique_id);
single spacing

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:34: static const char*
kInvalidAudioInputDevices[] = {
nit: I'm wondering if this could be [][] instead of *[] and then if we could use
arraysize(kInvalidAudioInputDevices)-1 to get the string length at compile time
and avoid the call to strlen().

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:176: device_names->clear();
nit: you could also make it a requirement that the container be empty and do a
DCHECK(device_names->empty());

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:197: while (!wrapper_->CardNext(&card)
&& (card >= 0)) {
nit: no need for () around the second expression

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:242: char* pret = strchr(desc.get(),
'\n');
nit: This won't replace all occurrences of \n but perhaps that's not really a
problem. However, if it is, then you could first assign to name.device_name and
then use STL for the parsing and do name.device_name.replace().

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:265:
strlen(kInvalidAudioInputDevices[i])))
nit: see comment at the top for the strlen call

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:273: &device_handle, device_name,
SND_PCM_STREAM_CAPTURE, SND_PCM_NONBLOCK))
I think this should be indented 4 more spaces so that the parameters aren't
indented at the same indent as the function.  Also, since there is room for some
of the parameters on the above line, they should be there according to the style
guide.

[no longer working on chromium, 2011-10-25 20:35:53]

Tommi, I have one question on your comment, it is inline.

Thanks a lot.

BR,
/SX

http://codereview.chromium.org/8361031/diff/6001/media/audio/audio_device_name.h
File media/audio/audio_device_name.h (right):

http://codereview.chromium.org/8361031/diff/6001/media/audio/audio_device_nam...
media/audio/audio_device_name.h:17: const  std::string& unique_id);
On 2011/10/24 07:05:01, tommi wrote:
> single spacing

Done.

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:34: static const char*
kInvalidAudioInputDevices[] = {
On 2011/10/24 07:05:01, tommi wrote:
> nit: I'm wondering if this could be [][] instead of *[] and then if we could
use
> arraysize(kInvalidAudioInputDevices)-1 to get the string length at compile
time
> and avoid the call to strlen().

Sorry, I don't fully understand here.
I need the string length of each item, so if I change to use [][], do I still
need to use arraysize(kInvalidAudioInputDevices[i]) -1 to get the length?
Compare with strlen, what is the gain for it?
This kInvalidAudioInputDevices is not supposed to be used too much.

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:176: device_names->clear();
On 2011/10/24 07:05:01, tommi wrote:
> nit: you could also make it a requirement that the container be empty and do a
> DCHECK(device_names->empty());

Done.

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:197: while (!wrapper_->CardNext(&card)
&& (card >= 0)) {
On 2011/10/24 07:05:01, tommi wrote:
> nit: no need for () around the second expression

Done.

http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:242: char* pret = strchr(desc.get(),
'\n');
On 2011/10/24 07:05:01, tommi wrote:
> nit: This won't replace all occurrences of \n but perhaps that's not really a
> problem. However, if it is, then you could first assign to name.device_name
and
> then use STL for the parsing and do name.device_name.replace().

It is not a problem here since the string only contains only 1 \n.

[tommi (sloooow) - chröme, 2011-10-26 08:54:04]

On Tue, Oct 25, 2011 at 10:35 PM, <xians@chromium.org> wrote:

> Tommi, I have one question on your comment, it is inline.
>
> Thanks a lot.
>
> BR,
> /SX
>
>
>
> http://codereview.chromium.**org/8361031/diff/6001/media/**
>
audio/audio_device_name.h<http://codereview.chromium.org/8361031/diff/6001/media/audio/audio_device_name.h>
> File media/audio/audio_device_name.**h (right):
>
> http://codereview.chromium.**org/8361031/diff/6001/media/**
>
audio/audio_device_name.h#**newcode17<http://codereview.chromium.org/8361031/diff/6001/media/audio/audio_device_name.h#newcode17>
> media/audio/audio_device_name.**h:17: const  std::string& unique_id);
> On 2011/10/24 07:05:01, tommi wrote:
>
>> single spacing
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8361031/diff/6001/media/**
>
audio/linux/audio_manager_**linux.cc<http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_manager_linux.cc>
> File media/audio/linux/audio_**manager_linux.cc (right):
>
> http://codereview.chromium.**org/8361031/diff/6001/media/**
>
audio/linux/audio_manager_**linux.cc#newcode34<http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_manager_linux.cc#newcode34>
> media/audio/linux/audio_**manager_linux.cc:34: static const char*
> kInvalidAudioInputDevices[] = {
> On 2011/10/24 07:05:01, tommi wrote:
>
>> nit: I'm wondering if this could be [][] instead of *[] and then if we
>>
> could use
>
>> arraysize(**kInvalidAudioInputDevices)-1 to get the string length at
>>
> compile time
>
>> and avoid the call to strlen().
>>
>
> Sorry, I don't fully understand here.
> I need the string length of each item, so if I change to use [][], do I
> still need to use arraysize(**kInvalidAudioInputDevices[i]) -1 to get the
> length? Compare with strlen, what is the gain for it?
> This kInvalidAudioInputDevices is not supposed to be used too much.


This is a pet peeve of mine.  I don't like calling methods like strlen where
a constant would suffice.

There are two justifications I can see for calling strlen(kConstant):
1) I don't know how to get the compiler to calculate the length.
2) I'm lazy :)

My justifications for calculating the length at compile time and not calling
strlen are:
1) It is faster
2) The code is smaller
3) Figuring out how it's done is a problem solving task and an opportunity
to learn more about the magic of programming! :)

I'm sure I'm missing a justification on either/both side(s), but I hope you
can see which option I find more fun :)

>
>
> http://codereview.chromium.**org/8361031/diff/6001/media/**
>
audio/linux/audio_manager_**linux.cc#newcode176<http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_manager_linux.cc#newcode176>
> media/audio/linux/audio_**manager_linux.cc:176: device_names->clear();
> On 2011/10/24 07:05:01, tommi wrote:
>
>> nit: you could also make it a requirement that the container be empty
>>
> and do a
>
>> DCHECK(device_names->empty());
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8361031/diff/6001/media/**
>
audio/linux/audio_manager_**linux.cc#newcode197<http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_manager_linux.cc#newcode197>
> media/audio/linux/audio_**manager_linux.cc:197: while
> (!wrapper_->CardNext(&card) && (card >= 0)) {
> On 2011/10/24 07:05:01, tommi wrote:
>
>> nit: no need for () around the second expression
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8361031/diff/6001/media/**
>
audio/linux/audio_manager_**linux.cc#newcode242<http://codereview.chromium.org/8361031/diff/6001/media/audio/linux/audio_manager_linux.cc#newcode242>
> media/audio/linux/audio_**manager_linux.cc:242: char* pret =
> strchr(desc.get(), '\n');
> On 2011/10/24 07:05:01, tommi wrote:
>
>> nit: This won't replace all occurrences of \n but perhaps that's not
>>
> really a
>
>> problem. However, if it is, then you could first assign to
>>
> name.device_name and
>
>> then use STL for the parsing and do name.device_name.replace().
>>
>
> It is not a problem here since the string only contains only 1 \n.
>
>
http://codereview.chromium.**org/8361031/<http://codereview.chromium.org/8361...
>

[no longer working on chromium, 2011-10-26 17:59:25]

On Wed, Oct 26, 2011 at 1:53 AM, Tommi <tommi@chromium.org> wrote:

> oInputDevices is not supposed to be used too much.
>
>
> This is a pet peeve of mine.  I don't like calling methods like strlen
> where a constant would suffice.
>
> There are two justifications I can see for calling strlen(kConstant):
> 1) I don't know how to get the compiler to calculate the length.
> 2) I'm lazy :)
>
>
Hah, I am both here.

I did some try today, but could not make it work.
First, it seems [][] does not work for the compiler, it complains
error: declaration of ‘kInvalidAudioInputDevices’ as multidimensional array
must have bounds for all dimensions except the first

So I have to use [][max_length] or declare the kInvalidAudioInputDevices to
be const std::string.
But either way works for me because both reports a same fixed length for all
the strings, and I need the dynamic length to compare the strings.

I am sure there must be some ways to make it work, Maybe I miss something
here, could you please give me more hint on how to make it work?

Thanks,

BR,
/SX



My justifications for calculating the length at compile time and not calling
> strlen are:
> 1) It is faster
> 2) The code is smaller
> 3) Figuring out how it's done is a problem solving task and an opportunity
> to learn more about the magic of programming! :)
>
> I'm sure I'm missing a justification on either/both side(s), but I hope you
> can see which option I find more fun :)
>
>>
>>

[tommi (sloooow) - chröme, 2011-10-27 07:50:34]

I think I am both as well actually :)  Well, since I'm the troublemaker I
should at least offer a solution. Feel free to not do this.

#define STRING_ENTRY(x) { x, arraysize(x) - 1 },

struct {
  const char* name;
  size_t length;
} kConstants[] = {
  STRING_ENTRY("Foo"),
  STRING_ENTRY("Bar"),
};

Then use like so:

for (int i = 0; i < arraysize(kConstants); ++i) {
  DoStuff(kConstants[i].name, kConstants[i].length);
}

On Wed, Oct 26, 2011 at 7:59 PM, Shijing Xian <xians@chromium.org> wrote:

>
>
> On Wed, Oct 26, 2011 at 1:53 AM, Tommi <tommi@chromium.org> wrote:
>
>> oInputDevices is not supposed to be used too much.
>>
>>
>> This is a pet peeve of mine.  I don't like calling methods like strlen
>> where a constant would suffice.
>>
>> There are two justifications I can see for calling strlen(kConstant):
>> 1) I don't know how to get the compiler to calculate the length.
>> 2) I'm lazy :)
>>
>>
> Hah, I am both here.
>
> I did some try today, but could not make it work.
> First, it seems [][] does not work for the compiler, it complains
> error: declaration of ‘kInvalidAudioInputDevices’ as multidimensional array
> must have bounds for all dimensions except the first
>
> So I have to use [][max_length] or declare the kInvalidAudioInputDevices to
> be const std::string.
> But either way works for me because both reports a same fixed length for
> all the strings, and I need the dynamic length to compare the strings.
>
> I am sure there must be some ways to make it work, Maybe I miss something
> here, could you please give me more hint on how to make it work?
>
> Thanks,
>
> BR,
> /SX
>
>
>
> My justifications for calculating the length at compile time and not
>> calling strlen are:
>> 1) It is faster
>> 2) The code is smaller
>> 3) Figuring out how it's done is a problem solving task and an opportunity
>> to learn more about the magic of programming! :)
>>
>> I'm sure I'm missing a justification on either/both side(s), but I hope
>> you can see which option I find more fun :)
>>
>>>
>>>

[no longer working on chromium, 2011-10-27 16:29:30]

On 2011/10/27 07:50:34, tommi wrote:
> I think I am both as well actually :)  Well, since I'm the troublemaker I
> should at least offer a solution. Feel free to not do this.
> 
> #define STRING_ENTRY(x) { x, arraysize(x) - 1 },
> 
> struct {
>   const char* name;
>   size_t length;
> } kConstants[] = {
>   STRING_ENTRY("Foo"),
>   STRING_ENTRY("Bar"),
> };
> 
> Then use like so:
> 
> for (int i = 0; i < arraysize(kConstants); ++i) {
>   DoStuff(kConstants[i].name, kConstants[i].length);
> }
> 
Thanks, it is a good solution to the problem.
But given kInvalidAudioInputDevices is not used intensively, I prefer to have
less code :)
Hope it is fine to leave it for now.

[tommi (sloooow) - chröme, 2011-10-27 21:52:54]

yes, that's fine. lgtm.

On Thu, Oct 27, 2011 at 6:29 PM, <xians@chromium.org> wrote:

> On 2011/10/27 07:50:34, tommi wrote:
>
>> I think I am both as well actually :)  Well, since I'm the troublemaker I
>> should at least offer a solution. Feel free to not do this.
>>
>
>  #define STRING_ENTRY(x) { x, arraysize(x) - 1 },
>>
>
>  struct {
>>   const char* name;
>>   size_t length;
>> } kConstants[] = {
>>   STRING_ENTRY("Foo"),
>>   STRING_ENTRY("Bar"),
>> };
>>
>
>  Then use like so:
>>
>
>  for (int i = 0; i < arraysize(kConstants); ++i) {
>>   DoStuff(kConstants[i].name, kConstants[i].length);
>> }
>>
>
> Thanks, it is a good solution to the problem.
> But given kInvalidAudioInputDevices is not used intensively, I prefer to
> have
> less code :)
> Hope it is fine to leave it for now.
>
>
>
http://codereview.chromium.**org/8361031/<http://codereview.chromium.org/8361...
>

[scherkus (not reviewing), 2011-10-28 17:47:37]

LGTM

[commit-bot: I haz the power, 2011-10-28 23:53:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/8361031/15002

[commit-bot: I haz the power, 2011-10-29 00:59:33]

Change committed as 107842