Redesign cast-mode-picker UX and switch to core/paper elements.

Redesign cast-mode-picker UX and switch to core/paper elements.

The cast mode popover has been removed and the cast mode list now appears in lieu of the sink picker when toggled by the arrow button. Since there is no immediate cast action that someone can take while viewing cast modes (unlike the sink picker or route details), any issues that arise will not appear while they are still in the cast mode list. Once they toggle back to the sink picker or select a cast mode, will the UI show any issue that has been set.

Other changes:
- Removes transparent overlay.
- Removes cast-mode-picker custom polymer element.
- Replaces cast-mode-picker content with core-menu and paper-items.
- Shows (new) localized "Select cast mode" string when showing the cast mode list.
- Update comments.

Committed: https://crrev.com/8e16642e2bbbc18c2d24b7e570a1347cc1c463d0
Cr-Commit-Position: refs/heads/master@{#330025}

[apacible, 2015-05-12 21:29:42]

Patchset #1 (id:1) has been deleted

[apacible, 2015-05-12 21:49:30]

Patchset #1 (id:20001) has been deleted

[apacible, 2015-05-12 23:15:04]

Patchset #1 (id:40001) has been deleted

[apacible, 2015-05-12 23:15:10]

Patchset #1 (id:60001) has been deleted

[apacible, 2015-05-12 23:43:03]

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

[apacible, 2015-05-12 23:46:15]

PTAL, thanks!

I'll be rewriting the show/hide logic when we switch to 0.8 since filters won't
be supported in v0.8+.

Sink Picker View:
- LTR: https://screenshot.googleplex.com/XvB4mif7DyU.png
- RTL: https://screenshot.googleplex.com/LCfQcjNHprF.png

Cast Mode Picker View:
- LTR: https://screenshot.googleplex.com/gAsGpcPo2pE.png
- RTL: https://screenshot.googleplex.com/2i1Py6Mb5bn.png

Preview UX (for reference):
- LTR: https://screenshot.googleplex.com/TdPBcbx0fLK.png
- RTL: https://screenshot.googleplex.com/mrGBpPbXokt.png

[mark a. foltz, 2015-05-13 01:47:10]

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

https://codereview.chromium.org/1140823003/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:16:
class="{{ {state: currentView_, castModeHidden: castModeHidden_} |
isHeaderHidden}}">
It seems like cast mode picking and sink picking are mutually exclusive now.  
Can we add a "picking cast mode" state to currentView_ and eliminate
castModeHidden_?

https://codereview.chromium.org/1140823003/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:18:
<span class="{{ {state: currentView_, castModeHidden: castModeHidden_} |
isSinkPickerHidden}}">
Why is the filter isSinkPickerHidden used to determine if the header text is
shown?  Wouldn't this be the same as isHeaderHidden?

https://codereview.chromium.org/1140823003/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:35:
https://github.com/Polymer/polymer/issues/946. Remove when core-menu
You could say "Remove here and elsewhere" so you only have to leave one TODO in
this file.

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

https://codereview.chromium.org/1140823003/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:289:
if (clickedMode.type == castMode.type) {
Is this just to check that clickedMode is member of this.castModeList?  But I
think castModeList is used to construct the menu.  So it seems like you don't
need the loop.

[apacible, 2015-05-14 21:24:45]

Patchset #2 (id:100001) has been deleted

[apacible, 2015-05-14 21:25:30]

Patchset #2 (id:120001) has been deleted

[apacible, 2015-05-14 21:30:59]

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

https://codereview.chromium.org/1140823003/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:16:
class="{{ {state: currentView_, castModeHidden: castModeHidden_} |
isHeaderHidden}}">
On 2015/05/13 01:47:09, mark a. foltz wrote:
> It seems like cast mode picking and sink picking are mutually exclusive now.  
> Can we add a "picking cast mode" state to currentView_ and eliminate
> castModeHidden_?

Talked offline, will keep this as is. Filters will be removed and show/hide
logic rewritten in upcoming patch to update elements to 0.8.

https://codereview.chromium.org/1140823003/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:18:
<span class="{{ {state: currentView_, castModeHidden: castModeHidden_} |
isSinkPickerHidden}}">
On 2015/05/13 01:47:09, mark a. foltz wrote:
> Why is the filter isSinkPickerHidden used to determine if the header text is
> shown?  Wouldn't this be the same as isHeaderHidden?

