Adding mojo MediaSessionClient to support media controls

third_party/WebKit/Source/modules/mediasession/MediaSession.cpp

Index: third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
diff --git a/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp b/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
index 0afbb0d8c1483b93121fc7f133fb10a5b3177c1b..8b05b5c6f7d3b5a5daf5364e41dff740d7bc3380 100644
--- a/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
+++ b/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
@@ -7,6 +7,7 @@
 #include "bindings/core/v8/ScriptState.h"
 #include "core/dom/Document.h"
 #include "core/dom/ExecutionContext.h"
+#include "core/events/Event.h"
 #include "core/frame/LocalFrame.h"
 #include "modules/EventTargetModules.h"
 #include "modules/mediasession/MediaMetadata.h"
@@ -16,13 +17,69 @@
 
 namespace blink {
 
+namespace {
+
+// Map from MediaSessionAction to EventTypeName.
+const Vector<AtomicString>& getActionEnumToEventNameVec() {
+  DEFINE_THREAD_SAFE_STATIC_LOCAL(
+      Vector<AtomicString>, actionEnumToEventNameVec, []() {

[haraken, 2016/10/18 19:42:37]

I'm asking why we need to create the vector in the first place.

Why is the switch statement not enough?

switch (eventTypeName) {
  case EventTypeNames::play;
    return ...;
  case ...;
}

[Zhiqiang Zhang (Slow), 2016/10/19 12:52:06]

OK, Done. Using switch is much cleaner. The cost is not super expensive :)

+        Vector<AtomicString>* vec = new Vector<AtomicString>();
+        // Must be in the same order. HashMap cannot be used since 0 is reserved
+        // and cannot be used as key.
+        vec->append(EventTypeNames::play);
+        vec->append(EventTypeNames::pause);
+        vec->append(EventTypeNames::playpause);
+        vec->append(EventTypeNames::previoustrack);
+        vec->append(EventTypeNames::nexttrack);
+        vec->append(EventTypeNames::seekforward);
+        vec->append(EventTypeNames::seekbackward);
+
+        return vec;
+      }());
+  return actionEnumToEventNameVec;
+}
+
+using StringToActionEnumMap =
+    HashMap<AtomicString, blink::mojom::blink::MediaSessionAction>;
+
+// Map from EventTypeName to MediaSessionAction.
+const StringToActionEnumMap& getEventNameToActionEnumMap() {
+  DEFINE_THREAD_SAFE_STATIC_LOCAL(
+      StringToActionEnumMap, eventNameToActionEnumMap, []() {

[haraken, 2016/10/18 19:42:37]

Ditto.

[Zhiqiang Zhang (Slow), 2016/10/19 12:52:06]

Done.

+        StringToActionEnumMap* map = new StringToActionEnumMap();
+        map->add(EventTypeNames::play,
+                 blink::mojom::blink::MediaSessionAction::PLAY);
+        map->add(EventTypeNames::pause,
+                 blink::mojom::blink::MediaSessionAction::PAUSE);
+        map->add(EventTypeNames::playpause,
+                 blink::mojom::blink::MediaSessionAction::PLAY_PAUSE);
+        map->add(EventTypeNames::previoustrack,
+                 blink::mojom::blink::MediaSessionAction::PREVIOUS_TRACK);
+        map->add(EventTypeNames::nexttrack,
+                 blink::mojom::blink::MediaSessionAction::NEXT_TRACK);
+        map->add(EventTypeNames::seekforward,
+                 blink::mojom::blink::MediaSessionAction::SEEK_FORWARD);
+        map->add(EventTypeNames::seekbackward,
+                 blink::mojom::blink::MediaSessionAction::SEEK_BACKWARD);
+
+        return map;
+      }());
+  return eventNameToActionEnumMap;
+}
+
+}  // anonymous namespace
+
 MediaSession::MediaSession(ScriptState* scriptState)
-    : m_scriptState(scriptState) {}
+    : m_scriptState(scriptState), m_clientBinding(this) {}
 
 MediaSession* MediaSession::create(ScriptState* scriptState) {
   return new MediaSession(scriptState);
 }
 
