[Media Router] Clean up issues related code.

[Media Router] Clean up issues related code.

- Split all fields except for ID in Issue into IssueInfo struct, which is safely copyable.
- Modified Register/UnregisterIssuesObserver behavior to not require a manaul Unregiser call. (similar to MediaSinksObserver)
- Use StructTraits when converting Issue from/to Mojo land
- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.
- No need for MediaRouterAction to retain a copy of Issue object.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/466a89e738c071195c6bdbe9fe06ce871ead7a9f
Cr-Commit-Position: refs/heads/master@{#438321}

[imcheng, 2016-07-22 20:36:19]

Description was changed from

==========
[Media Router] Clean up issues related code.

- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.

BTW, it is still sort of weird that constructing an Issue will
generate a new ID. I propose that we remove the ID from the Issue
class. Instead, we introduce IDs in IssueManager when an Issue is
added. When an issue is added to IssueManager, the manager will
generate a new ID and combine it with the ID (Let's say IssueWithId?).
The IssueWithId is then sent to IssueObservers.

BUG=571525
==========

to

==========
[Media Router] Clean up issues related code.

- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.

BTW, it is still sort of weird that constructing an Issue will
generate a new ID. I propose that we remove the ID from the Issue
class. Instead, we introduce IDs in IssueManager when an Issue is
added. When an issue is added to IssueManager, the manager will
generate a new ID and combine it with the ID (Let's say IssueWithId?).
The IssueWithId is then sent to IssueObservers.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[imcheng, 2016-07-22 20:37:59]

Description was changed from

==========
[Media Router] Clean up issues related code.

- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.

BTW, it is still sort of weird that constructing an Issue will
generate a new ID. I propose that we remove the ID from the Issue
class. Instead, we introduce IDs in IssueManager when an Issue is
added. When an issue is added to IssueManager, the manager will
generate a new ID and combine it with the ID (Let's say IssueWithId?).
The IssueWithId is then sent to IssueObservers.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router] Clean up issues related code.

- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.

BTW, it is still sort of weird that constructing an Issue will
generate a new ID. I propose that we remove the ID from the Issue
class and only introduce the concept of Issue IDs in IssueManager.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[imcheng, 2016-07-22 20:49:11]

Description was changed from

==========
[Media Router] Clean up issues related code.

- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.

BTW, it is still sort of weird that constructing an Issue will
generate a new ID. I propose that we remove the ID from the Issue
class and only introduce the concept of Issue IDs in IssueManager.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router] Clean up issues related code.

- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[imcheng, 2016-07-22 22:17:08]

imcheng@chromium.org changed reviewers:
+ mfoltz@chromium.org

[imcheng, 2016-07-22 22:17:09]

PTAL. I wanted to deal with the peculiarity with the Issue ID in a separate
patch. I have two ideas:

- Take out the ID field from Issue. The ID is not part of the Issue per se, but
rather something used by IssueManager and IssuesObserver to communicate. So we
should let IssueManager own the ID generation and mapping to actual Issues.

- Disallow copy and assign of Issues. We currently allow copy assignment, which
makes the operator== flawed since it compares by ID only. This is a more
involved change, since we have to convert a handful of places to use
unique_ptrs.

[imcheng, 2016-07-22 22:17:55]

Description was changed from

==========
[Media Router] Clean up issues related code.

- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router] Clean up issues related code.

- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.
- No need for MediaRouterAction to retain a copy of Issue object.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mark a. foltz, 2016-07-27 18:52:04]

Thanks Derek this is much, much cleaner.  Mostly minor style comments.

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:11: Issue::Id next_issue_id = 0;
Please use a threadsafe sequence number:

