Allow MediaSession in iframes to be routed

Allow MediaSession in iframes to be routed

This CL allows MediaSession in iframes to be routed, as per spec change:
https://github.com/WICG/mediasession/pull/149

To achieve this goal, class MediaSessionServiceRouter is added for selecting
which MediaSession object to route.

The current MediaSessionService routing strategy is:

* If the top-level frame uses MediaSession API, always select the
  top-level session.
* If the top-level frame has no MediaSession, select one of an
  audio-producing frame, and route its session (or null if the frame
  does not use MediaSession API).

CL explanation:
https://docs.google.com/a/google.com/document/d/1Ht6DxjOcfBctfRT3_wOkwGoNUaJY-Q8K18MV3xdAh_8/edit?usp=sharing

BUG=670319
Committed: https://crrev.com/ddc545cbe5a17717fbd3193c21ae2a63dc92ba40
Cr-Commit-Position: refs/heads/master@{#436266}

[Zhiqiang Zhang (Slow), 2016-11-22 18:01:00]

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

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

The CL is still rough. Just sending this for initial feedback. Take your time :)

[whywhat, 2016-11-22 23:13:17]

the description seems to be not descriptive enough.
do you think you could merge the router into media session as they have 1:1
relationship?

https://codereview.chromium.org/2526533002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2526533002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:61:
private List<Integer> mMediaSessionActions = null;
Why this change?

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_service_router.cc (right):

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_service_router.cc:46: if
(routed_service_)
nit: s/routed_service_/!routed_service_

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_service_router.cc:86: if
(!routed_service_) {
nit: s/!routed_service/routed_service

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_service_router.h (right):

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_service_router.h:29: void
OnServiceCreated(MediaSessionServiceImpl* service);
nit: document public methods and the class.

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_service_router.h:46:
MediaSessionServiceImpl* routed_service_;
What's the life scope of these pointers? session_ owns the router so keeping a
pointer is probably safe, what about the service?

[whywhat, 2016-11-22 23:17:31]

Also, the PR quoted in the description doesn't seem to say how exactly UA is
supposed to choose the frame for the media session. Even if the algorithm is not
final here, it should be described in the CL description.

[Zhiqiang Zhang (Slow), 2016-11-24 15:11:39]

Description was changed from

==========
Allow MediaSession in iframes to be routed

This CL allows MediaSession in iframes to be routed, as per spec change:
https://github.com/WICG/mediasession/pull/149

To achieve this goal, class MediaSessionServiceRouter is added for selecting
which MediaSession object to route.

BUG=<WIP>
==========

to

==========
Allow MediaSession in iframes to be routed

This CL allows MediaSession in iframes to be routed, as per spec change:
https://github.com/WICG/mediasession/pull/149

To achieve this goal, class MediaSessionServiceRouter is added for selecting
which MediaSession object to route.

The current MediaSessionService routing strategy is:

* If the top-level frame uses MediaSession API, always select the
  top-level session.
* If the top-level frame has no MediaSession, select one of an
  audio-producing frame, and route its session (or null if the frame
  does not use MediaSession API).

BUG=<WIP>
==========

[Zhiqiang Zhang (Slow), 2016-11-25 14:50:41]

PTAL w/ comments applied:

* Merged MediaSessionServiceRouter into MediaSessionImpl
* Updated CL description
* Nits

https://codereview.chromium.org/2526533002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2526533002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:61:
private List<Integer> mMediaSessionActions = null;
On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> Why this change?

Undone this change.

Actually I thought we are now passing an array through JNI, but we should keep
using Set<> here (as in native MediaSessionObserver).

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_service_router.cc (right):

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_service_router.cc:46: if
(routed_service_)
On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: s/routed_service_/!routed_service_

Done.

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_service_router.cc:86: if
(!routed_service_) {
On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: s/!routed_service/routed_service

Done.

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_service_router.h (right):

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_service_router.h:29: void
OnServiceCreated(MediaSessionServiceImpl* service);
On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: document public methods and the class.

Done.

https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_service_router.h:46:
MediaSessionServiceImpl* routed_service_;
On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> What's the life scope of these pointers? session_ owns the router so keeping a
> pointer is probably safe, what about the service?

Documented on the ownership.

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_impl.cc:611: RenderFrameHost*
main_frame = web_contents()->GetMainFrame();
This was the current plan when I talked with Mounir.

After implementing this, I'm a bit worried about security here. This means the
top-level frame can listen to whether the embedded iframe plays audio. WDYT?

[whywhat, 2016-11-28 19:54:32]

On 2016/11/25 at 14:50:41, zqzhang wrote:
> PTAL w/ comments applied:
> 
> * Merged MediaSessionServiceRouter into MediaSessionImpl
> * Updated CL description
> * Nits
> 
>
https://codereview.chromium.org/2526533002/diff/1/chrome/android/java/src/org...
> File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):
> 
>
https://codereview.chromium.org/2526533002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:61:
private List<Integer> mMediaSessionActions = null;
> On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> > Why this change?
> 
> Undone this change.
> 
> Actually I thought we are now passing an array through JNI, but we should keep
using Set<> here (as in native MediaSessionObserver).
> 
>
https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
> File content/browser/media/session/media_session_service_router.cc (right):
> 
>
https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
> content/browser/media/session/media_session_service_router.cc:46: if
(routed_service_)
> On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> > nit: s/routed_service_/!routed_service_
> 
> Done.
> 
>
https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
> content/browser/media/session/media_session_service_router.cc:86: if
(!routed_service_) {
> On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> > nit: s/!routed_service/routed_service
> 
> Done.
> 
>
https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
> File content/browser/media/session/media_session_service_router.h (right):
> 
>
https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
> content/browser/media/session/media_session_service_router.h:29: void
OnServiceCreated(MediaSessionServiceImpl* service);
> On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> > nit: document public methods and the class.
> 
> Done.
> 
>
https://codereview.chromium.org/2526533002/diff/1/content/browser/media/sessi...
> content/browser/media/session/media_session_service_router.h:46:
MediaSessionServiceImpl* routed_service_;
> On 2016/11/22 23:13:16, whywhat_OOO_till_Mon_Nov_28 wrote:
> > What's the life scope of these pointers? session_ owns the router so keeping
a
> > pointer is probably safe, what about the service?
> 
> Documented on the ownership.
> 
>
https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
> File content/browser/media/session/media_session_impl.cc (right):
> 
>
https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
> content/browser/media/session/media_session_impl.cc:611: RenderFrameHost*
main_frame = web_contents()->GetMainFrame();
> This was the current plan when I talked with Mounir.
> 
> After implementing this, I'm a bit worried about security here. This means the
top-level frame can listen to whether the embedded iframe plays audio. WDYT?

Eh, this sounds sketchy... Perhaps the session only works for the media from the
same frame. We have a problem with the default session in some cases then, but
it might be rare.

[whywhat, 2016-11-28 19:59:14]

lgtm modulo nits & the question about the main frame and embedded playback

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_controller.h (right):

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_controller.h:56: const
WebContentsObserver::MediaPlayerId& id() { return id_; };
nit: give it a more descriptive name, it's confusing that the type is
MediaPlayerId but it's not the same as the player_id_ and player_id passed to
the methods above?

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_impl.h (right):

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_impl.h:271: RenderFrameHost*
GetMostMeaningfulAudioProducingFrame();
nit: most-meaningful doesn't tell much to the reader, change to something more
descriptive that maybe matches the implementation even?

[Zhiqiang Zhang (Slow), 2016-11-30 16:38:07]

Patchset #5 (id:80001) has been deleted

[Zhiqiang Zhang (Slow), 2016-11-30 16:40:59]

Anton, can you PTAL at diff #5 vs. #4?

Because I changed the service selection strategy as we discussed in:
https://docs.google.com/a/google.com/document/d/1Ht6DxjOcfBctfRT3_wOkwGoNUaJY...

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_controller.h (right):

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_controller.h:56: const
WebContentsObserver::MediaPlayerId& id() { return id_; };
On 2016/11/28 19:59:14, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: give it a more descriptive name, it's confusing that the type is
> MediaPlayerId but it's not the same as the player_id_ and player_id passed to
> the methods above?

Done.

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_impl.h (right):

https://codereview.chromium.org/2526533002/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_impl.h:271: RenderFrameHost*
GetMostMeaningfulAudioProducingFrame();
On 2016/11/28 19:59:14, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: most-meaningful doesn't tell much to the reader, change to something more
> descriptive that maybe matches the implementation even?

Done.

[whywhat, 2016-11-30 22:16:41]

Thanks for asking for another review: added some comments.

https://codereview.chromium.org/2526533002/diff/100001/content/browser/media/...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2526533002/diff/100001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:612: // The service
selection strategy is: Select a frame that has a playing/paused
is there a test possible for this?

https://codereview.chromium.org/2526533002/diff/100001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:619: for (const auto& player
: normal_players_) {
I wonder if there's a potential bottleneck - ideally we would collect all RFH we
want to compare by depth first and maybe even memoirize the depth to avoid
recalculating it over and over again. Some edge case with many players in many
nested iframes and these loops will run forever :)

https://codereview.chromium.org/2526533002/diff/100001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:642:
static_cast<MediaSessionController*>(player_observer);
nit: could we avoid this assumption that MSC is the implementation of MSPO?
if that assumption ever changes, the potential breakage could be pretty bad imo

[Zhiqiang Zhang (Slow), 2016-12-01 16:33:19]

Patchset #6 (id:120001) has been deleted

[Zhiqiang Zhang (Slow), 2016-12-01 16:34:41]

PTAL at #5..#6

https://codereview.chromium.org/2526533002/diff/100001/content/browser/media/...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2526533002/diff/100001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:612: // The service
selection strategy is: Select a frame that has a playing/paused
On 2016/11/30 22:16:41, whywhat_OOO_till_Mon_Nov_28 wrote:
> is there a test possible for this?

Added unit tests.

https://codereview.chromium.org/2526533002/diff/100001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:619: for (const auto& player
: normal_players_) {
On 2016/11/30 22:16:41, whywhat_OOO_till_Mon_Nov_28 wrote:
> I wonder if there's a potential bottleneck - ideally we would collect all RFH
we
> want to compare by depth first and maybe even memoirize the depth to avoid
> recalculating it over and over again. Some edge case with many players in many
> nested iframes and these loops will run forever :)