+void MediaSession::dispose() {
+  m_clientBinding.Close();
+}
+
 void MediaSession::setMetadata(MediaMetadata* metadata) {
   if (mojom::blink::MediaSessionService* service =
           getService(m_scriptState.get())) {
@@ -55,6 +112,8 @@ mojom::blink::MediaSessionService* MediaSession::getService(
 
     if (interfaceProvider)
       interfaceProvider->getInterface(mojo::GetProxy(&m_service));
+
+    m_service->SetClient(m_clientBinding.CreateInterfacePtrAndBind());

[whywhat, 2016/10/18 21:02:57]

hm, at this point, if document->frame() is null or interfaceProvider is null,
you will call SetClient on an uninitialized m_service.
perhaps you'd better rewrite it early return style?

if (m_service) return m_service.get();

Document* document = ...
if (!document->frame()) return nullptr;

interfaceProvider = ...
if (!interfaceProvider) return nullptr;

m_service->SetClient(...);

return m_service.get();

[Zhiqiang Zhang (Slow), 2016/10/19 12:52:06]

Done.

   }
   return m_service.get();
 }
@@ -63,18 +122,46 @@ bool MediaSession::addEventListenerInternal(
     const AtomicString& eventType,
     EventListener* listener,
     const AddEventListenerOptionsResolved& options) {
-  // TODO(zqzhang): Notify MediaSessionService the handler has been set. See
-  // https://crbug.com/656563
-  return EventTarget::addEventListenerInternal(eventType, listener, options);
+  bool result =
+      EventTarget::addEventListenerInternal(eventType, listener, options);

[whywhat, 2016/10/18 21:02:57]

nit: do you have to do call the base class method before sending a message to
the service?

[Zhiqiang Zhang (Slow), 2016/10/19 12:52:06]

Ahh, I was afraid of threading issue. Seems not a problem in our case.
Done.

+
+  const auto& map = getEventNameToActionEnumMap();
+  auto iter = map.find(eventType);
+  if (iter != map.end()) {
+    if (mojom::blink::MediaSessionService* service =
+            getService(m_scriptState.get())) {
+      service->EnableAction(iter->value);
+    }
+  }
+  return result;
 }
 
 bool MediaSession::removeEventListenerInternal(
     const AtomicString& eventType,
     const EventListener* listener,
     const EventListenerOptions& options) {
-  // TODO(zqzhang): Notify MediaSessionService the handler has been unset. See
-  // https://crbug.com/656563
-  return EventTarget::removeEventListenerInternal(eventType, listener, options);
+  bool result =

[whywhat, 2016/10/18 21:02:57]

nit: ditto

[Zhiqiang Zhang (Slow), 2016/10/19 12:52:06]

Done.

+      EventTarget::removeEventListenerInternal(eventType, listener, options);
+
+  const auto& map = getEventNameToActionEnumMap();
+  auto iter = map.find(eventType);
+  if (iter != map.end()) {
+    if (mojom::blink::MediaSessionService* service =

[whywhat, 2016/10/18 21:02:57]

nit: what if the service returned is nullptr?

[Zhiqiang Zhang (Slow), 2016/10/19 12:52:06]

Then it won't reach the `then` branch :)

+            getService(m_scriptState.get())) {
+      service->DisableAction(iter->value);
+    }
+  }
+  return result;
+}
+
+void MediaSession::DidReceivedAction(
+    blink::mojom::blink::MediaSessionAction action) {
+  LOG(INFO) << static_cast<int>(action);
+  const auto& vec = getActionEnumToEventNameVec();

[whywhat, 2016/10/18 21:02:57]

do the opposite switch statement function (like mojomActionToEventName) here:

switch (action) {
  case ...:
    return eventName::;

...
 
}

[Zhiqiang Zhang (Slow), 2016/10/19 12:52:06]

Done.

+  if (static_cast<int>(action) >= 0 &&
+      static_cast<size_t>(action) < vec.size()) {
+    dispatchEvent(Event::create(vec[static_cast<int>(action)]));
+  }
 }
 
 DEFINE_TRACE(MediaSession) {