Let Flash join and be controlled by media session

Let Flash join and be controlled by media session

This CL lets Flash join and be controlled by media session. This feature
is only valid when default media session (on desktop) is enabled. If
Flash starts playing, it will join the media session, and suspend all
existing media players. However if other player starts playing after
this, Flash will always duck until all other media players are paused.

BUG=619084
Committed: https://crrev.com/181047e6d762633e8ff503c96952ce5d8892ab41
Cr-Commit-Position: refs/heads/master@{#403447}

[Zhiqiang Zhang (Slow), 2016-06-13 17:40:19]

zqzhang@chromium.org changed reviewers:
+ mlamouri@chromium.org

[Zhiqiang Zhang (Slow), 2016-06-13 17:40:19]

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
File content/browser/media/pepper/pepper_player_delegate.h (right):

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_player_delegate.h:14: class
PepperPlayerDelegate : public MediaSessionObserver {
Do you prefer to put "pepper/*" into "content/browser/media/" or
"content/browser/media/session"?

The latter seems better for code review, since we don't require reviewers from
content/ then.

[Zhiqiang Zhang (Slow), 2016-06-14 16:36:29]

Patchset #3 (id:40001) has been deleted

[mlamouri (slow - plz ping), 2016-06-23 13:23:20]

Sounds good but there are a lot of style issues. I will have another look later.

Thanks for working on this! :)

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
File content/browser/media/pepper/pepper_player_delegate.cc (right):

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_player_delegate.cc:19:
PepperPlayerDelegate::~PepperPlayerDelegate() {}
`= default;` instead of `{}`

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_player_delegate.cc:23:
OnSetVolume(player_id, 0.0f);
0.0f isn't really ducking. Was that intended?

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
File content/browser/media/pepper/pepper_player_delegate.h (right):

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_player_delegate.h:14: class
PepperPlayerDelegate : public MediaSessionObserver {
On 2016/06/13 at 17:40:19, Zhiqiang Zhang wrote:
> Do you prefer to put "pepper/*" into "content/browser/media/" or
"content/browser/media/session"?
> 
> The latter seems better for code review, since we don't require reviewers from
content/ then.

I guess this could be in media/session/.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
File content/browser/media/pepper/pepper_web_contents_observer.cc (right):

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:22: }
`= default;` instead of `{}`.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:45: bool handled =
true;
style: add an empty line :)

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:54: return handled;
... here too :)

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:71: if
(!players_map_.count(pp_instance)) return;
style: return needs to go on a new line.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:74: }
style: add empty line above and "  // namespace content"

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
File content/browser/media/pepper/pepper_web_contents_observer.h (right):

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.h:14: class
PepperPlayerDelegate;
style: leave empty line above.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.h:17: class
CONTENT_EXPORT PepperWebContentsObserver : public WebContentsObserver {
Why CONTENT_EXPORT?

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.h:35: MediaSession*
media_session_;
FWIW, I don't think you need this. You could as well call
`MediaSession::Get(web_contents)` when needed.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.h:39: }
style: add empty line above and "  // namespace content"

https://codereview.chromium.org/2060933002/diff/20001/media/base/media_switch...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2060933002/diff/20001/media/base/media_switch...
media/base/media_switches.cc:120: // Let flash join and be controlled by media
session, only valid when
style: leave an empty line above.

