[Media Router] Wiring for searching route providers for new sinks.

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 f22eb6060b7c1917e965499945928b544fe1b7b3..0bd8889028d2ae4caba64ed628a1dacc0d141273 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
@@ -2,6 +2,139 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+/**
+ * This class holds state that represents the UI's view of the search process.
+ * It is essentially waiting for three things: the sink ID of the found sink,
+ * the found sink itself, and a route from the sink to the the desired media
+ * source. This code is agnostic of the order in which these events occur, but
+ * the container will drop the route when it is received if we don't get the
+ * sink ID first. Lastly, onCreateRouteResponseReceived() also depends on the
+ * mapping from the pseudo sink ID to the real sink ID, which is only known to
+ * this object. So the process is also not complete until we have provided that
+ * call with a mapping from the pseudo sink ID to the real sink ID.
+ * @param {!media_router.Sink} pseudoSink Pseudo sink that started the search.
+ * @constructor
+ */
+var PseudoSinkSearchStateMachine = function(pseudoSink) {

[apacible, 2016/04/06 21:27:01]

Could this be pulled out into a separate file?

[amp, 2016/04/06 21:29:36]

Probably worthwhile putting this into it's own file rather than here with the
container.  I'm not sure how easy it is to pull files together in the MR UI
build system though.

Also do you have a diagram of the different states and what transitions go
between them?  (not for the code, but perhaps a link in a doc somewhere).

[btolsch, 2016/04/08 09:31:25]

There was no diagram but this class does a lot less work now. Let me know if you
would like more documentation for the new version.

[btolsch, 2016/04/08 09:31:25]

Done.

+  /**
+   * Pseudo sink that started the search.
+   * @private {!media_router.Sink}
+   */
+  this.pseudoSink_ = pseudoSink;
+
+  /**
+   * The ID of the sink that is found by search.
+   * @private {string}
+   */
+  this.realSinkId_ = '';
+
+  /**
+   * Whether we have received a sink in the sink list with ID |realSinkId_|.
+   * @private {boolean}
+   */
+  this.hasRealSink_ = false;
+
+  /**
+   * Whether we have the created route in the route map.
+   * @private {boolean}
+   */
+  this.hasRoute_ = false;
+
+  /**
+   * Tracks whether we have translated the pseudo sink ID into the real sink ID
+   * for onCreateRouteResponseReceived(), which will receive the pseudo sink ID
+   * as the sink ID for the route.
+   * @private {boolean}
+   */
+  this.haveMappedRouteSinkId_ = false;
+
+  /**
+   * Whether we are done collecting information.

[apacible, 2016/04/06 21:27:02]

Please go into more detail on what information is being collected.

[amp, 2016/04/06 21:29:36]

If done is true is that the final state of the state machine?  It mentions just
being done collecting information so it's not clear if there are other steps
after that.

[btolsch, 2016/04/08 09:31:25]

Removed with the simplification.

[btolsch, 2016/04/08 09:31:25]

Removed with the simplification.

+   * @private {boolean}
+   */
+  this.done_ = false;
+};
+
+/**
+ * Record the real sink ID returned from the Media Router.
+ * @param {string} sinkId Real sink ID that is the result of the search.
+ */
+PseudoSinkSearchStateMachine.prototype.receiveSinkResponse = function(sinkId) {
+  this.realSinkId_ = sinkId;
+  if (!sinkId) {

[amp, 2016/04/06 21:29:36]

Should you check this before assigning realSinkId_, or does that not matter?

[btolsch, 2016/04/08 09:31:25]

Removed with the simplification.

+    this.done_ = true;
+  }
+};
+
+/**
+ * Checks the progress of the search the sink list and route map.
+ * @param {!Array<!media_router.Sink>} sinkList List of all sinks to check.
+ * @param {!Object<!string, !media_router.Route>} sinkToRouteMap Map from sink
+ *     ID to route to search for the pending route.
+ */
+PseudoSinkSearchStateMachine.prototype.checkProgress = function(
+    sinkList, sinkToRouteMap) {
+  if (this.done_) {
+    return;
+  }
+  if (!this.hasRealSink_) {
+    this.hasRealSink_ = !!this.realSinkId_ && sinkList.some(function(sink) {
+      return (sink.id == this.realSinkId_);
+    }, this);
+  }
+  if (!this.hasRoute_ && this.realSinkId_) {
+    this.hasRoute_ = this.realSinkId_ in sinkToRouteMap;
+  }
+  this.updateDone_();
+};
+
+/**
+ * Computes the value for |currentLaunchingSinkId_| based on the state of the
+ * search. It should be the pseudo sink ID until the real sink arrives, then the
+ * real sink ID until the process is done.
+ * @return {string} New value for |currentLaunchingSinkId_|.
+ */
+PseudoSinkSearchStateMachine.prototype.getCurrentLaunchingSinkId = function() {
+  return !this.hasRealSink_ ? this.pseudoSink_.id : this.realSinkId_;
+};
+
+/**
+ * Return the real sink ID produced by the search if we are given the pseudo
+ * sink ID that started the search. This is needed for
+ * onCreateRouteResponseReceived().
+ * @param {string} sinkId Sink ID of a newly-created route.
+ * @return {string} The sink ID that should be used by
+ *     onCreateRouteResponseReceived() to check the  created route.
+ */
+PseudoSinkSearchStateMachine.prototype.mapRouteSinkId = function(sinkId) {
+  if (sinkId == this.pseudoSink_.id) {
+    this.haveMappedRouteSinkId_ = true;
+    this.updateDone_();
+    return this.realSinkId_;
+  }
+  return sinkId;
+};
+
+/**
+ * Update |done_| from the current state.
+ * @private
+ */
+PseudoSinkSearchStateMachine.prototype.updateDone_ = function() {
+  if (!this.done_) {
+    this.done_ =
+        this.hasRealSink_ && this.hasRoute_ && this.haveMappedRouteSinkId_;
+  }
+};
+
+/**
+ * Returns whether we are still waiting for more information from the search

[amp, 2016/04/06 21:29:36]

Maybe reword to 'Returns false if we are still waiting ...'

[btolsch, 2016/04/08 09:31:25]

This method is now unnecessary and removed due to some simplification.

+ * process. The object can be dropped once this is |true|.
+ * @return {boolean} Returns |done_|.
+ */
+PseudoSinkSearchStateMachine.prototype.isDone = function() {
+  return this.done_;
+};
+
 // This Polymer element contains the entire media router interface. It handles
 // hiding and showing specific components.
 Polymer({
@@ -298,6 +431,16 @@ Polymer({
     },
 
     /**
+     * Pseudo sinks from MRPs that represent their ability to accept sink search
+     * requests.
+     * @private {!Array<!media_router.Sink>}
+     */
+    pseudoSinks_: {
+      type: Array,
+      value: [],
+    },
+
+    /**
      * Whether the next character input should cause a filter action metric to
      * be sent.
      * @type {boolean}
@@ -339,6 +482,15 @@ Polymer({
       },
     },
 
+    /*
+     * Helps manage the state of creating a sink and a route from a pseudo sink.
+     * @private {PseudoSinkSearchStateMachine}
+     */
+    searchInProgress_: {

[apacible, 2016/04/06 21:27:02]

Rename this to refer to the PseudoSinkSearchStateMachine. "searchInProgress_" is
a bit confusing since we also have properties like "isUserSearching_".

[btolsch, 2016/04/08 09:31:25]

Done.

+      type: Object,
+      value: null,
+    },
+
     /**
      * Label text for the user search input.
      * @private {string}
@@ -1135,6 +1287,16 @@ Polymer({
                                 substrings: matchSubstrings});
     }
     searchResultsToShow.sort(this.compareSearchMatches_);
+    searchResultsToShow = this.pseudoSinks_.filter(function(pseudoSink) {

[amp, 2016/04/06 21:29:36]

Is this overwriting the compareSearchMatches, or are they combined?

I'm not sure I follow the logic here.  Maybe a comment to explain how this works
would be useful.

[btolsch, 2016/04/08 09:31:25]

The original list is hiding at the end of this block because any relevant pseudo
sinks should be first. I added a comment explaining this.

+      return !searchResultsToShow.find(function(searchResult) {
+        return searchResult.sinkItem.name == searchInputText &&
+               searchResult.sinkItem.iconType == pseudoSink.iconType;
+      });
+    }).map(function(pseudoSink) {
+      pseudoSink.name = searchInputText;
+      return {sinkItem: pseudoSink,
+              substrings: [[0, searchInputText.length - 1]]};
+    }).concat(searchResultsToShow);
     this.searchResultsToShow_ = searchResultsToShow;
   },
 
@@ -1164,6 +1326,16 @@ Polymer({
   },
 
   /**
+   * Returns whether the given sink represents a pseudo-sink from an MRP.
+   * @param {!media_router.Sink} sink
+   * @return {boolean} |true| if the sink is a pseudo-sink.
+   * @private
+   */
+  isPseudoSink_: function(sink) {
+    return sink.id.startsWith('pseudo:');

[amp, 2016/04/06 21:29:36]

Also I think this is fine to pass across the native/extension boundary as a
string, but this function is called quite a few times and will do a string check
against every sink id potentially slowing things down.

Should we create a (UI side only) boolean property on a sink so we don't have to
do string compares?  It could be set as we are doing the conversion from
extension sink to UI sink.

[amp, 2016/04/06 21:29:36]

I think I also hard coded a string that was shared with the extension, but is
there a way we can share these better (maybe in the mojo interface somehow?). 
How did we handle this for source urn prefixes?

[btolsch, 2016/04/08 09:31:25]

I added a UI-specific flag and set it in the UI message handler's
SinksAndIdentityToValue(). This is also where some domain-related strings are
hard-coded, so at least they're in the same place for now.

[btolsch, 2016/04/08 09:31:25]

I don't know if this method should now be removed and the flag should just be
used everywhere. I'm inclined to remove it so I have for now. Let me know if you
think it should be put back for flexibility.

+  },
+
+  /**
    * Updates sink list when user is searching.
    * @param {boolean} isUserSearching Whether the user is searching for sinks.
    */
@@ -1329,6 +1501,13 @@ Polymer({
       return;
     }
 
+    if (this.searchInProgress_) {
+      sinkId = this.searchInProgress_.mapRouteSinkId(sinkId);
+      if (this.searchInProgress_.isDone()) {
+        this.searchInProgress_ = null;
+      }
+    }
+
     // Check that |sinkId| exists and corresponds to |currentLaunchingSinkId_|.
     if (!this.sinkMap_[sinkId] || this.currentLaunchingSinkId_ != sinkId) {
       this.fire('report-resolved-route', {
@@ -1421,6 +1600,34 @@ Polymer({
   },
 
   /**
+   * Called when a search result has been returned and is in |allSinks|. If it
+   * doesn't have any cast modes set when this function is called, this function

[apacible, 2016/04/06 21:27:01]

What scenarios would there be no cast modes set?

Is it possible to have a mismatch between the cast modes in the pseudo sink and
the new sink?

[btolsch, 2016/04/08 09:31:25]

There actually shouldn't be any of these scenarios anymore. This was part of an
intermediate state of the change, so I removed it now.

+   * should copy the cast modes from the pseudo sink that created the search. In
+   * either case it should clear |pseudoSinkForSearchInProgress_|.
+   *
+   * @param {string} sinkId The ID of the sink that is the result of the
+   *     currently pending search.
+   */
+  onReceiveSearchResult: function(sinkId) {

[amp, 2016/04/06 21:29:36]

Is it possible that this never gets called (the result gets dropped somehow)? 
Should we have a timer to cleanup searchInProgress in that case?

[btolsch, 2016/04/08 09:31:25]

This is now cleaned up on route response, which will always be called.

+    if (!this.searchInProgress_) {
+      return;
+    }
+    this.searchInProgress_.receiveSinkResponse(sinkId);
+    if (this.searchInProgress_.isDone()) {

[apacible, 2016/04/06 21:27:02]

Could we simplify this (1616 - 1627) to:

this.searchInProgress_.checkProgress(this.allSinks, this.sinkToRouteMap_);
if (this.searchInProgress_.isDone()) {
  this.currentLaunchingSinkId_ = '';
  this.searchInProgress_ = null;
} else {
  this.currentLaunchingSinkId_ =
      this.searchInProgress_.getCurrentLaunchingSinkId();
}

[btolsch, 2016/04/08 09:31:25]

This has been even further simplified by some other changes.

+      this.currentLaunchingSinkId_ = '';
+      this.searchInProgress_ = null;
+      return;
+    }
+
+    this.searchInProgress_.checkProgress(this.allSinks, this.sinkToRouteMap_);
+    this.currentLaunchingSinkId_ =
+        this.searchInProgress_.getCurrentLaunchingSinkId();
+    if (this.searchInProgress_.isDone()) {
+      this.searchInProgress_ = null;
+    }
+  },
+
+  /**
    * Called when a sink is clicked.
    *
    * @param {!Event} event The event object.
@@ -1430,7 +1637,24 @@ Polymer({
     var clickedSink = (this.isUserSearching_) ?
         this.$$('#searchResults').itemForElement(event.target).sinkItem :
         this.$.sinkList.itemForElement(event.target);
-    this.showOrCreateRoute_(clickedSink);
+    if (this.isPseudoSink_(clickedSink)) {
+      if (!this.searchInProgress_) {

[amp, 2016/04/06 21:29:36]

Are we showing a spinner while search is in progress?

What happens with current sinks if you click them while we are trying to create
a route?

[btolsch, 2016/04/08 09:31:25]

Yes this shows a spinner.

Clicking another sink to create a route does nothing while a search is in
progress, which is the same behavior for other pending create route calls.

+        this.searchInProgress_ = new PseudoSinkSearchStateMachine(clickedSink);
+        this.fire('search-sinks-and-create-route', {
+          id: clickedSink.id,
+          name: clickedSink.name,
+          domain: clickedSink.domain,
+          selectedCastMode:
+              this.shownCastModeValue_ == media_router.CastModeType.AUTO ?
+                  clickedSink.castModes & -clickedSink.castModes :
+                  this.shownCastModeValue_
+        });
+        this.currentLaunchingSinkId_ =
+            this.searchInProgress_.getCurrentLaunchingSinkId();
+      }
+    } else {
+      this.showOrCreateRoute_(clickedSink);
+    }
     this.fire('sink-click', {index: event['model'].index});
   },
 
@@ -1453,6 +1677,15 @@ Polymer({
       tempSinkToRouteMap[route.sinkId] = route;
     }, this);
 
+    if (this.searchInProgress_) {
+      this.searchInProgress_.checkProgress(this.allSinks, tempSinkToRouteMap);
+      this.currentLaunchingSinkId_ =
+          this.searchInProgress_.getCurrentLaunchingSinkId();
+      if (this.searchInProgress_.isDone()) {
+        this.searchInProgress_ = null;
+      }
+    }
+
     // If there is route creation in progress, check if any of the route ids
     // correspond to |pendingCreatedRouteId_|. If so, the newly created route
     // is ready to be displayed; switch to route details view.
@@ -1488,11 +1721,13 @@ Polymer({
    * name.
    */
   rebuildSinksToShow_: function() {
-    var sinksToShow = [];
+    var sinksToShow = this.allSinks.filter(function(sink) {
+      return !this.isPseudoSink_(sink);
+    }, this);
     if (this.userHasSelectedCastMode_) {
       // If user explicitly selected a cast mode, then we show only sinks that
       // are compatible with current cast mode or sinks that are active.
-      sinksToShow = this.allSinks.filter(function(element) {
+      sinksToShow = sinksToShow.filter(function(element) {
         return (element.castModes & this.shownCastModeValue_) ||
                this.sinkToRouteMap_[element.id];
       }, this);
@@ -1503,7 +1738,6 @@ Polymer({
       // - Otherwise, the cast mode becomes auto mode.
       // Either way, all sinks will be shown.
       this.setShownCastMode_(this.computeCastMode_());
-      sinksToShow = this.allSinks;
     }
 
     this.sinksToShow_ = sinksToShow;
@@ -1518,9 +1752,20 @@ Polymer({
     this.sinkMap_ = {};
 
     this.allSinks.forEach(function(sink) {
-      this.sinkMap_[sink.id] = sink;
+      if (!this.isPseudoSink_(sink)) {
+        this.sinkMap_[sink.id] = sink;
+      }
     }, this);
 
+    if (this.searchInProgress_) {
+      this.searchInProgress_.checkProgress(this.allSinks, this.sinkToRouteMap_);
+      this.currentLaunchingSinkId_ =
+          this.searchInProgress_.getCurrentLaunchingSinkId();
+      if (this.searchInProgress_.isDone()) {
+        this.searchInProgress_ = null;
+      }
+    }
+    this.pseudoSinks_ = this.allSinks.filter(this.isPseudoSink_);
     this.rebuildSinksToShow_();
     if (this.isUserSearching_) {
       this.filterSinks_(this.searchInputText_);