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..777da13840a531005cc860db2eea4b867dc32d9c 100644
--- a/chrome/browser/ui/toolbar/media_router_action.h
+++ b/chrome/browser/ui/toolbar/media_router_action.h
@@ -5,8 +5,11 @@
 #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 MediaRouterActionPlatformDelegate;
@@ -15,9 +18,27 @@ namespace media_router {
 class MediaRouterDialogController;
 }  // namespace media_router
 
+// The current state of the MediaRouterAction.
+enum MediaRouterActionState {

[Peter Kasting, 2015/08/03 21:28:51]

Why not make this public inside MediaRouterAction (so MediaRouterAction::State)?
 That would scope all these to that class, meaning you could remove the
MEDIA_ROUTER_ACTION_ from each, and wouldn't seem to restrict the kind of usage
you're doing in this CL AFAICT.

[apacible, 2015/08/05 06:47:55]

Acknowledged; ended up removing the enum.

+  // 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 {
+class MediaRouterAction : public ToolbarActionViewController,
+                          public media_router::IssuesObserver,
+                          public media_router::MediaRoutesObserver {
  public:
   explicit MediaRouterAction(Browser* browser);
   ~MediaRouterAction() override;
@@ -43,17 +64,53 @@ class MediaRouterAction : public ToolbarActionViewController {
   void UpdateState() override;
   bool DisabledClickOpensMenu() const override;
 
+  // media_router::IssuesObserver:
+  void OnIssueUpdated(const media_router::Issue* issue) override;
+
+  // media_router::MediaRoutesObserver:
+  void OnRoutesUpdated(const std::vector<media_router::MediaRoute>& routes)
+      override;
+
  private:
+  friend class TestMediaRouterAction;
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, Initialization);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateIssues);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateRoutes);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateIssuesAndRoutes);

[Peter Kasting, 2015/08/03 21:28:51]

I think in this case these friend declarations signal that parts of the tests
are too low-level.  They look at things like the |state_| value directly when
ideally they would look at externally-visible data (e.g. the result of
GetIcon()).  This means that if someone modifies the implementation of the class
in a way that doesn't affect the externally-visible behavior, they still might
break all the tests and have to fix.

In many cases, the tests access members that have direct accessors, e.g. |id_|
instead of GetId(), or |name_| instead of GetActionName().  In other cases, like
the |state_| case mentioned above, perhaps the tests should be looking at
something other than the actual underlying |state_|.

If these sorts of issues are fixed correctly, it will hopefully be possible to
remove most or all of the friend and FRIEND_TEST declarations here as well as in
the unittest code.

[apacible, 2015/08/05 06:47:55]

Thanks for bringing this up; from the changes (based on this and other
comments), I ended up removing all the friend class/test related declarations.

+
   // 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.

[Peter Kasting, 2015/08/03 21:28:51]

Nit: "Overridden by tests." would probably be clearer, since the test code
overrides this, it doesn't call it

[apacible, 2015/08/05 06:47:55]

Done.

+  virtual media_router::MediaRouter* GetMediaRouter(Browser* browser);
+
+  // Checks if the currents state of MediaRouterAction has changed. Updates
+  // |state_|. If |state_| has changed, update |state_| and then update

[Peter Kasting, 2015/08/03 21:28:51]

Nit: update -> updates (2x)

[apacible, 2015/08/05 06:47:55]

Done.

+  // MediaRouterAction's icon.

[Peter Kasting, 2015/08/03 21:28:51]

This updates |state_|, not just the icon.  So either it should be called
MaybeUpdateState(), or perhaps we should eliminate the concept of the "current
state" entirely and deal directly in what the "current icon" is: replace
|state_| with a gfx::Image* that points at one of the member icons, and replace
GetMediaRouterActionState() with something like GetCurrentIcon() that returns
the correct pointer.  Then the implementation of the public GetIcon() method
becomes trivial, and the way to update test code which looks at the state
becomes obvious (it should look at the result of GetIcon() instead).

[apacible, 2015/08/05 06:47:55]

Switched to keeping track of the current icon rather than a state. We won't be
using the state for anything else.

+  void MaybeUpdateIcon();
+
+  // Called when |issue_| or |local_active_route_exists_| may have changed.

[Peter Kasting, 2015/08/03 21:28:51]

This comment is confusing, since it's not really part of the function's
contract, and it actually better describes when people call MaybeUpdateIcon()
(which in turn calls this unconditionally).

Either describe what this function does instead, or eliminate the comment.

[apacible, 2015/08/05 06:47:55]

Done.

+  MediaRouterActionState GetMediaRouterActionState();
+
   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.

[Peter Kasting, 2015/08/03 21:28:51]

This doesn't actually say what |issue_| is, how it's set, etc.

[apacible, 2015/08/05 06:47:55]

Done.

+  scoped_ptr<media_router::Issue> issue_;
+
+  // Whether there exists a local active route.
+  bool has_local_routes_;
 
   ToolbarActionViewDelegate* delegate_;