[MR] Add description field to MediaSink.

[MR] Add description field to MediaSink.

description is an optional field on MediaSink which can be used for
displaying information on the sink itself in the UI.

The sink description will be displayed as a subtext under the sink
name. If the sink is currently associated with a route, then the route
description is displayed in the subtext instead.

Applied conditional stamping to the subtext in media-router-container.
Currently, most sinks will not have subtexts in typical usage.

BUG=545138
Committed: https://crrev.com/a743f7f9316a2336257b6daf0570e4c2b2a982e6
Cr-Commit-Position: refs/heads/master@{#363314}

[imcheng, 2015-11-25 00:54:17]

Patchset #1 (id:1) has been deleted

[imcheng, 2015-11-25 00:54:44]

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

[imcheng, 2015-11-25 00:54:45]

Jennifer: PTAL

+CC amp

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:98:
.sink-subtext {
Instead of making the subtext belong to both class sink-text and sink-subtext, I
merged the attributes of sink-text into sink-subtext and made the subtext only
class sink-subtext. Intuitively I feel this is better but you probably know more
CSS than I do.

[apacible, 2015-11-25 01:09:01]

I haven't taken a look yet, but please add screenshots (route description vs
sink description, RTL vs LTR, anything else relevant to the changes). Thanks!

[imcheng, 2015-11-25 18:17:04]

LTR: https://screenshot.googleplex.com/o2HKnvBLQFp
RTL: https://screenshot.googleplex.com/s4ecJVvoAkk

[apacible, 2015-11-25 23:05:41]

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

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_sink.cc:12: : sink_id_(sink_id), name_(name),
icon_type_(icon_type) {}
What happens if |sink_id| or |name| isn't provided?

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sink.h (left):

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_sink.h:42: // Optional, can use an empty
string if no sink name is available.
Is |name_| no longer optional?

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:98:
.sink-subtext {
On 2015/11/25 00:54:45, imcheng1 wrote:
> Instead of making the subtext belong to both class sink-text and sink-subtext,
I
> merged the attributes of sink-text into sink-subtext and made the subtext only
> class sink-subtext. Intuitively I feel this is better but you probably know
more
> CSS than I do.

Let's switch back to the previous CSS classes and keep the ".sink-subtext"
renaming, and switch to multi-classes for the sink subtext <div>.

We want to keep the ".sink-text" and ".sink-subtext" styling consistent with
what's in "sink-text", with additional styling for ".sink-subtext". Having
multiple classes for an element is normal in cases like this.

Unfortunately, CSS doesn't support 'native' inheritance, though some
pre-processors like Less (runs on node) supports abstracting out common CSS.

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:92:
<template is="dom-if" if="[[!computeSinkSubtextHidden_(item,
sinkToRouteMap_)]]">
nit: Does the "if=...>" fit on the next line?

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:435:
computeSinkSubtext_: function(sink, sinkToRouteMap) {
nit: Here and computeSinkSubtextHidden_() -- move after
computeSinkListViewHidden_() for alphabetical order.

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:448:
return !sink.description && !sinkToRouteMap[sink.id];
Please also check that the strings are not empty/whitespace.

https://codereview.chromium.org/1475843002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/media_router/media_router_container_tests.js
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_tests.js:309: 
nit: remove newline.

https://codereview.chromium.org/1475843002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_tests.js:312:
container.allSinks = [
Could we just update |fakeSinkList| to have some descriptions, rather than
creating a new list here? If it's too fragile to keep updating, please add
comments below on why each Sink/Route is set up specifically like this.

[imcheng, 2015-12-01 19:29:27]

PTAL

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

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_sink.cc:12: : sink_id_(sink_id), name_(name),
icon_type_(icon_type) {}
On 2015/11/25 23:05:41, apacible wrote:
> What happens if |sink_id| or |name| isn't provided?

The assumption is that they will always be provided.

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sink.h (left):

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_sink.h:42: // Optional, can use an empty
string if no sink name is available.
On 2015/11/25 23:05:41, apacible wrote:
> Is |name_| no longer optional?

No. It was optional before because MR used to have a full MediaSink in a
MediaRoute object, but in MRPM a Route object only contained a sink id so we had
to use an empty string as name when converting from MRPM to MR. This is no
longer the case because MR MediaRoute only contains a sink id now.

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:98:
.sink-subtext {
On 2015/11/25 23:05:41, apacible wrote:
> On 2015/11/25 00:54:45, imcheng1 wrote:
> > Instead of making the subtext belong to both class sink-text and
sink-subtext,
> I
> > merged the attributes of sink-text into sink-subtext and made the subtext
only
> > class sink-subtext. Intuitively I feel this is better but you probably know
> more
> > CSS than I do.
> 
> Let's switch back to the previous CSS classes and keep the ".sink-subtext"
> renaming, and switch to multi-classes for the sink subtext <div>.
> 
> We want to keep the ".sink-text" and ".sink-subtext" styling consistent with
> what's in "sink-text", with additional styling for ".sink-subtext". Having
> multiple classes for an element is normal in cases like this.
> 
> Unfortunately, CSS doesn't support 'native' inheritance, though some
> pre-processors like Less (runs on node) supports abstracting out common CSS.

Okay. Removed the additional properties from sink-subtext.

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:92:
<template is="dom-if" if="[[!computeSinkSubtextHidden_(item,
sinkToRouteMap_)]]">
On 2015/11/25 23:05:41, apacible wrote:
> nit: Does the "if=...>" fit on the next line?

