[Media Router] 2nd take on fix route response callback lifetime in UI.

chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc

Index: chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
diff --git a/chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc b/chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
index 392ebfc891d04c8db732f31a3a90a99b26a880db..6be28d431482b377ce616c60bb6e59cf8e3f8469 100644
--- a/chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
+++ b/chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
@@ -7,6 +7,7 @@
 #include <string>
 #include <vector>
 
+#include "chrome/browser/media/router/presentation_service_delegate_impl.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/webui/constrained_web_dialog_ui.h"
 #include "chrome/browser/ui/webui/media_router/media_router_ui.h"
@@ -110,6 +111,134 @@ MediaRouterDialogController::GetOrCreateForWebContents(
   return MediaRouterDialogController::FromWebContents(web_contents);
 }
 
+// Base class used when a Media Router dialog needs to make a presentation
+// route request to the Media Router. The route request can be due to either
+// a Presentation API request or the browser (see subclasses below).
+// MediaRouterDialogCallbacks ensures that the route response is received and
+// handled properly even if the dialog was destroyed while the request is in
+// progress.
+// Instances are created by calling |AddDialogCallbacks()| on the
+// MediaRouterDialogController corresponding to the dialog's initiator
+// WebContents. |OnPresentationRouteResponseReceived()| of the created instance
+// can then be used as the response callback of |MediaRouter::CreateRoute()|.
+// Instances are deleted from MediaRouterDialogController when
+// |OnPresentationRouteResponseReceived()| is invoked.
+// TODO(imcheng): Currently it is assumed that MediaRouter requests will
+// always be eventually resolved, but this may not be true due to
+// implementation errors, in which case DialogCallbacks objects may be leaked.
+// We should have some timeout mechanism to make sure they self-destruct
+// after certain conditions and timeout are met.
+class MediaRouterDialogController::MediaRouterDialogCallbacks {
+ public:
+  MediaRouterDialogCallbacks(
+      const MediaRouteResponseCallback& route_response_callback,
+      MediaRouterDialogController* dialog_controller)
+      : route_response_callback_(route_response_callback),
+        dialog_controller_(dialog_controller),
+        weak_factory_(this) {
+    DCHECK(!route_response_callback_.is_null());
+    DCHECK(dialog_controller_);
+  }
+
+  virtual ~MediaRouterDialogCallbacks() {}
+
+  virtual void OnPresentationRouteResponseReceived(scoped_ptr<MediaRoute> route,

[Kevin M, 2015/07/13 21:55:26]

Make the body of this function call a different pure virtual method which is
implemented by subclasses. You'll get some nice guarantees that way: that the
callback is always run, that the callbacks are destroyed, and that the
subclasses must provide an implementation or else a compiler error will be
raised.

[imcheng, 2015/07/14 01:00:23]

Good idea. Done.

+                                                   const std::string& error) {
+    route_response_callback_.Run(route.Pass(), error);
+    dialog_controller_->DestroyDialogCallbacks(this);

[Kevin M, 2015/07/13 21:55:26]

Suggestion: make a note that no additional processing should be done after this
line, because |this| will be deleted immediately.

[imcheng, 2015/07/14 01:00:23]

Done.

+  }
+
+  base::WeakPtr<MediaRouterDialogCallbacks> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+ private:
+  MediaRouteResponseCallback route_response_callback_;
+
+  // Reference to the MediaRouterDialogController that owns this instance.
+  MediaRouterDialogController* const dialog_controller_;
+
+  base::WeakPtrFactory<MediaRouterDialogCallbacks> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaRouterDialogCallbacks);
+};
+
+class MediaRouterDialogController::BrowserInitiatedMediaRouterDialogCallbacks

[Kevin M, 2015/07/13 21:55:26]

Nit: that's a big identifier. This is just a child class so maybe we could use
that context to give it a shorter name?

BrowserInitiatedCallbacks? PresentationInitiatedCallbacks?

[imcheng, 2015/07/14 01:00:24]

Done.

