[MediaRouter] Moved the code useful for Android to MediaRouterDialogController

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h"
 
 #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"
 #include "chrome/common/url_constants.h"
 #include "components/web_modal/web_contents_modal_dialog_host.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #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"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(
     media_router::MediaRouterDialogControllerImpl);
 
 using content::LoadCommittedDetails;
 using content::NavigationController;
 using content::WebContents;
@@ skipping 108 matching lines @@
   }
 
   void RenderProcessGone(base::TerminationStatus status) override {
     // NOTE: |this| is deleted after CloseMediaRouterDialog() returns.
     dialog_controller_->CloseMediaRouterDialog();
   }
 
   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));
 }
 
 MediaRouterDialogControllerImpl::~MediaRouterDialogControllerImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 WebContents* MediaRouterDialogControllerImpl::ShowMediaRouterDialog() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Get the media router dialog for |initiator|, or create a new dialog
   // if not found.
   WebContents* media_router_dialog = GetMediaRouterDialog();
   if (!media_router_dialog) {
     CreateMediaRouterDialog();
     return GetMediaRouterDialog();
   }
 
   // Show the initiator holding the existing media router dialog.
-  initiator_->GetDelegate()->ActivateContents(initiator_);
+  ActivateInitiatorWebContents();
   return media_router_dialog;
 }
 
 bool MediaRouterDialogControllerImpl::ShowMediaRouterDialogForPresentation(
     scoped_ptr<CreatePresentationSessionRequest> request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Get the media router dialog for |initiator|, or create a new dialog
   // if not found.
   WebContents* media_router_dialog = GetMediaRouterDialog();
   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;
 }
 
 WebContents* MediaRouterDialogControllerImpl::GetMediaRouterDialog() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return dialog_observer_.get() ? dialog_observer_->web_contents() : nullptr;
 }
 
 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 =
         static_cast<MediaRouterUI*>(web_ui->GetController());
     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;
 
   // |web_dialog_delegate|'s owner is |constrained_delegate|.
   // |constrained_delegate| is owned by the parent |views::View|.
   // 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);
   gfx::Size max_size = gfx::Size(kWidth, kMaxHeight);
 
   // |ShowConstrainedWebDialogWithAutoResize()| will end up creating
   // ConstrainedWebDialogDelegateViewViews containing a WebContents containing
   // the MediaRouterUI, using the provided |web_dialog_delegate|. Then, the
   // view is shown as a modal dialog constrained to the |initiator| WebContents.
   // The dialog will resize between the |min_size| and |max_size| bounds based
   // 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(
     const content::LoadCommittedDetails& details) {
   DCHECK(thread_checker_.CalledOnValidThread());
   WebContents* media_router_dialog = GetMediaRouterDialog();
   CHECK(media_router_dialog);
   ui::PageTransition transition_type = details.entry->GetTransitionType();
   content::NavigationType nav_type = details.type;
 
   // New |media_router_dialog| is created.
   DCHECK(media_router_dialog_pending_);
   DCHECK(transition_type == ui::PAGE_TRANSITION_AUTO_TOPLEVEL &&
       nav_type == content::NAVIGATION_TYPE_NEW_PAGE)
       << "transition_type: " << transition_type << ", "
       << "nav_type: " << nav_type;
 
   media_router_dialog_pending_ = false;
 
   PopulateDialog(media_router_dialog);
 }
 
 void MediaRouterDialogControllerImpl::PopulateDialog(
     content::WebContents* media_router_dialog) {
   DCHECK(media_router_dialog);
-  DCHECK(initiator_observer_);
-  if (!initiator_observer_) {
+  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;
   }
-  content::WebContents* initiator = initiator_observer_->web_contents();
-  DCHECK(initiator);
-  if (!initiator || !media_router_dialog->GetWebUI()) {
-    Reset();
-    return;
-  }
 
   MediaRouterUI* media_router_ui = static_cast<MediaRouterUI*>(
       media_router_dialog->GetWebUI()->GetController());
   DCHECK(media_router_ui);
   if (!media_router_ui) {
     Reset();
     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());
   }
 }
 
 }  // namespace media_router