[mlamouri (slow - plz ping), 2016-06-24 15:33:33]

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
File content/browser/media/session/pepper/pepper_player_delegate.h (right):

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_player_delegate.h:6: #define
CONTENT_BROWSER_MEDIA_SESSION_PEPPER_PEPPER_PLAYER_DELEGATE_H_
No need to create a pepper/ directory.

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_player_delegate.h:17: explicit
PepperPlayerDelegate(
You shouldn't have explicit here: you have two parameters.

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
File content/browser/media/session/pepper/pepper_web_contents_observer.cc
(right):

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_web_contents_observer.cc:78: }
Why do you do nothing here? Actually, why does create add an entry in
`players_map_` and delete erase while start add an `active_players_` but stop
doesn't remove it?

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
File content/browser/media/session/pepper/pepper_web_contents_observer.h
(right):

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_web_contents_observer.h:15: class
PepperPlayerDelegate;
style: empty line after `namespace content {'

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_web_contents_observer.h:46: }
style: empty line before } and add "  // namespace content"

https://codereview.chromium.org/2060933002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2060933002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6250: if (render_frame)
style: add { }

https://codereview.chromium.org/2060933002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6259: if (render_frame)
style: add { }

https://codereview.chromium.org/2060933002/diff/100001/media/base/media_switc...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2060933002/diff/100001/media/base/media_switc...
media/base/media_switches.cc:129: // Let flash join and be controlled by media
session, only valid when
style: add line above.

[Zhiqiang Zhang (Slow), 2016-06-24 17:43:21]

PTAL, addressed comments.

Sorry the last patch was only for rebasing but I didn't labeled it, so I'm
seeing lots of duplicate comments.

This patch is now ready for review.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
File content/browser/media/pepper/pepper_player_delegate.cc (right):

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_player_delegate.cc:19:
PepperPlayerDelegate::~PepperPlayerDelegate() {}
On 2016/06/23 13:23:19, Mounir Lamouri (slow) wrote:
> `= default;` instead of `{}`

Done.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_player_delegate.cc:23:
OnSetVolume(player_id, 0.0f);
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> 0.0f isn't really ducking. Was that intended?

Using 0.2f

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
File content/browser/media/pepper/pepper_web_contents_observer.cc (right):

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:22: }
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> `= default;` instead of `{}`.

Done.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:45: bool handled =
true;
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> style: add an empty line :)

Done.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:54: return handled;
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> ... here too :)

Done.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:71: if
(!players_map_.count(pp_instance)) return;
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> style: return needs to go on a new line.

Done.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.cc:74: }
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> style: add empty line above and "  // namespace content"

Done.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
File content/browser/media/pepper/pepper_web_contents_observer.h (right):

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.h:14: class
PepperPlayerDelegate;
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> style: leave empty line above.

Done.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.h:17: class
CONTENT_EXPORT PepperWebContentsObserver : public WebContentsObserver {
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> Why CONTENT_EXPORT?

Removed.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.h:35: MediaSession*
media_session_;
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> FWIW, I don't think you need this. You could as well call
> `MediaSession::Get(web_contents)` when needed.

Done.

https://codereview.chromium.org/2060933002/diff/20001/content/browser/media/p...
content/browser/media/pepper/pepper_web_contents_observer.h:39: }
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> style: add empty line above and "  // namespace content"

Done.

https://codereview.chromium.org/2060933002/diff/20001/media/base/media_switch...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2060933002/diff/20001/media/base/media_switch...
media/base/media_switches.cc:120: // Let flash join and be controlled by media
session, only valid when
On 2016/06/23 13:23:20, Mounir Lamouri (slow) wrote:
> style: leave an empty line above.

Done.

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
File content/browser/media/session/pepper/pepper_player_delegate.h (right):

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_player_delegate.h:6: #define
CONTENT_BROWSER_MEDIA_SESSION_PEPPER_PEPPER_PLAYER_DELEGATE_H_
On 2016/06/24 15:33:32, Mounir Lamouri (slow) wrote:
> No need to create a pepper/ directory.

Done.

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_player_delegate.h:17: explicit
PepperPlayerDelegate(
On 2016/06/24 15:33:32, Mounir Lamouri (slow) wrote:
> You shouldn't have explicit here: you have two parameters.

Done.

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
File content/browser/media/session/pepper/pepper_web_contents_observer.cc
(right):

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_web_contents_observer.cc:78: }
On 2016/06/24 15:33:32, Mounir Lamouri (slow) wrote:
> Why do you do nothing here? Actually, why does create add an entry in
> `players_map_` and delete erase while start add an `active_players_` but stop
> doesn't remove it?

I was thinking letting one flash join MediaSession once per lifetime and keep it
persistent until flash is deleted, but we can change this after some discussion.
This also requires some change in MediaSession. So we can put it in a follow-up
CL.

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
File content/browser/media/session/pepper/pepper_web_contents_observer.h
(right):

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_web_contents_observer.h:15: class
PepperPlayerDelegate;
On 2016/06/24 15:33:32, Mounir Lamouri (slow) wrote:
> style: empty line after `namespace content {'

Done.

https://codereview.chromium.org/2060933002/diff/100001/content/browser/media/...
content/browser/media/session/pepper/pepper_web_contents_observer.h:46: }
On 2016/06/24 15:33:32, Mounir Lamouri (slow) wrote:
> style: empty line before } and add "  // namespace content"

Done.

https://codereview.chromium.org/2060933002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2060933002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6250: if (render_frame)
On 2016/06/24 15:33:32, Mounir Lamouri (slow) wrote:
> style: add { }

Done.

https://codereview.chromium.org/2060933002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6259: if (render_frame)
On 2016/06/24 15:33:32, Mounir Lamouri (slow) wrote:
> style: add { }

Done.

