[MediaRouter] Moved the code useful for Android to MediaRouterDialogController

chrome/browser/media/router/media_router_dialog_controller.h

 // 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.
 
 #ifndef CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTER_DIALOG_CONTROLLER_H_
 #define CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTER_DIALOG_CONTROLLER_H_
 
 #include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/threading/thread_checker.h"
 #include "chrome/browser/media/router/create_presentation_session_request.h"
+#include "content/public/browser/web_contents_observer.h"
 
 namespace content {
 class WebContents;
 }  // namespace content
 
 namespace media_router {
 
-// An implementation of this interface 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 Media Router dialog modal to the initiator WebContents.
+// An abstract base class for Media Router dialog controllers. Tied to a
+// WebContents known as the |initiator|, and is lazily created when a Media
+// Router dialog needs to be shown. The MediaRouterDialogController allows
+// showing and closing a Media Router dialog modal to the initiator WebContents.
 // This class is not thread safe and must be called on the UI thread.
 class MediaRouterDialogController {
  public:
-  virtual ~MediaRouterDialogController() = default;
+  virtual ~MediaRouterDialogController();
 
   // Gets a reference to the MediaRouterDialogController associated with
   // |web_contents|, creating one if it does not exist. The returned pointer is
   // guaranteed to be non-null.
   static MediaRouterDialogController* GetOrCreateForWebContents(

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

Does this need to be public or is it an implementation detail of
CreateMediaRouterDialog()?

       content::WebContents* web_contents);
 
   // Creates a Media Router modal dialog using the initiator and parameters
   // specified in |request|. If the dialog already exists, brings the dialog
   // to the front, but does not change the dialog with |request|.
   // Returns WebContents for the media router dialog if a dialog was created.
   // Otherwise returns false and |request| is deleted.
-  virtual bool ShowMediaRouterDialogForPresentation(
-      scoped_ptr<CreatePresentationSessionRequest> request) = 0;
+  bool ShowMediaRouterDialogForPresentation(
+      scoped_ptr<CreatePresentationSessionRequest> request);
+
+  // Closes the media router dialog. Must be called from the overrides.

[imcheng, 2015/07/24 22:09:28]

sounds like this should be moved to protected? Or how about making this a
non-virtual method, and in that method, invoke a pure virtual method (which
subclasses implement to perform specific closing logic?)

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

Done.

+  // It is an error to call this function if there is currently no dialog.
+  virtual void CloseMediaRouterDialog();
+
+  // Indicates if the media router dialog already exists.
+  virtual bool IsShowingMediaRouterDialog() = 0;

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

const?

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

Done.

+
+  // Creates a new media router dialog modal to |initiator_|.
+  virtual void CreateMediaRouterDialog() = 0;

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

Would a client ever want to create a dialog without showing it?
Perhaps this could be protected.

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

Done.

+
+  // Shows the media router dialog modal to the initiator WebContents.

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

(1) for consistency, "modal to |initiator_|."

(2) The documentation should clarify when this should be called vs.
ShowMediaRouterDialogForPresentation.

(3) Why does this return void and ShowMediaRouterDialogForPresentation return
bool?  Is this guaranteed to succeed?

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

Done.
I would like to know better :) I guess simple Show is called for mirroring?
bool in the ForPresentation flavor is used to indicate whether the dialog was
created when the method was called or not.
I added the same to the simple Show method.

+  // Creates the dialog if it did not exist prior to this call.
+  // If the dialog already exists, brings the dialog to the front.
+  virtual void ShowMediaRouterDialog();
+
+ protected:
+  // Use MediaRouterDialogController::GetOrCreateForWebContents() to create an

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

Or should the public API be ShowMediaRouterDialog{ForPresentation} ?

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

I believe there's some advantage to be able to cache the controller and then
call show/hide on it.
Since this change's original purpose was code reuse and separating Android and
desktop implementation I'd try to avoid major refactorings of the desktop code.

+  // instance.
+  explicit MediaRouterDialogController(content::WebContents* initiator);
+
+  void ActivateInitiatorWebContents();
+  void ObserveInitiatorWebContents();
+
+  scoped_ptr<CreatePresentationSessionRequest> PassPresentationRequest();
+  void TakePresentationRequest(
+      scoped_ptr<CreatePresentationSessionRequest> request);
+
+  content::WebContents* initiator() { return initiator_; }

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

Should this method be const?

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

If I can use const content::WebContents* everywhere? Will try.

+
+  // Resets the state of the controller. Must be called from the overrides.
+  virtual void Reset();

[imcheng, 2015/07/24 22:09:28]

similar comment to CloseMediaRouterDialog() above - might be a good idea to have
this as a nonvirtual, and call into a pure virtual method to do implementation
specific cleanups.

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

I don't think it's justified in the case of Reset and just introduces
boilerplate.

+
+  base::ThreadChecker thread_checker_;
+
+ private:
+  class InitiatorWebContentsObserver;
+
+  scoped_ptr<InitiatorWebContentsObserver> initiator_observer_;

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

It would be helpful to include a comment here since the type is fwd declared.

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

Done.

+  content::WebContents* const initiator_;
+
+  // Data for dialogs created under a Presentation API context.

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

created at the request of the Presentation API.

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

Done.

+  // Passed from the caller of ShowMediaRouterDialogForPresentation(), and

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

Passed from the caller via SMRDFP to the dialog when it is initialized.

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

Done.

+  // passed to the dialog when it is initialized.
+  scoped_ptr<CreatePresentationSessionRequest> presentation_request_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaRouterDialogController);
 };
 
 }  // namespace media_router
 
 #endif  // CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTER_DIALOG_CONTROLLER_H_