Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5202002:
changes for proxy audio (Closed)
|
Created:
10 years, 1 month ago by nfullagar Modified:
9 years, 7 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, stuartmorgan+watch_chromium.org, brettw-cc_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
Descriptionchanges for proxy audio
- includes Darin's changes to move StreamCreated() to main thread
- callback for delivering handles to proxy
- changes to trusted interface
BUG=none
TEST=chrome/src/ppapi/examples/audio/audio.cc
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67354
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 11
Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 33
Patch Set 11 : '' #
Total comments: 11
Patch Set 12 : '' #
Total comments: 4
Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #Patch Set 18 : '' #Patch Set 19 : '' #Patch Set 20 : '' #Patch Set 21 : '' #
Total comments: 18
Patch Set 22 : '' #
Messages
Total messages: 22 (0 generated)
http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... File ppapi/c/dev/ppb_audio_trusted_dev.h (right): http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:13: // This callback is invoked during StreamCreated() from the main thead, this comment refers to chromium implementation specific things. this API shouldn't be chromium specific, so you should probably simplify this comment. it is valuable to say that the callback is run on the main thread. http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:16: typedef PP_Bool (*PPB_AudioTrusted_Callback)(PP_Instance pp_instance, nit: PP_ instead of PPB_ PPB_ is reserved for interfaces implemented by the browser. http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:16: typedef PP_Bool (*PPB_AudioTrusted_Callback)(PP_Instance pp_instance, nit: we usually don't use the prefix "pp_" in variable names in the "c" headers http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:18: int64_t shared_memory_handle, int64_t -> uint64_t like PPB_ImageData_Trusted? http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:20: int64_t socket_handle); why is socket_handle int64_t? shouldn't it just be an "int" since it is a file descriptor? http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:23: struct PPB_AudioTrusted_Dev { As discussed in person, I think the API would be better like this: struct PPB_AudioTrusted_Dev { PP_Resource (*Create)(PP_Instance); int32_t (*Open)(PP_Resource audio, PP_Resource config, PP_CompletionCallback callback); void (*Close)(PP_Resource audio); uint64_t (*GetNativeMemoryHandle)(PP_Resource audio, uint32_t* byte_count); int (*GetSynchronizationSocket)(PP_Resource audio); }; This gives you several benefits: 1- Uses CompletionCallback for consistency. 2- Close may be used to interrupt an Open that hasn't completed. Releasing the last reference to the audio resource would implicitly close it. 3- GetNativeMemoryHandle is consistent with PPB_ImageData_Trusted. 4- GetSynchronizationSocket makes it more clear what the socket handle is that you are getting back. http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:28: PPB_AudioTrusted_Callback create_callback); nit: please indent this line so that you have: PP_Resource (*Create)(PP_Instance instance, PP_Resource config, PPB_AudioTrusted_Callback create_callback); http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_audio.cc (right): http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:129: NULL, NULL, created)) nit: please indent so the start of the arguments are aligned http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:270: #error "Unknown OS" nit: indent by 4 spaces like the other branches? http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:273: shared_memory_size, static_cast<int64_t>(socket_handle)); nit: align the arguments so they all start at the same column http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_plugin_delegate.h (right): http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_plugin_delegate.h:143: virtual void ShutDown() = 0; nit: add a new line above the protected label
responses below, thx -nicholas On Fri, Nov 19, 2010 at 9:45 AM, <darin@chromium.org> wrote: > > > http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... > File ppapi/c/dev/ppb_audio_trusted_dev.h (right): > > > http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... > ppapi/c/dev/ppb_audio_trusted_dev.h:13: // This callback is invoked > during StreamCreated() from the main thead, > this comment refers to chromium implementation specific things. this > API shouldn't be chromium specific, so you should probably simplify this > comment. it is valuable to say that the callback is run on the main > thread. > comment simplified > > > http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... > ppapi/c/dev/ppb_audio_trusted_dev.h:16: typedef PP_Bool > (*PPB_AudioTrusted_Callback)(PP_Instance pp_instance, > nit: PP_ instead of PPB_ > > PPB_ is reserved for interfaces implemented by the browser. > > done. > > http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... > ppapi/c/dev/ppb_audio_trusted_dev.h:16: typedef PP_Bool > (*PPB_AudioTrusted_Callback)(PP_Instance pp_instance, > nit: we usually don't use the prefix "pp_" in variable names in the "c" > headers > > done. > > http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... > ppapi/c/dev/ppb_audio_trusted_dev.h:18: int64_t shared_memory_handle, > int64_t -> uint64_t like PPB_ImageData_Trusted? > > done. > > http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... > ppapi/c/dev/ppb_audio_trusted_dev.h:20: int64_t socket_handle); > why is socket_handle int64_t? shouldn't it just be an "int" since it is > a file descriptor? > > done. > > http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... > ppapi/c/dev/ppb_audio_trusted_dev.h:23: struct PPB_AudioTrusted_Dev { > Hmm... I thought you signed off on the typedef'd callback during the discussion. Are you asking for this CL to implement your suggestion? Or a future CL? > As discussed in person, I think the API would be better like this: > > struct PPB_AudioTrusted_Dev { > PP_Resource (*Create)(PP_Instance); > > int32_t (*Open)(PP_Resource audio, > PP_Resource config, > PP_CompletionCallback callback); > void (*Close)(PP_Resource audio); > > uint64_t (*GetNativeMemoryHandle)(PP_Resource audio, > uint32_t* byte_count); > int (*GetSynchronizationSocket)(PP_Resource audio); > }; > > This gives you several benefits: > > 1- Uses CompletionCallback for consistency. > 2- Close may be used to interrupt an Open that hasn't completed. > Releasing the last reference to the audio resource would implicitly > close it. > 3- GetNativeMemoryHandle is consistent with PPB_ImageData_Trusted. > 4- GetSynchronizationSocket makes it more clear what the socket handle > is that you are getting back. > > http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... > ppapi/c/dev/ppb_audio_trusted_dev.h:28: PPB_AudioTrusted_Callback > create_callback); > nit: please indent this line so that you have: > > PP_Resource (*Create)(PP_Instance instance, PP_Resource config, > PPB_AudioTrusted_Callback create_callback); > > done. > > http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... > File webkit/glue/plugins/pepper_audio.cc (right): > > > http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... > webkit/glue/plugins/pepper_audio.cc:129: NULL, NULL, created)) > nit: please indent so the start of the arguments are aligned > > done. > > http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... > webkit/glue/plugins/pepper_audio.cc:270: #error "Unknown OS" > nit: indent by 4 spaces like the other branches? > > done. > > http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... > webkit/glue/plugins/pepper_audio.cc:273: shared_memory_size, > static_cast<int64_t>(socket_handle)); > nit: align the arguments so they all start at the same column > > done. > > http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... > File webkit/glue/plugins/pepper_plugin_delegate.h (right): > > > http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... > webkit/glue/plugins/pepper_plugin_delegate.h:143: virtual void > ShutDown() = 0; > nit: add a new line above the protected label > > done. > > http://codereview.chromium.org/5202002/ >
On Fri, Nov 19, 2010 at 2:36 PM, Nicholas Fullagar <nfullagar@google.com>wrote: > responses below, thx -nicholas > > On Fri, Nov 19, 2010 at 9:45 AM, <darin@chromium.org> wrote: > >> >> >> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >> File ppapi/c/dev/ppb_audio_trusted_dev.h (right): >> >> >> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >> ppapi/c/dev/ppb_audio_trusted_dev.h:13: // This callback is invoked >> during StreamCreated() from the main thead, >> this comment refers to chromium implementation specific things. this >> API shouldn't be chromium specific, so you should probably simplify this >> comment. it is valuable to say that the callback is run on the main >> thread. >> > > comment simplified > > >> >> >> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >> ppapi/c/dev/ppb_audio_trusted_dev.h:16: typedef PP_Bool >> (*PPB_AudioTrusted_Callback)(PP_Instance pp_instance, >> nit: PP_ instead of PPB_ >> >> PPB_ is reserved for interfaces implemented by the browser. >> >> > done. > > >> >> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >> ppapi/c/dev/ppb_audio_trusted_dev.h:16: typedef PP_Bool >> (*PPB_AudioTrusted_Callback)(PP_Instance pp_instance, >> nit: we usually don't use the prefix "pp_" in variable names in the "c" >> headers >> >> > done. > > >> >> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >> ppapi/c/dev/ppb_audio_trusted_dev.h:18: int64_t shared_memory_handle, >> int64_t -> uint64_t like PPB_ImageData_Trusted? >> >> > done. > > >> >> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >> ppapi/c/dev/ppb_audio_trusted_dev.h:20: int64_t socket_handle); >> why is socket_handle int64_t? shouldn't it just be an "int" since it is >> a file descriptor? >> >> > done. > > >> >> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >> ppapi/c/dev/ppb_audio_trusted_dev.h:23: struct PPB_AudioTrusted_Dev { >> > > Hmm... I thought you signed off on the typedef'd callback during the > discussion. Are you asking for this CL to implement your suggestion? Or a > future CL? > Yes, I accept the typedef'd callback interface as we discussed. I'm just voicing my opinion that I would do it differently. -Darin > > >> As discussed in person, I think the API would be better like this: >> >> struct PPB_AudioTrusted_Dev { >> PP_Resource (*Create)(PP_Instance); >> >> int32_t (*Open)(PP_Resource audio, >> PP_Resource config, >> PP_CompletionCallback callback); >> void (*Close)(PP_Resource audio); >> >> uint64_t (*GetNativeMemoryHandle)(PP_Resource audio, >> uint32_t* byte_count); >> int (*GetSynchronizationSocket)(PP_Resource audio); >> }; >> >> This gives you several benefits: >> >> 1- Uses CompletionCallback for consistency. >> 2- Close may be used to interrupt an Open that hasn't completed. >> Releasing the last reference to the audio resource would implicitly >> close it. >> 3- GetNativeMemoryHandle is consistent with PPB_ImageData_Trusted. >> 4- GetSynchronizationSocket makes it more clear what the socket handle >> is that you are getting back. >> >> > >> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >> ppapi/c/dev/ppb_audio_trusted_dev.h:28: PPB_AudioTrusted_Callback >> create_callback); >> nit: please indent this line so that you have: >> >> PP_Resource (*Create)(PP_Instance instance, PP_Resource config, >> PPB_AudioTrusted_Callback create_callback); >> >> > done. > > >> >> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >> File webkit/glue/plugins/pepper_audio.cc (right): >> >> >> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >> webkit/glue/plugins/pepper_audio.cc:129: NULL, NULL, created)) >> nit: please indent so the start of the arguments are aligned >> >> > done. > > >> >> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >> webkit/glue/plugins/pepper_audio.cc:270: #error "Unknown OS" >> nit: indent by 4 spaces like the other branches? >> >> > done. > > >> >> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >> webkit/glue/plugins/pepper_audio.cc:273: shared_memory_size, >> static_cast<int64_t>(socket_handle)); >> nit: align the arguments so they all start at the same column >> >> > done. > > >> >> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >> File webkit/glue/plugins/pepper_plugin_delegate.h (right): >> >> >> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >> webkit/glue/plugins/pepper_plugin_delegate.h:143: virtual void >> ShutDown() = 0; >> nit: add a new line above the protected label >> >> > done. > > >> >> http://codereview.chromium.org/5202002/ >> > >
New version uploaded w/ Darin's trusted interface suggestion. On Fri, Nov 19, 2010 at 4:15 PM, Darin Fisher <darin@chromium.org> wrote: > > > On Fri, Nov 19, 2010 at 2:36 PM, Nicholas Fullagar <nfullagar@google.com>wrote: > >> responses below, thx -nicholas >> >> On Fri, Nov 19, 2010 at 9:45 AM, <darin@chromium.org> wrote: >> >>> >>> >>> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >>> File ppapi/c/dev/ppb_audio_trusted_dev.h (right): >>> >>> >>> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >>> ppapi/c/dev/ppb_audio_trusted_dev.h:13: // This callback is invoked >>> during StreamCreated() from the main thead, >>> this comment refers to chromium implementation specific things. this >>> API shouldn't be chromium specific, so you should probably simplify this >>> comment. it is valuable to say that the callback is run on the main >>> thread. >>> >> >> comment simplified >> >> >>> >>> >>> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >>> ppapi/c/dev/ppb_audio_trusted_dev.h:16: typedef PP_Bool >>> (*PPB_AudioTrusted_Callback)(PP_Instance pp_instance, >>> nit: PP_ instead of PPB_ >>> >>> PPB_ is reserved for interfaces implemented by the browser. >>> >>> >> done. >> >> >>> >>> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >>> ppapi/c/dev/ppb_audio_trusted_dev.h:16: typedef PP_Bool >>> (*PPB_AudioTrusted_Callback)(PP_Instance pp_instance, >>> nit: we usually don't use the prefix "pp_" in variable names in the "c" >>> headers >>> >>> >> done. >> >> >>> >>> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >>> ppapi/c/dev/ppb_audio_trusted_dev.h:18: int64_t shared_memory_handle, >>> int64_t -> uint64_t like PPB_ImageData_Trusted? >>> >>> >> done. >> >> >>> >>> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >>> ppapi/c/dev/ppb_audio_trusted_dev.h:20: int64_t socket_handle); >>> why is socket_handle int64_t? shouldn't it just be an "int" since it is >>> a file descriptor? >>> >>> >> done. >> >> >>> >>> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >>> ppapi/c/dev/ppb_audio_trusted_dev.h:23: struct PPB_AudioTrusted_Dev { >>> >> >> Hmm... I thought you signed off on the typedef'd callback during the >> discussion. Are you asking for this CL to implement your suggestion? Or a >> future CL? >> > > Yes, I accept the typedef'd callback interface as we discussed. I'm just > voicing my opinion that I would do it differently. > > -Darin > > > >> >> >>> As discussed in person, I think the API would be better like this: >>> >>> struct PPB_AudioTrusted_Dev { >>> PP_Resource (*Create)(PP_Instance); >>> >>> int32_t (*Open)(PP_Resource audio, >>> PP_Resource config, >>> PP_CompletionCallback callback); >>> void (*Close)(PP_Resource audio); >>> >>> uint64_t (*GetNativeMemoryHandle)(PP_Resource audio, >>> uint32_t* byte_count); >>> int (*GetSynchronizationSocket)(PP_Resource audio); >>> }; >>> >>> This gives you several benefits: >>> >>> 1- Uses CompletionCallback for consistency. >>> 2- Close may be used to interrupt an Open that hasn't completed. >>> Releasing the last reference to the audio resource would implicitly >>> close it. >>> 3- GetNativeMemoryHandle is consistent with PPB_ImageData_Trusted. >>> 4- GetSynchronizationSocket makes it more clear what the socket handle >>> is that you are getting back. >>> >>> >> >>> http://codereview.chromium.org/5202002/diff/37001/ppapi/c/dev/ppb_audio_trust... >>> ppapi/c/dev/ppb_audio_trusted_dev.h:28: PPB_AudioTrusted_Callback >>> create_callback); >>> nit: please indent this line so that you have: >>> >>> PP_Resource (*Create)(PP_Instance instance, PP_Resource config, >>> PPB_AudioTrusted_Callback create_callback); >>> >>> >> done. >> >> >>> >>> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >>> File webkit/glue/plugins/pepper_audio.cc (right): >>> >>> >>> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >>> webkit/glue/plugins/pepper_audio.cc:129: NULL, NULL, created)) >>> nit: please indent so the start of the arguments are aligned >>> >>> >> done. >> >> >>> >>> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >>> webkit/glue/plugins/pepper_audio.cc:270: #error "Unknown OS" >>> nit: indent by 4 spaces like the other branches? >>> >>> >> done. >> >> >>> >>> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >>> webkit/glue/plugins/pepper_audio.cc:273: shared_memory_size, >>> static_cast<int64_t>(socket_handle)); >>> nit: align the arguments so they all start at the same column >>> >>> >> done. >> >> >>> >>> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >>> File webkit/glue/plugins/pepper_plugin_delegate.h (right): >>> >>> >>> http://codereview.chromium.org/5202002/diff/37001/webkit/glue/plugins/pepper_... >>> webkit/glue/plugins/pepper_plugin_delegate.h:143: virtual void >>> ShutDown() = 0; >>> nit: add a new line above the protected label >>> >>> >> done. >> >> >>> >>> http://codereview.chromium.org/5202002/ >>> >> >> >
http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... File ppapi/c/dev/ppb_audio_trusted_dev.h (right): http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:21: // Opens a paused audio interface, used by trusted side of proxy. Put blank lines between functions. http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:23: int32_t (*Open)(PP_Resource audio, PP_Resource config, Rather than "Returns 0 on success, negative error code on failure" this should say that it returns a standard PP_ error code, which will normally be PP_ERROR_WOULDBLOCK (meaning the callback will be called). Then changing the places where you return errors to use the correct enum values. http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:35: void (*Close)(PP_Resource audio); I don't think we need this. There's no way to interrupt the open call, unlike for URLLoader, which is what Darin was thinking when he speced the interface. Having the object be released should be sufficient. http://codereview.chromium.org/5202002/diff/51001/ppapi/examples/audio/audio.cc File ppapi/examples/audio/audio.cc (right): http://codereview.chromium.org/5202002/diff/51001/ppapi/examples/audio/audio.... ppapi/examples/audio/audio.cc:35: obtained_sample_count); This can be on the previous line. http://codereview.chromium.org/5202002/diff/51001/ppapi/examples/audio/audio.... ppapi/examples/audio/audio.cc:36: audio_ = pp::Audio_Dev( This can all be on one line now. http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_audio.cc (right): http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:8: #include "third_party/ppapi/c/pp_completion_callback.h" Alphabetize the headers. http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:136: PP_RunCompletionCallback(&created, -1); Don't hard code the error values. Use PP_ERROR_* instead. http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:152: if (audio) { Be consistent about using {} for single-line conditionals or not (same in the next function). Most Chrome code doesn't use them and I would remove them here as well. http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:295: // If the completion callback hasn't fired yet, do so here Be sure your comments are complete sentences with periods (same elsewhere). http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_audio.h (right): http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.h:70: PP_Instance GetInstance() { This should be called pp_instance() const {...} since it's an inline getter. http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.h:74: bool GetSyncSocket(int *sync_socket); Put the * next to the type.
http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... File chrome/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:182: MessageLoop* main_message_loop_; I'm not convinced you need to store this since this should be the same as filter_->message_loop(). You seem to be presuming that in the rest of the code. http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:308: client_->StreamCreated(handle, length, socket_handle); If the reference was taken to ensure that the object is alive while waiting for CreateAudioStream() to return, then it should be released here after StreamCreated() is called, and not in ShutDown(). The way I would go about writing this is to add another method called DispatchStreamCreated() or something, that would do: DispatchStreamCreated() { client->StreamCreated(handle, length, socket_handle); Release(); } and then here I'd do: filter_->message_loop()->PostTask(FROM_HERE, new RunnableMethod(this, &PlatformAudioImpl::DispatchStreamCreated, handle, socket_handle, length)); http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:324: filter_->RemoveDelegate(stream_id_); I think a way to avoid the race would be to move RemoveDelegate() into the destructor and make sure OnLowLatencyCreated() handles being called after ShutDown(). http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:328: filter_->message_loop()->ReleaseSoon(FROM_HERE, this); You are releasing twice, it seams? http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:333: Release(); // May end up deleting ourselves. I think this Release() doesn't belong here. http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... File ppapi/c/dev/ppb_audio_trusted_dev.h (right): http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:23: int32_t (*Open)(PP_Resource audio, PP_Resource config, Why is Open() separate from CreateTrusted(), aren't they always called in succession?
http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... File ppapi/c/dev/ppb_audio_trusted_dev.h (right): http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:23: int32_t (*Open)(PP_Resource audio, PP_Resource config, At least in my patch, I this separation because I wanted the callback to have a reference to the PP_Resource. This way, I can use the callback factory and add the resource as part of the closure. Combining these would make it hard for the proxy layer to track which resource the callback was for.
I would like to discuss (in person) some of Neb's concerns some more, maybe tomorrow. Thanks for the reviews, ptal. http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... File chrome/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:182: MessageLoop* main_message_loop_; On 2010/11/22 19:48:26, neb wrote: > I'm not convinced you need to store this since this should be the same as > filter_->message_loop(). You seem to be presuming that in the rest of the code. Printing out filter_->message_loop() and main_message_loop_ from the constructor show them to be different. http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:308: client_->StreamCreated(handle, length, socket_handle); On 2010/11/22 19:48:26, neb wrote: > If the reference was taken to ensure that the object is alive while waiting for > CreateAudioStream() to return, then it should be released here after > StreamCreated() is called, and not in ShutDown(). > > The way I would go about writing this is to add another method called > DispatchStreamCreated() or something, that would do: > > DispatchStreamCreated() { > client->StreamCreated(handle, length, socket_handle); > Release(); > } > > and then here I'd do: > > filter_->message_loop()->PostTask(FROM_HERE, new RunnableMethod(this, > &PlatformAudioImpl::DispatchStreamCreated, handle, socket_handle, length)); > What is here seems to be working, but I'd like to discuss more (maybe tomorrow?) http://codereview.chromium.org/5202002/diff/51001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:328: filter_->message_loop()->ReleaseSoon(FROM_HERE, this); On 2010/11/22 19:48:26, neb wrote: > You are releasing twice, it seams? There's an extra AddRef() above (line 288.) If I understand it correctly, this puts a release on the message queue. If there is a pending StreamCreated also in the queue, it will be invoked first, then this release will occur. http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... File ppapi/c/dev/ppb_audio_trusted_dev.h (right): http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:21: // Opens a paused audio interface, used by trusted side of proxy. On 2010/11/22 18:22:09, brettw wrote: > Put blank lines between functions. Done. http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:23: int32_t (*Open)(PP_Resource audio, PP_Resource config, On 2010/11/22 18:22:09, brettw wrote: > Rather than "Returns 0 on success, negative error code on failure" this should > say that it returns a standard PP_ error code, which will normally be > PP_ERROR_WOULDBLOCK (meaning the callback will be called). Then changing the > places where you return errors to use the correct enum values. Done. http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:23: int32_t (*Open)(PP_Resource audio, PP_Resource config, On 2010/11/22 19:48:26, neb wrote: > Why is Open() separate from CreateTrusted(), aren't they always called in > succession? Same as Brett, needed to pass PP_Resource in the completion callback. http://codereview.chromium.org/5202002/diff/51001/ppapi/c/dev/ppb_audio_trust... ppapi/c/dev/ppb_audio_trusted_dev.h:35: void (*Close)(PP_Resource audio); On 2010/11/22 18:22:09, brettw wrote: > I don't think we need this. There's no way to interrupt the open call, unlike > for URLLoader, which is what Darin was thinking when he speced the interface. > Having the object be released should be sufficient. Close() removed http://codereview.chromium.org/5202002/diff/51001/ppapi/examples/audio/audio.cc File ppapi/examples/audio/audio.cc (right): http://codereview.chromium.org/5202002/diff/51001/ppapi/examples/audio/audio.... ppapi/examples/audio/audio.cc:35: obtained_sample_count); On 2010/11/22 18:22:09, brettw wrote: > This can be on the previous line. Done. http://codereview.chromium.org/5202002/diff/51001/ppapi/examples/audio/audio.... ppapi/examples/audio/audio.cc:36: audio_ = pp::Audio_Dev( On 2010/11/22 18:22:09, brettw wrote: > This can all be on one line now. Done. http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_audio.cc (right): http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:8: #include "third_party/ppapi/c/pp_completion_callback.h" On 2010/11/22 18:22:09, brettw wrote: > Alphabetize the headers. Done. http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:136: PP_RunCompletionCallback(&created, -1); On 2010/11/22 18:22:09, brettw wrote: > Don't hard code the error values. Use PP_ERROR_* instead. replaced with PP_ERROR_FAILED and removed PP_RunCompletionCallback() http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:152: if (audio) { On 2010/11/22 18:22:09, brettw wrote: > Be consistent about using {} for single-line conditionals or not (same in the > next function). Most Chrome code doesn't use them and I would remove them here > as well. Done. http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:295: // If the completion callback hasn't fired yet, do so here On 2010/11/22 18:22:09, brettw wrote: > Be sure your comments are complete sentences with periods (same elsewhere). done (this comment continues on the next line :) http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_audio.h (right): http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.h:70: PP_Instance GetInstance() { On 2010/11/22 18:22:09, brettw wrote: > This should be called pp_instance() const {...} since it's an inline getter. Done. http://codereview.chromium.org/5202002/diff/51001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.h:74: bool GetSyncSocket(int *sync_socket); On 2010/11/22 18:22:09, brettw wrote: > Put the * next to the type. Done.
http://codereview.chromium.org/5202002/diff/63001/chrome/renderer/pepper_plug... File chrome/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/5202002/diff/63001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:326: filter_->message_loop()->ReleaseSoon(FROM_HERE, this); I'm confused about this since it looks like you're doing two releases, one on the I/O thread and one on the UI thread 4 lines below. I only see one AddRef. http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_audio.cc (right): http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:264: CHECK(!audio_); Normally we would DCHECK for something like this http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:265: create_callback_ = create_callback; Setting the callback should be done right before you return. The way it is now, we could return ERROR_FAILED bu tthe callback would still fire. http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:277: // At this point, we are guarranteeing ownership of the completion guarranteeing -> guaranteeing http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:340: #error "Unknown OS" I would unindent this 2 spaces.
On Mon, Nov 22, 2010 at 11:10 PM, <brettw@chromium.org> wrote: > > > http://codereview.chromium.org/5202002/diff/63001/chrome/renderer/pepper_plug... > > File chrome/renderer/pepper_plugin_delegate_impl.cc (right): > > > http://codereview.chromium.org/5202002/diff/63001/chrome/renderer/pepper_plug... > chrome/renderer/pepper_plugin_delegate_impl.cc:326: > > filter_->message_loop()->ReleaseSoon(FROM_HERE, this); > I'm confused about this since it looks like you're doing two releases, > one on the I/O thread and one on the UI thread 4 lines below. I only see > one AddRef. > The reference counting code is mine. I can explain what is going on. We take a reference for the filer->AddDelegate call, and so we need to call Release for the filter->RemoveDelegate call. However, since the filter may be poking us at that point in time, we need to perform that Release on the IO thread, so we do so by using a ReleaseSoon on the filter's message_loop. The final release is to balance the implicit AddRef taken in PepperPluginDelegateImpl::CreateAudio. See the call to scoped_refptr<T>::release(). If you have a better suggestion about how to implement this, I'm all ears :-) There are some tricky race conditions that the ReleaseSoon helps alleviate. One idea might be to implement a Traits class for RefCountedThreadSafe to hide the details of destroying the PlatformAudioImpl on the IO thread. -Darin > > http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... > > File webkit/glue/plugins/pepper_audio.cc (right): > > > http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... > webkit/glue/plugins/pepper_audio.cc:264: CHECK(!audio_); > Normally we would DCHECK for something like this > > > http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... > webkit/glue/plugins/pepper_audio.cc:265: create_callback_ = > create_callback; > Setting the callback should be done right before you return. The way it > is now, we could return ERROR_FAILED bu tthe callback would still fire. > > > http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... > webkit/glue/plugins/pepper_audio.cc:277: // At this point, we are > guarranteeing ownership of the completion > guarranteeing -> guaranteeing > > > http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... > webkit/glue/plugins/pepper_audio.cc:340: #error "Unknown OS" > I would unindent this 2 spaces. > > > http://codereview.chromium.org/5202002/ >
> The reference counting code is mine. I can explain what is going on. We > take a reference > for the filer->AddDelegate call, and so we need to call Release for the > filter->RemoveDelegate call. > However, since the filter may be poking us at that point in time, we need to > perform that Release > on the IO thread, so we do so by using a ReleaseSoon on the filter's > message_loop. > > The final release is to balance the implicit AddRef taken > in PepperPluginDelegateImpl::CreateAudio. > See the call to scoped_refptr<T>::release(). > > If you have a better suggestion about how to implement this, I'm all ears > :-) There are some tricky > race conditions that the ReleaseSoon helps alleviate. > > One idea might be to implement a Traits class for RefCountedThreadSafe to > hide the details of > destroying the PlatformAudioImpl on the IO thread. I see. I think if we're going to explicitly manage the PlatformAudioImpl's life cycle, then it shouldn't be a RefCounted - we should just allocate it in Init() and free it in Shutdown(). The original idea with RefCounted was that we ref one when we queue a callback and unref one when it gets back. This whole thing is based on the idea that we never get unsolicited messages. If we do get messages that we didn't initiate, then we shouldn't ref for callbacks - instead, we need to ref one for the filter, and remove it when we remove ourself from the filter. That's init and shutdown, and again we may just as well do new and delete, it's simpler.
On Tue, Nov 23, 2010 at 10:33 AM, <neb@chromium.org> wrote: > The reference counting code is mine. I can explain what is going on. We >> take a reference >> for the filer->AddDelegate call, and so we need to call Release for the >> filter->RemoveDelegate call. >> However, since the filter may be poking us at that point in time, we need >> to >> perform that Release >> on the IO thread, so we do so by using a ReleaseSoon on the filter's >> message_loop. >> > > The final release is to balance the implicit AddRef taken >> in PepperPluginDelegateImpl::CreateAudio. >> See the call to scoped_refptr<T>::release(). >> > > If you have a better suggestion about how to implement this, I'm all ears >> :-) There are some tricky >> race conditions that the ReleaseSoon helps alleviate. >> > > One idea might be to implement a Traits class for RefCountedThreadSafe to >> hide the details of >> destroying the PlatformAudioImpl on the IO thread. >> > > I see. I think if we're going to explicitly manage the PlatformAudioImpl's > life > cycle, then it shouldn't be a RefCounted - we should just allocate it in > Init() > and free it in Shutdown(). > That doesn't quite work. When Shutdown is called, the IO thread could be poking us. We could potentially solve this by using DeleteSoon to post the deletion to the IO thread. That would also work. -Darin > > The original idea with RefCounted was that we ref one when we queue a > callback > and unref one when it gets back. This whole thing is based on the idea that > we > never get unsolicited messages. If we do get messages that we didn't > initiate, > then we shouldn't ref for callbacks - instead, we need to ref one for the > filter, and remove it when we remove ourself from the filter. That's init > and > shutdown, and again we may just as well do new and delete, it's simpler. > > > > http://codereview.chromium.org/5202002/ >
On Tue, Nov 23, 2010 at 11:12 AM, Darin Fisher <darin@chromium.org> wrote: > > > On Tue, Nov 23, 2010 at 10:33 AM, <neb@chromium.org> wrote: > >> The reference counting code is mine. I can explain what is going on. We >>> take a reference >>> for the filer->AddDelegate call, and so we need to call Release for the >>> filter->RemoveDelegate call. >>> However, since the filter may be poking us at that point in time, we need >>> to >>> perform that Release >>> on the IO thread, so we do so by using a ReleaseSoon on the filter's >>> message_loop. >>> >> >> The final release is to balance the implicit AddRef taken >>> in PepperPluginDelegateImpl::CreateAudio. >>> See the call to scoped_refptr<T>::release(). >>> >> >> If you have a better suggestion about how to implement this, I'm all ears >>> :-) There are some tricky >>> race conditions that the ReleaseSoon helps alleviate. >>> >> >> One idea might be to implement a Traits class for RefCountedThreadSafe to >>> hide the details of >>> destroying the PlatformAudioImpl on the IO thread. >>> >> >> I see. I think if we're going to explicitly manage the PlatformAudioImpl's >> life >> cycle, then it shouldn't be a RefCounted - we should just allocate it in >> Init() >> and free it in Shutdown(). >> > > That doesn't quite work. When Shutdown is called, the IO thread could be > poking us. > We could potentially solve this by using DeleteSoon to post the deletion to > the > IO thread. That would also work. > > Yeah, I'm convinced DeleteSoon would be cleaner. -Darin > -Darin > > >> >> The original idea with RefCounted was that we ref one when we queue a >> callback >> and unref one when it gets back. This whole thing is based on the idea >> that we >> never get unsolicited messages. If we do get messages that we didn't >> initiate, >> then we shouldn't ref for callbacks - instead, we need to ref one for the >> filter, and remove it when we remove ourself from the filter. That's init >> and >> shutdown, and again we may just as well do new and delete, it's simpler. >> >> >> >> http://codereview.chromium.org/5202002/ >> > >
more fixes... ptal thanks! http://codereview.chromium.org/5202002/diff/63001/chrome/renderer/pepper_plug... File chrome/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/5202002/diff/63001/chrome/renderer/pepper_plug... chrome/renderer/pepper_plugin_delegate_impl.cc:326: filter_->message_loop()->ReleaseSoon(FROM_HERE, this); On 2010/11/23 07:10:19, brettw wrote: > I'm confused about this since it looks like you're doing two releases, one on > the I/O thread and one on the UI thread 4 lines below. I only see one AddRef. After talking with Darin, we removed one AddRef and only release now on the IO thread. http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_audio.cc (right): http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:264: CHECK(!audio_); On 2010/11/23 07:10:19, brettw wrote: > Normally we would DCHECK for something like this Done. http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:265: create_callback_ = create_callback; On 2010/11/23 07:10:19, brettw wrote: > Setting the callback should be done right before you return. The way it is now, > we could return ERROR_FAILED bu tthe callback would still fire. good catch, moved http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:265: create_callback_ = create_callback; On 2010/11/23 07:10:19, brettw wrote: > Setting the callback should be done right before you return. The way it is now, > we could return ERROR_FAILED bu tthe callback would still fire. good catch, moved http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:277: // At this point, we are guarranteeing ownership of the completion On 2010/11/23 07:10:19, brettw wrote: > guarranteeing -> guaranteeing Done. http://codereview.chromium.org/5202002/diff/63001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:340: #error "Unknown OS" On 2010/11/23 07:10:19, brettw wrote: > I would unindent this 2 spaces. Done.
LGTM http://codereview.chromium.org/5202002/diff/74001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_audio.cc (right): http://codereview.chromium.org/5202002/diff/74001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:274: // At this point, we are guaranteeing ownership of the completion Generally we would put blank lines before comment blocks like this to help separate things. http://codereview.chromium.org/5202002/diff/74001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:341: // Trusted side of proxy can specify a callback to recieve handles. Can you also add a blank line above this?
In addition, to pass trybots: - added "stuct" before PP_CompletionCallback to make c api compile (ppb_audio_trusted_dev.h), - moved the OS specific #if's about to satisfy dist-cc trybots (pepper_audio.cc) - shorten include paths from third_party/ppapi -> ppapi/ - small re-factor of the trusted handle interface, uses int for shm handle instead of uint64_t (make consistent with socket interface), removed need for extra shared_memory_handle_ member in Audio class, and added reinterpret_cast to hush the win trybot. - removed CloseTrusted() from pepper_audio.h http://codereview.chromium.org/5202002/diff/74001/webkit/glue/plugins/pepper_... File webkit/glue/plugins/pepper_audio.cc (right): http://codereview.chromium.org/5202002/diff/74001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:274: // At this point, we are guaranteeing ownership of the completion On 2010/11/23 22:24:39, brettw wrote: > Generally we would put blank lines before comment blocks like this to help > separate things. Done. http://codereview.chromium.org/5202002/diff/74001/webkit/glue/plugins/pepper_... webkit/glue/plugins/pepper_audio.cc:341: // Trusted side of proxy can specify a callback to recieve handles. On 2010/11/23 22:24:39, brettw wrote: > Can you also add a blank line above this? Done.
http://codereview.chromium.org/5202002/diff/125001/chrome/renderer/pepper_plu... File chrome/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/5202002/diff/125001/chrome/renderer/pepper_plu... chrome/renderer/pepper_plugin_delegate_impl.cc:324: // Release on the IO thread so that we avoid race problems with As Brett commented on the other file, please put a new line above the comment block here. http://codereview.chromium.org/5202002/diff/125001/chrome/renderer/pepper_plu... chrome/renderer/pepper_plugin_delegate_impl.cc:581: return audio.release(); I recommend inserting a comment here indicating that this "leak" is balanced in Shutdown. http://codereview.chromium.org/5202002/diff/125001/ppapi/c/dev/ppb_audio_trus... File ppapi/c/dev/ppb_audio_trusted_dev.h (right): http://codereview.chromium.org/5202002/diff/125001/ppapi/c/dev/ppb_audio_trus... ppapi/c/dev/ppb_audio_trusted_dev.h:30: // Get the sync socket. Use from completion callback. nit: "Use from completion callback" should really say "Use once Open has completed." http://codereview.chromium.org/5202002/diff/125001/ppapi/c/dev/ppb_audio_trus... ppapi/c/dev/ppb_audio_trusted_dev.h:34: // Get the shared memory interface. Use from completion callback. ditto. http://codereview.chromium.org/5202002/diff/125001/ppapi/c/dev/ppb_audio_trus... ppapi/c/dev/ppb_audio_trusted_dev.h:36: int32_t (*GetSharedMemory)(PP_Resource audio, why should this be different from PPB_ImageDataTrusted::GetNativeMemoryHandle? should that one be changed to match this one? http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... File webkit/glue/plugins/pepper_audio.cc (right): http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... webkit/glue/plugins/pepper_audio.cc:85: // TODO(neb): Require callback to be present for untrusted plugins. it seems like it'll be easy to enforce this TODO now that Create is not called by trusted code. http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... webkit/glue/plugins/pepper_audio.cc:263: int32_t Audio::OpenTrusted(PluginDelegate* plugin_delegate, nit: might be nice to call this "Open" to match up with the API method name. http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... webkit/glue/plugins/pepper_audio.cc:275: if (audio_ == NULL) nit: in chromium code, we usually write "if (!audio_)" instead. http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... webkit/glue/plugins/pepper_audio.cc:357: // Recurring callback to fill audio buffers. nit: this comment is confusing... what is it trying to tell me?
feedback addressed, thx http://codereview.chromium.org/5202002/diff/125001/chrome/renderer/pepper_plu... File chrome/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/5202002/diff/125001/chrome/renderer/pepper_plu... chrome/renderer/pepper_plugin_delegate_impl.cc:324: // Release on the IO thread so that we avoid race problems with On 2010/11/24 20:34:35, darin wrote: > As Brett commented on the other file, please put a new line above the comment > block here. Done. http://codereview.chromium.org/5202002/diff/125001/chrome/renderer/pepper_plu... chrome/renderer/pepper_plugin_delegate_impl.cc:581: return audio.release(); On 2010/11/24 20:34:35, darin wrote: > I recommend inserting a comment here indicating that this "leak" is balanced in > Shutdown. Done. http://codereview.chromium.org/5202002/diff/125001/ppapi/c/dev/ppb_audio_trus... File ppapi/c/dev/ppb_audio_trusted_dev.h (right): http://codereview.chromium.org/5202002/diff/125001/ppapi/c/dev/ppb_audio_trus... ppapi/c/dev/ppb_audio_trusted_dev.h:30: // Get the sync socket. Use from completion callback. On 2010/11/24 20:34:35, darin wrote: > nit: "Use from completion callback" should really say "Use once Open has > completed." Done. http://codereview.chromium.org/5202002/diff/125001/ppapi/c/dev/ppb_audio_trus... ppapi/c/dev/ppb_audio_trusted_dev.h:34: // Get the shared memory interface. Use from completion callback. On 2010/11/24 20:34:35, darin wrote: > ditto. Done. http://codereview.chromium.org/5202002/diff/125001/ppapi/c/dev/ppb_audio_trus... ppapi/c/dev/ppb_audio_trusted_dev.h:36: int32_t (*GetSharedMemory)(PP_Resource audio, On 2010/11/24 20:34:35, darin wrote: > why should this be different from PPB_ImageDataTrusted::GetNativeMemoryHandle? > should that one be changed to match this one? As discussed, shared memory will return int instead of uint64_t handle (to match sync socket call, and underlying OS handle type.) I like your suggestion of changing a few error code cases from PP_ERROR_BADARGUMENT to PP_ERROR_BADRESOURCE for when a resource id is bogus. Changes to PPB_ImageDataTrusted as a separate CL. http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... File webkit/glue/plugins/pepper_audio.cc (right): http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... webkit/glue/plugins/pepper_audio.cc:85: // TODO(neb): Require callback to be present for untrusted plugins. On 2010/11/24 20:34:35, darin wrote: > it seems like it'll be easy to enforce this TODO now that Create is not called > by trusted code. Added check for NULL user_callback here, and check for NULL completion callback in the trusted case. Removed todo comment. http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... webkit/glue/plugins/pepper_audio.cc:263: int32_t Audio::OpenTrusted(PluginDelegate* plugin_delegate, On 2010/11/24 20:34:35, darin wrote: > nit: might be nice to call this "Open" to match up with the API method name. Done. http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... webkit/glue/plugins/pepper_audio.cc:275: if (audio_ == NULL) On 2010/11/24 20:34:35, darin wrote: > nit: in chromium code, we usually write "if (!audio_)" instead. Done. http://codereview.chromium.org/5202002/diff/125001/webkit/glue/plugins/pepper... webkit/glue/plugins/pepper_audio.cc:357: // Recurring callback to fill audio buffers. On 2010/11/24 20:34:35, darin wrote: > nit: this comment is confusing... what is it trying to tell me? clarified
LGTM
committed as r67354
On Wed, Nov 24, 2010 at 4:30 PM, <nfullagar@google.com> wrote: > committed as r67354 Woo hoo! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