https://codereview.chromium.org/2060933002/diff/100001/media/base/media_switc...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2060933002/diff/100001/media/base/media_switc...
media/base/media_switches.cc:129: // Let flash join and be controlled by media
session, only valid when
On 2016/06/24 15:33:32, Mounir Lamouri (slow) wrote:
> style: add line above.

Done.

[Zhiqiang Zhang (Slow), 2016-06-24 17:44:48]

zqzhang@chromium.org changed reviewers:
+ jochen@chromium.org

[Zhiqiang Zhang (Slow), 2016-06-24 17:44:49]

+jochen to review content/

[Zhiqiang Zhang (Slow), 2016-06-24 18:00:20]

+dcheng@ to review frame_messages.h

[jochen (gone - plz use gerrit), 2016-06-27 08:16:19]

jochen@chromium.org changed reviewers:
+ raymes@chromium.org

[jochen (gone - plz use gerrit), 2016-06-27 08:16:19]

+raymes who might be a better reviewer, or can suggest a better reviewer

[Zhiqiang Zhang (Slow), 2016-06-27 10:13:14]

On 2016/06/27 08:16:19, jochen wrote:
> +raymes who might be a better reviewer, or can suggest a better reviewer

Hi jochen@, mlamouri@ and I will take care of the files in
content/browser/media/session/*. The changes in pepper has been reviewed by
bbudge (owner of pepper*) in a previous CL. Can you take a look at the other
changes in content/*? Changes in extenstions/* and media/* are trivial and we
can add reviewers later. Sorry for the confusion.

[jochen (gone - plz use gerrit), 2016-06-27 11:47:11]

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
File content/browser/media/session/pepper_player_delegate.h (right):

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.h:22: void OnSuspend(int
player_id) override;
add a comment like // MediaSessionObserver implementation.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.h:27: void OnSetVolume(int
player_id, double volume);
this should be private and be named SetVolume

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.h:32: };
disallow copy & assign

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
File content/browser/media/session/pepper_web_contents_observer.cc (right):

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_web_contents_observer.cc:71:
players_map_.erase(pp_instance);
why is this not in the previous if?

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
File content/browser/media/session/pepper_web_contents_observer.h (right):

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_web_contents_observer.h:24: void
WebContentsDestroyed() override;
// WebContentsObserver implementation.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_web_contents_observer.h:30: void
OnPepperStartsPlayback(int32_t pp_instance);
this is no longer WCO, please move to a separate block

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_web_contents_observer.h:36:
std::map<int32_t, std::unique_ptr<PepperPlayerDelegate>>;
base::IdMap?

https://codereview.chromium.org/2060933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2060933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1395:
std::unique_ptr<PepperWebContentsObserver> pepper_web_contents_observer_;
have you considered inlining this class in web contents? every observer adds
some overhead to each callback, whether it's implemented or not

[Zhiqiang Zhang (Slow), 2016-06-28 18:53:25]

PTAL. Addressed jochen's comments w/replies.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
File content/browser/media/session/pepper_player_delegate.h (right):

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.h:22: void OnSuspend(int
player_id) override;
On 2016/06/27 11:47:11, jochen wrote:
> add a comment like // MediaSessionObserver implementation.

Done.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.h:27: void OnSetVolume(int
player_id, double volume);
On 2016/06/27 11:47:11, jochen wrote:
> this should be private and be named SetVolume

Done.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.h:32: };
On 2016/06/27 11:47:11, jochen wrote:
> disallow copy & assign

Done.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
File content/browser/media/session/pepper_web_contents_observer.cc (right):

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_web_contents_observer.cc:71:
players_map_.erase(pp_instance);
On 2016/06/27 11:47:11, jochen wrote:
> why is this not in the previous if?

Sorry, this is code in an old patch. Fixed.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
File content/browser/media/session/pepper_web_contents_observer.h (right):

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_web_contents_observer.h:24: void
WebContentsDestroyed() override;
On 2016/06/27 11:47:11, jochen wrote:
> // WebContentsObserver implementation.

Done.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_web_contents_observer.h:30: void
OnPepperStartsPlayback(int32_t pp_instance);
On 2016/06/27 11:47:11, jochen wrote:
> this is no longer WCO, please move to a separate block 

Done.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/media/...
content/browser/media/session/pepper_web_contents_observer.h:36:
std::map<int32_t, std::unique_ptr<PepperPlayerDelegate>>;
On 2016/06/27 11:47:11, jochen wrote:
> base::IdMap?

Done.

https://codereview.chromium.org/2060933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2060933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1395:
std::unique_ptr<PepperWebContentsObserver> pepper_web_contents_observer_;
On 2016/06/27 11:47:11, jochen wrote:
> have you considered inlining this class in web contents? every observer adds
> some overhead to each callback, whether it's implemented or not

I made PepperWebContentsObserver not inheriting from WebContentsObserver (also
renamed as PepperPlaybackObserver). Then I delegate the call from
WebContentsImpl to PepperPlaybackObserver. Please see the diff.

Hope this is what you expected. Tell me if I'm wrong.

[jochen (gone - plz use gerrit), 2016-06-29 15:17:33]

lgtm

[Zhiqiang Zhang (Slow), 2016-06-29 16:11:34]

zqzhang@chromium.org changed reviewers:
+ dcheng@chromium.org
- raymes@chromium.org

[Zhiqiang Zhang (Slow), 2016-06-29 16:11:34]

+dcheng to review the IPC messages.
+cc:bbudge, -raymes (since bbudge has reviewed the previous patch)

[dcheng, 2016-06-30 07:02:39]

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:44:
players_map_.AddWithID(new PepperPlayerDelegate(
Would an ordinary std::map work here? It doesn't seem like we need the ID
allocation feature, right?

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:47:
players_map_.Lookup(pp_instance), 0, MediaSession::Type::Content);
Perhaps make 0 a constant so it's clear this is the player_id? I guess AddPlayer
/ RemovePlayer are keyed by both the observer and the player_id, so it's safe to
always use 0?

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
File content/browser/media/session/pepper_playback_observer.h (right):

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.h:35: WebContents*
contents_;
Nit: suggest just making this a WebContentsImpl, to avoid the static_casts in
the .cc

[mlamouri (slow - plz ping), 2016-06-30 11:01:18]

lgtm with dcheng@ comments applied.

\o/

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:26:
PepperStopsPlayback(iter.GetCurrentKey());
style: can you wrap this into { }

https://codereview.chromium.org/2060933002/diff/140001/media/base/media_switc...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2060933002/diff/140001/media/base/media_switc...
media/base/media_switches.cc:133: base::FEATURE_ENABLED_BY_DEFAULT};
Something we could do in a follow-up is to have a multi-choice for media session
on desktop like: "off", "on - wo/ flash", "on - w/ flash".

No need to deal with that now though. Changing this should be seamless.

[Zhiqiang Zhang (Slow), 2016-06-30 12:45:28]

Patchset #8 (id:160001) has been deleted

[Zhiqiang Zhang (Slow), 2016-06-30 12:45:36]

Patchset #8 (id:180001) has been deleted

[Zhiqiang Zhang (Slow), 2016-06-30 12:45:59]

Patchset #8 (id:200001) has been deleted

[Zhiqiang Zhang (Slow), 2016-06-30 12:46:54]

zqzhang@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[Zhiqiang Zhang (Slow), 2016-06-30 12:46:55]

PTAL, addressed dcheng and mlamouri's comments.
+dalecurtis for reviewing media_switches

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:26:
PepperStopsPlayback(iter.GetCurrentKey());
On 2016/06/30 11:01:18, Mounir Lamouri wrote:
> style: can you wrap this into { }

Done.

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:44:
players_map_.AddWithID(new PepperPlayerDelegate(
On 2016/06/30 07:02:39, dcheng wrote:
> Would an ordinary std::map work here? It doesn't seem like we need the ID
> allocation feature, right?

Actually it was suggested by jochen. IDMap allows the user to manage ids
themselves. IDMap says it is optimized for small data sets, which should be
effecient in our case.

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:47:
players_map_.Lookup(pp_instance), 0, MediaSession::Type::Content);
On 2016/06/30 07:02:39, dcheng wrote:
> Perhaps make 0 a constant so it's clear this is the player_id? I guess
AddPlayer
> / RemovePlayer are keyed by both the observer and the player_id, so it's safe
to
> always use 0?

Done.

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
File content/browser/media/session/pepper_playback_observer.h (right):

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.h:35: WebContents*
contents_;
On 2016/06/30 07:02:39, dcheng wrote:
> Nit: suggest just making this a WebContentsImpl, to avoid the static_casts in
> the .cc

Done.

https://codereview.chromium.org/2060933002/diff/140001/media/base/media_switc...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2060933002/diff/140001/media/base/media_switc...
media/base/media_switches.cc:133: base::FEATURE_ENABLED_BY_DEFAULT};
On 2016/06/30 11:01:18, Mounir Lamouri wrote:
> Something we could do in a follow-up is to have a multi-choice for media
session
> on desktop like: "off", "on - wo/ flash", "on - w/ flash".
> 
> No need to deal with that now though. Changing this should be seamless.

Acknowledged.

[DaleCurtis, 2016-06-30 18:00:54]

media/ lgtm, though I got excited and thought you had found some way to pause
flash content given the CL description, this is ducking/muting instead :)

[dcheng, 2016-07-01 05:57:59]

mojom lgtm

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:44:
players_map_.AddWithID(new PepperPlayerDelegate(
On 2016/06/30 12:46:54, Zhiqiang Zhang wrote:
> On 2016/06/30 07:02:39, dcheng wrote:
> > Would an ordinary std::map work here? It doesn't seem like we need the ID
> > allocation feature, right?
> 
> Actually it was suggested by jochen. IDMap allows the user to manage ids
> themselves. IDMap says it is optimized for small data sets, which should be
> effecient in our case.

I just don't see much of a point to using IDMap here: we manage the IDs
ourselves and we don't need the 'safe to iterate while removing' behavior. My
general preference is to use something generic if I don't need the specialized
behavior, since it's more standard. For example, if this was using std::map, the
teardown in the dtor would just be a standard for-each loop.

https://codereview.chromium.org/2060933002/diff/220001/content/browser/media/...
File content/browser/media/session/pepper_player_delegate.cc (right):

https://codereview.chromium.org/2060933002/diff/220001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.cc:16: const int kPlayerId
= 0;
Note: this is fine, but my comment was directed mostly at the
pepper_playback_observer.cc. When calling a function, the context for an
argument that's just '0' isn't so clear. Whereas in this file, it's always
compared to a variable named player_id, so it's more obvious from the context

[Zhiqiang Zhang (Slow), 2016-07-01 11:01:07]

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2060933002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:44:
players_map_.AddWithID(new PepperPlayerDelegate(
On 2016/07/01 05:57:59, dcheng wrote:
> On 2016/06/30 12:46:54, Zhiqiang Zhang wrote:
> > On 2016/06/30 07:02:39, dcheng wrote:
> > > Would an ordinary std::map work here? It doesn't seem like we need the ID
> > > allocation feature, right?
> > 
> > Actually it was suggested by jochen. IDMap allows the user to manage ids
> > themselves. IDMap says it is optimized for small data sets, which should be
> > effecient in our case.
> 
> I just don't see much of a point to using IDMap here: we manage the IDs
> ourselves and we don't need the 'safe to iterate while removing' behavior. My
> general preference is to use something generic if I don't need the specialized
> behavior, since it's more standard. For example, if this was using std::map,
the
> teardown in the dtor would just be a standard for-each loop.

OK, I agree with you. changed to std::map. But one thing to mention is that
erase using for-each loop will crash since the std::map::iterator will be
invalidated when the current iterator is being erased.

https://codereview.chromium.org/2060933002/diff/220001/content/browser/media/...
File content/browser/media/session/pepper_player_delegate.cc (right):

https://codereview.chromium.org/2060933002/diff/220001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.cc:16: const int kPlayerId
= 0;
On 2016/07/01 05:57:59, dcheng wrote:
> Note: this is fine, but my comment was directed mostly at the
> pepper_playback_observer.cc. When calling a function, the context for an
> argument that's just '0' isn't so clear. Whereas in this file, it's always
> compared to a variable named player_id, so it's more obvious from the context

OK, now I moved all the Ids in one place (pepper_player_delegate.h), so I think
it will be more consistent.

[Zhiqiang Zhang (Slow), 2016-07-01 11:01:27]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-07-01 11:01:27]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mlamouri@chromium.org, dalecurtis@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2060933002/#ps240001
(title: "addressed dcheng's comments")

[commit-bot: I haz the power, 2016-07-01 11:01:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 13:37:38]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2016-07-01 13:39:13]

Description was changed from

==========
Let Flash join and be controlled by media session

This CL lets Flash join and be controlled by media session. This feature
is only valid when default media session (on desktop) is enabled. If
Flash starts playing, it will join the media session, and suspend all
existing media players. However if other player starts playing after
this, Flash will always duck until all other media players are paused.

BUG=619084
==========

to

==========
Let Flash join and be controlled by media session

This CL lets Flash join and be controlled by media session. This feature
is only valid when default media session (on desktop) is enabled. If
Flash starts playing, it will join the media session, and suspend all
existing media players. However if other player starts playing after
this, Flash will always duck until all other media players are paused.

BUG=619084

Committed: https://crrev.com/181047e6d762633e8ff503c96952ce5d8892ab41
Cr-Commit-Position: refs/heads/master@{#403447}
==========

[commit-bot: I haz the power, 2016-07-01 13:39:14]

Patchset 9 (id:??) landed as
https://crrev.com/181047e6d762633e8ff503c96952ce5d8892ab41
Cr-Commit-Position: refs/heads/master@{#403447}