Done. Now we collect all the frames first and then compare them in batch.

https://codereview.chromium.org/2526533002/diff/100001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:642:
static_cast<MediaSessionController*>(player_observer);
On 2016/11/30 22:16:41, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: could we avoid this assumption that MSC is the implementation of MSPO?
> if that assumption ever changes, the potential breakage could be pretty bad
imo

Added a `GetRenderFrameHost()` method to MediaSessionPlayerObserver. Though we
are now assuming MediaSessionPlayerObserver is related to RFH.

[Zhiqiang Zhang (Slow), 2016-12-01 16:38:59]

Description was changed from

==========
Allow MediaSession in iframes to be routed

This CL allows MediaSession in iframes to be routed, as per spec change:
https://github.com/WICG/mediasession/pull/149

To achieve this goal, class MediaSessionServiceRouter is added for selecting
which MediaSession object to route.

The current MediaSessionService routing strategy is:

* If the top-level frame uses MediaSession API, always select the
  top-level session.
* If the top-level frame has no MediaSession, select one of an
  audio-producing frame, and route its session (or null if the frame
  does not use MediaSession API).

BUG=<WIP>
==========

to

==========
Allow MediaSession in iframes to be routed

This CL allows MediaSession in iframes to be routed, as per spec change:
https://github.com/WICG/mediasession/pull/149