https://cs-staging.chromium.org/chromium/src/base/atomic_sequence_num.h?l=22

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:29: return severity_ ==
Issue::Severity::FATAL || is_blocking_;
or initialize is_blocking_ to severity_ == Issue::Severity::FATAL and make
is_blocking a public member?

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:23: enum class ActionType {
Or just Action, so Issue::Action::DISMISS, etc.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:28: MAX /* Denotes enum value boundary. */
NUM_VALUES?

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:43: Issue(const Issue& other);
Usually if an object implements the copy constructor it also implements
assignment (but see comments about == below)

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:47: // Setters for optional arguments.
All of the getters or setters are trivial; this could be a struct with public
members which would make all of this code unnecessary.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:69: bool operator==(const Issue& other)
const { return id_ == other.id_; }
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.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:73: bool IsBlocking() const;
This shoudn't be necessary if is_blocking is initialized correctly in the
constructor.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:87: // Optional fields.
I don't think this comment makes sense in context. 

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:106: // Otherwise. Defaults to -1.
Declare constant for kUnknownHelpPageId?

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.h:72: const Issue* top_issue_;
Consider base::Optional<Issue>.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:156: void
MediaRouterAction::OnIssueUpdated(const media_router::Issue* issue) {
I don't think |issue| should be modified here.  Can this be const Issue&
instead?

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.h:104: bool has_issue_;
It seems more straightforward for MediaRouterAction to have a
base::Optional<Issue> with a copy of the current issue.

[mark a. foltz, 2016-09-12 17:00:50]

Noticed this issue was still open...

[imcheng, 2016-09-12 20:47:45]

Working on this. It has been on my backlog for a while since returning from
vacation.

[imcheng, 2016-09-13 20:27:55]

I made some more changes in addition to addressing your comments.

- I extracted a IssueInfo struct from the original Issue class. The new Issue
class contains an ID and a IssueInfo. I think the code is cleaner now that we
separate issue data from issue management.
-- The IssueInfo contains all fields except for the ID, which means it is safe
to copy. The struct itself contains minimal logic. The client passes in an
IssueInfo when adding an issue.
-- The IssueManager wraps IssueInfo in an Issue object, which means it also owns
the ID generation. The Issue class is immutable and instances are maintained by
IssueManager and passed to registered IssuesObserver's.

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:11: Issue::Id next_issue_id = 0;
On 2016/07/27 18:52:03, mark a. foltz wrote:
> Please use a threadsafe sequence number:
> 
> https://cs-staging.chromium.org/chromium/src/base/atomic_sequence_num.h?l=22

Done in IssueManager.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.cc:29: return severity_ ==
Issue::Severity::FATAL || is_blocking_;
On 2016/07/27 18:52:03, mark a. foltz wrote:
> or initialize is_blocking_ to severity_ == Issue::Severity::FATAL and make
> is_blocking a public member?

Done.

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:23: enum class ActionType {
On 2016/07/27 18:52:03, mark a. foltz wrote:
> Or just Action, so Issue::Action::DISMISS, etc.

Done.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:28: MAX /* Denotes enum value boundary. */
On 2016/07/27 18:52:03, mark a. foltz wrote:
> NUM_VALUES?
> 
> Also, add a comment that new Actions should be added above here.

Done.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:43: Issue(const Issue& other);
On 2016/07/27 18:52:03, mark a. foltz wrote:
> Usually if an object implements the copy constructor it also implements
> assignment (but see comments about == below)

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:47: // Setters for optional arguments.
On 2016/07/27 18:52:03, mark a. foltz wrote:
> All of the getters or setters are trivial; this could be a struct with public
> members which would make all of this code unnecessary.

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:69: bool operator==(const Issue& other)
const { return id_ == other.id_; }
On 2016/07/27 18:52:03, mark a. foltz wrote:
> 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.

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.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:73: bool IsBlocking() const;
On 2016/07/27 18:52:03, mark a. foltz wrote:
> This shoudn't be necessary if is_blocking is initialized correctly in the
> constructor.

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.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:87: // Optional fields.
On 2016/07/27 18:52:03, mark a. foltz wrote:
> I don't think this comment makes sense in context. 
> 
> - The message_ is noted as required below
> - Other fields have defaults

Removed the comments.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue.h:106: // Otherwise. Defaults to -1.
On 2016/07/27 18:52:03, mark a. foltz wrote:
> Declare constant for kUnknownHelpPageId?

Done.

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

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/issue_manager.h:72: const Issue* top_issue_;
On 2016/07/27 18:52:04, mark a. foltz wrote:
> Consider base::Optional<Issue>.

Keeping this as a pointer so we don't have to copy construct an extra Issue.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:156: void
MediaRouterAction::OnIssueUpdated(const media_router::Issue* issue) {
On 2016/07/27 18:52:04, mark a. foltz wrote:
> I don't think |issue| should be modified here.  Can this be const Issue&
> instead?

There was a similar comment in Takumi's patch. We currently use nullptr to
denote "no more issues" in the IssuesObserver API. Using "const Issue*" ensures
the input issue doesn't get modified.

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/2176613003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.h:104: bool has_issue_;
On 2016/07/27 18:52:04, mark a. foltz wrote:
> It seems more straightforward for MediaRouterAction to have a
> base::Optional<Issue> with a copy of the current issue.

I think the issue (no pun intended) here is that MRA doesn't need to know the
entirety of the issue - just the issue severity is sufficient to determine the
state of the icon. We would also have to implement copy construction which we
are trying to eliminate.

We can make it base::Optional<Severity> to get rid of the |has_issue_| though.

[imcheng, 2016-09-14 19:48:30]

There are a couple other things I want to do in this patch:

- Fix the IssuesObserver implementation to not require manual
UnregisterObserver()
- Migrate Issue mojo conversion to use StructTraits

[mark a. foltz, 2016-10-03 19:54:36]

On 2016/09/14 at 19:48:30, imcheng wrote:
> There are a couple other things I want to do in this patch:
> 
> - Fix the IssuesObserver implementation to not require manual
UnregisterObserver()
> - Migrate Issue mojo conversion to use StructTraits

Are you still working on this or would you like a re-review?

[imcheng, 2016-10-17 23:33:34]

Description was changed from

==========
[Media Router] Clean up issues related code.

- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.
- No need for MediaRouterAction to retain a copy of Issue object.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router] Clean up issues related code.

- Split all fields except for ID in Issue into IssueInfo struct, which is safely
copyable.
- Modified Register/UnregisterIssuesObserver behavior to not require a manaul
Unregiser call. (similar to MediaSinksObserver)
- Use StructTraits when converting Issue from/to Mojo land
- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.
- No need for MediaRouterAction to retain a copy of Issue object.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[imcheng, 2016-10-17 23:34:51]

Yep, ready for review now. Uploaded a new patchset with the following changes:

- Fix the IssuesObserver implementation to not require manual
UnregisterObserver()
- Migrate Issue mojo conversion to use StructTraits

[mark a. foltz, 2016-10-18 17:16:52]

Sending some comments now.  Will follow up with rest soon.

It looks like IssueInfo is passed by pointer and reference.  Is the copy
constructor/assignment actually used?  If not, you could combine Issue and
IssueInfo into one class/structs.

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

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:59: // Array of options the user can take to
resolve the issue in addition to the
Maybe just drop "array of" since this is actually a vector?

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:61: // action is supported.
The last sentence was ambiguous: does it mean this doesn't need to be a vector
if there can be only one secondary action?

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:74: // Otherwise. Defaults to
|kUnknownHelpPageId|.
Nit: remove "Otherwise."

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

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.h:54:
std::vector<std::unique_ptr<Issue>> issues_;
Consider making this a set<unique_ptr<Issue>> with a comparator for Issue.  Then
the top issue is just *(issues_.begin()).

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.h:59: // The top Issue in |issues_|,
or |nullptr| if there are none.
nit: s/are/is/

[mark a. foltz, 2016-10-18 21:13:44]

Overall looks good with nits addressed. One substantial question about the
typemapping.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc
(right):

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:653:
.WillOnce(SaveArg<0>(&issue_from_observer1));
Would it be simpler to declare a Matcher for Issue and assert that both
observers are called with OnIssueUpdated(&issue)?  Or do you need to verify that
the pointers are identical?

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:658:
InvokeWithoutArgs([&run_loop]() { run_loop.Quit(); })));
Add run_loop.QuitWhenIdle() after Run() and then this can be removed.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:24: return false;
Would this only happen if we forgot to update the .h at the same time as the
.mojom?  The mojo stub should validate the value IIUC.

Same question applies to IsValidIssueSeverity.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:118:
base::Optional<media_router::IssueInfo::Severity> current_issue_severity_;
It seems odd to cherry-pick one value out of the data model object.  Is there an
issue with holding a unique_ptr<Issue> as was done before?  Or is this to avoid
a potential copy?

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc
(right):

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:562:
int actual_issue_id = -1;
Add a comment explaining this is an invalid issue id.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:571:
int action_type_num = -1;
Add a comment explaining this is an invalid action type value.

[mark a. foltz, 2016-11-03 17:48:51]

Ping on open code review.

[imcheng, 2016-11-18 23:45:14]

Sorry for the delay. Comments addressed.

Regarding IssueInfo vs Issue:

Copy constructor/assignment is used in unit tests for convenience. Since they
have well defined behavior, I decided to allow them.

The same could be said for Issue - as long as we are only allocating new Issue
ID in the regular constructor.

Issue ID is problematic - we create it so that we can quickly compare if two
issues are identical. But with the current behavior, two issues are considered
different even if they have the same contents. I propose that IssueManager
should be responsible for Issue ID management, as well as preventing duplicates.

TL;DR: Both are copyable in theory, but are differentiated since I want to
separate data model of Issue and how it is managed.

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

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:59: // Array of options the user can take to
resolve the issue in addition to the
On 2016/10/18 17:16:52, mark a. foltz wrote:
> Maybe just drop "array of" since this is actually a vector?

Done.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:61: // action is supported.
On 2016/10/18 17:16:52, mark a. foltz wrote:
> The last sentence was ambiguous: does it mean this doesn't need to be a vector
> if there can be only one secondary action?

That is correct. I don't know if we will ever support more than one secondary
action. At least we will have to wait until we redesign the UX.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:74: // Otherwise. Defaults to
|kUnknownHelpPageId|.
On 2016/10/18 17:16:52, mark a. foltz wrote:
> Nit: remove "Otherwise."

Done.

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

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.h:54:
std::vector<std::unique_ptr<Issue>> issues_;
On 2016/10/18 17:16:52, mark a. foltz wrote:
> Consider making this a set<unique_ptr<Issue>> with a comparator for Issue. 
Then
> the top issue is just *(issues_.begin()).

I am inclined to keep this a vector for now since there aren't likely many
issues at any given time.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.h:59: // The top Issue in |issues_|,
or |nullptr| if there are none.
On 2016/10/18 17:16:52, mark a. foltz wrote:
> nit: s/are/is/

Done.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc
(right):

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:653:
.WillOnce(SaveArg<0>(&issue_from_observer1));
On 2016/10/18 21:13:44, mark a. foltz wrote:
> Would it be simpler to declare a Matcher for Issue and assert that both
> observers are called with OnIssueUpdated(&issue)?  Or do you need to verify
that
> the pointers are identical?

The pointers should be identical since they are called with the same Issue
object.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:658:
InvokeWithoutArgs([&run_loop]() { run_loop.Quit(); })));
On 2016/10/18 21:13:44, mark a. foltz wrote:
> Add run_loop.QuitWhenIdle() after Run() and then this can be removed.