There's two header texts -- One is for the sink picker view (e.g. "Cast to..."),
which is updated every time a cast mode is selected, and the other for the cast
mode view ("Select cast mode"). See L21 for the other option.

https://codereview.chromium.org/1140823003/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:35:
https://github.com/Polymer/polymer/issues/946. Remove when core-menu
On 2015/05/13 01:47:09, mark a. foltz wrote:
> You could say "Remove here and elsewhere" so you only have to leave one TODO
in
> this file.

Done.

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

https://codereview.chromium.org/1140823003/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:289:
if (clickedMode.type == castMode.type) {
On 2015/05/13 01:47:10, mark a. foltz wrote:
> Is this just to check that clickedMode is member of this.castModeList?  But I
> think castModeList is used to construct the menu.  So it seems like you don't
> need the loop.

Done.

[michaelpg, 2015-05-14 22:02:16]

polymer stuff lgtm w/ suggestion

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:16:
class="{{ {state: currentView_, castModeHidden: castModeHidden_} |
isHeaderHidden}}">
instead of using classes, you can just bind to hidden:

    hidden?="{{ {...} | isHeaderHidden}}"

and have isHeaderHidden return a boolean (which, from the name, would be
expected).

https://www.polymer-project.org/0.5/docs/polymer/binding-types.html#condition...

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:187:
*   castModeHidden: boolean}} value The parameters passed into this filter.
nit: 4-space indent. might look better if each property is on a separate line.

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:216:
*   The parameters passed into this filter.
nit: 4 space indent

[apacible, 2015-05-14 22:46:52]

Patchset #3 (id:160001) has been deleted

[apacible, 2015-05-14 22:47:27]

Patchset #3 (id:180001) has been deleted

[mark a. foltz, 2015-05-14 22:49:37]

lgtm

[apacible, 2015-05-14 23:15:10]

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:16:
class="{{ {state: currentView_, castModeHidden: castModeHidden_} |
isHeaderHidden}}">
On 2015/05/14 22:02:15, michaelpg wrote:
> instead of using classes, you can just bind to hidden:
> 
>     hidden?="{{ {...} | isHeaderHidden}}"
> 
> and have isHeaderHidden return a boolean (which, from the name, would be
> expected).
> 
>
https://www.polymer-project.org/0.5/docs/polymer/binding-types.html#condition...

Done, thanks!

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:187:
*   castModeHidden: boolean}} value The parameters passed into this filter.
On 2015/05/14 22:02:15, michaelpg wrote:
> nit: 4-space indent. might look better if each property is on a separate line.

Done. Also updated other function comments for 4-space indent with more than one
line for @param.

https://codereview.chromium.org/1140823003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:216:
*   The parameters passed into this filter.
On 2015/05/14 22:02:15, michaelpg wrote:
> nit: 4 space indent

Done.

[apacible, 2015-05-14 23:27:50]

The CQ bit was checked by apacible@chromium.org

[apacible, 2015-05-14 23:27:51]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1140823003/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-05-14 23:28:51]

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

[apacible, 2015-05-14 23:36:35]

Oops, didn't get OWNER lgtm for chrome/app/media_router_strings.grdp. jhawkins@,
PTAL. Thanks!

[apacible, 2015-05-14 23:36:40]

The CQ bit was unchecked by apacible@chromium.org

[apacible, 2015-05-15 01:22:34]

Patchset #4 (id:220001) has been deleted

[apacible, 2015-05-15 01:29:56]

> Oops, didn't get OWNER lgtm for chrome/app/media_router_strings.grdp. 

Talked offline; added mfoltz@ and wez@ as OWNERs of media_router_strings.grdp.

[apacible, 2015-05-15 01:30:00]

The CQ bit was checked by apacible@chromium.org

[apacible, 2015-05-15 01:30:00]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1140823003/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-15 01:30:34]

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

[commit-bot: I haz the power, 2015-05-15 02:41:38]

Committed patchset #4 (id:240001)

[commit-bot: I haz the power, 2015-05-15 02:42:29]

Patchset 4 (id:??) landed as
https://crrev.com/8e16642e2bbbc18c2d24b7e570a1347cc1c463d0
Cr-Commit-Position: refs/heads/master@{#330025}