[Media Router] Clean up issues related code.

chrome/browser/media/router/issue.h

Index: chrome/browser/media/router/issue.h
diff --git a/chrome/browser/media/router/issue.h b/chrome/browser/media/router/issue.h
index 16744f502cd9c5488de58a503218dac40efcfcee..58ec87f274a646e4342a0014bfe48e070d28d78e 100644
--- a/chrome/browser/media/router/issue.h
+++ b/chrome/browser/media/router/issue.h
@@ -13,83 +13,97 @@
 
 namespace media_router {
 
-// The text that corresponds with an action a user can take for a Issue.
-class IssueAction {
- public:
-  enum Type {
-    TYPE_DISMISS,
-    TYPE_LEARN_MORE,
-    TYPE_MAX /* Denotes enum value boundary. */
-  };
-
-  explicit IssueAction(const Type type);
-  ~IssueAction();
-
-  Type type() const { return type_; }
-
-  bool Equals(const IssueAction& other) const { return type_ == other.type_; }
-
- private:
-  Type type_;
-};
-
 // Contains the information relevant to an issue.
 class Issue {
  public:
-  using Id = std::string;
+  // TODO(imcheng): The ID field should really be owned by IssueManager.
+  using Id = int;
+
+  // Possible actions for an Issue.
+  enum class ActionType {

[mark a. foltz, 2016/07/27 18:52:03]

Or just Action, so Issue::Action::DISMISS, etc.

[imcheng, 2016/09/13 20:27:54]

Done.

+    DISMISS,
+    // NOTE: If LEARN_MORE is set as a possible action for an Issue, then its
+    // |help_page_id_| must also be set to a valid value.
+    LEARN_MORE,
+    MAX /* Denotes enum value boundary. */

[mark a. foltz, 2016/07/27 18:52:03]

NUM_VALUES?

Also, add a comment that new Actions should be added above here.

[imcheng, 2016/09/13 20:27:54]

Done.

+  };
 
-  enum Severity { FATAL, WARNING, NOTIFICATION };
+  // Severity type of an Issue. A FATAL issue is considered blocking, although
+  // issues of lower severity may also be blocking.
+  enum class Severity { FATAL, WARNING, NOTIFICATION };
 
-  // Creates a custom issue.
+  // Creates a new issue. A new ID is assigned to the issue created with this
+  // constructor.
   // |title|: The title for the issue.
-  // |message|: The optional description message for the issue.
   // |default_action|: Default action user can take to resolve the issue.
-  // |secondary_actions|: Array of options user can take to resolve the
-  //                      issue. Can be empty. Currently only one secondary
-  //                      action is supported.
-  // |route_id|: The route id, or empty if global.
   // |severity|: The severity of the issue.
-  // |is_blocking|: True if the issue needs to be resolved before continuing.
-  // |help_page_id|: Required if one of the actions is learn-more. Passes in -1
-  //                 if the issue is not associated with a help page.
   Issue(const std::string& title,
-        const std::string& message,
-        const IssueAction& default_action,
-        const std::vector<IssueAction>& secondary_actions,
-        const MediaRoute::Id& route_id,
-        const Severity severity,
-        bool is_blocking,
-        int help_page_id);
-
+        const ActionType& default_action,
+        Severity severity);
   Issue(const Issue& other);

[mark a. foltz, 2016/07/27 18:52:03]

Usually if an object implements the copy constructor it also implements
assignment (but see comments about == below)

[imcheng, 2016/09/13 20:27:54]

My new design makes breaks Issue into ID and data. The data part is a struct
that is safe to support copy assignment/construction.

 
   ~Issue();
 
+  // Setters for optional arguments.

[mark a. foltz, 2016/07/27 18:52:03]

All of the getters or setters are trivial; this could be a struct with public
members which would make all of this code unnecessary.

[imcheng, 2016/09/13 20:27:54]

Right. What I did in the latest PS is to take out ID, then turn this into a
struct (renamed IssueInfo) that supports copy.

+  void set_message(const std::string& message) { message_ = message; }
+  void set_secondary_actions(const std::vector<ActionType>& secondary_actions) {
+    secondary_actions_ = secondary_actions;
+  }
+  void set_route_id(const MediaRoute::Id& route_id) { route_id_ = route_id; }
+  void set_is_blocking(bool is_blocking) { is_blocking_ = is_blocking; }
+  void set_help_page_id(int help_page_id) { help_page_id_ = help_page_id; }
+
   // See constructor comments for more information about these fields.
   const std::string& title() const { return title_; }
   const std::string& message() const { return message_; }
-  IssueAction default_action() const { return default_action_; }
-  const std::vector<IssueAction>& secondary_actions() const {
+  ActionType default_action() const { return default_action_; }
+  const std::vector<ActionType>& secondary_actions() const {
     return secondary_actions_;
   }
   MediaRoute::Id route_id() const { return route_id_; }
   Severity severity() const { return severity_; }
   const Issue::Id& id() const { return id_; }
-  bool is_blocking() const { return is_blocking_; }
-  bool is_global() const { return route_id_.empty(); }
+
   int help_page_id() const { return help_page_id_; }
 
-  bool Equals(const Issue& other) const;
+  bool operator==(const Issue& other) const { return id_ == other.id_; }

[mark a. foltz, 2016/07/27 18:52:03]

I think this is an okay definition of equals as long as you only pass an Issue
as const out of the issue manager (and document that restriction).  Otherwise
the copy constructor will generate two mutable copies of an Issue with the same
id and bugs could ensue.

A safer approach would be to declare this as DISALLOW_COPY_AND_ASSIGN which
should prevent this situation.

[imcheng, 2016/09/13 20:27:54]

The ID is tripping things up here and doesn't play well with copy
assignment/construction. So now I have IssueManager wrap a IssueInfo with a
generated ID to create an Issue.

I also removed the Issue matching logic in IssueManager since it is not very
useful. If we really need deduping (which we don't currently) then we can add in
some logic to match IssueInfo.

+  bool operator!=(const Issue& other) const { return id_ != other.id_; }
+
+  // Returns |true| if the issue needs to be resolved before continuing.
+  bool IsBlocking() const;

[mark a. foltz, 2016/07/27 18:52:03]

This shoudn't be necessary if is_blocking is initialized correctly in the
constructor.

[imcheng, 2016/09/13 20:27:54]

I now set is_blocking to true in the constructor if severity is FATAL. Other
than that, I am treating it as a "dumb" struct without validation logic.

+
+  // Returns |true| if the issue is not associated with a route.
+  bool IsGlobal() const;
 
  private:
+  // ID assigned by the normal constructor.
+  Id id_;
+
+  // Required fields.
   std::string title_;
+  ActionType default_action_;
+  Severity severity_;
+
+  // Optional fields.

[mark a. foltz, 2016/07/27 18:52:03]

I don't think this comment makes sense in context. 

- The message_ is noted as required below
- Other fields have defaults

[imcheng, 2016/09/13 20:27:54]

Removed the comments.

+  // Description message for the issue. Required.
   std::string message_;
-  IssueAction default_action_;
-  std::vector<IssueAction> secondary_actions_;
+
+  // Array of options the user can take to resolve the issue in addition to the
+  // default action. Can be empty. If non-empty, currently only one secondary
+  // action is supported.
+  std::vector<ActionType> secondary_actions_;
+
+  // ID of route associated with the Issue, or empty if no route is associated
+  // with it.
   std::string route_id_;
-  Severity severity_;
-  Issue::Id id_;
+
+  // |true| if the issue needs to be resolved before continuing. Note that an
+  // Issue is considered blocking if its severity is FATAL, even if
+  // |is_blocking_| is |false|. Defaults to |false|.
   bool is_blocking_;
+
+  // ID of help page to link to, if one of the actions is LEARN_MORE. -1
+  // Otherwise. Defaults to -1.

[mark a. foltz, 2016/07/27 18:52:03]

Declare constant for kUnknownHelpPageId?

[imcheng, 2016/09/13 20:27:54]

Done.

   int help_page_id_;
 };