[Media Router] Custom Controls 3 - add plumbing through MRUI and MRWebUIMessageHandler

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

Index: chrome/browser/ui/webui/media_router/media_router_ui.h
diff --git a/chrome/browser/ui/webui/media_router/media_router_ui.h b/chrome/browser/ui/webui/media_router/media_router_ui.h
index 2fb4a2c4fd65e0b547ed4290ce86f81b0f1676c6..699b4031948bbdc76d6461dc4a42e3e4dafb786d 100644
--- a/chrome/browser/ui/webui/media_router/media_router_ui.h
+++ b/chrome/browser/ui/webui/media_router/media_router_ui.h
@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
+#include "chrome/browser/media/router/mojo/media_route_controller.h"
 #include "chrome/browser/media/router/presentation_service_delegate_impl.h"
 #include "chrome/browser/ui/webui/constrained_web_dialog_ui.h"
 #include "chrome/browser/ui/webui/media_router/media_cast_mode.h"
@@ -164,6 +165,22 @@ class MediaRouterUI : public ConstrainedWebDialogUI,
 
   void UpdateMaxDialogHeight(int height);
 
+  // Sends media commands to |route_controller_| if it is set. Marked virtual
+  // for tests.
+  virtual void PlayRoute();

[mark a. foltz, 2017/04/26 21:01:18]

This doesn't seem like quite the right API - you're not playing or pausing the
UI.  It also duplicates what is already defined in MediaRouteController.

Can the MediaRouterUI return a constant reference to the MediaRouteController so
its API can be used directly in the handler?

Or can these be rephrased in terms of what is being controlled:
PlayCurrentMedia(), etc.?

[takumif, 2017/04/27 03:04:17]

Making MessageHandler call the controller methods directly.

+  virtual void PauseRoute();
+  virtual void SeekRoute(base::TimeDelta time);
+  virtual void SetRouteMute(bool mute);
+  virtual void SetRouteVolume(float volume);
+
+  // Gets a reference to the MediaRouteController for |route_id| and

[mark a. foltz, 2017/04/26 21:01:18]

Does this need a return value?

[takumif, 2017/04/27 03:04:17]

Maybe the part about obtaining a reference shouldn't be mentioned since it's
more of an implementation detail. Changing the comment.

+  // instantiates an observer for media status updates for the route. Marked
+  // virtual for tests.

[mark a. foltz, 2017/04/26 21:01:18]

I'm not sure the "Marked virtual for tests" comments add a lot.  My thoughts:

- If it's marked virtual the assumption is that it is or can be overridden, in
tests or elsewhere.
- This isn't performance sensitive code so we don't need to justify the expense
of a vtable lookup.

I do think it's important to note public or protected APIs that have that
visibility because of tests.

[takumif, 2017/04/27 03:04:17]

Okay, makes sense. removed.

+  virtual void OnUIDetailsViewOpened(const MediaRoute::Id& route_id);

[mark a. foltz, 2017/04/26 21:01:18]

This name seems specific to the UI configuration (and not the purpose of the
method).
Perhaps OnMediaControllerUIAvailable() and OnMediaControllerUIClosed()?

[takumif, 2017/04/27 03:04:17]

Done.

+  // Resets |route_controller_| and |route_status_observer_|. Marked virtual for
+  // tests.
+  virtual void OnUIDetailsViewClosed();
+
   void InitForTest(MediaRouter* router,
                    content::WebContents* initiator,
                    MediaRouterWebUIMessageHandler* handler,
@@ -171,6 +188,8 @@ class MediaRouterUI : public ConstrainedWebDialogUI,
                        create_session_request);
 
  private:
+  friend class MediaRouterUITest;
+
   FRIEND_TEST_ALL_PREFIXES(MediaRouterUITest, SortedSinks);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterUITest, SortSinksByIconType);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterUITest, FilterNonDisplayRoutes);
@@ -187,6 +206,10 @@ class MediaRouterUI : public ConstrainedWebDialogUI,
   FRIEND_TEST_ALL_PREFIXES(MediaRouterUITest,
                            RouteCreationTimeoutForPresentation);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterUITest, RouteRequestFromIncognito);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterUITest, OpenAndCloseUIDetailsView);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterUITest, SendMediaCommands);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterUITest, SendMediaStatusUpdate);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterUITest, IsValidRouteId);
 
   class UIIssuesObserver;
 
@@ -212,6 +235,28 @@ class MediaRouterUI : public ConstrainedWebDialogUI,
     DISALLOW_COPY_AND_ASSIGN(UIMediaRoutesObserver);
   };
 
+  class UIMediaRouteControllerObserver : public MediaRouteController::Observer {
+   public:
+    explicit UIMediaRouteControllerObserver(
+        MediaRouterUI* ui,
+        scoped_refptr<MediaRouteController> controller);
+    ~UIMediaRouteControllerObserver() override;
+
+    // MediaRouteController::Observer
+    void OnMediaStatusUpdated(const MediaStatus& status) override;
+    void OnControllerInvalidated() override;
+
+    // Gets the current MediaStatus. Returns a nullptr if there has not been any
+    // MediaStatus updates.
+    const MediaStatus* GetCurrentMediaStatus() const;
+
+   private:
+    MediaRouterUI* ui_;
+    base::Optional<MediaStatus> current_media_status_;

[mark a. foltz, 2017/04/26 21:01:18]

This would be simpler as a base::unique_ptr<>.

[takumif, 2017/04/27 03:04:17]

Removing, since we'll be calling the controller methods from the MessageHandler
directly.

+
+    DISALLOW_COPY_AND_ASSIGN(UIMediaRouteControllerObserver);
+  };
+
   static std::string GetExtensionName(const GURL& url,
                                       extensions::ExtensionRegistry* registry);
 
@@ -291,6 +336,13 @@ class MediaRouterUI : public ConstrainedWebDialogUI,
   // opaque origin ("null") if |initiator_| is not set.
   std::string GetSerializedInitiatorOrigin() const;
 
+  // Returns true if |routes_| contains a route with |route_id|.
+  bool IsValidRouteId(const MediaRoute::Id& route_id) const;
+
+  // Destroys the route controller observer. Called when the route controller is
+  // invalidated.
+  void OnRouteControllerInvalidated();
+
   // Owned by the |web_ui| passed in the ctor, and guaranteed to be deleted
   // only after it has deleted |this|.
   MediaRouterWebUIMessageHandler* handler_ = nullptr;
@@ -348,6 +400,10 @@ class MediaRouterUI : public ConstrainedWebDialogUI,
   // been painted and initialized with initial data, this should be cleared.
   base::Time start_time_;
 
+  // The observer for the route controller. Notifies |handler_| of media status
+  // updates.
+  std::unique_ptr<UIMediaRouteControllerObserver> route_controller_observer_;
+
   // NOTE: Weak pointers must be invalidated before all other member variables.
   // Therefore |weak_factory_| must be placed at the end.
   base::WeakPtrFactory<MediaRouterUI> weak_factory_;