[MediaRouter] Moved the code useful for Android to MediaRouterDialogController

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

Index: chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc b/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
index 3129e65a3565aa319c89187763d50923a482f2fd..487a7650af3c1ad9a6852316ebef95a2b20f1661 100644
--- a/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
+++ b/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
@@ -20,8 +20,6 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
-#include "content/public/browser/web_contents.h"
-#include "content/public/browser/web_contents_delegate.h"
 #include "ui/web_dialogs/web_dialog_delegate.h"
 #include "ui/web_dialogs/web_dialog_web_contents_delegate.h"
 #include "url/gurl.h"
@@ -150,41 +148,10 @@ class MediaRouterDialogControllerImpl::DialogWebContentsObserver
   MediaRouterDialogControllerImpl* const dialog_controller_;
 };
 
-class MediaRouterDialogControllerImpl::InitiatorWebContentsObserver
-    : public content::WebContentsObserver {
- public:
-  InitiatorWebContentsObserver(
-      WebContents* web_contents,
-      MediaRouterDialogControllerImpl* dialog_controller)
-      : content::WebContentsObserver(web_contents),
-        dialog_controller_(dialog_controller) {
-  }
-
- private:
-  void WebContentsDestroyed() override {
-    // NOTE: |this| is deleted after CloseMediaRouterDialog() returns.
-    dialog_controller_->CloseMediaRouterDialog();
-  }
-
-  void NavigationEntryCommitted(const 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();
-  }
-
-  MediaRouterDialogControllerImpl* const dialog_controller_;
-};
-
 MediaRouterDialogControllerImpl::MediaRouterDialogControllerImpl(
     WebContents* web_contents)
-    : initiator_(web_contents),
+    : MediaRouterDialogController(web_contents),
       media_router_dialog_pending_(false) {
-  DCHECK(initiator_);
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 }
 
@@ -204,7 +171,7 @@ WebContents* MediaRouterDialogControllerImpl::ShowMediaRouterDialog() {
   }
 
   // Show the initiator holding the existing media router dialog.
-  initiator_->GetDelegate()->ActivateContents(initiator_);
+  ActivateInitiatorWebContents();
   return media_router_dialog;
 }
 
@@ -218,12 +185,12 @@ bool MediaRouterDialogControllerImpl::ShowMediaRouterDialogForPresentation(
   if (!media_router_dialog) {
     CreateMediaRouterDialog();

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

I think there might have been a bad merge:

1) SetPresentationRequest should occur before CreateMediaRouterDialog just to be
safe
2) The variable |media_router_dialog| is not needed
3) We don't call ActivateInitiatorWebContents() in case of false.

In addition, can the definition of this method be moved to the base class?

[whywhat, 2015/07/23 21:20:09]

Bad merge when? This matches the code on trunk as far as I can see.
Changed according to your notes and moved to the base class.

     media_router_dialog = GetMediaRouterDialog();
-    presentation_request_ = request.Pass();
+    SetPresentationRequest(request.Pass());
     return true;
   }
 
   // Show the initiator holding the existing media router dialog.
-  initiator_->GetDelegate()->ActivateContents(initiator_);
+  ActivateInitiatorWebContents();
   return false;
 }
 
