[Media Router] Erase PresentationFrame entry when corresponding RenderFrameHost is gone

chrome/browser/media/router/presentation_service_delegate_impl.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/router/presentation_service_delegate_impl.h"
 
 #include <string>
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@ skipping 101 matching lines @@
   }
 
  private:
   const content::PresentationConnectionMessageCallback message_cb_;
 
   DISALLOW_COPY_AND_ASSIGN(PresentationSessionMessagesObserver);
 };
 
 }  // namespace
 
 // Used by PresentationServiceDelegateImpl to manage

[imcheng, 2017/01/27 20:00:01]

Could you update this comment please?

[zhaobin, 2017/01/27 23:42:50]

Done.

 // listeners and default presentation info in a render frame.
 // Its lifetime:
 //  * PresentationFrameManager AddDelegateObserver
 //  * Reset 0+ times.
 //  * PresentationFrameManager.RemoveDelegateObserver.
 class PresentationFrame {
  public:
   PresentationFrame(const RenderFrameHostId& render_frame_host_id,
                     content::WebContents* web_contents,
                     MediaRouter* router);
@@ skipping 20 matching lines @@
                         const MediaRoute::Id& route_id);
 
   const MediaRoute::Id GetRouteId(const std::string& presentation_id) const;
   const std::vector<MediaRoute::Id> GetRouteIds() const;
 
   void OnPresentationSessionStarted(
       const content::PresentationSessionInfo& session,
       const MediaRoute& route);
   void OnPresentationServiceDelegateDestroyed() const;
 
-  void set_delegate_observer(DelegateObserver* observer) {
-    delegate_observer_ = observer;
-  }
-
  private:
   MediaSource GetMediaSourceFromListener(
       content::PresentationScreenAvailabilityListener* listener) const;
   base::SmallMap<std::map<std::string, MediaRoute::Id>>
       presentation_id_to_route_id_;
   base::SmallMap<
       std::map<std::string, std::unique_ptr<PresentationMediaSinksObserver>>>
       url_to_sinks_observer_;
   std::unordered_map<
       MediaRoute::Id,
       std::unique_ptr<PresentationConnectionStateSubscription>>
       connection_state_subscriptions_;
   std::unordered_map<
       MediaRoute::Id,
       std::unique_ptr<PresentationSessionMessagesObserver>>
       session_messages_observers_;
 
   RenderFrameHostId render_frame_host_id_;
 
   // References to the owning WebContents, and the corresponding MediaRouter.
   content::WebContents* web_contents_;
   MediaRouter* router_;
-
-  DelegateObserver* delegate_observer_;
 };
 
 PresentationFrame::PresentationFrame(
     const RenderFrameHostId& render_frame_host_id,
     content::WebContents* web_contents,
     MediaRouter* router)
     : render_frame_host_id_(render_frame_host_id),
       web_contents_(web_contents),
       router_(router) {
   DCHECK(web_contents_);
@@ skipping 189 matching lines @@
   void SetMediaRouterForTest(MediaRouter* router);
 
   void OnPresentationSessionStarted(
       const RenderFrameHostId& render_frame_host_id,
       const content::PresentationSessionInfo& session,
       const MediaRoute& route);
   void OnDefaultPresentationSessionStarted(
       const PresentationRequest& request,
       const content::PresentationSessionInfo& session,
       const MediaRoute& route);
+  void AddPresentationFrame(const RenderFrameHostId& render_frame_host_id);
+  void RemovePresentationFrame(const RenderFrameHostId& render_frame_host_id);
 
   const MediaRoute::Id GetRouteId(const RenderFrameHostId& render_frame_host_id,
                                   const std::string& presentation_id) const;
   const std::vector<MediaRoute::Id> GetRouteIds(
       const RenderFrameHostId& render_frame_host_id) const;
 
   const PresentationRequest* default_presentation_request() const {
     return default_presentation_request_.get();
   }
 
@@ skipping 62 matching lines @@
   const auto it = presentation_frames_.find(request.render_frame_host_id());
   if (it != presentation_frames_.end())
     it->second->OnPresentationSessionStarted(session, route);
 
   if (default_presentation_request_ &&
       default_presentation_request_->Equals(request)) {
     default_presentation_started_callback_.Run(session);
   }
 }
 
+void PresentationFrameManager::AddPresentationFrame(
+    const RenderFrameHostId& render_frame_host_id) {
+  GetOrAddPresentationFrame(render_frame_host_id);
+}
+
+void PresentationFrameManager::RemovePresentationFrame(
+    const RenderFrameHostId& render_frame_host_id) {
+  const auto it = presentation_frames_.find(render_frame_host_id);

[imcheng, 2017/01/27 20:00:01]

This can just be presentation_frames_.erase(render_frame_host_id);

[zhaobin, 2017/01/27 23:42:50]

Code removed.

+  if (it != presentation_frames_.end())
+    presentation_frames_.erase(it);
+}
+
 const MediaRoute::Id PresentationFrameManager::GetRouteId(
     const RenderFrameHostId& render_frame_host_id,
     const std::string& presentation_id) const {
   const auto it = presentation_frames_.find(render_frame_host_id);
   return it != presentation_frames_.end()
       ? it->second->GetRouteId(presentation_id) : MediaRoute::Id();
 }
 
 const std::vector<MediaRoute::Id> PresentationFrameManager::GetRouteIds(
     const RenderFrameHostId& render_frame_host_id) const {
@@ skipping 168 matching lines @@
 }
 
 PresentationServiceDelegateImpl::~PresentationServiceDelegateImpl() {
 }
 
 void PresentationServiceDelegateImpl::AddObserver(int render_process_id,
                                                   int render_frame_id,
                                                   DelegateObserver* observer) {
   DCHECK(observer);
   observers_.AddObserver(render_process_id, render_frame_id, observer);
+  frame_manager_->AddPresentationFrame(

[imcheng, 2017/01/27 20:00:01]

It seems to me we don't need to explicitly add a PresentationFrame because it
gets created on demand.

[zhaobin, 2017/01/27 23:42:50]

Done.

+      RenderFrameHostId(render_process_id, render_frame_id));
 }
 
 void PresentationServiceDelegateImpl::RemoveObserver(int render_process_id,
                                                      int render_frame_id) {
   observers_.RemoveObserver(render_process_id, render_frame_id);
+  frame_manager_->RemovePresentationFrame(

[imcheng, 2017/01/27 20:00:01]

I think it would make more sense to erase the PresentationFrame on Reset(). It
would change the lifetime of PresentationFrame slightly but it would align it
with the lifetime of a document, which is more intuitive. WDYT?

[zhaobin, 2017/01/27 23:42:50]

Done.

+      RenderFrameHostId(render_process_id, render_frame_id));
 }
 
 bool PresentationServiceDelegateImpl::AddScreenAvailabilityListener(
     int render_process_id,
     int render_frame_id,
     content::PresentationScreenAvailabilityListener* listener) {
   DCHECK(listener);
   return frame_manager_->SetScreenAvailabilityListener(
       RenderFrameHostId(render_process_id, render_frame_id), listener);
 }
@@ skipping 303 matching lines @@
   const base::ListValue* origins =
       Profile::FromBrowserContext(web_contents_->GetBrowserContext())
           ->GetPrefs()
           ->GetList(prefs::kMediaRouterTabMirroringSources);
   return origins &&
          origins->Find(base::StringValue(origin.Serialize())) != origins->end();
 }
 #endif  // !defined(OS_ANDROID)
 
 }  // namespace media_router