Done.

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:435:
computeSinkSubtext_: function(sink, sinkToRouteMap) {
On 2015/11/25 23:05:41, apacible wrote:
> nit: Here and computeSinkSubtextHidden_() -- move after
> computeSinkListViewHidden_() for alphabetical order.

Done.

https://codereview.chromium.org/1475843002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:448:
return !sink.description && !sinkToRouteMap[sink.id];
On 2015/11/25 23:05:41, apacible wrote:
> Please also check that the strings are not empty/whitespace.

Done.

https://codereview.chromium.org/1475843002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/media_router/media_router_container_tests.js
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_tests.js:309: 
On 2015/11/25 23:05:41, apacible wrote:
> nit: remove newline.

Done.

https://codereview.chromium.org/1475843002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_tests.js:312:
container.allSinks = [
On 2015/11/25 23:05:41, apacible wrote:
> Could we just update |fakeSinkList| to have some descriptions, rather than
> creating a new list here? If it's too fragile to keep updating, please add
> comments below on why each Sink/Route is set up specifically like this.

Done. In general, I prefer having separate test data for each test case even if
that means duplicated code.

[apacible, 2015-12-02 23:27:27]

lgtm

https://codereview.chromium.org/1475843002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/media_router/media_router_container_tests.js
(right):

https://codereview.chromium.org/1475843002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_tests.js:312:
container.allSinks = [
On 2015/12/01 19:29:27, imcheng1 wrote:
> On 2015/11/25 23:05:41, apacible wrote:
> > Could we just update |fakeSinkList| to have some descriptions, rather than
> > creating a new list here? If it's too fragile to keep updating, please add
> > comments below on why each Sink/Route is set up specifically like this.
> 
> Done. In general, I prefer having separate test data for each test case even
if
> that means duplicated code.

Acknowledged.

https://codereview.chromium.org/1475843002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1475843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:499:
return isEmptyOrWhitespace_(route.description);
return !route || this.isEmptyOrWhitespace_(route.description);

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:469:
* Returns the subtext to be shown for |sink|. |computeSinkSubtextHidden_|
Phrasing suggestion:

Returns the subtext to be shown for |sink|. |computeSinkSubtext_| is only called
if |computeSinkSubtextHidden_| returns |false| for the corresponding |sink| and
|sinkToRouteMap|.

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:485:
* Returns whether the sink subtext <div> for |sink| should be hidden.
nit: Here and L488, omit "<div>" to keep comments more flexible/general in case
the HTML elements change. The element detail is also not needed for this
function's comments to make sense.

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:62: 
Were the new lines here and below added/deleted just for readability?

[imcheng, 2015-12-03 23:11:07]

https://codereview.chromium.org/1475843002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1475843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:499:
return isEmptyOrWhitespace_(route.description);
On 2015/12/02 23:27:26, apacible wrote:
> return !route || this.isEmptyOrWhitespace_(route.description);

route is already not null due to L496-497.

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:469:
* Returns the subtext to be shown for |sink|. |computeSinkSubtextHidden_|
On 2015/12/02 23:27:26, apacible wrote:
> Phrasing suggestion:
> 
> Returns the subtext to be shown for |sink|. |computeSinkSubtext_| is only
called
> if |computeSinkSubtextHidden_| returns |false| for the corresponding |sink|
and
> |sinkToRouteMap|.

Done.

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:485:
* Returns whether the sink subtext <div> for |sink| should be hidden.
On 2015/12/02 23:27:26, apacible wrote:
> nit: Here and L488, omit "<div>" to keep comments more flexible/general in
case
> the HTML elements change. The element detail is also not needed for this
> function's comments to make sense.

Done.

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1475843002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:62: 
On 2015/12/02 23:27:26, apacible wrote:
> Were the new lines here and below added/deleted just for readability?

Yes. I find new line after indented statement helps readability.

[apacible, 2015-12-03 23:12:45]

https://codereview.chromium.org/1475843002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1475843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:499:
return isEmptyOrWhitespace_(route.description);
On 2015/12/03 23:11:07, imcheng1 wrote:
> On 2015/12/02 23:27:26, apacible wrote:
> > return !route || this.isEmptyOrWhitespace_(route.description);
> 
> route is already not null due to L496-497.

Sorry, I meant to suggest consolidating L496-499 as a one liner.

[imcheng, 2015-12-03 23:20:48]

https://codereview.chromium.org/1475843002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1475843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:499:
return isEmptyOrWhitespace_(route.description);
On 2015/12/03 23:12:45, apacible wrote:
> On 2015/12/03 23:11:07, imcheng1 wrote:
> > On 2015/12/02 23:27:26, apacible wrote:
> > > return !route || this.isEmptyOrWhitespace_(route.description);
> > 
> > route is already not null due to L496-497.
> 
> Sorry, I meant to suggest consolidating L496-499 as a one liner.

Ah gotcha. Done.

[imcheng, 2015-12-04 01:01:47]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2015-12-04 01:01:48]

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/1475843002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-12-04 01:05:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1475843002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1475843002/120001

[commit-bot: I haz the power, 2015-12-04 03:11:41]

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

[commit-bot: I haz the power, 2015-12-04 03:11:42]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[imcheng, 2015-12-04 22:36:28]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2015-12-05 00:36:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1475843002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1475843002/120001

[commit-bot: I haz the power, 2015-12-05 00:41:40]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-12-05 00:42:22]

Description was changed from

==========
[MR] Add description field to MediaSink.

description is an optional field on MediaSink which can be used for
displaying information on the sink itself in the UI.

The sink description will be displayed as a subtext under the sink
name. If the sink is currently associated with a route, then the route
description is displayed in the subtext instead.

Applied conditional stamping to the subtext in media-router-container.
Currently, most sinks will not have subtexts in typical usage.

BUG=545138
==========

to

==========
[MR] Add description field to MediaSink.

description is an optional field on MediaSink which can be used for
displaying information on the sink itself in the UI.

The sink description will be displayed as a subtext under the sink
name. If the sink is currently associated with a route, then the route
description is displayed in the subtext instead.

Applied conditional stamping to the subtext in media-router-container.
Currently, most sinks will not have subtexts in typical usage.

BUG=545138

Committed: https://crrev.com/a743f7f9316a2336257b6daf0570e4c2b2a982e6
Cr-Commit-Position: refs/heads/master@{#363314}
==========

[commit-bot: I haz the power, 2015-12-05 00:42:23]

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