[MediaRouter] Renames CloseRoute() to Terminate() and creates DetachRoute()

chrome/browser/media/router/presentation_service_delegate_impl.cc

Index: chrome/browser/media/router/presentation_service_delegate_impl.cc
diff --git a/chrome/browser/media/router/presentation_service_delegate_impl.cc b/chrome/browser/media/router/presentation_service_delegate_impl.cc
index 670fe4c1b58cf491815f989c9d383ea4e77fb5ab..b180a61d92b9a499375bf1b11d3a4b1d0f692aeb 100644
--- a/chrome/browser/media/router/presentation_service_delegate_impl.cc
+++ b/chrome/browser/media/router/presentation_service_delegate_impl.cc
@@ -83,6 +83,8 @@ class PresentationFrame {
       const content::PresentationSessionMessageCallback& message_cb);
 
   void Reset();
+  void RemoveConnection(const std::string& presentation_id,
+                        const MediaRoute::Id& route_id);
 
   const MediaRoute::Id GetRouteId(const std::string& presentation_id) const;
   const std::vector<MediaRoute::Id> GetRouteIds() const;
@@ -184,7 +186,7 @@ bool PresentationFrame::HasScreenAvailabilityListenerForTest(
 
 void PresentationFrame::Reset() {
   for (const auto& pid_route_id : presentation_id_to_route_id_)
-    router_->OnPresentationSessionDetached(pid_route_id.second);
+    router_->DetachRoute(pid_route_id.second);
 
   presentation_id_to_route_id_.clear();
   sinks_observer_.reset();
@@ -192,6 +194,23 @@ void PresentationFrame::Reset() {
   session_messages_observers_.clear();
 }
 
+void PresentationFrame::RemoveConnection(const std::string& presentation_id,
+                                         const MediaRoute::Id& route_id) {
+  // Remove the presentation id mapping so a later call to Reset is a no-op.
+  presentation_id_to_route_id_.erase(presentation_id);
+  // We no longer need to observe route messages.

[imcheng, 2015/12/10 19:50:46]

nit: new line above comment

[mark a. foltz, 2015/12/10 23:46:49]

Done

+  auto observer_iter = std::find_if(
+      session_messages_observers_.begin(), session_messages_observers_.end(),
+      [&route_id](const PresentationSessionMessagesObserver* observer) {
+        return route_id == observer->route_id();
+      });
+  if (observer_iter != session_messages_observers_.end())
+    session_messages_observers_.erase(observer_iter);
+  // We keep the PresentationConnectionStateChangedCallback registered with MR

[imcheng, 2015/12/10 19:50:46]

nit: new line above comment

[mark a. foltz, 2015/12/10 23:46:49]

Done

+  // so the MRP can tell us when terminate() completed.
+  router_->DetachRoute(route_id);

[imcheng, 2015/12/10 19:50:46]

Looks like you are already calling DetachRoute in PSDImpl::CloseConnection(), so
no need to call it again here?

[mark a. foltz, 2015/12/10 23:46:49]

You're right, good catch.  

+}
+
 void PresentationFrame::ListenForConnectionStateChange(
     const content::PresentationSessionInfo& connection,
     const content::PresentationConnectionStateChangedCallback&
@@ -280,6 +299,9 @@ class PresentationFrameManager {
       PresentationServiceDelegateImpl::DefaultPresentationRequestObserver*
           observer);
   void Reset(const RenderFrameHostId& render_frame_host_id);
+  void RemoveConnection(const RenderFrameHostId& render_frame_host_id,
+                        const MediaRoute::Id& route_id,
+                        const std::string& presentation_id);
   bool HasScreenAvailabilityListenerForTest(
       const RenderFrameHostId& render_frame_host_id,
       const MediaSource::Id& source_id) const;
@@ -505,6 +527,15 @@ void PresentationFrameManager::Reset(
   }
 }
 
+void PresentationFrameManager::RemoveConnection(
+    const RenderFrameHostId& render_frame_host_id,
+    const MediaRoute::Id& route_id,
+    const std::string& presentation_id) {
+  auto presentation_frame = presentation_frames_.get(render_frame_host_id);
+  if (presentation_frame)

[mlamouri (slow - plz ping), 2015/12/10 15:39:51]

Is `presentation_frame == nullptr` even possible? Should that be a DCHECK
instead?

[mark a. foltz, 2015/12/10 23:46:49]

My current understanding is a frame can call connection.close() or .terminate()
multiple times on the same connection, until the connection state change update
comes from content/ and Blink changes the connection state to disconnected or
terminated.

There's a TODO elsewhere to have Blink make the state change for
connection.close(), and have the Mojo call be fire and forget to content/ to
notify the MRP.   For terminate() I think we want the event to be fired when the
actual side effect takes place (so we'll wait for the callback from the MRP). 
At least, that's my thinking based on a conversation I had with imcheng@
yesterday.

+    presentation_frame->RemoveConnection(route_id, presentation_id);
+}
+
 PresentationFrame* PresentationFrameManager::GetOrAddPresentationFrame(
     const RenderFrameHostId& render_frame_host_id) {
   if (!presentation_frames_.contains(render_frame_host_id)) {
@@ -715,35 +746,38 @@ void PresentationServiceDelegateImpl::JoinSession(
       web_contents_, route_response_callbacks);
 }
 
-void PresentationServiceDelegateImpl::CloseSession(
+void PresentationServiceDelegateImpl::CloseConnection(
     int render_process_id,
     int render_frame_id,
     const std::string& presentation_id) {
-  const MediaRoute::Id& route_id = frame_manager_->GetRouteId(
-      RenderFrameHostId(render_process_id, render_frame_id), presentation_id);
+  const RenderFrameHostId rfh_id(render_process_id, render_frame_id);
+  const MediaRoute::Id& route_id =
+      frame_manager_->GetRouteId(rfh_id, presentation_id);
   if (route_id.empty()) {
     DVLOG(1) << "No active route for: " << presentation_id;
     return;
   }
 
-  // TODO(mfoltz, mlamouri): implement CloseSession().
-  // This could call router_->OnPresentationSessionDetached(route_id).
-  // PresentationFrame::Reset() should probably call CloseSession() too.
-  // Rename CloseRoute() to something else to avoid confusion?
-  NOTIMPLEMENTED();
+  router_->DetachRoute(route_id);
+  frame_manager_->RemoveConnection(rfh_id, presentation_id, route_id);
+  // TODO(mfoltz): close() should always succeed so there is no need to keep the
+  // state_changed_cb around - remove it and fire the ChangeEvent on the
+  // PresentationConnection in Blink.
 }
 
-void PresentationServiceDelegateImpl::TerminateSession(
+void PresentationServiceDelegateImpl::Terminate(
     int render_process_id,
     int render_frame_id,
     const std::string& presentation_id) {
-  const MediaRoute::Id& route_id = frame_manager_->GetRouteId(
-      RenderFrameHostId(render_process_id, render_frame_id), presentation_id);
+  const RenderFrameHostId rfh_id(render_process_id, render_frame_id);
+  const MediaRoute::Id& route_id =
+      frame_manager_->GetRouteId(rfh_id, presentation_id);
   if (route_id.empty()) {
     DVLOG(1) << "No active route for: " << presentation_id;
     return;
   }
-  router_->CloseRoute(route_id);
+  router_->TerminateRoute(route_id);
+  frame_manager_->RemoveConnection(rfh_id, presentation_id, route_id);

[imcheng, 2015/12/10 19:50:46]

So we are calling TerminateRoute, and then DetachRoute in
PresentationFrame::RemoveConnection()? Do we only need to call TerminateRoute?

[mark a. foltz, 2015/12/10 23:46:49]

Yeah removed call to router_->DetachRoute() from RemoveConnection.

 }
 
 void PresentationServiceDelegateImpl::ListenForSessionMessages(