New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 655781 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Android MediaRouter only (left Chro...
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Clean up naming in MediaSession

Project Member Reported by zqzh...@chromium.org, Oct 13 2016

Issue description

Found it very hard to find a name for a new class in media/session. We need some clean up. We have too many "controller", "delegate", "observer" and they don't mean exactly what they are.

Current plan:
MediaSessionObserver -> MediaSessionController
MedidSessionController -> MediaSessionControllerImpl
MediaSessionDelegate -> AudioFocusDelegate (C++ & Android)

Future plan:
Move MediaSession-related callbacks in WebContentsObserver to MediaSessionObserver
 
Description: Show this description
Description: Show this description
I don't think changing the name from Observer to Controller helps here. The Observer means an interface (or its implementation) that observes events happening with something and MediaSessionObserver fits this definition perfectly.
The Controller on the other hand manages or controls something.

The terms are confusing because we mix the MediaSession that reflects Android's MediaSession (remote controls like notification, lock screen, Watch and headsets) and MediaSession that's the tab's default session and soon the one per frame (although with it's being on the navigator they somewhat are the same). We could simply say it's BrowserMediaSession and RendererMediaSession, for instance. So you would have BrowserMediaSessionObserver interface and BrowserMediaSession (instead of simply MediaSession) in the browser implemented by RendererMediaSessionController that sends messages to the renderer.

Obviously, better names than BrowserMediaSession and RendererMediaSession are welcome, but simply changing the name from observer to controller without changing how the class looks sounds bad.

I agree with MediaSessionDelegate to AudioFocusDelegate although we discussed further refactoring on codereview.chromium.org/ :

"- MediaSession talks directly to AudioFocusManager on all platforms (behavior is consistent between all platforms, etc)
- AudioFocusManager has a delegate (on desktop it's a no-op, on Android it takes the system audio focus for the app, not each tab)."

So AudioFocusDelegate is fine with me but we could look into having AudioFocusManager and platform-specific AudioFocusManagerImpl/Delegate-s.
So the main reason behind renaming MediaSessionObserver is that we are going to have MediaSessionTabHelper talking to MediaSession directly, so we don't need plumbing for media controls. MediaSessionTabHelper should fit the name "MediaSessionObserver" better, so we need to let the old "MediaSessionObserver" give out the name.

I talked with Mounir, he suggested to do:
MediaSessionObserver->MediaSessionPlayerObserver,
and keep MediaSessionController.

WDYT?

Currently, I think it's hard to split the blink MediaSession and audio focus MediaSession on the browser side since we need a lot of refactoring. I suggested renaming the current MediaSession to something like "AudioSession".
> I suggested renaming the current MediaSession to something like "AudioSession".
This is unlikely to happen in the near future :-/
MediaSessionObserver is an interface that one has to inherit from to observe MediaSession events or actions. Maybe MediaSessionEventObserver/MediaSessionActionObserver then?

I don't see how MediaSessionTabHelper would fit MediaSessionObserver with what you described - it will still be a Tab and WebContents observer updating the notification when those happen, right? Is there a description of what the plans are in some form (doc, diagram)?
Talked with Anton and Mounir yesterday, we agreed the following plan:

MediaSessionObserver -> MediaSessionPlayerObserver
MedidSessionController (keep the name)
MediaSessionDelegate -> AudioFocusDelegate (C++ & Android)
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a2147f8ab712703c6513078635fcacd595a88979

commit a2147f8ab712703c6513078635fcacd595a88979
Author: zqzhang <zqzhang@chromium.org>
Date: Tue Oct 18 22:53:26 2016

Fixing naming issues in MediaSession

There are a handful of classes in media/session that have bad
names which do not match what these class really are. This CL
fixes this naming issue:

- MediaSessionObserver -> MediaSessionPlayerObserver
- MediaSessionDelegate -> AudioFocusDelegate (C++ & Android)

Overall work description:
https://docs.google.com/a/google.com/document/d/1duLlKQ6Y8ogC9VUbDvdTz8-Rn45Mrove0uD5j089Le8/edit?usp=sharing

BUG= 655781 

Review-Url: https://codereview.chromium.org/2416853005
Cr-Commit-Position: refs/heads/master@{#426075}

[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/BUILD.gn
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/android/browser_jni_registrar.cc
[add] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/audio_focus_delegate.h
[add] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/audio_focus_delegate_android.cc
[rename] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/audio_focus_delegate_android.h
[rename] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/audio_focus_delegate_android_browsertest.cc
[add] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/audio_focus_delegate_default.cc
[rename] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/audio_focus_delegate_default_browsertest.cc
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/audio_focus_manager_unittest.cc
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/media_session.cc
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/media_session.h
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/media_session_browsertest.cc
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/media_session_controller.h
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/media_session_controllers_manager.cc
[delete] https://crrev.com/5ab22dff550158922f4a443d86cf993ac46348f8/content/browser/media/session/media_session_delegate.h
[delete] https://crrev.com/5ab22dff550158922f4a443d86cf993ac46348f8/content/browser/media/session/media_session_delegate_android.cc
[delete] https://crrev.com/5ab22dff550158922f4a443d86cf993ac46348f8/content/browser/media/session/media_session_delegate_default.cc
[rename] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/media_session_player_observer.h
[delete] https://crrev.com/5ab22dff550158922f4a443d86cf993ac46348f8/content/browser/media/session/mock_media_session_observer.cc
[add] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/mock_media_session_player_observer.cc
[rename] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/mock_media_session_player_observer.h
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/browser/media/session/pepper_player_delegate.h
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/public/android/BUILD.gn
[add] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/public/android/java/src/org/chromium/content/browser/AudioFocusDelegate.java
[delete] https://crrev.com/5ab22dff550158922f4a443d86cf993ac46348f8/content/public/android/java/src/org/chromium/content/browser/MediaSessionDelegate.java
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/public/android/java/src/org/chromium/content/browser/OWNERS
[modify] https://crrev.com/a2147f8ab712703c6513078635fcacd595a88979/content/test/BUILD.gn

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4c90d75c4c83b567a250af8b0ee242f58f81a80

commit e4c90d75c4c83b567a250af8b0ee242f58f81a80
Author: zqzhang <zqzhang@chromium.org>
Date: Wed Oct 26 15:12:29 2016

Follow-up rename of MediaSessionDelegate->AudioFocusDelegate

This CL fixes a test naming issue missed in the previous CL.

BUG= 655781 

Review-Url: https://codereview.chromium.org/2448013004
Cr-Commit-Position: refs/heads/master@{#427692}

[modify] https://crrev.com/e4c90d75c4c83b567a250af8b0ee242f58f81a80/content/browser/media/session/audio_focus_delegate_default_browsertest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment