These data structures support the Media Router Dialog webui.

These data structures support the Media Router Dialog webui.

media_cast_mode.{cc,h}
A cast mode represents one way that the current WebContents (i.e., tab) may be presented to a media sink.

media_sink_with_cast_modes.{cc,h}
Contains information on a MediaSink and the set of cast modes it is compatible with.

Also includes strings, unit tests and .gyp/.gn updates.

BUG=464208
Committed: https://crrev.com/d6185128f992fb93bde77718c36a1cd8aec58dd9
Cr-Commit-Position: refs/heads/master@{#326962}

[mark a. foltz, 2015-04-16 23:48:48]

mfoltz@chromium.org changed reviewers:
+ apacible@chromium.org, jhawkins@chromium.org

[James Hawkins, 2015-04-17 14:58:06]

Please fix trybots.

[mark a. foltz, 2015-04-20 20:15:46]

On 2015/04/17 14:58:06, James Hawkins wrote:
> Please fix trybots.

Done and resubmitted.  PTAL

[mark a. foltz, 2015-04-20 20:22:49]

On 2015/04/20 20:15:46, mark a. foltz wrote:
> On 2015/04/17 14:58:06, James Hawkins wrote:
> > Please fix trybots.
> 
> Done and resubmitted.  PTAL

Never mind, it looks like there is a different GYP issue.

[mark a. foltz, 2015-04-21 23:37:41]

jhawkins@/apacible@: Sorted out the gyp/gn issues and this passed trybots
(browser test failure is apparently unrelated).

PTAL.

[apacible, 2015-04-22 21:49:15]

https://codereview.chromium.org/1066683008/diff/120001/chrome/app/media_route...
File chrome/app/media_router_strings.grdp (right):

https://codereview.chromium.org/1066683008/diff/120001/chrome/app/media_route...
chrome/app/media_router_strings.grdp:34: 
nit: remove empty line

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_cast_mode.h (right):

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.h:30: NUM_CAST_MODES,
Please add a comment making it very clear that NUM_CAST_MODES is a count and not
a type of cast mode.

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.h:42: bool
IsValidCastModeNum(int cast_mode_num);
Add a comment describing the function.

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_sink_with_cast_modes.h (right):

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_sink_with_cast_modes.h:23: MediaSink
sink;
Rename to sink_?

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_sink_with_cast_modes.h:24:
CastModeSet cast_modes;
Where is cast_modes initialized? Rename to cast_modes_?

[mark a. foltz, 2015-04-23 04:57:58]

Thanks

https://codereview.chromium.org/1066683008/diff/120001/chrome/app/media_route...
File chrome/app/media_router_strings.grdp (right):

https://codereview.chromium.org/1066683008/diff/120001/chrome/app/media_route...
chrome/app/media_router_strings.grdp:34: 
On 2015/04/22 21:49:15, apacible wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_cast_mode.h (right):

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.h:30: NUM_CAST_MODES,
On 2015/04/22 21:49:15, apacible wrote:
> Please add a comment making it very clear that NUM_CAST_MODES is a count and
not
> a type of cast mode.

Done.

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.h:42: bool
IsValidCastModeNum(int cast_mode_num);
On 2015/04/22 21:49:15, apacible wrote:
> Add a comment describing the function.

Done.

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_sink_with_cast_modes.h (right):

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_sink_with_cast_modes.h:23: MediaSink
sink;
On 2015/04/22 21:49:15, apacible wrote:
> Rename to sink_?

Data members of structs don't have trailing underscores (as they are public
members).

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_sink_with_cast_modes.h:24:
CastModeSet cast_modes;
On 2015/04/22 21:49:15, apacible wrote:
> Where is cast_modes initialized?

CastModeSet is a std::set and so cast_modes will be initialized with the default
constructor for std::set when declared this way.

> Rename to cast_modes_?

See above.

[apacible, 2015-04-23 17:47:20]

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_cast_mode.h (right):

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.h:30: NUM_CAST_MODES,
On 2015/04/23 04:57:58, mark a. foltz wrote:
> On 2015/04/22 21:49:15, apacible wrote:
> > Please add a comment making it very clear that NUM_CAST_MODES is a count and
> not
> > a type of cast mode.
> 
> Done.

Was this included in patch set 8? I'm not seeing this on the code review tool.

https://codereview.chromium.org/1066683008/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.h:42: bool
IsValidCastModeNum(int cast_mode_num);
On 2015/04/23 04:57:58, mark a. foltz wrote:
> On 2015/04/22 21:49:15, apacible wrote:
> > Add a comment describing the function.
> 
> Done.

I don't see this either.

[mark a. foltz, 2015-04-23 18:17:08]

Sorry, somehow the changes got dropped from that patchset.  PTAL at PS#9.

[apacible, 2015-04-23 20:14:40]

lgtm, thanks

[mark a. foltz, 2015-04-23 23:06:41]

On 2015/04/23 20:14:40, apacible wrote:
> lgtm, thanks

Thanks.

@jhawkins - ping for reviewing webui/ changes.

[mark a. foltz, 2015-04-23 23:06:42]

On 2015/04/23 20:14:40, apacible wrote:
> lgtm, thanks

Thanks.

@jhawkins - ping for reviewing webui/ changes.

[James Hawkins, 2015-04-23 23:09:03]

https://codereview.chromium.org/1066683008/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_cast_mode.cc (right):

https://codereview.chromium.org/1066683008/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.cc:61:
CHECK(!cast_modes.empty());
Who calls this?

https://codereview.chromium.org/1066683008/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_cast_mode.h (right):

https://codereview.chromium.org/1066683008/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.h:14: // be presented to a
media sink.  These should be declared in the priority order
Optional nit: s/should be/are/ or s/should be/must be/

[mark a. foltz, 2015-04-24 18:29:14]

https://codereview.chromium.org/1066683008/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_cast_mode.cc (right):

https://codereview.chromium.org/1066683008/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.cc:61:
CHECK(!cast_modes.empty());
On 2015/04/23 23:09:03, James Hawkins wrote:
> Who calls this?

Code in our upstream repo:

http://2-dot-cmrdiff.appspot.com/sidebyside?project=chrome_media_router&filen...

This function doesn't make any sense with an empty set, but I changed it to a
LOG(ERROR) and returning a value just in case there is an upstream bug.

https://codereview.chromium.org/1066683008/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_cast_mode.h (right):

https://codereview.chromium.org/1066683008/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_cast_mode.h:14: // be presented to a
media sink.  These should be declared in the priority order
On 2015/04/23 23:09:03, James Hawkins wrote:
> Optional nit: s/should be/are/ or s/should be/must be/

Done.

[James Hawkins, 2015-04-24 23:42:46]

lgtm

[mark a. foltz, 2015-04-25 05:04:38]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2015-04-25 05:04:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/1066683008/#ps180001
(title: "Respond to jhawkins@ comments.")

[commit-bot: I haz the power, 2015-04-25 05:04:46]

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

[commit-bot: I haz the power, 2015-04-25 06:15:01]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-25 06:16:06]

Patchset 10 (id:??) landed as
https://crrev.com/d6185128f992fb93bde77718c36a1cd8aec58dd9
Cr-Commit-Position: refs/heads/master@{#326962}