+    : public MediaRouterDialogController::MediaRouterDialogCallbacks {
+ public:
+  BrowserInitiatedMediaRouterDialogCallbacks(
+      const MediaRouteResponseCallback& route_response_callback,
+      MediaRouterDialogController* dialog_controller,
+      content::WebContents* initiator)
+      : MediaRouterDialogController::MediaRouterDialogCallbacks(
+            route_response_callback,
+            dialog_controller),
+        initiator_(initiator) {
+    DCHECK(initiator_);
+  }
+  ~BrowserInitiatedMediaRouterDialogCallbacks() override {}
+
+  // MediaRouterDialogCallbacks override.
+  void OnPresentationRouteResponseReceived(scoped_ptr<MediaRoute> route,
+                                           const std::string& error) override {
+    if (route) {

[Kevin M, 2015/07/13 21:55:26]

Suggestion: DCHECK(route || !error.empty()) ?

[imcheng, 2015/07/14 01:00:24]

Done.

+      PresentationServiceDelegateImpl* delegate =
+          PresentationServiceDelegateImpl::FromWebContents(initiator_);
+      if (delegate) {
+        // Response was due to a browser-initiated request. Let
+        // PresentationServiceDelegateImpl perform the match against the default
+        // presentation URL.
+        delegate->OnRouteCreated(*route);
+      }
+    }
+
+    MediaRouterDialogCallbacks::OnPresentationRouteResponseReceived(
+        route.Pass(), error);
+  }
+
+ private:
+  // Used for obtaining a reference to its PresentationServiceDelegateImpl
+  // in case of browser-initiated presentation. As this instance is owned
+  // by the MediaRouterDialogController, |initiator_| will outlive this
+  // instance.
+  content::WebContents* initiator_;
+};
+
+class MediaRouterDialogController::PresentationMediaRouterDialogCallbacks
+    : public MediaRouterDialogController::MediaRouterDialogCallbacks {
+ public:
+  PresentationMediaRouterDialogCallbacks(
+      const MediaRouteResponseCallback& route_response_callback,
+      MediaRouterDialogController* dialog_controller,
+      scoped_ptr<CreateSessionRequest> presentation_request)
+      : MediaRouterDialogController::MediaRouterDialogCallbacks(
+            route_response_callback,
+            dialog_controller),
+        presentation_request_(presentation_request.Pass()) {
+    DCHECK(presentation_request_);
+  }
+
+  ~PresentationMediaRouterDialogCallbacks() override {}
+
+  // MediaRouterDialogCallbacks overrides.
+  void OnPresentationRouteResponseReceived(scoped_ptr<MediaRoute> route,
+                                           const std::string& error) override {
+    if (!route) {
+      presentation_request_->MaybeInvokeErrorCallback(
+          content::PresentationError(content::PRESENTATION_ERROR_UNKNOWN,
+                                     error));
+    } else {
+      presentation_request_->MaybeInvokeSuccessCallback(
+          route->media_route_id());
+    }
+    MediaRouterDialogCallbacks::OnPresentationRouteResponseReceived(
+        route.Pass(), error);
+  }
+
+ private:
+  scoped_ptr<CreateSessionRequest> presentation_request_;
+};
+
 class MediaRouterDialogController::DialogWebContentsObserver
     : public content::WebContentsObserver {
  public:
@@ -332,10 +461,11 @@ void MediaRouterDialogController::PopulateDialog(
   }
 
   if (!presentation_request_.get()) {
-    PresentationServiceDelegateImpl::CreateForWebContents(initiator);
-    PresentationServiceDelegateImpl* delegate =
-        PresentationServiceDelegateImpl::FromWebContents(initiator);
-    CHECK(delegate);
+    // TODO(imcheng): Don't create PresentationServiceDelegateImpl if it doesn't
+    // exist (crbug.com/508695).
+    base::WeakPtr<PresentationServiceDelegateImpl> delegate =
+        PresentationServiceDelegateImpl::GetOrCreateForWebContents(initiator)
+            ->GetWeakPtr();
     media_router_ui->InitWithDefaultMediaSource(delegate);
   } else {
     media_router_ui->InitWithPresentationSessionRequest(
@@ -343,5 +473,32 @@ void MediaRouterDialogController::PopulateDialog(
   }
 }
 
+MediaRouteResponseCallback
+MediaRouterDialogController::AddPresentationRouteRequest(
+    const MediaRouteResponseCallback& route_response_callback,
+    scoped_ptr<CreateSessionRequest> presentation_request) {
+  scoped_ptr<MediaRouterDialogCallbacks> dialog_callbacks;
+  if (!presentation_request) {
+    dialog_callbacks.reset(new BrowserInitiatedMediaRouterDialogCallbacks(
+        route_response_callback, this, initiator_));
+  } else {
+    dialog_callbacks.reset(new PresentationMediaRouterDialogCallbacks(
+        route_response_callback, this, presentation_request.Pass()));
+  }
+  MediaRouteResponseCallback new_route_response_callback = base::Bind(
+      &MediaRouterDialogCallbacks::OnPresentationRouteResponseReceived,
+      dialog_callbacks->GetWeakPtr());
+  pending_dialog_callbacks_.push_back(dialog_callbacks.Pass());
+  return new_route_response_callback;
+}
+
+void MediaRouterDialogController::DestroyDialogCallbacks(
+    MediaRouterDialogCallbacks* callbacks) {
+  auto it = std::find(pending_dialog_callbacks_.begin(),
+                      pending_dialog_callbacks_.end(), callbacks);
+  if (it != pending_dialog_callbacks_.end())
+    pending_dialog_callbacks_.erase(it);
+}
+
 }  // namespace media_router