Clean up naming in MediaSession |
|||
Issue descriptionFound 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
,
Oct 14 2016
,
Oct 14 2016
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.
,
Oct 17 2016
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".
,
Oct 17 2016
> I suggested renaming the current MediaSession to something like "AudioSession". This is unlikely to happen in the near future :-/
,
Oct 17 2016
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)?
,
Oct 18 2016
Talked with Anton and Mounir yesterday, we agreed the following plan: MediaSessionObserver -> MediaSessionPlayerObserver MedidSessionController (keep the name) MediaSessionDelegate -> AudioFocusDelegate (C++ & Android)
,
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
,
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
,
Oct 27 2016
|
|||
►
Sign in to add a comment |
|||
Comment 1 by zqzh...@chromium.org
, Oct 13 2016