Upstream the Media Router's Issue class.

Upstream the Media Router's Issue class.
Issues contain status and error information and are propagated from Media Route Providers to the Media Router, and are presented to the user in the UI.
Let me know if you have any questions about this. Thanks.

BUG=464216
R=dalecurtis@chromium.org

Committed: https://crrev.com/7c17dcdabaacd30035b9c09e76a276ccacae60a1
Cr-Commit-Position: refs/heads/master@{#328249}

[Kevin Marshall, 2015-04-30 00:29:50]

marshallk@google.com changed reviewers:
+ marshallk@google.com

[DaleCurtis, 2015-04-30 17:49:02]

Do you have a reviewer from your team who is looking over this code?

[Kevin Marshall, 2015-04-30 17:53:09]

marshallk@google.com changed reviewers:
+ apabicle@chromium.org

[Kevin Marshall, 2015-04-30 17:53:10]

Added apacible@.

[Kevin M, 2015-04-30 17:53:53]

kmarshall@chromium.org changed reviewers:
+ apacible@chromium.org
- apabicle@chromium.org

[DaleCurtis, 2015-04-30 18:18:15]

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.cc (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:34: DCHECK(severity_ != FATAL ||
is_blocking_);
If you don't split this DCHECK() out the contents of those variables may be
lost. I recommend adding a "<<" which prints the values to the DCHECK, a la:

DCHECK(severity_ != FATAL || is_blocking_) << "Severity is: " << severity_

etc.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:38: DCHECK(default_action_.type() !=
secondary_actions_[0].type());
DCHECK_NE

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.h (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:16: using IssueId = std::string;
I'm wary of having this live outside of a class definition, you're defining
media_router::IssueId for everyone including this header. Is that what you want?

Also is it worth having a specialized IssueId type? It's just an std::string :)

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:76: bool IsBlocking() const;
Seems like these should be hacker style too?

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:92: const GURL help_url_;
Const values will prevent copyable classes. Is that intended? If so please add
DISALLOW_COPY_AND_ASSIGN() to each of these classes.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.cc:93: while (it !=
media_issue_map_.end()) {
Seems like you might want some form of a priority based heap instead of having
to iterate over the map every time.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.h (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.h:51: void
RegisterObserver(IssuesObserver* observer);
Is it IssueObserver in that each observer will be called for every issue, or
IssuesObserver where each observer may be called for multiple issues at once?

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_unittest.cc (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue_unittest.cc:12: class IssueUnitTest : public
::testing::Test {
Just use TEST() instead of TEST_F() and you can remove this.

[apacible, 2015-04-30 19:02:06]

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.h (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:68: Severity severity() const { return
severity_; }
nit: Can you put these in the same order as the parameters and member variables?

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:76: bool IsBlocking() const;
On 2015/04/30 18:18:15, DaleCurtis wrote:
> Seems like these should be hacker style too?

What does hacker style mean?

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.h (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.h:51: void
RegisterObserver(IssuesObserver* observer);
On 2015/04/30 18:18:15, DaleCurtis wrote:
> Is it IssueObserver in that each observer will be called for every issue, or
> IssuesObserver where each observer may be called for multiple issues at once?

All IssueObservers will be called for each issue that replaces the 'top issue'
(e.g. high priority to show). We show the 'top issue' in the WebUI.

[DaleCurtis, 2015-04-30 19:05:02]

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.h (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:76: bool IsBlocking() const;
On 2015/04/30 19:02:06, apacible wrote:
> On 2015/04/30 18:18:15, DaleCurtis wrote:
> > Seems like these should be hacker style too?
> 
> What does hacker style mean? 

hacker_style()

[Kevin M, 2015-05-01 17:59:18]

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.cc (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:34: DCHECK(severity_ != FATAL ||
is_blocking_);
On 2015/04/30 18:18:14, DaleCurtis wrote:
> If you don't split this DCHECK() out the contents of those variables may be
> lost. I recommend adding a "<<" which prints the values to the DCHECK, a la:
> 
> DCHECK(severity_ != FATAL || is_blocking_) << "Severity is: " << severity_
> 
> etc.

Great point. Fixed.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:38: DCHECK(default_action_.type() !=
secondary_actions_[0].type());
On 2015/04/30 18:18:14, DaleCurtis wrote:
> DCHECK_NE

Done.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.h (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:16: using IssueId = std::string;
On 2015/04/30 18:18:14, DaleCurtis wrote:
> I'm wary of having this live outside of a class definition, you're defining
> media_router::IssueId for everyone including this header. Is that what you
want?
> 
> Also is it worth having a specialized IssueId type? It's just an std::string
:)

Moved it into Issue.

Regarding specialized IssueIds: we decided to use aliases for IDs across the
Media Router codebase to make the function signatures self-documenting and to
allow for easier transitioning to other types such as structs down the line.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:68: Severity severity() const { return
severity_; }
On 2015/04/30 19:02:06, apacible wrote:
> nit: Can you put these in the same order as the parameters and member
variables?

Done.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:76: bool IsBlocking() const;
On 2015/04/30 19:05:02, DaleCurtis wrote:
> On 2015/04/30 19:02:06, apacible wrote:
> > On 2015/04/30 18:18:15, DaleCurtis wrote:
> > > Seems like these should be hacker style too?
> > 
> > What does hacker style mean? 
> 
> hacker_style()

Done.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:92: const GURL help_url_;
On 2015/04/30 18:18:14, DaleCurtis wrote:
> Const values will prevent copyable classes. Is that intended? If so please add
> DISALLOW_COPY_AND_ASSIGN() to each of these classes.

Done for this class.

Re: IssueAction, it needs to remain copyable and assignable for compatibility
with STL containers, so in that case I left the implicit ctors intact.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.cc:93: while (it !=
media_issue_map_.end()) {
On 2015/04/30 18:18:15, DaleCurtis wrote:
> Seems like you might want some form of a priority based heap instead of having
> to iterate over the map every time.

Hmmm. There are multiple different access patterns which complicate the choice
of an efficient single container. MaybeUpdateTopIssue could use a minheap,
ClearIssuesWithRouteId/ClearGlobalIssues perform an exhaustive linear search,
and ClearIssue/AddIssue could take advantage of map lookups. Can't make
everything happy, so I'll just go with a std::list here instead. apacible@,
PTAL.

(I checked with haibinlu@ and he said that the collection will likely only have
0-10 elements, anyways.)

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.h (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.h:51: void
RegisterObserver(IssuesObserver* observer);
On 2015/04/30 18:18:15, DaleCurtis wrote:
> Is it IssueObserver in that each observer will be called for every issue, or
> IssuesObserver where each observer may be called for multiple issues at once?

PTAL at the newest comments, hopefully it is clearer now.

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_unittest.cc (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue_unittest.cc:12: class IssueUnitTest : public
::testing::Test {
On 2015/04/30 18:18:15, DaleCurtis wrote:
> Just use TEST() instead of TEST_F() and you can remove this.

Done.

[apacible, 2015-05-01 20:18:09]

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.cc:93: while (it !=
media_issue_map_.end()) {
On 2015/05/01 17:59:18, Kevin M wrote:
> On 2015/04/30 18:18:15, DaleCurtis wrote:
> > Seems like you might want some form of a priority based heap instead of
having
> > to iterate over the map every time.
> 
> Hmmm. There are multiple different access patterns which complicate the choice
> of an efficient single container. MaybeUpdateTopIssue could use a minheap,
> ClearIssuesWithRouteId/ClearGlobalIssues perform an exhaustive linear search,
> and ClearIssue/AddIssue could take advantage of map lookups. Can't make
> everything happy, so I'll just go with a std::list here instead. apacible@,
> PTAL.
> 
> (I checked with haibinlu@ and he said that the collection will likely only
have
> 0-10 elements, anyways.)

std::list looks fine to me.

https://codereview.chromium.org/1103273013/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.cc (right):

https://codereview.chromium.org/1103273013/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:34: DCHECK(severity_ != FATAL ||
is_blocking_) << "Severity is " << severity_;
What gets printed out here since severity enum values don't have corresponding
strings?

https://codereview.chromium.org/1103273013/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.cc:59: top_issue_ = nullptr;
Do we need a comment about invalidating top_issue_ similar to L32 here? Same
with L75.

[Kevin Marshall, 2015-05-01 20:26:03]

https://codereview.chromium.org/1103273013/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.cc (right):

https://codereview.chromium.org/1103273013/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:34: DCHECK(severity_ != FATAL ||
is_blocking_) << "Severity is " << severity_;
On 2015/05/01 20:18:08, apacible wrote:
> What gets printed out here since severity enum values don't have corresponding
> strings?

The ordinal value of |severity_| is printed.

https://codereview.chromium.org/1103273013/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.cc:59: top_issue_ = nullptr;
On 2015/05/01 20:18:08, apacible wrote:
> Do we need a comment about invalidating top_issue_ similar to L32 here? Same
> with L75.

Done.

[DaleCurtis, 2015-05-02 18:13:33]

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.cc (right):

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:44: bool Issue::is_blocking() const {
These have to go into the .h file if you use hacker_style().

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.cc:30: if (issue_iter->id() ==
issue_id) {
Seems like this could be a helper function since all paths are using a minor
variation?

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.h (right):

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.h:64: std::list<Issue> issues_;
If you only have 10 elements, using a deque / vector may even be faster than a
list. You can just swap elements to the end for erasing w/o cost.

[Kevin M, 2015-05-04 18:45:48]

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.cc (right):

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:44: bool Issue::is_blocking() const {
On 2015/05/02 18:13:32, DaleCurtis wrote:
> These have to go into the .h file if you use hacker_style().

Done. I can't find this in the style guide, though; is this rule documented
somewhere else?

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.cc:30: if (issue_iter->id() ==
issue_id) {
On 2015/05/02 18:13:32, DaleCurtis wrote:
> Seems like this could be a helper function since all paths are using a minor
> variation?

Done.

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/issue_manager.h (right):

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.h:64: std::list<Issue> issues_;
On 2015/05/02 18:13:32, DaleCurtis wrote:
> If you only have 10 elements, using a deque / vector may even be faster than a
> list. You can just swap elements to the end for erasing w/o cost.

OK. I switched top_issue_ from an Issue* to an IssueId to support vector element
erasure. I think it's fine to continue using vector::erase() for clarity, as
there will only be a small number of issues active at any given time.

[DaleCurtis, 2015-05-04 20:03:29]

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.cc (right):

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:44: bool Issue::is_blocking() const {
On 2015/05/04 18:45:48, Kevin M wrote:
> On 2015/05/02 18:13:32, DaleCurtis wrote:
> > These have to go into the .h file if you use hacker_style().
> 
> Done. I can't find this in the style guide, though; is this rule documented
> somewhere else?

See inline functions at: https://www.chromium.org/developers/coding-style

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:28: for (auto it = issues_.begin();
it != issues_.end(); ++it) {
You can use std::remove_if() with a lambda for id equality to reduce this
substantially and avoid the RemoveIssue method below.

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:51: while (it != issues_.end()) {
Ditto.

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:65: if (it->route_id() == route_id)
{
Ditto.

[apacible, 2015-05-04 21:14:32]

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/issue.h (right):

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:71: bool is_blocking() const {
Nit: Can this fit into one line? (Same below)

[Kevin M, 2015-05-04 23:40:18]

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/issue.cc (right):

https://codereview.chromium.org/1103273013/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:44: bool Issue::is_blocking() const {
On 2015/05/04 20:03:28, DaleCurtis wrote:
> On 2015/05/04 18:45:48, Kevin M wrote:
> > On 2015/05/02 18:13:32, DaleCurtis wrote:
> > > These have to go into the .h file if you use hacker_style().
> > 
> > Done. I can't find this in the style guide, though; is this rule documented
> > somewhere else?
> 
> See inline functions at: https://www.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/issue.h (right):

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:71: bool is_blocking() const {
On 2015/05/04 21:14:32, apacible wrote:
> Nit: Can this fit into one line? (Same below)

Done.

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:28: for (auto it = issues_.begin();
it != issues_.end(); ++it) {
On 2015/05/04 20:03:29, DaleCurtis wrote:
> You can use std::remove_if() with a lambda for id equality to reduce this
> substantially and avoid the RemoveIssue method below. 

Done.

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:51: while (it != issues_.end()) {
On 2015/05/04 20:03:29, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/1103273013/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:65: if (it->route_id() == route_id)
{
On 2015/05/04 20:03:29, DaleCurtis wrote:
> Ditto.

Done.

[apacible, 2015-05-04 23:46:05]

lgtm

[DaleCurtis, 2015-05-04 23:48:30]

lgtm

https://codereview.chromium.org/1103273013/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/issue_manager.cc (right):

https://codereview.chromium.org/1103273013/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:49: void
IssueManager::ClearGlobalIssues() {
Have you tried calling this when there are no global issues? I forget if
remove_if protects you against erasing() and empty iterator.

https://codereview.chromium.org/1103273013/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:61: [route_id](const Issue& issue)
{
&route_id ?

[Kevin M, 2015-05-05 00:00:02]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2015-05-05 00:00:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1103273013/#ps140001
(title: "Code review feedback.")

[commit-bot: I haz the power, 2015-05-05 00:02:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1103273013/140001

[commit-bot: I haz the power, 2015-05-05 00:08:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-05 00:08:33]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kevin M, 2015-05-05 00:19:27]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2015-05-05 00:19:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1103273013/#ps160001
(title: "Resync with master.")

[commit-bot: I haz the power, 2015-05-05 00:19:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1103273013/160001

[commit-bot: I haz the power, 2015-05-05 01:36:24]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-05-05 01:38:06]

Patchset 9 (id:??) landed as
https://crrev.com/7c17dcdabaacd30035b9c09e76a276ccacae60a1
Cr-Commit-Position: refs/heads/master@{#328249}