Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8276034:
This patch will loop through the soundcard and return a list of available devices when the AudioI... (Closed)
|
Created:
9 years, 2 months ago by no longer working on chromium Modified:
9 years, 1 month ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, acolwell+watch_chromium.org, annacc+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionThis patch will loop through the soundcard and return a list of available devices when the AudioInputDeviceManager does the device enumeration.
Previously, only default device will be returned.
This is a step forward to support device selection on Mac.
I have another similar CL but on Linux:
http://codereview.chromium.org/8162015/
Examples for how the device enumeration with this patch:
name id
Built-in Line Input AppleHDAEngineInput:1B,0,1,1:4
Built-in Digital Input AppleHDAEngineInput:1B,0,1,2:5
SB Arena Headset AppleUSBAudioEngine:Creative Technology:SB Arena Headset:5d100000:2,1
Bug=None
Test=media_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107771
Patch Set 1 #
Total comments: 24
Patch Set 2 : update #
Total comments: 8
Patch Set 3 : use AudioObjectGetPropertyDataSize to avoid deprecated AudioHardwareGetProperty #
Total comments: 4
Patch Set 4 : update #
Total comments: 22
Patch Set 5 : '' #
Total comments: 8
Patch Set 6 : adding CFRelease to fix the memory leak #Patch Set 7 : replace the deprecated AudioDevice* API with AudioObject* API #
Total comments: 4
Patch Set 8 : fixed the tests and updated code based on comment from Avi #
Messages
Total messages: 22 (0 generated)
Hi, This CL is to get a full list of devices for GetAudioInputDeviceNames(), there will be a followup CL which allows opening an audio stream with a non-default device after this patch gets in. Please review, thanks. BR, -SX
Looks pretty good overall - some comments below: http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:68: static std::string CFStringToSTLString(const CFStringRef cfstr) I think it would be a lot better to avoid creating this function and see if you can use something in "base" like: CFStringToSTLStringWithEncodingT() in base/sys_string_conversions_mac.mm http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:84: int device_number = 0; "device_count" is probably a better name than "device_number" http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:85: UInt32 size = 0; Please move variables to closest point of usage. I know this might seem a little silly, but declaring all variables near the top of the function is more of an old C-language style and not C++ and chromium style which is to declare all variables to just before they are actually used. For example, move the "uid" to around line 139/140 move "device_number" (renamed to "device_count") to line 96 (and initialize to value on same line) etc... http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:87: device_names->clear(); Just to be careful, probably best to check "device_names" is not NULL and return early... http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:93: if (result) Can make this simpler: if (result || !size) then you don't need to check "device_number" below http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:111: for(int i = 0; i < device_number; ++i) { nit: space after "for" http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:132: for(uint32 j = 0; j < buffer_list->mNumberBuffers; ++j) nit: space after "for" http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:139: size = sizeof(uid); I'd move this line down just below the comment http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:192: device_names->push_front(name); Probably worth a comment to describe why you're putting it at the front of the list. I assume it's so you have the default device show up first... I guess it seems kind of obvious from the code itself - but maybe worth considering a comment too
Update the code based on the comments from Chris, and also update the description to have an example how to device enumeration list looks like. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:68: static std::string CFStringToSTLString(const CFStringRef cfstr) On 2011/10/19 01:31:50, Chris Rogers wrote: > I think it would be a lot better to avoid creating this function and see if you > can use something in "base" like: CFStringToSTLStringWithEncodingT() > in base/sys_string_conversions_mac.mm Done. Thanks for pointing out, it has a API called SysCFStringRefToUTF8 to use. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:84: int device_number = 0; On 2011/10/19 01:31:50, Chris Rogers wrote: > "device_count" is probably a better name than "device_number" Done. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:85: UInt32 size = 0; On 2011/10/19 01:31:50, Chris Rogers wrote: > Please move variables to closest point of usage. > > I know this might seem a little silly, but declaring all variables near the top > of the function is more of an old C-language style and not C++ and chromium > style which is to declare all variables to just before they are actually used. > > For example, move the "uid" to around line 139/140 > move "device_number" (renamed to "device_count") to line 96 (and initialize to > value on same line) > etc... Done. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:87: device_names->clear(); On 2011/10/19 01:31:50, Chris Rogers wrote: > Just to be careful, probably best to check "device_names" is not NULL and return > early... Done. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:93: if (result) On 2011/10/19 01:31:50, Chris Rogers wrote: > Can make this simpler: > > if (result || !size) > > then you don't need to check "device_number" below Done. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:111: for(int i = 0; i < device_number; ++i) { On 2011/10/19 01:31:50, Chris Rogers wrote: > nit: space after "for" Done. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:132: for(uint32 j = 0; j < buffer_list->mNumberBuffers; ++j) On 2011/10/19 01:31:50, Chris Rogers wrote: > nit: space after "for" Done. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:139: size = sizeof(uid); On 2011/10/19 01:31:50, Chris Rogers wrote: > I'd move this line down just below the comment Done. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:192: device_names->push_front(name); On 2011/10/19 01:31:50, Chris Rogers wrote: > Probably worth a comment to describe why you're putting it at the front of the > list. I assume it's so you have the default device show up first... > > I guess it seems kind of obvious from the code itself - but maybe worth > considering a comment too Done.
few nits also should we be using AudioHardwareGetProperty()? according to a previous code review that function is deprecated in 10.6 in favour of AudioObjectGetPropertyData(): http://codereview.chromium.org/210009 http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:69: { { should go on previous line http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:81: bool is_input, media::AudioDeviceNames* device_names) { nit: should go on previous line then align parameters at ( http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:102: devices(reinterpret_cast<AudioDeviceID*>(malloc(size))); I'm curious -- is malloc required? http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:70: bool is_input, media::AudioDeviceNames* device_names) { nit: should go on previous line and align parameters at ( http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:91: result = AudioHardwareGetProperty(kAudioHardwarePropertyDevices, I'm curious do we need malloc() or does new[] work? http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:99: { why do we have the separate scope? http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:110: buffer(reinterpret_cast<AudioBufferList*>(malloc(size))); ditto
Thanks Andrew for pointing out the deprecated AudioHardwareGetProperty. I have already changed to use the AudioObjectGetPropertyData as in http://codereview.chromium.org/210009 http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:69: { On 2011/10/19 17:38:51, scherkus wrote: > { should go on previous line The code has been removed. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:81: bool is_input, media::AudioDeviceNames* device_names) { On 2011/10/19 17:38:51, scherkus wrote: > nit: should go on previous line then align parameters at ( Done. http://codereview.chromium.org/8276034/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:102: devices(reinterpret_cast<AudioDeviceID*>(malloc(size))); On 2011/10/19 17:38:51, scherkus wrote: > I'm curious -- is malloc required? That is how the apple sample code looks like, I think both new and malloc should work. http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:70: bool is_input, media::AudioDeviceNames* device_names) { On 2011/10/19 17:38:51, scherkus wrote: > nit: should go on previous line and align parameters at ( Done. http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:91: result = AudioHardwareGetProperty(kAudioHardwarePropertyDevices, On 2011/10/19 17:38:51, scherkus wrote: > I'm curious do we need malloc() or does new[] work? The Apple sample code is using malloc, I am not sure here but believe both new should work. http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:99: { On 2011/10/19 17:38:51, scherkus wrote: > why do we have the separate scope? Because it is in a loop and we need to allocate dynamic memory to the variable "buffer", which may have differ size for each device. Putting it in a separate scope is to free the memory. http://codereview.chromium.org/8276034/diff/3001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:110: buffer(reinterpret_cast<AudioBufferList*>(malloc(size))); On 2011/10/19 17:38:51, scherkus wrote: > ditto ditto.
http://codereview.chromium.org/8276034/diff/5001/media/audio/mac/audio_manage... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/5001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:71: if (!device_names) is this valid? seems like programmer error -- I'd rather crash in this case by dereferencing device_names http://codereview.chromium.org/8276034/diff/5001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:120: buffer(reinterpret_cast<AudioBufferList*>(malloc(size))); maybe I'm missing something -- but doesn't the for loop give you the appropriate level of scoping? each time the loop repeats buffer will get reallocated without the extra level of scoping
http://codereview.chromium.org/8276034/diff/5001/media/audio/mac/audio_manage... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/5001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:71: if (!device_names) On 2011/10/19 18:19:26, scherkus wrote: > is this valid? seems like programmer error -- I'd rather crash in this case by > dereferencing device_names Agree, let me just put a DCHECK here. http://codereview.chromium.org/8276034/diff/5001/media/audio/mac/audio_manage... media/audio/mac/audio_manager_mac.cc:120: buffer(reinterpret_cast<AudioBufferList*>(malloc(size))); On 2011/10/19 18:19:26, scherkus wrote: > maybe I'm missing something -- but doesn't the for loop give you the appropriate > level of scoping? > > each time the loop repeats buffer will get reallocated without the extra level > of scoping True, I was quite tired at that late evening that I wrote a wrong answer. And I later recalled the real reason why I put an extra scope here, I was thinking about extracting this section into a separate function to make the function short. But I decided to postpone it until we get another use case to reuse the code. Thanks pointing it ou. I have removed the scoping.
LGTM w/ nit http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:72: nit: get rid of blank line
Looks good but I feel that some clarifications can be done.
http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:77: kAudioHardwarePropertyDevices, // mSelector Why did you add these comments? Does not say much. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:87: if (result || !size) Assume that there are no devices if we enter this state. Perhaps add a comment? http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:91: device_count = size / sizeof(AudioDeviceID); Why not: int device_count = size / .. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:93: // Get the array of device id for each device. Can you make this comment more clear. What does the array of device IDs correspond to? E.g. input and output? http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:106: // Iterate each device to gather information. I would say "Iterate over all available devices ...." or something like that. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:108: int channels = 0; Why declaration here? http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:110: // those devices without any right type of channel. What does "without any right type of channel" mean and to which scope does this comment belong? http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:127: for (uint32 j = 0; j < buffer_list->mNumberBuffers; ++j) What does this section do? http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:130: if (!channels) Sorry, this part is not clear. Where has it been set what a valid number of channels is? http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:156: media::AudioDeviceName device_name; Add comment about what you actually produce and store here.
http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:72: On 2011/10/24 18:32:13, scherkus wrote: > nit: get rid of blank line Done. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:77: kAudioHardwarePropertyDevices, // mSelector On 2011/10/24 18:51:36, henrika1 wrote: > Why did you add these comments? Does not say much. It is code/comment copied from CL http://codereview.chromium.org/210009 which replaced the deprecated AudioHardwareGetProperty with AudioObjectGetPropertyData. But I can remove them. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:87: if (result || !size) On 2011/10/24 18:51:36, henrika1 wrote: > Assume that there are no devices if we enter this state. Perhaps add a comment? I think the code has been quite clear. hope it is fine here. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:91: device_count = size / sizeof(AudioDeviceID); On 2011/10/24 18:51:36, henrika1 wrote: > Why not: > > int device_count = size / .. Good catch, thanks. Done http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:93: // Get the array of device id for each device. On 2011/10/24 18:51:36, henrika1 wrote: > Can you make this comment more clear. What does the array of device IDs > correspond to? E.g. input and output? Done. The story behind this is that we need to check if the device has any input channel or output channel, and exclude those device without any channel we are interested in. For example, if we are looking for input device, we exclude the device without any input channel. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:106: // Iterate each device to gather information. On 2011/10/24 18:51:36, henrika1 wrote: > I would say "Iterate over all available devices ...." or something like that. Done. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:108: int channels = 0; On 2011/10/24 18:51:36, henrika1 wrote: > Why declaration here? For each device, we need to check if it has channel we need. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:110: // those devices without any right type of channel. On 2011/10/24 18:51:36, henrika1 wrote: > What does "without any right type of channel" mean and to which scope does this > comment belong? Done. I improved the comments a bit by separating them. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:127: for (uint32 j = 0; j < buffer_list->mNumberBuffers; ++j) On 2011/10/24 18:51:36, henrika1 wrote: > What does this section do? It just gets the buffer list about the device which contains the channel information. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:130: if (!channels) On 2011/10/24 18:51:36, henrika1 wrote: > Sorry, this part is not clear. Where has it been set what a valid number of > channels is? Just two lines above we add to channels with the number of channels in the buffer list. http://codereview.chromium.org/8276034/diff/10001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:156: media::AudioDeviceName device_name; On 2011/10/24 18:51:36, henrika1 wrote: > Add comment about what you actually produce and store here. Done.
Looks good. LGTM.
LGTM - but please fix the memory leak http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:160: } I believe you have a memory leak here, since you need to release the two CFString objects: if (uid) CFRelease(uid); if (name) CFRelease(name);
http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:160: } On 2011/10/26 18:52:44, Chris Rogers wrote: > I believe you have a memory leak here, since you need to release the two > CFString objects: > > if (uid) > CFRelease(uid); > if (name) > CFRelease(name); Thanks, you are right, there is a leak there. It failed the mac_valgrind trybot. Now it is fixed. I will re-run the trybot to make sure.
Calls starting with AudioDevice are obsolete; please use the AudioObject calls instead. http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:109: AudioDeviceGetPropertyInfo(device_ids[i], obsolete http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:118: AudioDeviceGetProperty(device_ids[i], obsolete http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:135: result = AudioDeviceGetProperty(device_ids[i], AudioDeviceGetProperty is deprecated (see TN2223), and the replacement, AudioObjectGetPropertyData, is available in 10.5. Please switch. http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:146: result = AudioDeviceGetProperty(device_ids[i], obsolete http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:160: } I'm a little confused on why. Does AudioDeviceGetProperty return a referenced object? Where is that documented?
http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/17001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:160: } It's documented in AudioHardware.h in the discussion about the individual properties: "The caller is responsible for releasing the returned CFObject." On 2011/10/26 21:44:25, Avi wrote: > I'm a little confused on why. Does AudioDeviceGetProperty return a referenced > object? Where is that documented?
> "The caller is responsible for releasing the returned CFObject." A comment in the code would be greatly appreciated; this is a departure from the usual naming conventions.
Added the comment "We are responsible for releasing the returned CFObject." Replaced the deprecated AudioDevice* API with AudioObject* API.
lgtm http://codereview.chromium.org/8276034/diff/26002/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/26002/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:175: // We are responsible for releasing the returned CFObject. Be a little more clear here. "Despite being obtained via a Get function, we are responsible for releasing the returned CFObjects. See the comment in the xx.h file for the function xxx for details" or the like. Give me the 'why' rather than the 'what'. http://codereview.chromium.org/8276034/diff/26002/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:178: if(name) space before (
Fixed the tests and changed the comment/code corresponding to the review feedback from Avi. Ready to commit soon. http://codereview.chromium.org/8276034/diff/26002/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/8276034/diff/26002/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:175: // We are responsible for releasing the returned CFObject. On 2011/10/27 05:07:14, Avi wrote: > Be a little more clear here. "Despite being obtained via a Get function, we are > responsible for releasing the returned CFObjects. See the comment in the xx.h > file for the function xxx for details" or the like. Give me the 'why' rather > than the 'what'. Done. http://codereview.chromium.org/8276034/diff/26002/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:178: if(name) On 2011/10/27 05:07:14, Avi wrote: > space before ( Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8276034/29001
Change committed as 107771 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
