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

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

Index: chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
diff --git a/chrome/browser/ui/webui/media_router/media_router_dialog_controller.h b/chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
index f357d00bb173adb44d65fe467591b1280f31dc7a..e77ea43c87f36c7cfe49e44db1f1367599c9d001 100644
--- a/chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
+++ b/chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
@@ -6,12 +6,15 @@
 #define CHROME_BROWSER_UI_WEBUI_MEDIA_ROUTER_MEDIA_ROUTER_DIALOG_CONTROLLER_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/media/router/create_session_request.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
 namespace media_router {
 
+class PresentationServiceDelegateImpl;
+
 // An instance of this class is tied to a WebContents known as the initiator,
 // and is lazily created when a Media Router dialog needs to be shown.
 // The MediaRouterDialogController allows creating, querying, and removing a
@@ -89,6 +92,103 @@ class MediaRouterDialogController
   DISALLOW_COPY_AND_ASSIGN(MediaRouterDialogController);
 };
 
+// Class used by the Media Router dialog to signal route response and dialog
+// destruction. In addition, it contains logic for handling Presentation API
+// startSession requests and ondefaultsessionstart event handler. As such, this
+// class can be used for both Presentation-API-initiated and browser-initiated
+// dialogs.
+// Instances of this class correspond 1:1 with Media Router dialogs and are
+// self-managed.
+// An instance will get notified in the following cases:
+// (1) When the response of a Media Router route request initiated from the
+// dialog arrives.
+// (2) When the dialog is being destroyed.
+// There is also an API for the dialog to attach additional listeners for (1),
+// e.g., to update the UI on route response.
+// Instances are self-managed and generally have the same lifetime as the
+// dialog (i.e., they delete themselves in (2)), but will outlive the dialog
+// if there is a pending route request at the time of dialog destruction.
+// In that case, the instance will be destroyed on the next invocation of (1),
+// which is assumed to always happen eventually.
+// It also assumes that the dialog will not make concurrent route requests
+// (That is, it will wait to make the next route request until a response has
+// come back).
+class MediaRouterDialogCallbacks {
+ public:
+  using DialogRouteResponseCallback =
+      base::Callback<void(const MediaRoute*, const std::string&)>;
+
+  // Constructs an instance without a presentation request. Used for
+  // browser-initiated dialogs.
+  // |delegate|: PresentationServiceDelegateImpl instance corresponding to the
+  // initiator tab. Used for notifying ondefaultsessionstart event.
+  explicit MediaRouterDialogCallbacks(
+      const base::WeakPtr<PresentationServiceDelegateImpl>& delegate);

[haibinlu, 2015/07/09 21:45:59]

per offline discussion, it is clearer to pass in webcontent there instead of
psdImpl since it is only available when there is a presentation API client.
Users may mirror a tab or desktop without presentation API client. 

[imcheng, 2015/07/09 22:22:29]

Done. To do that, I needed to make sure initiator WebContents refenrece is valid
throughout lifetime of MediaRouterDialogCallbacks. For that I made MRDC take
ownership of MediaRouterDialogCallbacks. This ownership is needed in order to
avoid memory leaks that I previously hadn't thought of. This also means I am
passing in a WeakPtr<MediaRouterDialogCallbacks> to MRUI since the initiator can
be destroyed before MRUI.

+  // Used for Presentation-API-initiated dialogs.
+  // |presentation_request|: The Presentation API startSession request.
+  // |delegate|: PresentationServiceDelegateImpl instance corresponding to the
+  // initiator tab. Used for notifying ondefaultsessionstart event.
+  MediaRouterDialogCallbacks(
+      scoped_ptr<CreateSessionRequest> presentation_request,
+      const base::WeakPtr<PresentationServiceDelegateImpl>& delegate);
+
+  ~MediaRouterDialogCallbacks();
+
+  // Attaches |callback| to be invoked when |OnRouteResponseReceived| is
+  // invoked.
+  void SetRouteResponseCallback(const DialogRouteResponseCallback& callback);
+
+  // Invoked when the corresponding Media Router dialog is being destroyed.
+  // If there is a Presentation API request and there is no pending presentation
+  // route response, the request will be rejected.
+  // |has_pending_presentation_response|: True if there is a pending
+  // presentation route response from the MediaRouter. If true, this instance
+  // will continue existing until the route response comes back. Otherwise, it
+  // will be destroyed at the end of this method.
+  void OnDialogDestroyed(bool has_pending_presentation_response);
+
+  // Invoked when a route response for a Media Router route request initiated
+  // from the
+  // corresponding Media Router dialog comes back.
+  // If the response was for a presentationthere is a Presentation API request,
+  // two things can happen:
+  // (1) If there is a Presentation API request, then the request will be
+  // resolved/rejected accordingly.
+  // (2) Otherwise, if route response was a success, then
+  // PresentationServiceDelegateImpl will be notified that a browser-initiated
+  // presentation has started.
+  // If |route_response_cb_| is set, it will be invoked with the response.
+  // Finally, if the response arrived after the dialog has been destroyed, then
+  // this instance will be destroyed at the end of this method.

[haibinlu, 2015/07/09 21:45:59]

If UX wants enabling multiple launch simultaneously, then it is possible for one
UI instance to generate several create-route requests to different sinks. In
that case, this instance can only be destroyed after the last create-route
request comes back.

[imcheng, 2015/07/09 22:22:29]

UX mentioned that they will only allow one at a time. But in any case, we can
simply change the boolean to an int to count number of outstanding requests, and
only delete if it reaches zero.

+  void OnRouteResponseReceived(bool is_presentation,
+                               scoped_ptr<MediaRoute> route,
+                               const std::string& error);
+
+  base::WeakPtr<MediaRouterDialogCallbacks> GetWeakPtr();
+
+  const CreateSessionRequest* presentation_request() const {
+    return presentation_request_.get();
+  }
+
+  void set_route_response_cb(const DialogRouteResponseCallback& callback) {
+    route_response_cb_ = callback;
+  }
+
+ private:
+  void HandleRouteResponseForPresentation(const MediaRoute* route,
+                                          const std::string& error);
+
+  DialogRouteResponseCallback route_response_cb_;
+  scoped_ptr<CreateSessionRequest> presentation_request_;
+  bool pending_presentation_response_after_dialog_destroyed_;
+
+  base::WeakPtr<PresentationServiceDelegateImpl> presentation_service_delegate_;
+
+  base::WeakPtrFactory<MediaRouterDialogCallbacks> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaRouterDialogCallbacks);
+};
+
 }  // namespace media_router
 
 #endif  // CHROME_BROWSER_UI_WEBUI_MEDIA_ROUTER_MEDIA_ROUTER_DIALOG_CONTROLLER_H_