To achieve this goal, class MediaSessionServiceRouter is added for selecting
which MediaSession object to route.

The current MediaSessionService routing strategy is:

* If the top-level frame uses MediaSession API, always select the
  top-level session.
* If the top-level frame has no MediaSession, select one of an
  audio-producing frame, and route its session (or null if the frame
  does not use MediaSession API).

CL explanation:
https://docs.google.com/a/google.com/document/d/1Ht6DxjOcfBctfRT3_wOkwGoNUaJY...

BUG=670319
==========

[whywhat, 2016-12-01 22:13:00]

lgtm, thanks for the tests!

maybe you could pass RFH along with the player observer to AddPlayer and such
and keep it in PlayerIdentifier?

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

https://codereview.chromium.org/2526533002/diff/140001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:33: rfh = rfh->GetParent();
nit: this could still result in an N-square complexity if you have N frames
nested into each other, right?
maybe RenderFrameHost could get a GetDepth() method that caches the result...

https://codereview.chromium.org/2526533002/diff/140001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:609: // The service
selection strategy is: Select a frame that has a playing/paused
nit: s/Select/select/

https://codereview.chromium.org/2526533002/diff/140001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:626: for (const auto& player
: pepper_players_) {
does it make sense to check these if they will never return a non-null render
frame host?
same above for one_shot_players I assume?

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

https://codereview.chromium.org/2526533002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.cc:63: return nullptr;
if pepper player's RFH is always null, why do we iterate over them to find one
in media_session_impl.cc?

[Zhiqiang Zhang (Slow), 2016-12-02 14:17:27]

zqzhang@chromium.org changed reviewers:
+ creis@chromium.org, tedchoc@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-02 14:17:28]

+creis: content/public/browser
+tedchoc: content/public/android

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

