[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..df015f14c79bba80c4caf9a3ffc4136a9b6c292f 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,138 @@ 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.

[mark a. foltz, 2015/07/14 21:36:32]

This description doesn't quite match up with the behavior.  This object keeps a
ptr to the dialog controller and can invoke a method on it at any time through
its public API.  Where is the protection against the destruction of the dialog
controller?

+// 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(

[mark a. foltz, 2015/07/14 21:36:32]

Nit: This object wraps a single callback, so maybe just
MediaRouterDialogCallback?

+      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() {}
+
+  void OnPresentationRouteResponseReceived(scoped_ptr<MediaRoute> route,
+                                           const std::string& error) {
+    DCHECK(route || !error.empty());
+    ProcessPresentationRouteResponse(route.get(), error);

[haibinlu, 2015/07/14 01:44:55]

ProcessCreateRouteResponse

[imcheng, 2015/07/14 16:45:37]

ditto

+    route_response_callback_.Run(route.Pass(), error);
+    // No additional processing should be done after this call as |this| will
+    // be deleted immediately afterwards.
+    dialog_controller_->DestroyDialogCallbacks(this);
+  }
+
+  base::WeakPtr<MediaRouterDialogCallbacks> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+ private:
+  // Invoked from |ProcessPresentationRouteResponse|, before
+  // |route_response_callback_| is run and the instance is deleted.
+  virtual void ProcessPresentationRouteResponse(const MediaRoute* route,
+                                                const std::string& error) = 0;
+
+  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::BrowserInitiatedCallbacks
+    : public MediaRouterDialogController::MediaRouterDialogCallbacks {
+ public:
+  BrowserInitiatedCallbacks(
+      const MediaRouteResponseCallback& route_response_callback,
+      MediaRouterDialogController* dialog_controller,
+      content::WebContents* initiator)
+      : MediaRouterDialogController::MediaRouterDialogCallbacks(
+            route_response_callback,
+            dialog_controller),
+        initiator_(initiator) {
+    DCHECK(initiator_);
+  }
+  ~BrowserInitiatedCallbacks() override {}
+
+ private:
+  // MediaRouterDialogCallbacks override.
+  void ProcessPresentationRouteResponse(const MediaRoute* route,
+                                        const std::string& error) override {
+    if (route) {
+      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);

[haibinlu, 2015/07/14 01:44:55]

how is browser initiated tab-mirroring handled?

[imcheng, 2015/07/14 16:45:37]

For mirroring, instead of creating an instance of this class and binding its
callback, we bind MRUI::OnRouteResponseReceived as the CreateRoute response
callback.

+      }
+    }
+  }
+
+  // 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::PresentationInitiatedCallbacks
+    : public MediaRouterDialogController::MediaRouterDialogCallbacks {
+ public:
+  PresentationInitiatedCallbacks(
+      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_);
+  }
+
+  ~PresentationInitiatedCallbacks() override {}
+
+ private:
+  // MediaRouterDialogCallbacks overrides.
+  void ProcessPresentationRouteResponse(const 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());
+    }
+  }
+
+  scoped_ptr<CreateSessionRequest> presentation_request_;
+};
+
 class MediaRouterDialogController::DialogWebContentsObserver
     : public content::WebContentsObserver {
  public:
@@ -332,10 +465,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

[mark a. foltz, 2015/07/14 21:36:32]

Does there just need to be a separate method on PSDI to check for existence? 
Seems like it would be reasonable to do this now.

[imcheng, 2015/07/16 01:06:52]

Yes, we could call FromWebContents only to check for existence. The only tricky
case is for browser-initiated dialogs, if PSDImpl didn't exist before and is
created when the dialog is already open. In that case, the ideal behavior is to
update the UI with the info from PSDImpl and add a default presentation
observer. Without that logic, the user has to close and reopen the dialog to see
the updated UI. The current logic is correct in that regard. I guess in that
sense, this doesn't need to be fixed. I think splitting up PSDImpl and/or
introducing an interface to talk to it will help make this code cleaner.

+    // exist (crbug.com/508695).
+    base::WeakPtr<PresentationServiceDelegateImpl> delegate =
+        PresentationServiceDelegateImpl::GetOrCreateForWebContents(initiator)
+            ->GetWeakPtr();
     media_router_ui->InitWithDefaultMediaSource(delegate);
   } else {
     media_router_ui->InitWithPresentationSessionRequest(
@@ -343,5 +477,32 @@ void MediaRouterDialogController::PopulateDialog(
   }
 }
 
+MediaRouteResponseCallback
+MediaRouterDialogController::AddPresentationRouteRequest(

[mark a. foltz, 2015/07/14 21:36:32]

Please split this into two methods:

AddPresentationRouteRequest()
AddBrowserRouteRequest()

instead of having an optional parameter.

+    const MediaRouteResponseCallback& route_response_callback,
+    scoped_ptr<CreateSessionRequest> presentation_request) {
+  scoped_ptr<MediaRouterDialogCallbacks> dialog_callbacks;
+  if (!presentation_request) {
+    dialog_callbacks.reset(new BrowserInitiatedCallbacks(
+        route_response_callback, this, initiator_));
+  } else {
+    dialog_callbacks.reset(new PresentationInitiatedCallbacks(
+        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