Shows a launch spinner next to a sink once it is selected to create a route. The launch spinner sur…

chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js

Index: chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
diff --git a/chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js b/chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
index 0ae3e15187235c0be7a294917f08e92d4302045d..66e12bff7145247d1e86e400a7bbd9fc25fdf5c9 100644
--- a/chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
+++ b/chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
@@ -103,6 +103,16 @@ Polymer({
     },
 
     /**
+     * True if the Media Router is creating a route.

[apacible, 2015/08/10 14:13:26]

Can you be more specific with this comment? Perhaps something along the lines
of: "True if we have requested route creation / False when the WebUI gets a
response and/or is not expecting a response anymore".

[haibinlu, 2015/08/11 01:31:47]

MR (now extension) handles when to stop marking sink.isLaunching true. so, it
does not make sense to list when this value is false.

[apacible, 2015/08/11 18:29:58]

Acknowledged.

+     *

[apacible, 2015/08/10 14:13:26]

nit: remove this line

[haibinlu, 2015/08/11 01:31:47]

Done.

+     * @private {boolean}
+     */
+    isLaunching_: {

[apacible, 2015/08/10 14:13:26]

nit: put in alphabetical order

[haibinlu, 2015/08/11 01:31:47]

Done.

+      type: Boolean,
+      value: false,
+    },
+
+    /**
      * The number of current local routes.
      * @private {number}
      */
@@ -167,7 +177,7 @@ Polymer({
     sinkList: {
       type: Array,
       value: [],
-      observer: 'rebuildSinkMap_',
+      observer: 'onSinkListUpdated_',
     },
 
     /**
@@ -191,7 +201,7 @@ Polymer({
 
   ready: function() {
     this.addEventListener('close-route-click', this.removeRoute);
-    this.currentView_ = this.CONTAINER_VIEW_.SINK_LIST;
+    this.showSinkList_();
   },
 
   attached: function() {
@@ -202,11 +212,21 @@ Polymer({
   },
 
   /**
-   * Adds |route| to |routeList|.
+   * Handles response of previous create route attempt.
    *
-   * @param {!media_router.Route} route The route to add.
+   * @param {string} sinkId The ID of the sink to which the Media Route was
+   *     creating a route.
+   * @param {?media_router.Route} route The newly create route to the sink

[apacible, 2015/08/10 14:13:26]

nit: created

[haibinlu, 2015/08/11 01:31:46]

Done.

+   *     if succeeded; null otherwise.
    */
-  addRoute: function(route) {
+  onCreateRouteResponseReceived: function(sinkId, route) {
+    this.isLaunching_ = false;
+    this.updateLaunchSpinner_(sinkId, false);
+    if (!route) {

[apacible, 2015/08/10 14:13:26]

Are we pushing an issue from MR if route creation failed?

[haibinlu, 2015/08/11 01:31:46]

That is one way to do that assuming MR has more info about failure. If we decide
to show a generic error, then UI can handle it.

[apacible, 2015/08/11 18:29:58]

Acknowledged.

+      // TODO(apacible) Show launch failure.
+      return;
+    }
+
     // Check if |route| already exists or if its associated sink
     // does not exist.
     if (this.routeMap_[route.id] || !this.sinkMap_[route.sinkId])
@@ -216,6 +236,7 @@ Polymer({
     // |sinkToRouteMap_| entry will be overwritten with that of the new route,
     // which results in the correct sink to route mapping.
     this.routeList.push(route);
+    this.showRouteDetails_(route);
   },
 
   /**
@@ -317,7 +338,8 @@ Polymer({
    * @private
    */
   computeSinkIconClass_: function(sinkId, sinkToRouteMap) {
-    return sinkToRouteMap[sinkId] ? 'active-sink' : '';
+    return sinkToRouteMap[sinkId] && sinkToRouteMap[sinkId].isLocal ?

[apacible, 2015/08/10 14:13:26]

Per UX review / discussion, we'll show the blue icon if:
...in WebUI (this case): any active route exists (local and non-local)
...in Toolbar: a local route exists

The sinkToRouteMap[sinkId].isLocal condition here is not needed.

[haibinlu, 2015/08/11 01:31:47]

Done.

+        'sink-icon active-sink' : 'sink-icon';
   },
 
   /**
@@ -358,23 +380,24 @@ Polymer({
    */
   maybeShowRouteDetailsOnOpen_: function(route) {
     if (this.localRouteCount_ == 1 && this.justOpened_ && route) {

[apacible, 2015/08/10 14:13:26]

nit: no curly braces

[haibinlu, 2015/08/11 01:31:47]

Done.

-      this.currentRoute_ = route;
-      this.currentView_ = this.CONTAINER_VIEW_.ROUTE_DETAILS;
+      this.showRouteDetails_(route);
     }
   },
 
   /**
-   * Creates a new route if |route| is null. Otherwise, shows the route
-   * details.
+   * Creates a new route if there is no route to the |sink| . Otherwise,
+   * shows the route details.
    *
    * @param {!media_router.Sink} sink The sink to use.
-   * @param {?media_router.Route} route The current route tied to |sink|.
    * @private
    */
-  showOrCreateRoute_: function(sink, route) {
+  showOrCreateRoute_: function(sink) {
+    var route = this.sinkToRouteMap_[sink.id];
     if (route) {
-      this.showRouteDetails_();
+      this.showRouteDetails_(route);
     } else {
+      this.isLaunching_ = true;
+      this.updateLaunchSpinner_(sink.id, true);
       this.fire('create-route', {
         sinkId: sink.id,
         selectedCastModeValue: this.selectedCastModeValue_
@@ -383,6 +406,23 @@ Polymer({
   },
 
   /**
+   * Shows or hides the launch spinner next to a sink.
+   *
+   * @param {string} sinkId The ID of the sink.
+   * @param {boolean} isLaunching Whether or not the media router is creating
+   *    a route to the sink.
+   * @private
+   */
+  updateLaunchSpinner_: function(sinkId, isLaunching) {
+    for (var index = 0; index < this.sinkList.length; index++) {
+      if (this.sinkList[index].id == sinkId) {
+        this.set(['sinkList', index, 'isLaunching'], isLaunching);

[apacible, 2015/08/10 14:13:26]

It looks like |isLaunching| param always corresponds with |this.isLaunching_|.
We can remove |isLaunching| and replace with |this.isLaunching_| directly in the
function.

[haibinlu, 2015/08/11 01:31:47]

right now, yes. but we ever allow multiple launches, the param is needed.
Plus, this method sets individual sink field, it is cleaner to have a param for
it.

[apacible, 2015/08/11 18:29:58]

Acknowledged.

+        return;
+      }
+    }
+  },
+
+  /**
    * Handles a cast mode selection. Updates |headerText|, |headerTextTooltip|,
    * and |selectedCastModeValue_|.
    *
@@ -406,15 +446,16 @@ Polymer({
   },
 
   /**
-   * Called when a sink is clicked. Updates |currentRoute_|.
+   * Called when a sink is clicked.
    *
    * @param {!Event} event The event object.
    * @private
    */
   onSinkClick_: function(event) {
-    var clickedSink = this.$.sinkList.itemForElement(event.target);
-    this.currentRoute_ = this.sinkToRouteMap_[clickedSink.id];
-    this.showOrCreateRoute_(clickedSink, this.currentRoute_);
+    if (this.isLaunching_)
+      return;
+    this.showOrCreateRoute_(
+      this.$.sinkList.itemForElement(event.target));

[apacible, 2015/08/10 14:13:26]

Can this fit on the previous line?

[haibinlu, 2015/08/11 01:31:47]

Done.

   },
 
   /**
@@ -453,6 +494,16 @@ Polymer({
   },
 
   /**
+   * Called when |sinkList| is updated.
+   *
+   * @private
+   */
+  onSinkListUpdated_: function() {

[apacible, 2015/08/10 14:13:26]

nit: alphabetize

[haibinlu, 2015/08/11 01:31:46]

Done.

+    this.rebuildSinkMap_();
+    this.rebuildLaunchState_();
+  },
+
+  /**
    * Called when |sinkList| is updated. Rebuilds |sinkMap_|.
    *
    * @private
@@ -466,20 +517,33 @@ Polymer({
   },
 
   /**
+   * Called when |sinkList| is updated. Rebuilds |isLaunching_|.

[apacible, 2015/08/10 14:13:26]

nit: "Update" rather than "Rebuilds" since |isLaunching_| is just a boolean,
here and for function name.

[haibinlu, 2015/08/11 01:31:47]

Done.

+   *
+   * @private
+   */
+  rebuildLaunchState_: function() {

[apacible, 2015/08/10 14:13:26]

nit: alphabetize

[haibinlu, 2015/08/11 01:31:47]

Done.

+    this.isLaunching_ = this.sinkList.some(function(sink) {
+      return sink.isLaunching;
+    }, this);
+  },
+
+  /**
    * Shows the cast mode list.
    *
    * @private
    */
   showCastModeList_: function() {
+    this.currentRoute_ = null;
     this.currentView_ = this.CONTAINER_VIEW_.CAST_MODE_LIST;
   },
 
   /**
    * Shows the route details.
-   *
+   * @param {?media_router.Route} route The route to show.

[apacible, 2015/08/10 14:13:26]

nit: newline between description and @param

[haibinlu, 2015/08/11 01:31:46]

Done.

    * @private
    */
-  showRouteDetails_: function() {
+  showRouteDetails_: function(route) {
+    this.currentRoute_ = route;
     this.currentView_ = this.CONTAINER_VIEW_.ROUTE_DETAILS;
   },
 
@@ -489,6 +553,7 @@ Polymer({
    * @private
    */
   showSinkList_: function() {
+    this.currentRoute_ = null;
     this.currentView_ = this.CONTAINER_VIEW_.SINK_LIST;
   },
 
@@ -498,6 +563,8 @@ Polymer({
    * @private
    */
   toggleCastModeHidden_: function() {
+    if (this.isLaunching_)

[apacible, 2015/08/10 14:13:26]

Can we gray out the arrow toggle icon? Otherwise, there won't be any visual
feedback for why the cast mode list isn't being toggled if we just do nothing.

[haibinlu, 2015/08/11 01:31:46]

Done.

+      return;
     if (this.currentView_ == this.CONTAINER_VIEW_.CAST_MODE_LIST)
       this.showSinkList_();
     else