Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2065513004:
Expose flash playback status and volume control to content renderer (Closed)
|
Created:
4 years, 6 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 6 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@audio_focus_manager Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose flash playback status and volume control to content renderer
This CL plumbs flash playback status and volume control to the
content renderer. It basically lets the renderer know when flash
starts/stops to play sound, and let it be able to control it's
volume.
The plumbing is to be continued to WebContents, and then we are
able to let flash be controlled by MediaSession, which prevents
multiple tabs from playing sound at the same time (which usually
annoys the user).
Follow-up CL:
https://codereview.chromium.org/2060933002/
BUG=619084
Committed: https://crrev.com/c885f6ba83d8687b840c0fa684a941d45f9162be
Cr-Commit-Position: refs/heads/master@{#401687}
Patch Set 1 #Patch Set 2 : added TODO #
Total comments: 6
Patch Set 3 : addressed bbudge's comments #
Total comments: 4
Patch Set 4 : fixed nits for bbudge #
Total comments: 8
Patch Set 5 : addressed dcheng's comments #Patch Set 6 : moved start/stop/created/deleted to the follow-up CL #
Total comments: 4
Patch Set 7 : addressing sievers' comments #Patch Set 8 : moving audio delegate to pepper #
Total comments: 24
Patch Set 9 : addressed bbudge's comments #
Total comments: 20
Patch Set 10 : addressing nits #
Total comments: 4
Patch Set 11 : addressing nits #
Total comments: 1
Messages
Total messages: 41 (12 generated)
Description was changed from ========== Expose pepper playback status and volume control to WebContents This CL plumbs pepper playback status and volume control from render to WebContents. It is intended to be used by a follow-up CL for letting pepper be controlled by MediaSession. BUG=619084 ========== to ========== Expose flash playback status and volume control to WebContents This CL plumbs flash playback status and volume control from render to WebContents. It is intended to be used by a follow-up CL for letting flash be controlled by MediaSession. BUG=619084 ==========
Description was changed from ========== Expose flash playback status and volume control to WebContents This CL plumbs flash playback status and volume control from render to WebContents. It is intended to be used by a follow-up CL for letting flash be controlled by MediaSession. BUG=619084 ========== to ========== Expose flash playback status and volume control to WebContents This CL plumbs flash playback status and volume control from render to WebContents. It is intended to be used by a follow-up CL for letting flash be controlled by MediaSession. Follow-up CL: https://codereview.chromium.org/2060933002/ BUG=619084 ==========
Description was changed from ========== Expose flash playback status and volume control to WebContents This CL plumbs flash playback status and volume control from render to WebContents. It is intended to be used by a follow-up CL for letting flash be controlled by MediaSession. Follow-up CL: https://codereview.chromium.org/2060933002/ BUG=619084 ========== to ========== Expose flash playback status and volume control to WebContents This CL plumbs flash playback status and volume control from render to WebContents. It basically lets WebContents know when flash starts to play sound, and let WebContents be able to control it's volume. It is intended to be used by a follow-up CL for letting flash be controlled by MediaSession, which prevents multiple tabs from playing sound at the same time (which usually annoys the user). Follow-up CL: https://codereview.chromium.org/2060933002/ BUG=619084 ==========
zqzhang@chromium.org changed reviewers: + bbudge@chromium.org, dcheng@chromium.org, mlamouri@chromium.org, sievers@chromium.org
PTAL. Adding reviewers for initial round of review. +sievers@ to review: content/* +dcheng@ to review: content/common/frame_messages.h +bbudge@ to review: content/renderer/pepper/* +mlamouri@ for local review
Pepper review https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper... File content/renderer/pepper/ppb_audio_impl.cc (right): https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper... content/renderer/pepper/ppb_audio_impl.cc:106: PepperPluginInstance::Get(pp_instance())); Check for null instance, or add a CHECK or DCHECK if instance cannot be null. https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper... content/renderer/pepper/ppb_audio_impl.cc:188: PepperPluginInstance::Get(pp_instance())); Ditto. https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper... File content/renderer/pepper/ppb_audio_impl.h (right): https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper... content/renderer/pepper/ppb_audio_impl.h:55: int32_t pepper_audio_id() { return pepper_audio_id_; } This doesn't seem to be used anywhere. Is it for the follow up CL?
PTAL. Addressed bbudge's comments.
A question for bbudge:
Now I observe PepperInstance{Created/Destroyed } and Pepper{Start/Stop}Playback
events.
Do I need to handle any other pepper life-cycle-related events, such as pepper
crash, etc.?
https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper...
File content/renderer/pepper/ppb_audio_impl.cc (right):
https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper...
content/renderer/pepper/ppb_audio_impl.cc:106:
PepperPluginInstance::Get(pp_instance()));
On 2016/06/13 17:51:01, bbudge wrote:
> Check for null instance, or add a CHECK or DCHECK if instance cannot be null.
Done.
https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper...
content/renderer/pepper/ppb_audio_impl.cc:188:
PepperPluginInstance::Get(pp_instance()));
On 2016/06/13 17:51:01, bbudge wrote:
> Ditto.
Done.
https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper...
File content/renderer/pepper/ppb_audio_impl.h (right):
https://codereview.chromium.org/2065513004/diff/20001/content/renderer/pepper...
content/renderer/pepper/ppb_audio_impl.h:55: int32_t pepper_audio_id() { return
pepper_audio_id_; }
On 2016/06/13 17:51:01, bbudge wrote:
> This doesn't seem to be used anywhere. Is it for the follow up CL?
Sorry. This was in an experimental patch, and it's not needed any more.
Removed this method and related code.
Patchset #3 (id:40001) has been deleted
I don't think there are any other events you need to observe. https://codereview.chromium.org/2065513004/diff/60001/content/renderer/pepper... File content/renderer/pepper/ppb_audio_impl.cc (right): https://codereview.chromium.org/2065513004/diff/60001/content/renderer/pepper... content/renderer/pepper/ppb_audio_impl.cc:190: PepperPluginInstance::Get(pp_instance())); null check for instance here. https://codereview.chromium.org/2065513004/diff/60001/content/renderer/pepper... content/renderer/pepper/ppb_audio_impl.cc:199: void PPB_Audio_Impl::SetVolume(double volume) { Check that audio_ isn't null? See StopPlayback.
Sorry I forgot to do null checks in other places. PTAL.
https://codereview.chromium.org/2065513004/diff/60001/content/renderer/pepper... File content/renderer/pepper/ppb_audio_impl.cc (right): https://codereview.chromium.org/2065513004/diff/60001/content/renderer/pepper... content/renderer/pepper/ppb_audio_impl.cc:190: PepperPluginInstance::Get(pp_instance())); On 2016/06/14 17:19:13, bbudge wrote: > null check for instance here. Done. https://codereview.chromium.org/2065513004/diff/60001/content/renderer/pepper... content/renderer/pepper/ppb_audio_impl.cc:199: void PPB_Audio_Impl::SetVolume(double volume) { On 2016/06/14 17:19:13, bbudge wrote: > Check that audio_ isn't null? See StopPlayback. Done.
content/renderer/pepper lgtm
https://codereview.chromium.org/2065513004/diff/80001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/2065513004/diff/80001/content/common/frame_me... content/common/frame_messages.h:1547: int32_t, Can you comment on what these fields mean? I assume int32_t is the plugin instance, and double is the volume, but it's not so clear here. https://codereview.chromium.org/2065513004/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2065513004/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:6257: for (auto ppb_audio : pepper_audios_map_[pp_instance]) { Nit: auto*, since this is a pointer. https://codereview.chromium.org/2065513004/diff/80001/extensions/browser/exte... File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/2065513004/diff/80001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:194: void ExtensionWebContentsObserver::PepperInstanceDeleted(int32_t pp_instance) { Do we need to pass pp_instance needed here (and above)? It doesn't seem to be used.
PTAL. Addressed dcheng's comments. https://codereview.chromium.org/2065513004/diff/80001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/2065513004/diff/80001/content/common/frame_me... content/common/frame_messages.h:1547: int32_t, On 2016/06/14 20:00:32, dcheng wrote: > Can you comment on what these fields mean? I assume int32_t is the plugin > instance, and double is the volume, but it's not so clear here. Done. Also added comments for other messages, and put FrameMsg_ to the correct place. https://codereview.chromium.org/2065513004/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2065513004/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:6257: for (auto ppb_audio : pepper_audios_map_[pp_instance]) { On 2016/06/14 20:00:32, dcheng wrote: > Nit: auto*, since this is a pointer. Done. https://codereview.chromium.org/2065513004/diff/80001/extensions/browser/exte... File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/2065513004/diff/80001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:194: void ExtensionWebContentsObserver::PepperInstanceDeleted(int32_t pp_instance) { On 2016/06/14 20:00:32, dcheng wrote: > Do we need to pass pp_instance needed here (and above)? It doesn't seem to be > used. It is needed in a follow-up CL (2060933002), where the browser need to know which Pepper instance has been deleted.
https://codereview.chromium.org/2065513004/diff/80001/extensions/browser/exte... File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/2065513004/diff/80001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:194: void ExtensionWebContentsObserver::PepperInstanceDeleted(int32_t pp_instance) { On 2016/06/20 19:11:54, Zhiqiang Zhang wrote: > On 2016/06/14 20:00:32, dcheng wrote: > > Do we need to pass pp_instance needed here (and above)? It doesn't seem to be > > used. > > It is needed in a follow-up CL (2060933002), where the browser need to know > which Pepper instance has been deleted. OK. This CL is fine, but that one has at least one bug (it assumes that instance_id sent by the renderer is always valid. It can get into trouble if the renderer sends a playback started message without sending a pepper instance created message). I would suggest move this particular IPC change into the other CL (and getting a security review) or merging the two CLs.
PTAL. Addressed dcheng's comments (moving some IPC message changes to the follow-up CL). See replies below. https://codereview.chromium.org/2065513004/diff/80001/extensions/browser/exte... File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/2065513004/diff/80001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:194: void ExtensionWebContentsObserver::PepperInstanceDeleted(int32_t pp_instance) { On 2016/06/20 19:32:08, dcheng wrote: > On 2016/06/20 19:11:54, Zhiqiang Zhang wrote: > > On 2016/06/14 20:00:32, dcheng wrote: > > > Do we need to pass pp_instance needed here (and above)? It doesn't seem to > be > > > used. > > > > It is needed in a follow-up CL (2060933002), where the browser need to know > > which Pepper instance has been deleted. > > OK. This CL is fine, but that one has at least one bug (it assumes that > instance_id sent by the renderer is always valid. It can get into trouble if the > renderer sends a playback started message without sending a pepper instance > created message). > > I would suggest move this particular IPC change into the other CL (and getting a > security review) or merging the two CLs. Yes, that CL is pretty rough, and there are several UX-related stuffs need to be figured out, so I don't want to merge the two. As you suggested, I think the Created/Deleted and Starts/Stops IPC messages should be moved to the other CL together, so I reverted all of them except for the SetPepperVolume one. Does this make sense to you?
ipc change lgtm but it feels weird to plumb this through RenderFrameImpl. I don't have any better suggestions though.
can you update the comment since WebContents is actually untouched in this CL? https://codereview.chromium.org/2065513004/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2065513004/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:6254: // delegates the SetVolume() method of all it's audio instances. can you DCHECK() that it does not exist? https://codereview.chromium.org/2065513004/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:6260: pepper_audios_map_[instance->pp_instance()].erase(ppb_audio); can you DCHECK() that it exists?
Description was changed from ========== Expose flash playback status and volume control to WebContents This CL plumbs flash playback status and volume control from render to WebContents. It basically lets WebContents know when flash starts to play sound, and let WebContents be able to control it's volume. It is intended to be used by a follow-up CL for letting flash be controlled by MediaSession, which prevents multiple tabs from playing sound at the same time (which usually annoys the user). Follow-up CL: https://codereview.chromium.org/2060933002/ BUG=619084 ========== to ========== Expose flash playback status and volume control to content renderer This CL plumbs flash playback status and volume control to the content renderer. It basically lets the renderer know when flash starts/stops to play sound, and let it be able to control it's volume. The plumbing is to be continued to WebContents, and then we are able to let flash be controlled by MediaSession, which prevents multiple tabs from playing sound at the same time (which usually annoys the user). Follow-up CL: https://codereview.chromium.org/2060933002/ BUG=619084 ==========
PTAL. Addressed sievers' comments. I'm using early returns instead of DCHECK() because we should not trust Flash. > can you update the comment since WebContents is actually untouched in this CL? Done. https://codereview.chromium.org/2065513004/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2065513004/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:6254: // delegates the SetVolume() method of all it's audio instances. On 2016/06/21 18:58:58, sievers wrote: > can you DCHECK() that it does not exist? I think we should not trust Flash, so I added early return here instead of DCHECK(). https://codereview.chromium.org/2065513004/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:6260: pepper_audios_map_[instance->pp_instance()].erase(ppb_audio); On 2016/06/21 18:58:58, sievers wrote: > can you DCHECK() that it exists? Ditto.
On 2016/06/21 19:32:02, Zhiqiang Zhang wrote: > PTAL. Addressed sievers' comments. I'm using early returns instead of DCHECK() > because we should not trust Flash. > That's a bit worrying because you are storing pointers in the map and OnSetPepperVolume() might cause use-after-free. Also how is this code going to work correctly if it doesn't track reliably but it's sort of only a rough effort in knowing what's playing?
On 2016/06/21 21:08:11, sievers wrote: > On 2016/06/21 19:32:02, Zhiqiang Zhang wrote: > > PTAL. Addressed sievers' comments. I'm using early returns instead of DCHECK() > > because we should not trust Flash. > > > > That's a bit worrying because you are storing pointers in the map and > OnSetPepperVolume() might cause use-after-free. Ah, maybe PepperInstanceDeleted() should be cleaning up this map too? But maybe it'd be even better if we were just storing this on the PepperPluginInstanceImpl itself so we don't have to make sure we're properly cleaning up? Is that possible? > > Also how is this code going to work correctly if it doesn't track reliably but > it's sort of only a rough effort in knowing what's playing?
On 2016/06/22 12:34:08, dcheng wrote: > Ah, maybe PepperInstanceDeleted() should be cleaning up this map too? > > But maybe it'd be even better if we were just storing this on the > PepperPluginInstanceImpl itself so we don't have to make sure we're properly > cleaning up? Is that possible? Yes, this is exactly what I'm think of. Letting PPB_Audio_Impl managed by PepperPluginInstanceImpl should be much cleaner. Working on it now. :)
Patchset #8 (id:160001) has been deleted
PTAL. I moved all the audio delegating into PepperPluginInstanceImpl. RenderFrameImpl now only forwards the IPC messages from/to the browser. All the logics are handled in PepperAudioControlDelegate. bbudge@, can you PTAL at pepper/* since there are a few new changes?
https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_control_delegate.cc (right): https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:20: DCHECK(!pepper_instance_); It seems to me like you could instead: if (instance_) OnPepperInstanceDeleted(); https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:25: if (ppb_audios_.count(audio)) return; It might be better to DCHECK that the instance isn't already in the set. Is there a reason to silently do nothing? https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:39: if (!ppb_audios_.count(audio)) return; DCHECK? https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:58: DCHECK(!pepper_instance_); DCHECK(instance_); https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:64: } This duplicated code could be moved to a private helper method: e.g. StopPlaybackOnEmpty. The code here could then clear the audio set, then call the method, then set instance_ to null. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_control_delegate.h (right): https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:20: * PepperPluginInstanceImpl. nits: s/it's/its s/audios/audio resources s/sharable/shareable https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:22: class PepperAudioControlDelegate { This class seems more like a tracker / controller. 'PepperAudioControlDelegate' is less clear to me than 'PepperAudioController' as a name. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:27: // be called before calling destructor. Instead of leaving it up to the client, why not automatically call OnPepperInstanceDeleted? The destructor shouldn't need a comment then. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:30: // An audio instance has started playback. Comments should tell what method does, e.g. // Adds an audio instance to the controller. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:31: void AddActiveAudioInstance(PPB_Audio_Impl* audio); s/AddActiveAudioInstance/AddInstance should be enough. Similarly for Remove. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:34: // Sets the volume of all delegated audios. s/audios/audio resources Otherwise, comment is good https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:46: PepperPluginInstanceImpl* pepper_instance_; s/pepper_instance_/instance_ Pepper should be obvious in this context.
Patchset #9 (id:200001) has been deleted
PTAL. Addressed bbudge's comments with replies. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_control_delegate.cc (right): https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:20: DCHECK(!pepper_instance_); On 2016/06/22 18:14:55, bbudge wrote: > It seems to me like you could instead: > > if (instance_) > OnPepperInstanceDeleted(); Done. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:25: if (ppb_audios_.count(audio)) return; On 2016/06/22 18:14:54, bbudge wrote: > It might be better to DCHECK that the instance isn't already in the set. Is > there a reason to silently do nothing? The StartPlayback signal originates from Pepper. I think we shouldn't trust it not calling two consequtive starts (or stops)? WDYT? https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:39: if (!ppb_audios_.count(audio)) return; On 2016/06/22 18:14:54, bbudge wrote: > DCHECK? Ditto. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:58: DCHECK(!pepper_instance_); On 2016/06/22 18:14:54, bbudge wrote: > DCHECK(instance_); Done. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.cc:64: } On 2016/06/22 18:14:54, bbudge wrote: > This duplicated code could be moved to a private helper method: e.g. > StopPlaybackOnEmpty. The code here could then clear the audio set, then call the > method, then set instance_ to null. Done. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_control_delegate.h (right): https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:20: * PepperPluginInstanceImpl. On 2016/06/22 18:14:55, bbudge wrote: > nits: s/it's/its > s/audios/audio resources > s/sharable/shareable Done. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:22: class PepperAudioControlDelegate { On 2016/06/22 18:14:55, bbudge wrote: > This class seems more like a tracker / controller. 'PepperAudioControlDelegate' > is less clear to me than 'PepperAudioController' as a name. Done. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:27: // be called before calling destructor. On 2016/06/22 18:14:55, bbudge wrote: > Instead of leaving it up to the client, why not automatically call > OnPepperInstanceDeleted? The destructor shouldn't need a comment then. Done. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:30: // An audio instance has started playback. On 2016/06/22 18:14:55, bbudge wrote: > Comments should tell what method does, e.g. > > // Adds an audio instance to the controller. Done. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:31: void AddActiveAudioInstance(PPB_Audio_Impl* audio); On 2016/06/22 18:14:55, bbudge wrote: > s/AddActiveAudioInstance/AddInstance should be enough. > Similarly for Remove. Done. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:34: // Sets the volume of all delegated audios. On 2016/06/22 18:14:55, bbudge wrote: > s/audios/audio resources > Otherwise, comment is good Done. https://codereview.chromium.org/2065513004/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_control_delegate.h:46: PepperPluginInstanceImpl* pepper_instance_; On 2016/06/22 18:14:55, bbudge wrote: > s/pepper_instance_/instance_ > > Pepper should be obvious in this context. Done.
lgtm with comments applied https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_controller.cc (right): https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.cc:26: if (ppb_audios_.count(audio)) return; style: I think that's okay in Java but not in C++, you should write: ``` if (!instance_) return; if (ppb_audios_.count(audio)) return; ``` https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.cc:39: if (!ppb_audios_.count(audio)) return; ditto https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.cc:48: if (!instance_) return; ditto https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_controller.h (right): https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:20: */ style: we don't use /** comments. I think it should be: ``` // Class for controlling ... // This class ... ``` https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:24: PepperAudioController(PepperPluginInstanceImpl* pepper_instance); add explicit. Also, I don't think the comment is super useful :) https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:25: // Destructor of PepperAudioController. ditto for the comment :) https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:48: PepperPluginInstanceImpl* instance_; style: leave an empty line between two methods or members https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_platform_audio_output.cc (right): https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_platform_audio_output.cc:64: if (ipc_) { I would prefer `if (!ipc_) return false;` (ie. early return) but I understand it doesn't match the style of the file and that's sad :) https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... File content/renderer/pepper/ppb_audio_impl.cc (right): https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/ppb_audio_impl.cc:51: instance->audio_controller().RemoveInstance(this); nit: you could do: ``` if (instance) { if (instantce->throttler()) instance->throttler()->RemoveObserver(this); instance->audio_controller().RemoveInstance(this); } ``` https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/ppb_audio_impl.cc:189: instance->audio_controller().RemoveInstance(this); Shouldn't that be AddInstance?
sievers@, could you kindly take a quick look at render_frame_impl.*? The changes in these files are small now. This CL is blocking some other CLs. bbudge@, are you okay with the new changes in pepper/? Addressed nits' from Mounir. https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_controller.cc (right): https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.cc:26: if (ppb_audios_.count(audio)) return; On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > style: I think that's okay in Java but not in C++, you should write: > ``` > if (!instance_) > return; > if (ppb_audios_.count(audio)) > return; > ``` Done. https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.cc:39: if (!ppb_audios_.count(audio)) return; On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > ditto Done. https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.cc:48: if (!instance_) return; On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > ditto Done. https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_controller.h (right): https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:20: */ On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > style: we don't use /** comments. I think it should be: > ``` > // Class for controlling ... > // This class ... > ``` Done. https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:24: PepperAudioController(PepperPluginInstanceImpl* pepper_instance); On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > add explicit. > > Also, I don't think the comment is super useful :) Done. https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:25: // Destructor of PepperAudioController. On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > ditto for the comment :) Done. https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:48: PepperPluginInstanceImpl* instance_; On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > style: leave an empty line between two methods or members Done (and for all the methods above as well). https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_platform_audio_output.cc (right): https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_platform_audio_output.cc:64: if (ipc_) { On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > I would prefer `if (!ipc_) return false;` (ie. early return) but I understand it > doesn't match the style of the file and that's sad :) Acknowledged. https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... File content/renderer/pepper/ppb_audio_impl.cc (right): https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/ppb_audio_impl.cc:51: instance->audio_controller().RemoveInstance(this); On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > nit: you could do: > ``` > if (instance) { > if (instantce->throttler()) > instance->throttler()->RemoveObserver(this); > instance->audio_controller().RemoveInstance(this); > } > ``` Done. https://codereview.chromium.org/2065513004/diff/220001/content/renderer/peppe... content/renderer/pepper/ppb_audio_impl.cc:189: instance->audio_controller().RemoveInstance(this); On 2016/06/23 13:04:13, Mounir Lamouri (slow) wrote: > Shouldn't that be AddInstance? Thanks! Done.
lgtm w/nit https://codereview.chromium.org/2065513004/diff/240001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_controller.h (right): https://codereview.chromium.org/2065513004/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:32: // Sets the volume of all delegated audio resources. nit: s/delegated// s/resources/instances
lgtm https://codereview.chromium.org/2065513004/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2065513004/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:6246: // TODO(zqzhang): send PepperStartsPlayback message to the browser. nit: bug numbers are a better reference than user names
https://codereview.chromium.org/2065513004/diff/240001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_controller.h (right): https://codereview.chromium.org/2065513004/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.h:32: // Sets the volume of all delegated audio resources. On 2016/06/23 17:12:42, bbudge wrote: > nit: s/delegated// > s/resources/instances Done. https://codereview.chromium.org/2065513004/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2065513004/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:6246: // TODO(zqzhang): send PepperStartsPlayback message to the browser. On 2016/06/23 17:25:58, sievers wrote: > nit: bug numbers are a better reference than user names Done.
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, mlamouri@chromium.org, bbudge@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2065513004/#ps260001 (title: "addressing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2065513004/260001
https://codereview.chromium.org/2065513004/diff/260001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_controller.cc (right): https://codereview.chromium.org/2065513004/diff/260001/content/renderer/peppe... content/renderer/pepper/pepper_audio_controller.cc:27: if (ppb_audios_.count(audio)) Why do we have to verify this here and below? Why would something get added twice in a row or removed twice in a row?
Message was sent while issue was closed.
Description was changed from ========== Expose flash playback status and volume control to content renderer This CL plumbs flash playback status and volume control to the content renderer. It basically lets the renderer know when flash starts/stops to play sound, and let it be able to control it's volume. The plumbing is to be continued to WebContents, and then we are able to let flash be controlled by MediaSession, which prevents multiple tabs from playing sound at the same time (which usually annoys the user). Follow-up CL: https://codereview.chromium.org/2060933002/ BUG=619084 ========== to ========== Expose flash playback status and volume control to content renderer This CL plumbs flash playback status and volume control to the content renderer. It basically lets the renderer know when flash starts/stops to play sound, and let it be able to control it's volume. The plumbing is to be continued to WebContents, and then we are able to let flash be controlled by MediaSession, which prevents multiple tabs from playing sound at the same time (which usually annoys the user). Follow-up CL: https://codereview.chromium.org/2060933002/ BUG=619084 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Expose flash playback status and volume control to content renderer This CL plumbs flash playback status and volume control to the content renderer. It basically lets the renderer know when flash starts/stops to play sound, and let it be able to control it's volume. The plumbing is to be continued to WebContents, and then we are able to let flash be controlled by MediaSession, which prevents multiple tabs from playing sound at the same time (which usually annoys the user). Follow-up CL: https://codereview.chromium.org/2060933002/ BUG=619084 ========== to ========== Expose flash playback status and volume control to content renderer This CL plumbs flash playback status and volume control to the content renderer. It basically lets the renderer know when flash starts/stops to play sound, and let it be able to control it's volume. The plumbing is to be continued to WebContents, and then we are able to let flash be controlled by MediaSession, which prevents multiple tabs from playing sound at the same time (which usually annoys the user). Follow-up CL: https://codereview.chromium.org/2060933002/ BUG=619084 Committed: https://crrev.com/c885f6ba83d8687b840c0fa684a941d45f9162be Cr-Commit-Position: refs/heads/master@{#401687} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c885f6ba83d8687b840c0fa684a941d45f9162be Cr-Commit-Position: refs/heads/master@{#401687} |
