Split MediaContentType and AudioFocusType

content/browser/media/session/media_session.cc

Index: content/browser/media/session/media_session.cc
diff --git a/content/browser/media/session/media_session.cc b/content/browser/media/session/media_session.cc
index 33fa652b363248c485df89dd634ee4aed9951a49..a0092cc1da5e63d0f3eca2966f5c38af3e6bc5b5 100644
--- a/content/browser/media/session/media_session.cc
+++ b/content/browser/media/session/media_session.cc
@@ -9,6 +9,7 @@
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
+#include "media/base/media_content_type.h"
 
 namespace content {
 
@@ -68,24 +69,35 @@ void MediaSession::SetMetadata(const MediaMetadata& metadata) {
 
 bool MediaSession::AddPlayer(MediaSessionObserver* observer,
                              int player_id,
-                             Type type) {
+                             media::MediaContentType type) {
+  if (type == media::MediaContentType::Uncontrollable) {
+    return true;
+  }

[whywhat, 2016/08/18 20:33:41]

nit: no need for {} here

[Zhiqiang Zhang (Slow), 2016/08/19 15:05:55]

Done.

   observer->OnSetVolumeMultiplier(player_id, volume_multiplier_);
 
   // If the audio focus is already granted and is of type Content, there is

[whywhat, 2016/08/18 20:33:41]

nit: this comment needs to be updated

[Zhiqiang Zhang (Slow), 2016/08/19 15:05:55]

Moved these to the next block (early return).

   // nothing to do. If it is granted of type Transient the requested type is
   // also transient, there is also nothing to do. Otherwise, the session needs
   // to request audio focus again.
+  // TODO(zqzhang): handle duckable and uncontrollable.

[whywhat, 2016/08/18 20:33:41]

nit: do you need to file a crbug to track the TODO?

[Zhiqiang Zhang (Slow), 2016/08/19 15:05:55]

Done.

+  AudioFocusManager::AudioFocusType required_audio_focus_type =

[whywhat, 2016/08/18 20:33:42]

could this be a media::AudioFocutTypeFromMediaContentType or something?
nit: also, prefer if's to ?: unless it fits on one line
nit: s/required/requested/

[Zhiqiang Zhang (Slow), 2016/08/19 15:05:55]

Using if for now. After adding Pepper&WebRTC, we'll decide whether to move the
logic into a helper function.
I think "required" is better, which means the required audio focus for playing
the added player.

+      (type == media::MediaContentType::Gain)

[whywhat, 2016/08/18 20:33:41]

there's no MediaContentType::Gain, right?

[Zhiqiang Zhang (Slow), 2016/08/19 15:05:55]

Done.

+          ? AudioFocusManager::AudioFocusType::Gain
+          : AudioFocusManager::AudioFocusType::GainTransientMayDuck;
+
   if (audio_focus_state_ == State::ACTIVE &&
-      (audio_focus_type_ == Type::Content || audio_focus_type_ == type)) {
+      (audio_focus_type_ == AudioFocusManager::AudioFocusType::Gain ||
+       audio_focus_type_ == required_audio_focus_type)) {
     players_.insert(PlayerIdentifier(observer, player_id));
     return true;
   }
 
   State old_audio_focus_state = audio_focus_state_;
-  State audio_focus_state = RequestSystemAudioFocus(type) ? State::ACTIVE
-                                                          : State::INACTIVE;
+  State audio_focus_state = RequestSystemAudioFocus(required_audio_focus_type)
+                                ? State::ACTIVE
+                                : State::INACTIVE;
   SetAudioFocusState(audio_focus_state);
-  audio_focus_type_ = type;
+  audio_focus_type_ = required_audio_focus_type;
 
   if (audio_focus_state_ != State::ACTIVE)
     return false;
@@ -214,7 +226,7 @@ bool MediaSession::IsSuspended() const {
 bool MediaSession::IsControllable() const {
   // Only content type media session can be controllable unless it is inactive.

[whywhat, 2016/08/18 20:33:42]

nit: update comment?

[Zhiqiang Zhang (Slow), 2016/08/19 15:05:55]

Done.

   return audio_focus_state_ != State::INACTIVE &&
-         audio_focus_type_ == Type::Content;
+         audio_focus_type_ == AudioFocusManager::AudioFocusType::Gain;
 }
 
 std::unique_ptr<base::CallbackList<void(MediaSession::State)>::Subscription>
@@ -232,7 +244,8 @@ bool MediaSession::IsActiveForTest() const {
   return audio_focus_state_ == State::ACTIVE;
 }
 
-MediaSession::Type MediaSession::audio_focus_type_for_test() const {
+AudioFocusManager::AudioFocusType MediaSession::audio_focus_type_for_test()
+    const {
   return audio_focus_type_;
 }
 
@@ -306,15 +319,16 @@ void MediaSession::OnResumeInternal(SuspendType type) {
 MediaSession::MediaSession(WebContents* web_contents)
     : WebContentsObserver(web_contents),
       audio_focus_state_(State::INACTIVE),
-      audio_focus_type_(Type::Transient),
-      volume_multiplier_(kDefaultVolumeMultiplier) {
-}
+      audio_focus_type_(
+          AudioFocusManager::AudioFocusType::GainTransientMayDuck),
+      volume_multiplier_(kDefaultVolumeMultiplier) {}
 
 void MediaSession::Initialize() {
   delegate_ = MediaSessionDelegate::Create(this);
 }
 
-bool MediaSession::RequestSystemAudioFocus(Type type) {
+bool MediaSession::RequestSystemAudioFocus(
+    AudioFocusManager::AudioFocusType type) {
   bool result = delegate_->RequestAudioFocus(type);
   uma_helper_.RecordRequestAudioFocusResult(result);
   return result;