https://codereview.chromium.org/2526533002/diff/140001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:33: rfh = rfh->GetParent();
On 2016/12/01 22:13:00, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: this could still result in an N-square complexity if you have N frames
> nested into each other, right?

Now using a cache to compute the depths, so the complexity is O(N).

> maybe RenderFrameHost could get a GetDepth() method that caches the result...

Better not to do in this CL, since we are the only user.

https://codereview.chromium.org/2526533002/diff/140001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:609: // The service
selection strategy is: Select a frame that has a playing/paused
On 2016/12/01 22:13:00, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: s/Select/select/

Done.

https://codereview.chromium.org/2526533002/diff/140001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:626: for (const auto& player
: pepper_players_) {
On 2016/12/01 22:13:00, whywhat_OOO_till_Mon_Nov_28 wrote:
> does it make sense to check these if they will never return a non-null render
> frame host?
> same above for one_shot_players I assume?

This is https://crbug.com/670273. Pepper players should have associated RFH, but
currently there's a mistake.

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

https://codereview.chromium.org/2526533002/diff/140001/content/browser/media/...
content/browser/media/session/pepper_player_delegate.cc:63: return nullptr;
On 2016/12/01 22:13:00, whywhat_OOO_till_Mon_Nov_28 wrote:
> if pepper player's RFH is always null, why do we iterate over them to find one
> in media_session_impl.cc?

Ditto http://crbug.com/670273. Added a TODO here.

[Zhiqiang Zhang (Slow), 2016-12-02 14:19:49]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 14:20:10]

Dry run: 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-12-02 14:32:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 14:32:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[whywhat, 2016-12-02 16:09:18]

Thanks!

https://codereview.chromium.org/2526533002/diff/160001/content/browser/media/...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2526533002/diff/160001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:33: size_t depth = 0;
nit: IIUC, you could DCHECK(rfh && map->find(rfh) == map->end()) due to how the
calling algorithm works and start with depth = 1 and current_frame =
rfh->GetParent();

