Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6502007:
Rewrote/added documentation for audio (Closed)
|
Created:
9 years, 10 months ago by jond Modified:
9 years, 7 months ago CC:
chromium-reviews, piman+watch_chromium.org, jhartman_google.com, J Coco, awatson1 Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionPatch Set 1 #
Total comments: 68
Patch Set 2 : '' #
Total comments: 9
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Messages
Total messages: 10 (0 generated)
http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h File ppapi/c/ppb_audio.h (right): http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode48 ppapi/c/ppb_audio.h:48: * that the buffer needs to be filled. Refer to "Refer to..." ? It seems like there is a link missing here? http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode65 ppapi/c/ppb_audio.h:65: * Create is a pointer to a function that creates an audio resource. Just asking - we don't use @a Create for the fcn pointers? I notice that dropping the @a is consistent everywhere, I'm just wondering. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h File ppapi/c/ppb_audio_config.h (right): http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:136: * IsAudioConfit is a pointer to a function that determines if the given IsAudioConfit -> IsAudioConfig (ends in 'g').
http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h File ppapi/c/ppb_audio.h (right): http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode30 ppapi/c/ppb_audio.h:30: * This value contains a pointer to an audio callback function used to fill this is a type, not a value. Maybe: "PPB_Audio_Callback defines the type of an audio callback function used to fill the audio buffer with data." http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode46 ppapi/c/ppb_audio.h:46: * audio resources on the browser. This interface is a Callback-based audio callback should probably be lower-case. 2 spaces after the period. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode47 ppapi/c/ppb_audio.h:47: * interface. User of audio must set the callback that will be called each time nit: 2-spaces after period 'User' should be 'Users', or even better 'Developers using the PPB_Audio API' http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode68 ppapi/c/ppb_audio.h:68: * buffer needs to be filled. From within the callback, you should not 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode70 ppapi/c/ppb_audio.h:70: * thread than the one which created the interface. For performance-critical 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode72 ppapi/c/ppb_audio.h:72: * or calling functions that can obtain locks, such as malloc. The layout and 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode122 ppapi/c/ppb_audio.h:122: * Also returns PP_TRUE (and be a no-op) if called while playback is already s/and be/and is http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode123 ppapi/c/ppb_audio.h:123: * stopped. If a callback is in progress, StopPlayback will block until 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode124 ppapi/c/ppb_audio.h:124: * callback completes. 'the' callback http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h File ppapi/c/ppb_audio_config.h (right): http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:19: * audio configuration resource within the browser. Refer to the 2 spaces after the period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:54: */ I liked this extra bit of info in the web document about Audio, but I don't know if it should be here. May just be confusing... 'what if I'm not using a CD or DVD?!?'. Maybe instead explain what the sample rate is, but don't describe each value? I could go either way, it just strikes me as a little odd here. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:72: * establishing your audio configuration within the browser. This interface 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:73: * only supports stereo * 16bit output. This class is not mutable, therefore one space after supports http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:79: * configuration resource. The |sample_frame_count| should be the result of 2 spaces after period. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:79: * configuration resource. The |sample_frame_count| should be the result of s/|sample_frame_count|/@a sample_frame_count (right?) http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:80: * calling RecommendSampleFrameCount. If the sample frame count or bit rate 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:81: * aren't supported, this function will fail and return a null resource. s/aren't/isn't http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:95: * sample These should probably all end in a period. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:101: * @param[in] sample_frame_count A uint_32t frame count returned from the s/uint_32t/uint32_t http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:112: * supported sample frame count closest to the requested count. The sample 2 spaces after period. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:113: * frame count determines the overall latency of audio. Since one "frame" is 2 spaces after period. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:114: * always buffered in advance, smaller frame counts will yield lower latency, I think the comma after latency is superfluous. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:119: * audio workstation). Sample counts less than PP_AUDIOMINSAMPLEFRAMECOUNT 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:121: * system, but values in between aren't necessarily valid. This function 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:157: * fame count for the given audio config resource. s/fame/frame Maybe instead of 'audio config' resource it should specifically say 'PPB_Audio_Config' (here and elsewhere) http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:161: * 0 if the resource is invalid. See RecommendSampleFrameCount for 2 spaces after period.
http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h File ppapi/c/ppb_audio.h (right): http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode46 ppapi/c/ppb_audio.h:46: * audio resources on the browser. This interface is a Callback-based audio On 2011/02/11 22:31:07, dmichael wrote: > callback should probably be lower-case. > 2 spaces after the period. Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode47 ppapi/c/ppb_audio.h:47: * interface. User of audio must set the callback that will be called each time There arn't actually two spaces after the periods. On 2011/02/11 22:31:07, dmichael wrote: > nit: 2-spaces after period > 'User' should be 'Users', or even better 'Developers using the PPB_Audio API' http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode48 ppapi/c/ppb_audio.h:48: * that the buffer needs to be filled. Refer to On 2011/02/11 22:12:33, David Springer wrote: > "Refer to..." ? It seems like there is a link missing here? Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode48 ppapi/c/ppb_audio.h:48: * that the buffer needs to be filled. Refer to On 2011/02/11 22:12:33, David Springer wrote: > "Refer to..." ? It seems like there is a link missing here? Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode65 ppapi/c/ppb_audio.h:65: * Create is a pointer to a function that creates an audio resource. Not sure what you are asking... On 2011/02/11 22:12:33, David Springer wrote: > Just asking - we don't use @a Create for the fcn pointers? I notice that > dropping the @a is consistent everywhere, I'm just wondering. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode68 ppapi/c/ppb_audio.h:68: * buffer needs to be filled. From within the callback, you should not On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode68 ppapi/c/ppb_audio.h:68: * buffer needs to be filled. From within the callback, you should not On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode70 ppapi/c/ppb_audio.h:70: * thread than the one which created the interface. For performance-critical On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode72 ppapi/c/ppb_audio.h:72: * or calling functions that can obtain locks, such as malloc. The layout and So far only one of these (the one on line 69) had two spaces. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode122 ppapi/c/ppb_audio.h:122: * Also returns PP_TRUE (and be a no-op) if called while playback is already On 2011/02/11 22:31:07, dmichael wrote: > s/and be/and is Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode122 ppapi/c/ppb_audio.h:122: * Also returns PP_TRUE (and be a no-op) if called while playback is already On 2011/02/11 22:31:07, dmichael wrote: > s/and be/and is Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode123 ppapi/c/ppb_audio.h:123: * stopped. If a callback is in progress, StopPlayback will block until Nope. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode124 ppapi/c/ppb_audio.h:124: * callback completes. On 2011/02/11 22:31:07, dmichael wrote: > 'the' callback Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h File ppapi/c/ppb_audio_config.h (right): http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:19: * audio configuration resource within the browser. Refer to the Nope. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after the period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:72: * establishing your audio configuration within the browser. This interface Nope. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:73: * only supports stereo * 16bit output. This class is not mutable, therefore On 2011/02/11 22:31:07, dmichael wrote: > one space after supports Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:79: * configuration resource. The |sample_frame_count| should be the result of Nope. It dawned on me you might be asking for me to put in 2 spaces after the peroid, but one space is our standard. Two spaces is sort of dated. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:80: * calling RecommendSampleFrameCount. If the sample frame count or bit rate Nope. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:81: * aren't supported, this function will fail and return a null resource. On 2011/02/11 22:31:07, dmichael wrote: > s/aren't/isn't Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:95: * sample On 2011/02/11 22:31:07, dmichael wrote: > These should probably all end in a period. Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:101: * @param[in] sample_frame_count A uint_32t frame count returned from the On 2011/02/11 22:31:07, dmichael wrote: > s/uint_32t/uint32_t Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:101: * @param[in] sample_frame_count A uint_32t frame count returned from the On 2011/02/11 22:31:07, dmichael wrote: > s/uint_32t/uint32_t Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:112: * supported sample frame count closest to the requested count. The sample Nope. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:113: * frame count determines the overall latency of audio. Since one "frame" is Nope. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:114: * always buffered in advance, smaller frame counts will yield lower latency, On 2011/02/11 22:31:07, dmichael wrote: > I think the comma after latency is superfluous. Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:114: * always buffered in advance, smaller frame counts will yield lower latency, On 2011/02/11 22:31:07, dmichael wrote: > I think the comma after latency is superfluous. Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:119: * audio workstation). Sample counts less than PP_AUDIOMINSAMPLEFRAMECOUNT Nope. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:121: * system, but values in between aren't necessarily valid. This function Nope. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:136: * IsAudioConfit is a pointer to a function that determines if the given On 2011/02/11 22:12:33, David Springer wrote: > IsAudioConfit -> IsAudioConfig (ends in 'g'). Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:157: * fame count for the given audio config resource. On 2011/02/11 22:31:07, dmichael wrote: > s/fame/frame > Maybe instead of 'audio config' resource it should specifically say > 'PPB_Audio_Config' (here and elsewhere) Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:157: * fame count for the given audio config resource. On 2011/02/11 22:31:07, dmichael wrote: > s/fame/frame > Maybe instead of 'audio config' resource it should specifically say > 'PPB_Audio_Config' (here and elsewhere) Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio_config.h#newc... ppapi/c/ppb_audio_config.h:161: * 0 if the resource is invalid. See RecommendSampleFrameCount for Nope. On 2011/02/11 22:31:07, dmichael wrote: > 2 spaces after period.
Oops, I think you misunderstood my 2-space comments. There *should* be two spaces after periods, and through most of the comments, there's usually only 1 space. It's kind of a nit, but we try to follow usual grammar rules in our documentation. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h File ppapi/c/ppb_audio.h (right): http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode30 ppapi/c/ppb_audio.h:30: * This value contains a pointer to an audio callback function used to fill On 2011/02/11 22:31:07, dmichael wrote: > this is a type, not a value. Maybe: > "PPB_Audio_Callback defines the type of an audio callback function used to fill > the audio buffer with data." I still think this should be reworded... http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode47 ppapi/c/ppb_audio.h:47: * interface. User of audio must set the callback that will be called each time On 2011/02/15 17:02:25, jond wrote: > There arn't actually two spaces after the periods. > Exactly ;-). There should be. > On 2011/02/11 22:31:07, dmichael wrote: > > nit: 2-spaces after period > > 'User' should be 'Users', or even better 'Developers using the PPB_Audio API' > http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode65 ppapi/c/ppb_audio.h:65: * Create is a pointer to a function that creates an audio resource. On 2011/02/15 17:02:25, jond wrote: > Not sure what you are asking... > > On 2011/02/11 22:12:33, David Springer wrote: > > Just asking - we don't use @a Create for the fcn pointers? I notice that > > dropping the @a is consistent everywhere, I'm just wondering. > For doxygen, we decided to use @a for names from the code (such as parameter names, function names, variable names, and type names). Dave was pointing out that you could have done that for the function pointer type names like 'Create' in this case, but you did not. For instance, this could say '@a Create is a pointer...'. If it's omitted on purpose due to a docs team decision, then that's fine. But I would have expected us to use @a for these function pointer type names, so that the resulting doxygen documentation will make them stand out. http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio.h#newcode69 ppapi/c/ppb_audio.h:69: * call PPB_Audio functions. The callback will be called on a different Oops, this was one of the few places that had 2 spaces :-p http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h File ppapi/c/ppb_audio_config.h (right): http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h#n... ppapi/c/ppb_audio_config.h:114: * always buffered in advance smaller frame counts will yield lower latency, I think the comma after advance needs to be there. It's the one after latency that seems unnecessary. But I'm not very good at the comma rules. http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h#n... ppapi/c/ppb_audio_config.h:137: * resource is an PPB_Audio_Config. s/an/a
Based on our e-mail conversation, I retract all my nits about changing to 2 spaces after the period. On 2011/02/15 17:41:45, dmichael wrote: > Oops, I think you misunderstood my 2-space comments. There *should* be two > spaces after periods, and through most of the comments, there's usually only 1 > space. It's kind of a nit, but we try to follow usual grammar rules in our > documentation. > > http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h > File ppapi/c/ppb_audio.h (right): > > http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode30 > ppapi/c/ppb_audio.h:30: * This value contains a pointer to an audio callback > function used to fill > On 2011/02/11 22:31:07, dmichael wrote: > > this is a type, not a value. Maybe: > > "PPB_Audio_Callback defines the type of an audio callback function used to > fill > > the audio buffer with data." > I still think this should be reworded... > > http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode47 > ppapi/c/ppb_audio.h:47: * interface. User of audio must set the callback that > will be called each time > On 2011/02/15 17:02:25, jond wrote: > > There arn't actually two spaces after the periods. > > > Exactly ;-). There should be. > > On 2011/02/11 22:31:07, dmichael wrote: > > > nit: 2-spaces after period > > > 'User' should be 'Users', or even better 'Developers using the PPB_Audio > API' > > > > http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode65 > ppapi/c/ppb_audio.h:65: * Create is a pointer to a function that creates an > audio resource. > On 2011/02/15 17:02:25, jond wrote: > > Not sure what you are asking... > > > > On 2011/02/11 22:12:33, David Springer wrote: > > > Just asking - we don't use @a Create for the fcn pointers? I notice that > > > dropping the @a is consistent everywhere, I'm just wondering. > > > For doxygen, we decided to use @a for names from the code (such as parameter > names, function names, variable names, and type names). Dave was pointing out > that you could have done that for the function pointer type names like 'Create' > in this case, but you did not. For instance, this could say '@a Create is a > pointer...'. If it's omitted on purpose due to a docs team decision, then > that's fine. But I would have expected us to use @a for these function pointer > type names, so that the resulting doxygen documentation will make them stand > out. > > http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio.h#newcode69 > ppapi/c/ppb_audio.h:69: * call PPB_Audio functions. The callback will be called > on a different > Oops, this was one of the few places that had 2 spaces :-p > > http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h > File ppapi/c/ppb_audio_config.h (right): > > http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h#n... > ppapi/c/ppb_audio_config.h:114: * always buffered in advance smaller frame > counts will yield lower latency, > I think the comma after advance needs to be there. It's the one after latency > that seems unnecessary. But I'm not very good at the comma rules. > > http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h#n... > ppapi/c/ppb_audio_config.h:137: * resource is an PPB_Audio_Config. > s/an/a
http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h File ppapi/c/ppb_audio.h (right): http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode30 ppapi/c/ppb_audio.h:30: * This value contains a pointer to an audio callback function used to fill On 2011/02/15 17:41:45, dmichael wrote: > On 2011/02/11 22:31:07, dmichael wrote: > > this is a type, not a value. Maybe: > > "PPB_Audio_Callback defines the type of an audio callback function used to > fill > > the audio buffer with data." > I still think this should be reworded... Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode30 ppapi/c/ppb_audio.h:30: * This value contains a pointer to an audio callback function used to fill On 2011/02/15 17:41:45, dmichael wrote: > On 2011/02/11 22:31:07, dmichael wrote: > > this is a type, not a value. Maybe: > > "PPB_Audio_Callback defines the type of an audio callback function used to > fill > > the audio buffer with data." > I still think this should be reworded... Done. http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode47 ppapi/c/ppb_audio.h:47: * interface. User of audio must set the callback that will be called each time On 2011/02/15 17:41:45, dmichael wrote: > On 2011/02/15 17:02:25, jond wrote: > > There arn't actually two spaces after the periods. > > > Exactly ;-). There should be. > > On 2011/02/11 22:31:07, dmichael wrote: > > > nit: 2-spaces after period > > > 'User' should be 'Users', or even better 'Developers using the PPB_Audio > API' > > > Done. http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio.h#newcode69 ppapi/c/ppb_audio.h:69: * call PPB_Audio functions. The callback will be called on a different On 2011/02/15 17:41:45, dmichael wrote: > Oops, this was one of the few places that had 2 spaces :-p Done. http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio.h#newcode69 ppapi/c/ppb_audio.h:69: * call PPB_Audio functions. The callback will be called on a different On 2011/02/15 17:41:45, dmichael wrote: > Oops, this was one of the few places that had 2 spaces :-p Done. http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h File ppapi/c/ppb_audio_config.h (right): http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h#n... ppapi/c/ppb_audio_config.h:114: * always buffered in advance smaller frame counts will yield lower latency, On 2011/02/15 17:41:45, dmichael wrote: > I think the comma after advance needs to be there. It's the one after latency > that seems unnecessary. But I'm not very good at the comma rules. Done. http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h#n... ppapi/c/ppb_audio_config.h:137: * resource is an PPB_Audio_Config. On 2011/02/15 17:41:45, dmichael wrote: > s/an/a Done.
http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h File ppapi/c/ppb_audio.h (right): http://codereview.chromium.org/6502007/diff/1/ppapi/c/ppb_audio.h#newcode30 ppapi/c/ppb_audio.h:30: * This value contains a pointer to an audio callback function used to fill On 2011/02/17 21:40:45, jond wrote: > On 2011/02/15 17:41:45, dmichael wrote: > > On 2011/02/11 22:31:07, dmichael wrote: > > > this is a type, not a value. Maybe: > > > "PPB_Audio_Callback defines the type of an audio callback function used to > > fill > > > the audio buffer with data." > > I still think this should be reworded... > > Done. I don't see it here... can you reupload? http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h File ppapi/c/ppb_audio_config.h (right): http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h#n... ppapi/c/ppb_audio_config.h:114: * always buffered in advance smaller frame counts will yield lower latency, On 2011/02/17 21:40:45, jond wrote: > On 2011/02/15 17:41:45, dmichael wrote: > > I think the comma after advance needs to be there. It's the one after latency > > that seems unnecessary. But I'm not very good at the comma rules. > > Done. Please re-upload... I don't see it here. http://codereview.chromium.org/6502007/diff/5001/ppapi/c/ppb_audio_config.h#n... ppapi/c/ppb_audio_config.h:137: * resource is an PPB_Audio_Config. On 2011/02/17 21:40:45, jond wrote: > On 2011/02/15 17:41:45, dmichael wrote: > > s/an/a > > Done. Ditto.
LGTM
|
