[MediaRouter] Moved the code useful for Android to MediaRouterDialogController

chrome/browser/media/router/media_router_dialog_controller.cc

Index: chrome/browser/media/router/media_router_dialog_controller.cc
diff --git a/chrome/browser/media/router/media_router_dialog_controller.cc b/chrome/browser/media/router/media_router_dialog_controller.cc
index d9f80cc75805b71c733e0376807c6da84a604de5..9282fcfc0336cc57678cc9d5b3f8891da4459342 100644
--- a/chrome/browser/media/router/media_router_dialog_controller.cc
+++ b/chrome/browser/media/router/media_router_dialog_controller.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/browser/media/router/media_router_dialog_controller.h"
 
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_delegate.h"
+
 #if defined(OS_ANDROID)
 #include "chrome/browser/media/android/router/media_router_dialog_controller_android.h"
 #else
@@ -24,4 +28,110 @@ MediaRouterDialogController::GetOrCreateForWebContents(
 #endif
 }
 
+class MediaRouterDialogController::InitiatorWebContentsObserver
+    : public content::WebContentsObserver {
+ public:
+  InitiatorWebContentsObserver(
+      content::WebContents* web_contents,
+      MediaRouterDialogController* dialog_controller)
+      : content::WebContentsObserver(web_contents),
+        dialog_controller_(dialog_controller) {
+    DCHECK(dialog_controller_);
+  }
+
+ private:
+  void WebContentsDestroyed() override {
+    // NOTE: |this| is deleted after CloseMediaRouterDialog() returns.
+    dialog_controller_->CloseMediaRouterDialog();
+  }
+
+  void NavigationEntryCommitted(
+      const content::LoadCommittedDetails& load_details) override {
+    // NOTE: |this| is deleted after CloseMediaRouterDialog() returns.
+    dialog_controller_->CloseMediaRouterDialog();
+  }
+
+  void RenderProcessGone(base::TerminationStatus status) override {
+    // NOTE: |this| is deleted after CloseMediaRouterDialog() returns.
+    dialog_controller_->CloseMediaRouterDialog();
+  }
+
+  MediaRouterDialogController* const dialog_controller_;
+};
+
+MediaRouterDialogController::MediaRouterDialogController(
+    content::WebContents* initiator)
+    : initiator_(initiator) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(initiator_);
+}
+
+MediaRouterDialogController::~MediaRouterDialogController() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+void MediaRouterDialogController::ActivateInitiatorWebContents() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  initiator_->GetDelegate()->ActivateContents(initiator_);
+}
+
+bool MediaRouterDialogController::ShowMediaRouterDialogForPresentation(
+    scoped_ptr<CreatePresentationSessionRequest> request) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Check if the media router dialog exists for |initiator| and return if so.
+  if (IsShowingMediaRouterDialog())
+    return false;
+
+  TakePresentationRequest(request.Pass());
+  CreateMediaRouterDialog();
+
+  // Show the initiator holding the existing media router dialog.
+  ActivateInitiatorWebContents();
+
+  return true;
+}
+
+void MediaRouterDialogController::ShowMediaRouterDialog() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Create a new dialog if it doesn't exist yet.
+  if (!IsShowingMediaRouterDialog())
+    CreateMediaRouterDialog();
+
+  // Show the initiator holding the existing media router dialog.
+  ActivateInitiatorWebContents();
+}
+
+void MediaRouterDialogController::CloseMediaRouterDialog() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(initiator_observer_.get());

[mark a. foltz, 2015/07/24 20:10:34]

Why does the state of the observer matter for closing the dialog?

I think Close() and Reset() should be no-ops if the dialog is not open.

[whywhat, 2015/07/28 18:52:53]

I merely moved the code that was already there. I guess the checks were in place
to underline/ensure that there's no redundant calls to Reset and Close. Note
that Close in the desktop implementation had a strong CHECK statement...

Done.

+  Reset();
+}
+
+void MediaRouterDialogController::ObserveInitiatorWebContents() {

[mark a. foltz, 2015/07/24 20:10:34]

Shouldn't this be called when the dialog is created to guarantee
|initiator_observer_| is set?  Some things in this API will break if it's not
done.  For example it's not not called in the Android impl.

[whywhat, 2015/07/28 18:52:53]

My guess that it was done to avoid redundant Close calls. Done.

+  DCHECK(thread_checker_.CalledOnValidThread());

[mark a. foltz, 2015/07/24 20:10:34]

Generally I don't bother DCHECKING on non-public methods, as there's no easy way
for a caller to post a task to a private/protected method on the wrong thread. 
(Unless the implementation is doing its own thread hopping which is not the case
here)

[whywhat, 2015/07/28 18:52:53]

Done.

+  DCHECK(!initiator_observer_.get());
+  initiator_observer_.reset(new InitiatorWebContentsObserver(
+      initiator_, this));
+}
+
+void MediaRouterDialogController::TakePresentationRequest(

[mark a. foltz, 2015/07/24 20:10:34]

I think this method can be inlined.  It's a one-liner called once.

[whywhat, 2015/07/28 18:52:53]

Done.

+    scoped_ptr<CreatePresentationSessionRequest> request) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  presentation_request_ = request.Pass();
+}
+
+scoped_ptr<CreatePresentationSessionRequest>
+MediaRouterDialogController::PassPresentationRequest() {

[mark a. foltz, 2015/07/24 20:10:34]

This is a one-liner called once - consider inlining it.

[whywhat, 2015/07/28 18:52:53]

I think this will be used by the Android implementation therefore I'll leave it.
Also inlining it would mean making the request a protected member.

+  DCHECK(thread_checker_.CalledOnValidThread());
+  return presentation_request_.Pass();
+}
+
+void MediaRouterDialogController::Reset() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(initiator_observer_.get());
+  initiator_observer_.reset();
+  presentation_request_.reset();
+}
+
 }  // namespace media_router