[Media Router] Clean up issues related code.

chrome/browser/media/router/issue.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_ISSUE_H_
 #define CHROME_BROWSER_MEDIA_ROUTER_ISSUE_H_
 
 #include <string>
 #include <vector>
 
 #include "base/logging.h"
+#include "base/optional.h"

[dcheng, 2016/12/07 18:58:02]

Nit: remove this header, since it's not actually used here

[imcheng, 2016/12/09 23:19:53]

Done.

 #include "chrome/browser/media/router/media_route.h"
 
 namespace media_router {
 
-// The text that corresponds with an action a user can take for a Issue.
-class IssueAction {
+// Contains the information relevant to an issue.
+struct IssueInfo {
  public:
-  enum Type {
-    TYPE_DISMISS,
-    TYPE_LEARN_MORE,
-    TYPE_MAX /* Denotes enum value boundary. */
+  // Possible actions for an issue.
+  enum class Action {
+    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,
+
+    // Denotes enum value boundary. New values should be added above.
+    NUM_VALUES
   };
 
-  explicit IssueAction(const Type type);
-  ~IssueAction();
+  // Severity type of an issue. A FATAL issue is considered blocking. Although
+  // issues of other severity levels may also be blocking.
+  enum class Severity { FATAL, WARNING, NOTIFICATION };
 
-  Type type() const { return type_; }
+  static const int kUnknownHelpPageId = 0;
 
-  bool Equals(const IssueAction& other) const { return type_ == other.type_; }
+  // Used by Mojo only.
+  IssueInfo();
+
+  // |title|: The title for the issue.
+  // |default_action|: Default action user can take to resolve the issue.
+  // |severity|: The severity of the issue. If FATAL, then |is_blocking| is set
+  // to |true|.
+  IssueInfo(const std::string& title,
+            const Action& default_action,

[dcheng, 2016/12/07 18:58:02]

Nit: pass by value is (usually) more efficient here, since Action is an enum

[imcheng, 2016/12/09 23:19:53]

Done.

+            Severity severity);
+  IssueInfo(const IssueInfo& other);
+  ~IssueInfo();
+
+  IssueInfo& operator=(const IssueInfo& other);
+  bool operator==(const IssueInfo& other) const;
+
+  // Fields set with values provided to the constructor.
+  std::string title;
+  Action default_action;
+  Severity severity;
+
+  // Description message for the issue. Defaults to empty string.
+  std::string message;
+
+  // 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<Action> secondary_actions;
+
+  // ID of route associated with the Issue, or empty if no route is associated
+  // with it.
+  std::string route_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;

[dcheng, 2016/12/07 18:58:02]

Why set this as a separate field? Shouldn't this just a getter which derives the
value from |severity|? It seems very easy for this to get out of sync with
|severity|

[imcheng, 2016/12/09 23:19:53]

We allow supporting blocking non-fatal issues by setting this boolean to true.
All fatal issues are blocking, however.

Ideally, I would like to remove the decouple the concept of is_blocking from
FATAL severity. is_blocking is just an indicator to the UI of whether the issue
should be shown before everything else. We aren't creating any FATAL issues
currently. What I can do here is to say FATAL issues are blocking by default,
but allows the caller to change it if applicable.

+
+  // ID of help page to link to, if one of the actions is LEARN_MORE.
+  // Defaults to |kUnknownHelpPageId|.
+  int help_page_id;
+};
+
+// An issue that is associated with a globally unique ID. Created by
+// IssueManager when an IssueInfo is added to it.
+class Issue {
+ public:
+  using Id = int;
+  // ID is generated during construction.
+  explicit Issue(const IssueInfo& info);
+  Issue(const Issue& other) = default;
+  Issue& operator=(const Issue& other) = default;
+  ~Issue();
+
+  const Id& id() const { return id_; }
+  const IssueInfo& info() const { return info_; }
 
  private:
-  Type type_;
-};
-
-// Contains the information relevant to an issue.
-class Issue {
- public:
-  using Id = std::string;
-
-  enum Severity { FATAL, WARNING, NOTIFICATION };
-
-  // Creates a custom issue.
-  // |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);
-
-  Issue(const Issue& other);
-
-  ~Issue();
-
-  // 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 {
-    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;
-
- private:
-  std::string title_;
-  std::string message_;
-  IssueAction default_action_;
-  std::vector<IssueAction> secondary_actions_;
-  std::string route_id_;
-  Severity severity_;
-  Issue::Id id_;
-  bool is_blocking_;
-  int help_page_id_;
+  Id id_;
+  IssueInfo info_;
 };
 
 }  // namespace media_router
 
 #endif  // CHROME_BROWSER_MEDIA_ROUTER_ISSUE_H_