Done.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:24: return false;
On 2016/10/18 21:13:44, mark a. foltz wrote:
> Would this only happen if we forgot to update the .h at the same time as the
> .mojom?  The mojo stub should validate the value IIUC.
> 
> Same question applies to IsValidIssueSeverity.

Yeah, also media_router_bindings.js converts unknown values to a default enum
value. I guess this makes the check here redundant.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:118:
base::Optional<media_router::IssueInfo::Severity> current_issue_severity_;
On 2016/10/18 21:13:44, mark a. foltz wrote:
> It seems odd to cherry-pick one value out of the data model object.  Is there
an
> issue with holding a unique_ptr<Issue> as was done before?  Or is this to
avoid
> a potential copy?

This was an issue before because of how we allocated a new issue id with
copy-constructing Issue (and also we didn't have use for other fields). I
changed this to make a copy of the IssueInfo.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc
(right):

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:562:
int actual_issue_id = -1;
On 2016/10/18 21:13:44, mark a. foltz wrote:
> Add a comment explaining this is an invalid issue id.

Done.

https://codereview.chromium.org/2176613003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:571:
int action_type_num = -1;
On 2016/10/18 21:13:44, mark a. foltz wrote:
> Add a comment explaining this is an invalid action type value.

Done.

[mark a. foltz, 2016-11-29 21:27:08]

LGTM with comments addressed.  Thanks for adding typemapping and general
cleanup.

I am still a little leery of the design that passes an Issue* back to observers
which have no clear constraint on their object lifetime.

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

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:32: // issues of lower severity may also be
blocking.
This comment is a little confusing because FATAL has a value of 0.  Maybe say
'WARNING or NOTIFICATION level issues may also...'

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:53: // Fields set in the constructor.
Pedantic: do you mean passed in the constructor?  The ctor sets all fields below
to some value.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/issues_observer.h (right):

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/issues_observer.h:27: // If |issue| is nullptr, then
there is currently no issue.
Document that the lifetime of |issue| is tied to the IssueManager.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/issues_observer.h:28: virtual void
OnIssueUpdated(const Issue* issue) {}
Would it make more sense to pass a const reference?  That way the caller is not
tempted to keep a pointer to an Issue it doesn't own.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:27: return
media_router::IssueInfo::Action::DISMISS;
The Mojo enum reader should validate enum values.  Of course there could be a
missing value in IssueInfo::Action...

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:127:
out->default_action = IssueActionFromMojo(default_action);
Are Mojo enums zero indexed?  Can you just static cast here?

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:133: out->severity
= IssueSeverityFromMojo(severity);
Similar question as above.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:172: case
media_router::IssueInfo::Severity::FATAL:
Can this be a static cast?

[imcheng, 2016-12-05 23:17:57]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[imcheng, 2016-12-05 23:18:23]

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

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:32: // issues of lower severity may also be
blocking.
On 2016/11/29 21:27:07, mark a. foltz wrote:
> This comment is a little confusing because FATAL has a value of 0.  Maybe say
> 'WARNING or NOTIFICATION level issues may also...'

Done.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:53: // Fields set in the constructor.
On 2016/11/29 21:27:07, mark a. foltz wrote:
> Pedantic: do you mean passed in the constructor?  The ctor sets all fields
below
> to some value.

Done.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/issues_observer.h (right):

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/issues_observer.h:27: // If |issue| is nullptr, then
there is currently no issue.
On 2016/11/29 21:27:07, mark a. foltz wrote:
> Document that the lifetime of |issue| is tied to the IssueManager.

Done.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/issues_observer.h:28: virtual void
OnIssueUpdated(const Issue* issue) {}
On 2016/11/29 21:27:07, mark a. foltz wrote:
> Would it make more sense to pass a const reference?  That way the caller is
not
> tempted to keep a pointer to an Issue it doesn't own.

Unfortunately we are also using this API to notify the observer that there are
no more issues. I would prefer that generally since as you say, we can make
OnIssueUpdated use a const ref. I can do that in a separate patch set.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:27: return
media_router::IssueInfo::Action::DISMISS;
On 2016/11/29 21:27:08, mark a. foltz wrote:
> The Mojo enum reader should validate enum values.  Of course there could be a
> missing value in IssueInfo::Action...

IMO, it's good practice to always have default cases to catch the unexpected.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:127:
out->default_action = IssueActionFromMojo(default_action);
On 2016/11/29 21:27:08, mark a. foltz wrote:
> Are Mojo enums zero indexed?  Can you just static cast here?

I think so. But I would prefer a safer approach that doesn't assume how enum
values are generated in Mojo.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:133: out->severity
= IssueSeverityFromMojo(severity);
On 2016/11/29 21:27:07, mark a. foltz wrote:
> Similar question as above.

Acknowledged.

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:172: case
media_router::IssueInfo::Severity::FATAL:
On 2016/11/29 21:27:08, mark a. foltz wrote:
> Can this be a static cast?

See above.

[commit-bot: I haz the power, 2016-12-05 23:18:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[imcheng, 2016-12-05 23:19:47]

imcheng@chromium.org changed reviewers:
+ dcheng@chromium.org, msw@chromium.org

[imcheng, 2016-12-05 23:19:48]

Adding more reviewers:

msw:
chrome/browser/ui/toolbar/*

dcheng:
chrome/browser/media/router/mojo/* (mojo ipc stuff)

[mark a. foltz, 2016-12-05 23:24:09]

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:127:
out->default_action = IssueActionFromMojo(default_action);
On 2016/12/05 at 23:18:19, imcheng wrote:
> On 2016/11/29 21:27:08, mark a. foltz wrote:
> > Are Mojo enums zero indexed?  Can you just static cast here?
> 
> I think so. But I would prefer a safer approach that doesn't assume how enum
values are generated in Mojo.

Mojo must provide some strong guarantees about its deserialization API. 
Otherwise how can the browser trust any of the data coming via Mojo?

[commit-bot: I haz the power, 2016-12-05 23:30:29]

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

[commit-bot: I haz the power, 2016-12-05 23:30:31]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

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

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

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:12: #include "base/optional.h"
Nit: remove this header, since it's not actually used here

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:45: const Action& default_action,
Nit: pass by value is (usually) more efficient here, since Action is an enum

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:73: bool is_blocking;
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|

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

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:9: #include
"base/memory/ptr_util.h"
Nit: remove this, as it's unused

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/issue_manager.h (right):

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.h:10: #include <memory>
Nit: remove this header, as it's unused here

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/issues_observer.cc (right):

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issues_observer.cc:27: initialized_ = true;
Can you help me understand why this isn't just part of the constructor? Doing so
would remove the need for |initialized_| completely.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:57: }
Please typemap the enums as well: then we can just write EnumTraits for this.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:119:
media_router::IssueInfo* out) {
Please out-of-line this method (since it's pretty long) into a .cc file.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:157:
out->is_blocking = data.is_blocking();
There's nothing that requires is_blocking() to agree with severity. As mentioned
earlier, I think we can get around this by not having this as an explicit field
and just deriving the value as a getter.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:195: return
issue.message.empty() ? base::Optional<std::string>()
Maybe the C++ struct should just use base::Optional<> directly?

Getters are actually called multiple times during serialization, so it's best to
make them very cheap getters.

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

Thanks for the review.

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

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:12: #include "base/optional.h"
On 2016/12/07 18:58:02, dcheng wrote:
> Nit: remove this header, since it's not actually used here

Done.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:45: const Action& default_action,
On 2016/12/07 18:58:02, dcheng wrote:
> Nit: pass by value is (usually) more efficient here, since Action is an enum

Done.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue.h:73: bool is_blocking;
On 2016/12/07 18:58:02, dcheng wrote:
> 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|

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.

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

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.cc:9: #include
"base/memory/ptr_util.h"
On 2016/12/07 18:58:02, dcheng wrote:
> Nit: remove this, as it's unused

Done.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/issue_manager.h (right):

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issue_manager.h:10: #include <memory>
On 2016/12/07 18:58:02, dcheng wrote:
> Nit: remove this header, as it's unused here

Done.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/issues_observer.cc (right):

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/issues_observer.cc:27: initialized_ = true;
On 2016/12/07 18:58:02, dcheng wrote:
> Can you help me understand why this isn't just part of the constructor? Doing
so
> would remove the need for |initialized_| completely.

RegisterIssuesObserver will synchronously invoke OnIssue() if there is currently
an issue, which is a virtual method -- this results in undefined behavior. Also
doing work in constructors is discouraged by style guideline.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:57: }
On 2016/12/07 18:58:02, dcheng wrote:
> Please typemap the enums as well: then we can just write EnumTraits for this.

Done.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:119:
media_router::IssueInfo* out) {
On 2016/12/07 18:58:02, dcheng wrote:
> Please out-of-line this method (since it's pretty long) into a .cc file.

Done.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:157:
out->is_blocking = data.is_blocking();
On 2016/12/07 18:58:02, dcheng wrote:
> There's nothing that requires is_blocking() to agree with severity. As
mentioned
> earlier, I think we can get around this by not having this as an explicit
field
> and just deriving the value as a getter.

See other comment.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:195: return
issue.message.empty() ? base::Optional<std::string>()
On 2016/12/07 18:58:02, dcheng wrote:
> Maybe the C++ struct should just use base::Optional<> directly?
> 
> Getters are actually called multiple times during serialization, so it's best
to
> make them very cheap getters.

Ok. OTOH, we are not converting Issues from C++ to mojo -- is there a way to
avoid writing these code which seem unnecessary to me?

[imcheng, 2016-12-09 23:28:41]

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:127:
out->default_action = IssueActionFromMojo(default_action);
On 2016/12/05 23:24:09, mark a. foltz wrote:
> On 2016/12/05 at 23:18:19, imcheng wrote:
> > On 2016/11/29 21:27:08, mark a. foltz wrote:
> > > Are Mojo enums zero indexed?  Can you just static cast here?
> > 
> > I think so. But I would prefer a safer approach that doesn't assume how enum
> values are generated in Mojo.
> 
> Mojo must provide some strong guarantees about its deserialization API. 
> Otherwise how can the browser trust any of the data coming via Mojo?

dcheng@ - can you advise on whether we can do a static_cast? The helper
functions (now EnumTraits class) seem a bit heavyweight just for converting
enums.

[dcheng, 2016-12-12 01:45:36]

On 2016/12/09 23:28:41, imcheng wrote:
>
https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
> File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2176613003/diff/140001/chrome/browser/media/r...
> chrome/browser/media/router/mojo/media_router_struct_traits.h:127:
> out->default_action = IssueActionFromMojo(default_action);
> On 2016/12/05 23:24:09, mark a. foltz wrote:
> > On 2016/12/05 at 23:18:19, imcheng wrote:
> > > On 2016/11/29 21:27:08, mark a. foltz wrote:
> > > > Are Mojo enums zero indexed?  Can you just static cast here?
> > > 
> > > I think so. But I would prefer a safer approach that doesn't assume how
enum
> > values are generated in Mojo.
> > 
> > Mojo must provide some strong guarantees about its deserialization API. 
> > Otherwise how can the browser trust any of the data coming via Mojo?
> 
> dcheng@ - can you advise on whether we can do a static_cast? The helper
> functions (now EnumTraits class) seem a bit heavyweight just for converting
> enums.

The compiler will optimize it down to the same thing, and explicitly listing out
switches should help prevent the mojom and the C++ enums from getting out of
sync. In general, I think it's preferable to write it out explicitly.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:119:
media_router::IssueInfo* out) {
On 2016/12/09 23:19:53, imcheng wrote:
> On 2016/12/07 18:58:02, dcheng wrote:
> > Please out-of-line this method (since it's pretty long) into a .cc file.
> 
> Done.

It looks like the .cc file didn't get uploaded.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:195: return
issue.message.empty() ? base::Optional<std::string>()
On 2016/12/09 23:19:53, imcheng wrote:
> On 2016/12/07 18:58:02, dcheng wrote:
> > Maybe the C++ struct should just use base::Optional<> directly?
> > 
> > Getters are actually called multiple times during serialization, so it's
best
> to
> > make them very cheap getters.
> 
> Ok. OTOH, we are not converting Issues from C++ to mojo -- is there a way to
> avoid writing these code which seem unnecessary to me?

I suppose this doesn't compile if we omit these: I guess we can just annotate
them all with NOTREACHED().

(I would personally also just return bogus values, but I don't feel strongly)

https://codereview.chromium.org/2176613003/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:152: return
EnumTraits<
If this is typemapped, it won't be necessary to manually instantiate the
EnumTrait here: the getters/setters can just return the C++ enum types.

[imcheng, 2016-12-12 19:09:12]

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:119:
media_router::IssueInfo* out) {
On 2016/12/12 01:45:36, dcheng wrote:
> On 2016/12/09 23:19:53, imcheng wrote:
> > On 2016/12/07 18:58:02, dcheng wrote:
> > > Please out-of-line this method (since it's pretty long) into a .cc file.
> > 
> > Done.
> 
> It looks like the .cc file didn't get uploaded.

My bad. Forgot to git add it.

https://codereview.chromium.org/2176613003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:195: return
issue.message.empty() ? base::Optional<std::string>()
On 2016/12/12 01:45:36, dcheng wrote:
> On 2016/12/09 23:19:53, imcheng wrote:
> > On 2016/12/07 18:58:02, dcheng wrote:
> > > Maybe the C++ struct should just use base::Optional<> directly?
> > > 
> > > Getters are actually called multiple times during serialization, so it's
> best
> > to
> > > make them very cheap getters.
> > 
> > Ok. OTOH, we are not converting Issues from C++ to mojo -- is there a way to
> > avoid writing these code which seem unnecessary to me?
> 
> I suppose this doesn't compile if we omit these: I guess we can just annotate
> them all with NOTREACHED().
> 
> (I would personally also just return bogus values, but I don't feel strongly)

Ok. I will just keep them as is.

https://codereview.chromium.org/2176613003/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:152: return
EnumTraits<
On 2016/12/12 01:45:36, dcheng wrote:
> If this is typemapped, it won't be necessary to manually instantiate the
> EnumTrait here: the getters/setters can just return the C++ enum types.

Done.

[dcheng, 2016-12-12 20:59:56]

https://codereview.chromium.org/2176613003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right):

https://codereview.chromium.org/2176613003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.cc:16:
media_router::mojom::Issue::ActionType default_action;
I think you can avoid the manual calls to EnumTraits by using the C++ enum type
here instead of the mojo enum type as well.

[imcheng, 2016-12-12 22:40:01]

https://codereview.chromium.org/2176613003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right):

https://codereview.chromium.org/2176613003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.cc:16:
media_router::mojom::Issue::ActionType default_action;
On 2016/12/12 20:59:56, dcheng wrote:
> I think you can avoid the manual calls to EnumTraits by using the C++ enum
type
> here instead of the mojo enum type as well.

I am not sure how we can set out->default_action without a C++ value here
otherwise. Do you have some examples?

[dcheng, 2016-12-13 00:46:38]

https://codereview.chromium.org/2176613003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right):

https://codereview.chromium.org/2176613003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.cc:16:
media_router::mojom::Issue::ActionType default_action;
On 2016/12/12 22:40:01, imcheng wrote:
> On 2016/12/12 20:59:56, dcheng wrote:
> > I think you can avoid the manual calls to EnumTraits by using the C++ enum
> type
> > here instead of the mojo enum type as well.
> 
> I am not sure how we can set out->default_action without a C++ value here
> otherwise. Do you have some examples?

I think it should Just Work if typemapped; my recollection is the ReadX(...)
helpers are templated, and will figure out how the right thing.

[imcheng, 2016-12-13 00:51:45]

https://codereview.chromium.org/2176613003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right):

https://codereview.chromium.org/2176613003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.cc:16:
media_router::mojom::Issue::ActionType default_action;
On 2016/12/13 00:46:38, dcheng wrote:
> On 2016/12/12 22:40:01, imcheng wrote:
> > On 2016/12/12 20:59:56, dcheng wrote:
> > > I think you can avoid the manual calls to EnumTraits by using the C++ enum
> > type
> > > here instead of the mojo enum type as well.
> > 
> > I am not sure how we can set out->default_action without a C++ value here
> > otherwise. Do you have some examples?
> 
> I think it should Just Work if typemapped; my recollection is the ReadX(...)
> helpers are templated, and will figure out how the right thing.

Thanks -- using ReadX() worked.

[dcheng, 2016-12-13 01:13:33]

LGTM with nits addressed

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:26: default:
One final nit: remove the default, and move the NOTREACHED() / return outside
the switch. That way, the compiler will warn if we add new, unhandled enum
values.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:146: return
issue.route_id.empty() ? base::Optional<std::string>()
Let's annotate these getters (since they're unused) with NOTREACHED() for now
(base::Optional isn't /that/ expensive to construct, but it's also not free)

[imcheng, 2016-12-13 17:57:16]

msw@, can you please take a look at the toolbar code?

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:26: default:
On 2016/12/13 01:13:33, dcheng wrote:
> One final nit: remove the default, and move the NOTREACHED() / return outside
> the switch. That way, the compiler will warn if we add new, unhandled enum
> values.

Done. We do have a dummy value that denotes the boundary for the enum (used in
converting an int value coming from WebUI) so I will go ahead and include it in
the switch case as well?

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:146: return
issue.route_id.empty() ? base::Optional<std::string>()
On 2016/12/13 01:13:33, dcheng wrote:
> Let's annotate these getters (since they're unused) with NOTREACHED() for now
> (base::Optional isn't /that/ expensive to construct, but it's also not free)

Turns out we do use these in tests when we invoke a mojo proxy with an C++ Issue
object. So I will leave them as is.

[dcheng, 2016-12-13 18:00:50]

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:26: default:
On 2016/12/13 17:57:16, imcheng wrote:
> On 2016/12/13 01:13:33, dcheng wrote:
> > One final nit: remove the default, and move the NOTREACHED() / return
outside
> > the switch. That way, the compiler will warn if we add new, unhandled enum
> > values.
> 
> Done. We do have a dummy value that denotes the boundary for the enum (used in
> converting an int value coming from WebUI) so I will go ahead and include it
in
> the switch case as well?

If it's defined to be the same integer value as another enumerator, it's not
necessary to include it (and usually it's omitted, to avoid noise), e.g.
something like:

enum class Foo {
  Bar,
  Baz,
  Max = Baz,
}

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:146: return
issue.route_id.empty() ? base::Optional<std::string>()
On 2016/12/13 17:57:15, imcheng wrote:
> On 2016/12/13 01:13:33, dcheng wrote:
> > Let's annotate these getters (since they're unused) with NOTREACHED() for
now
> > (base::Optional isn't /that/ expensive to construct, but it's also not free)
> 
> Turns out we do use these in tests when we invoke a mojo proxy with an C++
Issue
> object. So I will leave them as is.

I see. Can we just make these Optional in the C++ struct as well? Then we can
just passthrough, and future reviews won't have to worry about this potentially
becoming more expensive.

(It is a fairly minor issue, in the scheme of things, but it would be better to
make this idiomatic)

[dcheng, 2016-12-13 18:07:22]

On 2016/12/13 18:00:50, dcheng wrote:
>
https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
> File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
> chrome/browser/media/router/mojo/media_router_struct_traits.h:26: default:
> On 2016/12/13 17:57:16, imcheng wrote:
> > On 2016/12/13 01:13:33, dcheng wrote:
> > > One final nit: remove the default, and move the NOTREACHED() / return
> outside
> > > the switch. That way, the compiler will warn if we add new, unhandled enum
> > > values.
> > 
> > Done. We do have a dummy value that denotes the boundary for the enum (used
in
> > converting an int value coming from WebUI) so I will go ahead and include it
> in
> > the switch case as well?
> 
> If it's defined to be the same integer value as another enumerator, it's not
> necessary to include it (and usually it's omitted, to avoid noise), e.g.
> something like:
> 
> enum class Foo {
>   Bar,
>   Baz,
>   Max = Baz,
> }
> 
>
https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
> chrome/browser/media/router/mojo/media_router_struct_traits.h:146: return
> issue.route_id.empty() ? base::Optional<std::string>()
> On 2016/12/13 17:57:15, imcheng wrote:
> > On 2016/12/13 01:13:33, dcheng wrote:
> > > Let's annotate these getters (since they're unused) with NOTREACHED() for
> now
> > > (base::Optional isn't /that/ expensive to construct, but it's also not
free)
> > 
> > Turns out we do use these in tests when we invoke a mojo proxy with an C++
> Issue
> > object. So I will leave them as is.
> 
> I see. Can we just make these Optional in the C++ struct as well? Then we can
> just passthrough, and future reviews won't have to worry about this
potentially
> becoming more expensive.
> 
> (It is a fairly minor issue, in the scheme of things, but it would be better
to
> make this idiomatic)

(Also, I wasn't clear, but it's OK to do the Optional fix in a followup CL, to
avoid making this one any bigger)

[msw, 2016-12-13 18:45:40]

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:274:
media_router::IssueInfo::Severity severity = current_issue_->severity;
optional nit: just inline current_issue_->severity in the conditionals below

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:277: else if (severity ==
media_router::IssueInfo::Severity::WARNING)
No else after return.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:116: // The current issue shown
in the Media Router WebUI, or not set if there are
nit: this comment mentions OnIssueUpdated, which no longer exists. Consider:
"The current issue shown in the Media Router WebUI, set in OnIssue() and cleared
in OnIssuesCleared()." or similar.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:119:
base::Optional<media_router::IssueInfo> current_issue_;
q: Just curious, why is base::Optional better than std::unique_ptr here?

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:580: if
(ui_initialized_)
aside: I prefer this conditional formatting over single-line if/then, but being
consistent locally is helpful...

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:196:
if (!issue_info.secondary_actions.empty()) {
tangential q: Are there ever instances where there are multiple secondary
actions? Should this DCHECK that there's only one, or should the vector just
become a single item?

[imcheng, 2016-12-13 19:22:47]

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:26: default:
On 2016/12/13 18:00:49, dcheng wrote:
> On 2016/12/13 17:57:16, imcheng wrote:
> > On 2016/12/13 01:13:33, dcheng wrote:
> > > One final nit: remove the default, and move the NOTREACHED() / return
> outside
> > > the switch. That way, the compiler will warn if we add new, unhandled enum
> > > values.
> > 
> > Done. We do have a dummy value that denotes the boundary for the enum (used
in
> > converting an int value coming from WebUI) so I will go ahead and include it
> in
> > the switch case as well?
> 
> If it's defined to be the same integer value as another enumerator, it's not
> necessary to include it (and usually it's omitted, to avoid noise), e.g.
> something like:
> 
> enum class Foo {
>   Bar,
>   Baz,
>   Max = Baz,
> }

You're right. I forgot about that. Done.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_struct_traits.h:146: return
issue.route_id.empty() ? base::Optional<std::string>()
On 2016/12/13 18:00:49, dcheng wrote:
> On 2016/12/13 17:57:15, imcheng wrote:
> > On 2016/12/13 01:13:33, dcheng wrote:
> > > Let's annotate these getters (since they're unused) with NOTREACHED() for
> now
> > > (base::Optional isn't /that/ expensive to construct, but it's also not
free)
> > 
> > Turns out we do use these in tests when we invoke a mojo proxy with an C++
> Issue
> > object. So I will leave them as is.
> 
> I see. Can we just make these Optional in the C++ struct as well? Then we can
> just passthrough, and future reviews won't have to worry about this
potentially
> becoming more expensive.
> 
> (It is a fairly minor issue, in the scheme of things, but it would be better
to
> make this idiomatic)

Ok. Will do in another patch.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:274:
media_router::IssueInfo::Severity severity = current_issue_->severity;
On 2016/12/13 18:45:39, msw wrote:
> optional nit: just inline current_issue_->severity in the conditionals below

Acknowledged.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:277: else if (severity ==
media_router::IssueInfo::Severity::WARNING)
On 2016/12/13 18:45:39, msw wrote:
> No else after return.

Done.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:116: // The current issue shown
in the Media Router WebUI, or not set if there are
On 2016/12/13 18:45:39, msw wrote:
> nit: this comment mentions OnIssueUpdated, which no longer exists. Consider:
> "The current issue shown in the Media Router WebUI, set in OnIssue() and
cleared
> in OnIssuesCleared()." or similar.

Done.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:119:
base::Optional<media_router::IssueInfo> current_issue_;
On 2016/12/13 18:45:39, msw wrote:
> q: Just curious, why is base::Optional better than std::unique_ptr here?

It's mostly identical in terms of how they're used here, so I will change it
back to unique_ptr to avoid extra #include.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:580: if
(ui_initialized_)
On 2016/12/13 18:45:39, msw wrote:
> aside: I prefer this conditional formatting over single-line if/then, but
being
> consistent locally is helpful...

Done.

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2176613003/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:196:
if (!issue_info.secondary_actions.empty()) {
On 2016/12/13 18:45:39, msw wrote:
> tangential q: Are there ever instances where there are multiple secondary
> actions? Should this DCHECK that there's only one, or should the vector just
> become a single item?

Our UI does not support showing multiple secondary actions, hence only the first
one is taken here. There aren't any Issues with >1 secondary actions (we also
own the code that creates the Issues). I can add a DCHECK here.

We will rewrite our UX in the future, and it would be a good time to consider
the following to simplify our data model: whether there needs to be a
distinction between default and secondary actions, and whether >2 actions are
needed (and if there should be an upper limit, how they should be shown if so).

[msw, 2016-12-13 19:35:11]

lgtm with a q

https://codereview.chromium.org/2176613003/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2176613003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:212:
issue_action_type_num <=
q: why change this to consider NUM_VALUES valid?

[imcheng, 2016-12-13 19:54:17]

https://codereview.chromium.org/2176613003/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2176613003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:212:
issue_action_type_num <=
On 2016/12/13 19:35:11, msw wrote:
> q: why change this to consider NUM_VALUES valid?

I have changed NUM_VALUES to be equal to the last valid value (so we can write
switch statements without using default in the mojo conversion code).
Previously, it was a value on its own.

[imcheng, 2016-12-13 20:04:49]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 20:05:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 20:18:50]

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

[commit-bot: I haz the power, 2016-12-13 20:18:51]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[imcheng, 2016-12-13 21:12:23]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 21:12:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 22:38:24]

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

[commit-bot: I haz the power, 2016-12-13 22:38:25]

Dry run: This issue passed the CQ dry run.

[imcheng, 2016-12-13 22:43:38]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2016-12-13 22:43:39]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
dcheng@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2176613003/#ps320001
(title: "fix tests")

[imcheng, 2016-12-13 22:44:07]

The CQ bit was unchecked by imcheng@chromium.org

[imcheng, 2016-12-13 22:49:31]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2016-12-13 22:50:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 22:56:31]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1481669371120860,
"parent_rev": "76cb3c7b5bcb84e9eec703f1ff8123a6f9497872", "commit_rev":
"8925e18bf11ab705c72495e69feb338843ff28e7"}

[commit-bot: I haz the power, 2016-12-13 22:57:03]

Description was changed from

==========
[Media Router] Clean up issues related code.

- Split all fields except for ID in Issue into IssueInfo struct, which is safely
copyable.
- Modified Register/UnregisterIssuesObserver behavior to not require a manaul
Unregiser call. (similar to MediaSinksObserver)
- Use StructTraits when converting Issue from/to Mojo land
- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.
- No need for MediaRouterAction to retain a copy of Issue object.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router] Clean up issues related code.

- Split all fields except for ID in Issue into IssueInfo struct, which is safely
copyable.
- Modified Register/UnregisterIssuesObserver behavior to not require a manaul
Unregiser call. (similar to MediaSinksObserver)
- Use StructTraits when converting Issue from/to Mojo land
- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.
- No need for MediaRouterAction to retain a copy of Issue object.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2176613003
==========

[commit-bot: I haz the power, 2016-12-13 22:57:05]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-12-13 22:59:45]

Description was changed from

==========
[Media Router] Clean up issues related code.

- Split all fields except for ID in Issue into IssueInfo struct, which is safely
copyable.
- Modified Register/UnregisterIssuesObserver behavior to not require a manaul
Unregiser call. (similar to MediaSinksObserver)
- Use StructTraits when converting Issue from/to Mojo land
- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.
- No need for MediaRouterAction to retain a copy of Issue object.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2176613003
==========

to

==========
[Media Router] Clean up issues related code.

- Split all fields except for ID in Issue into IssueInfo struct, which is safely
copyable.
- Modified Register/UnregisterIssuesObserver behavior to not require a manaul
Unregiser call. (similar to MediaSinksObserver)
- Use StructTraits when converting Issue from/to Mojo land
- Replace IssueAction class with Issue::ActionType as there is no need
for the class.
- Fully qualify Issue::Severity enum values for clarity.
- Reduce Issue ctor param list from 8 to 3. Introduce setters for
optional args.
- Rename is_global -> IsGlobal, is_blocking -> IsBlocking as they are
not simple getters.
- Clean up IssueManager logic.
- Replace string Issue ID with int. There is no need for GUID.
- No need for MediaRouterAction to retain a copy of Issue object.

BUG=571525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/466a89e738c071195c6bdbe9fe06ce871ead7a9f
Cr-Commit-Position: refs/heads/master@{#438321}
==========

[commit-bot: I haz the power, 2016-12-13 22:59:47]

Patchset 17 (id:??) landed as
https://crrev.com/466a89e738c071195c6bdbe9fe06ce871ead7a9f
Cr-Commit-Position: refs/heads/master@{#438321}