https://codereview.chromium.org/2526533002/diff/160001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:36: if
(map_rfh_to_depth->count(current_frame)) {
you could use an iterator to avoid double lookup in the map:

auto it = map->find(current_frame);
if (it != map->end()) {
  depth += it->second;
  break;
}

[Ted C, 2016-12-02 21:08:30]

On 2016/12/02 16:09:18, whywhat_OOO_till_Mon_Nov_28 wrote:
> Thanks!
> 
>
https://codereview.chromium.org/2526533002/diff/160001/content/browser/media/...
> File content/browser/media/session/media_session_impl.cc (right):
> 
>
https://codereview.chromium.org/2526533002/diff/160001/content/browser/media/...
> content/browser/media/session/media_session_impl.cc:33: size_t depth = 0;
> nit: IIUC, you could DCHECK(rfh && map->find(rfh) == map->end()) due to how
the
> calling algorithm works and start with depth = 1 and current_frame =
> rfh->GetParent();
> 
>
https://codereview.chromium.org/2526533002/diff/160001/content/browser/media/...
> content/browser/media/session/media_session_impl.cc:36: if
> (map_rfh_to_depth->count(current_frame)) {
> you could use an iterator to avoid double lookup in the map:
> 
> auto it = map->find(current_frame);
> if (it != map->end()) {
>   depth += it->second;
>   break;
> }

content/public/android - lgtm

[Ted C, 2016-12-02 21:08:44]

https://codereview.chromium.org/2526533002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java
(right):

https://codereview.chromium.org/2526533002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:101:

It's unfortunate that you can't use Collections.unmodifiableSet here since it is
API 24.  You have no way of preventing the observers from modifying the set
here.  I guess if it ever becomes a problem then you just duplicate the set for
each observer.

[Charlie Reis, 2016-12-02 22:43:04]

content/ LGTM.

[Zhiqiang Zhang (Slow), 2016-12-05 11:30:44]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-05 11:30:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, tedchoc@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2526533002/#ps180001
(title: "nits")

[commit-bot: I haz the power, 2016-12-05 11:30:57]

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

[Zhiqiang Zhang (Slow), 2016-12-05 11:37:32]

https://codereview.chromium.org/2526533002/diff/160001/content/browser/media/...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2526533002/diff/160001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:33: size_t depth = 0;
On 2016/12/02 16:09:17, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: IIUC, you could DCHECK(rfh && map->find(rfh) == map->end()) due to how
the
> calling algorithm works and start with depth = 1 and current_frame =
> rfh->GetParent();

Added DCHECK(rfh) here.

I prefer not to DCHECK(map->find(rfh) == map->end()) and start from 1 here,
because it will introduce an additional lookup in the map at the call site.

https://codereview.chromium.org/2526533002/diff/160001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:36: if
(map_rfh_to_depth->count(current_frame)) {
On 2016/12/02 16:09:18, whywhat_OOO_till_Mon_Nov_28 wrote:
> you could use an iterator to avoid double lookup in the map:
> 
> auto it = map->find(current_frame);
> if (it != map->end()) {
>   depth += it->second;
>   break;
> }

Done.

https://codereview.chromium.org/2526533002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java
(right):

https://codereview.chromium.org/2526533002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:101:

On 2016/12/02 21:08:44, Ted C wrote:
> It's unfortunate that you can't use Collections.unmodifiableSet here since it
is
> API 24.  You have no way of preventing the observers from modifying the set
> here.  I guess if it ever becomes a problem then you just duplicate the set
for
> each observer.

Acknowledged.

[commit-bot: I haz the power, 2016-12-05 12:19:00]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480937444843660,
"parent_rev": "7b1d9cdfcf1a2b9471f1cdbd116a2970fb962757", "commit_rev":
"863a9f8e154436c794dc1f3abdd3dcb328f1b880"}

[commit-bot: I haz the power, 2016-12-05 12:19:24]

Description was changed from

==========
Allow MediaSession in iframes to be routed

This CL allows MediaSession in iframes to be routed, as per spec change:
https://github.com/WICG/mediasession/pull/149

To achieve this goal, class MediaSessionServiceRouter is added for selecting
which MediaSession object to route.

The current MediaSessionService routing strategy is:

* If the top-level frame uses MediaSession API, always select the
  top-level session.
* If the top-level frame has no MediaSession, select one of an
  audio-producing frame, and route its session (or null if the frame
  does not use MediaSession API).

CL explanation:
https://docs.google.com/a/google.com/document/d/1Ht6DxjOcfBctfRT3_wOkwGoNUaJY...

BUG=670319
==========

to

==========
Allow MediaSession in iframes to be routed

This CL allows MediaSession in iframes to be routed, as per spec change:
https://github.com/WICG/mediasession/pull/149

To achieve this goal, class MediaSessionServiceRouter is added for selecting
which MediaSession object to route.

The current MediaSessionService routing strategy is:

* If the top-level frame uses MediaSession API, always select the
  top-level session.
* If the top-level frame has no MediaSession, select one of an
  audio-producing frame, and route its session (or null if the frame
  does not use MediaSession API).

CL explanation:
https://docs.google.com/a/google.com/document/d/1Ht6DxjOcfBctfRT3_wOkwGoNUaJY...

BUG=670319
==========

[commit-bot: I haz the power, 2016-12-05 12:19:25]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-12-05 12:21:47]

Description was changed from

==========
Allow MediaSession in iframes to be routed

This CL allows MediaSession in iframes to be routed, as per spec change:
https://github.com/WICG/mediasession/pull/149

To achieve this goal, class MediaSessionServiceRouter is added for selecting
which MediaSession object to route.

The current MediaSessionService routing strategy is:

* If the top-level frame uses MediaSession API, always select the
  top-level session.
* If the top-level frame has no MediaSession, select one of an
  audio-producing frame, and route its session (or null if the frame
  does not use MediaSession API).

CL explanation:
https://docs.google.com/a/google.com/document/d/1Ht6DxjOcfBctfRT3_wOkwGoNUaJY...

BUG=670319
==========

to

==========
Allow MediaSession in iframes to be routed

This CL allows MediaSession in iframes to be routed, as per spec change:
https://github.com/WICG/mediasession/pull/149

To achieve this goal, class MediaSessionServiceRouter is added for selecting
which MediaSession object to route.

The current MediaSessionService routing strategy is:

* If the top-level frame uses MediaSession API, always select the
  top-level session.
* If the top-level frame has no MediaSession, select one of an
  audio-producing frame, and route its session (or null if the frame
  does not use MediaSession API).

CL explanation:
https://docs.google.com/a/google.com/document/d/1Ht6DxjOcfBctfRT3_wOkwGoNUaJY...

BUG=670319

Committed: https://crrev.com/ddc545cbe5a17717fbd3193c21ae2a63dc92ba40
Cr-Commit-Position: refs/heads/master@{#436266}
==========

[commit-bot: I haz the power, 2016-12-05 12:21:48]

Patchset 8 (id:??) landed as
https://crrev.com/ddc545cbe5a17717fbd3193c21ae2a63dc92ba40
Cr-Commit-Position: refs/heads/master@{#436266}