Reflect the current state in Media Router Action icon.

chrome/browser/ui/toolbar/media_router_action.h

Index: chrome/browser/ui/toolbar/media_router_action.h
diff --git a/chrome/browser/ui/toolbar/media_router_action.h b/chrome/browser/ui/toolbar/media_router_action.h
index 6cc6073cf8ca7248525faab2f2efbcf26108f187..8242742444bb21961589ff8c064837f6d61a06ee 100644
--- a/chrome/browser/ui/toolbar/media_router_action.h
+++ b/chrome/browser/ui/toolbar/media_router_action.h
@@ -5,16 +5,73 @@
 #ifndef CHROME_BROWSER_UI_TOOLBAR_MEDIA_ROUTER_ACTION_H_
 #define CHROME_BROWSER_UI_TOOLBAR_MEDIA_ROUTER_ACTION_H_
 
+#include "chrome/browser/media/router/issues_observer.h"
+#include "chrome/browser/media/router/media_routes_observer.h"
 #include "chrome/browser/ui/toolbar/media_router_contextual_menu.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
+#include "extensions/browser/process_manager.h"
 
 class Browser;
+class MediaRouterAction;
 class MediaRouterActionPlatformDelegate;
 
 namespace media_router {
 class MediaRouterDialogController;
+class Issue;

[Kevin M, 2015/07/31 18:18:40]

Sorting

[apacible, 2015/08/01 02:05:29]

Done.

+class MediaRoute;
+class MediaRouter;
 }  // namespace media_router
 
+// Observes when there is an updated issue and passes it to |action_|.

[Kevin M, 2015/07/31 18:18:39]

You can create a single Observer class that implements IssuesObserver and
MediaRoutesObserver.

Is there a reason why the MediaRouterAction can't also be an observer? The
instances are bound one-to-one with an Action object. It would simplify test
code, because you could just trigger the Observer methods on the Action
directly.

[apacible, 2015/08/01 02:05:29]

Switching to single Observer class.

We don't always have a MediaRouter* we can get within the entire lifetime of
MediaRouterAction, but both IssuesObserver and MediaRoutesObserver require
MediaRouter* to exist.

+class ActionIssuesObserver : public media_router::IssuesObserver {
+ public:
+  ActionIssuesObserver(media_router::MediaRouter* router,
+                       MediaRouterAction* action);
+  ~ActionIssuesObserver() override;
+
+  // media_router::IssuesObserver:
+  void OnIssueUpdated(const media_router::Issue* issue) override;
+
+ private:
+  MediaRouterAction* action_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActionIssuesObserver);
+};
+
+// Observes when there is an updated list of routes and passes them to
+// |action_|.
+class ActionMediaRoutesObserver : public media_router::MediaRoutesObserver {
+ public:
+  ActionMediaRoutesObserver(media_router::MediaRouter* router,
+                            MediaRouterAction* action);
+  ~ActionMediaRoutesObserver() override;
+
+  // media_router::MediaRoutesObserver:
+  void OnRoutesUpdated(const std::vector<media_router::MediaRoute>& routes)
+      override;
+
+ private:
+  MediaRouterAction* action_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActionMediaRoutesObserver);
+};
+
+// The current state of the MediaRouterAction.
+enum MediaRouterActionState {
+  // Indicates that the current Chrome profile is not using any devices.
+  // Devices may or may not be available.
+  MEDIA_ROUTER_ACTION_IDLE,
+
+  // Indicates that the current Chrome profile is using at least one device.
+  MEDIA_ROUTER_ACTION_ACTIVE,
+
+  // Indicates a failure, e.g. session launch failure.
+  MEDIA_ROUTER_ACTION_ERROR,
+
+  // Indicates warning messages.
+  MEDIA_ROUTER_ACTION_WARNING,
+};
+
 // The class for the Media Router component action that will be shown in
 // the toolbar.
 class MediaRouterAction : public ToolbarActionViewController {
@@ -43,25 +100,62 @@ class MediaRouterAction : public ToolbarActionViewController {
   void UpdateState() override;
   bool DisabledClickOpensMenu() const override;
 
+  // Updates |issue_|. |issue| may be null.
+  void UpdateCurrentIssue(const media_router::Issue* issue);

[Kevin M, 2015/07/31 18:18:40]

I think "SetCurrentIssue" works better here, because this function does not
modify the issue object itself.

[apacible, 2015/08/01 02:05:29]

Done.

+
+  // Updates |local_active_route_exists_| if |routes| has an active local route.
+  // |routes| may be empty.
+  void UpdateCurrentRoutes(const std::vector<media_router::MediaRoute>& routes);

[Kevin M, 2015/07/31 18:18:39]

Suggestion: IMO it makes more sense for this to take a bool for
local_active_route_exists vs. taking the vector of routes - let the caller do
the computation.

[apacible, 2015/08/01 02:05:29]

Done.

+
  private:
   // Returns a reference to the MediaRouterDialogController associated with
   // |delegate_|'s current WebContents. Guaranteed to be non-null.
   // |delegate_| and its current WebContents must not be null.
   media_router::MediaRouterDialogController* GetMediaRouterDialogController();
 
+  // Marked virtual to use in tests.
+  virtual media_router::MediaRouter* GetMediaRouter();
+
+  // Checks if the currents state of MediaRouterAction has changed. Updates
+  // |state_|. If |state_| has changed, update |state_| and then update
+  // MediaRouterAction's icon.
+  void MaybeUpdateIcon();
+
+  // Called when |issue_| or |local_active_route_exists_| may have changed.
+  MediaRouterActionState GetUpdatedMediaRouterActionState();

[Kevin M, 2015/07/31 18:18:40]

No "updated"?

[apacible, 2015/08/01 02:05:29]

Done.

+
   const std::string id_;
   const base::string16 name_;
 
   // Cached icons.
+  gfx::Image media_router_active_icon_;
+  gfx::Image media_router_error_icon_;
   gfx::Image media_router_idle_icon_;
+  gfx::Image media_router_warning_icon_;
+
+  // Current state of the MediaRouterAction.
+  MediaRouterActionState state_;
+
+  // Used to determine current state of the MediaRouterAction.
+  scoped_ptr<const media_router::Issue> issue_;

[Kevin M, 2015/07/31 18:18:39]

Why have a const here? Callers will wrap this pointer with constness as needed.

[apacible, 2015/08/01 02:05:29]

Removed.

+
+  // Whether there exists a local active route.
+  bool local_active_route_exists_;
 
   ToolbarActionViewDelegate* delegate_;
+  scoped_ptr<media_router::IssuesObserver> issues_observer_;
+  scoped_ptr<media_router::MediaRoutesObserver> routes_observer_;
 
   // The delegate to handle platform-specific implementations.
   scoped_ptr<MediaRouterActionPlatformDelegate> platform_delegate_;
 
   MediaRouterContextualMenu contextual_menu_;
 
+  friend class TestMediaRouterAction;
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, Initialization);

[Kevin M, 2015/07/31 18:18:39]

These go at the top of the private: section

[apacible, 2015/08/01 02:05:29]

Done.

+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateIssues);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateRoutes);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateIssuesAndRoutes);
   DISALLOW_COPY_AND_ASSIGN(MediaRouterAction);
 };