@@ -234,11 +201,8 @@ WebContents* MediaRouterDialogControllerImpl::GetMediaRouterDialog() const {
 
 void MediaRouterDialogControllerImpl::CloseMediaRouterDialog() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(initiator_observer_.get());
   WebContents* media_router_dialog = GetMediaRouterDialog();
   CHECK(media_router_dialog);
-  Reset();
-
   content::WebUI* web_ui = media_router_dialog->GetWebUI();
   if (web_ui) {
     MediaRouterUI* media_router_ui =
@@ -246,14 +210,14 @@ void MediaRouterDialogControllerImpl::CloseMediaRouterDialog() {
     if (media_router_ui)
       media_router_ui->Close();
   }
+  MediaRouterDialogController::CloseMediaRouterDialog();

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

this should be placed above the web_ui above to preserve the call order.

[whywhat, 2015/07/23 21:20:09]

I guess, although it becomes a bit fragile:
1. The base class call can't be in the beginning because it calls Reset() and
deletes the dialog_observer_, so it has to be after we get the dialog pointer.
2. Once we got the dialog pointer, we can call the base class implementation
which will |Reset| the state but the dialog pointer will still be valid?
3. Calling it in the end doesn't seem to have side effects but is more natural -
cleanup the derived class and call the base class method.

 }
 
 void MediaRouterDialogControllerImpl::CreateMediaRouterDialog() {
-  DCHECK(!initiator_observer_.get());
   DCHECK(!dialog_observer_.get());
 
   Profile* profile =
-      Profile::FromBrowserContext(initiator_->GetBrowserContext());
+      Profile::FromBrowserContext(initiator()->GetBrowserContext());

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

nit: suggest having a initiator local variable so we don't have to repeatedly
call the getter method.

[whywhat, 2015/07/23 21:20:09]

Since it's an inline getter, I don't think there's much difference.

[whywhat, 2015/07/23 21:20:09]

Since it's an inline getter method, I don't think it's worth it.

   DCHECK(profile);
 
   WebDialogDelegate* web_dialog_delegate = new MediaRouterDialogDelegate;
@@ -263,7 +227,7 @@ void MediaRouterDialogControllerImpl::CreateMediaRouterDialog() {
   // TODO(apacible): Remove after autoresizing is implemented for OSX.
 #if defined(OS_MACOSX)
   ConstrainedWebDialogDelegate* constrained_delegate =
-      ShowConstrainedWebDialog(profile, web_dialog_delegate, initiator_);
+      ShowConstrainedWebDialog(profile, web_dialog_delegate, initiator());
 #else
   // TODO(apacible): Adjust min and max sizes based on new WebUI design.
   gfx::Size min_size = gfx::Size(kWidth, kMinHeight);
@@ -277,26 +241,23 @@ void MediaRouterDialogControllerImpl::CreateMediaRouterDialog() {
   // on the currently rendered contents.
   ConstrainedWebDialogDelegate* constrained_delegate =
       ShowConstrainedWebDialogWithAutoResize(
-          profile, web_dialog_delegate, initiator_, min_size, max_size);
+          profile, web_dialog_delegate, initiator(), min_size, max_size);
 #endif
 
   WebContents* media_router_dialog = constrained_delegate->GetWebContents();
 
   media_router_dialog_pending_ = true;
 
-  initiator_observer_.reset(new InitiatorWebContentsObserver(
-      initiator_, this));
+  MediaRouterDialogController::CreateMediaRouterDialog();
   dialog_observer_.reset(new DialogWebContentsObserver(
       media_router_dialog, this));
 }
 
 void MediaRouterDialogControllerImpl::Reset() {
+  MediaRouterDialogController::Reset();

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

this should be after the DCHECKs

[whywhat, 2015/07/23 21:20:09]

Done.

   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(initiator_observer_.get());
   DCHECK(dialog_observer_.get());
-  initiator_observer_.reset();
   dialog_observer_.reset();
-  presentation_request_.reset();
 }
 
 void MediaRouterDialogControllerImpl::OnDialogNavigated(
@@ -322,14 +283,8 @@ void MediaRouterDialogControllerImpl::OnDialogNavigated(
 void MediaRouterDialogControllerImpl::PopulateDialog(
     content::WebContents* media_router_dialog) {
   DCHECK(media_router_dialog);
-  DCHECK(initiator_observer_);
-  if (!initiator_observer_) {
-    Reset();
-    return;
-  }
-  content::WebContents* initiator = initiator_observer_->web_contents();
-  DCHECK(initiator);
-  if (!initiator || !media_router_dialog->GetWebUI()) {
+  DCHECK(initiator());

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

nit: suggest having a initiator local variable so we don't have to repeatedly
call the getter method.

[whywhat, 2015/07/23 21:20:09]

Same as above.

+  if (!initiator() || !media_router_dialog->GetWebUI()) {
     Reset();
     return;
   }
@@ -342,16 +297,18 @@ void MediaRouterDialogControllerImpl::PopulateDialog(
     return;
   }
 
-  if (!presentation_request_.get()) {
+  scoped_ptr<CreatePresentationSessionRequest> presentation_request(
+      PassPresentationRequest());
+  if (!presentation_request.get()) {
     // TODO(imcheng): Don't create PresentationServiceDelegateImpl if it doesn't
     // exist (crbug.com/508695).
     base::WeakPtr<PresentationServiceDelegateImpl> delegate =
-        PresentationServiceDelegateImpl::GetOrCreateForWebContents(initiator)
+        PresentationServiceDelegateImpl::GetOrCreateForWebContents(initiator())
             ->GetWeakPtr();
     media_router_ui->InitWithDefaultMediaSource(delegate);
   } else {
     media_router_ui->InitWithPresentationSessionRequest(
-        initiator, presentation_request_.Pass());
+        initiator(), presentation_request.Pass());
   }
 }