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

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org,mfoltz@chromium.org,isherman@chromium.org,avayvod@chromium.org

Committed: https://crrev.com/2443442ccda2d3ce929704bdefb8965067c09045
Cr-Commit-Position: refs/heads/master@{#388591}

[btolsch, 2016-03-16 02:52:47]

Naming and how to deal with cast modes/media sources is still up in the air.  I
haven't looked at testing yet. PTAL, thanks!

[amp, 2016-03-16 17:20:11]

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

[amp, 2016-03-16 17:20:12]

Adding Derek as I am not a committer (may need someone else if Derek is not
available).

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.mojom:268: // |sink_id| that is
compatible with the source urn |media_source|.
As mentioned on extension side, this makes more sense to me as search_criteria
rather than sink_id, but I may be misunderstanding how this is wired up.

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.mojom:311: // Called when the Media
Route Manager receives a new list of sinks.
Also as mentioned on the extension side change, this appears to be the same call
as OnSinksReceived above (in fact the comments are exactly the same).

I haven't followed through exactly why we would need a new method, there may be
a good reason I am missing.

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1049:
* @param {string} name Name of the new hangout.
Can we include the domain here as well (it should have been passed in to the
container.

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1092:
searchResultsToShow.unshift(this.createManualHangoutItem_(searchInputText));
Do we need to tie this into the mrps better somehow?  Something like have the
mrp's send back a signal that they support searching for sinks (maybe send back
a psuedo sink with the correct icon type etc) and then use that to add in a
search result entry that would actually trigger the search?

This would definitely work, but it feels too tightly tied to Hangouts (which the
UI ideally would know nothing about).

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1319:
this.fire('add-hangout', {
maybe use a more generic name instead of referring to hangouts specifically.

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1327:
this.showOrCreateRoute_(clickedSink);
I don't see where we do the route creation for the newly added sink.  Is that in
a coming patch/change?

Ideally clicking on a one of these pending sinks should start the spinner (same
as if a route was being created) and keep spinning until 1. the sink has been
searched with the provider and 2. the route from the source to this sink has
been created.

[amp, 2016-03-16 17:23:35]

On 2016/03/16 17:20:12, amp wrote:
> Adding Derek as I am not a committer (may need someone else if Derek is not
> available).
> 
>
https://codereview.chromium.org/1805813002/diff/1/chrome/browser/media/router...
> File chrome/browser/media/router/media_router.mojom (right):
> 
>
https://codereview.chromium.org/1805813002/diff/1/chrome/browser/media/router...
> chrome/browser/media/router/media_router.mojom:268: // |sink_id| that is
> compatible with the source urn |media_source|.
> As mentioned on extension side, this makes more sense to me as search_criteria
> rather than sink_id, but I may be misunderstanding how this is wired up.
> 
>
https://codereview.chromium.org/1805813002/diff/1/chrome/browser/media/router...
> chrome/browser/media/router/media_router.mojom:311: // Called when the Media
> Route Manager receives a new list of sinks.
> Also as mentioned on the extension side change, this appears to be the same
call
> as OnSinksReceived above (in fact the comments are exactly the same).
> 
> I haven't followed through exactly why we would need a new method, there may
be
> a good reason I am missing.
> 
>
https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
> File
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
> (right):
> 
>
https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1049:
> * @param {string} name Name of the new hangout.
> Can we include the domain here as well (it should have been passed in to the
> container.
> 
>
https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1092:
> searchResultsToShow.unshift(this.createManualHangoutItem_(searchInputText));
> Do we need to tie this into the mrps better somehow?  Something like have the
> mrp's send back a signal that they support searching for sinks (maybe send
back
> a psuedo sink with the correct icon type etc) and then use that to add in a
> search result entry that would actually trigger the search?
> 
> This would definitely work, but it feels too tightly tied to Hangouts (which
the
> UI ideally would know nothing about).
> 
>
https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1319:
> this.fire('add-hangout', {
> maybe use a more generic name instead of referring to hangouts specifically.
> 
>
https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1327:
> this.showOrCreateRoute_(clickedSink);
> I don't see where we do the route creation for the newly added sink.  Is that
in
> a coming patch/change?
> 
> Ideally clicking on a one of these pending sinks should start the spinner
(same
> as if a route was being created) and keep spinning until 1. the sink has been
> searched with the provider and 2. the route from the source to this sink has
> been created.

Also update description of change to reference crbug/565696 and perhaps make it
more generic about searching for sinks rather than Hangouts only.

[btolsch, 2016-03-16 18:33:40]

Description was changed from

==========
[Media Router] Wiring for adding a manual hangout.

This change adds the ability for the filter UI to present the user with the
option to create a new hangout that is named with the currently entered search
text. This change adds all the necessary wiring for the UI to talk to the Media
Router and for the Media Router to talk to the MRPM extension.

BUG=
R=amp@chromium.org
==========

to

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

This change adds the ability for the filter UI to present the user with the
option to create a new hangout that is named with the currently entered search
text. This change adds all the necessary wiring for the UI to talk to the Media
Router and for the Media Router to talk to the MRPM extension.

BUG=565696
R=amp@chromium.org
==========

[btolsch, 2016-03-16 18:35:46]

Description was changed from

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

This change adds the ability for the filter UI to present the user with the
option to create a new hangout that is named with the currently entered search
text. This change adds all the necessary wiring for the UI to talk to the Media
Router and for the Media Router to talk to the MRPM extension.

BUG=565696
R=amp@chromium.org
==========

to

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension.

BUG=565696
R=amp@chromium.org
==========

[btolsch, 2016-03-16 18:36:30]

I added some explanations but we can sync offline too. PTAL, thanks!

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.mojom:268: // |sink_id| that is
compatible with the source urn |media_source|.
On 2016/03/16 17:20:12, amp wrote:
> As mentioned on extension side, this makes more sense to me as search_criteria
> rather than sink_id, but I may be misunderstanding how this is wired up.

Done.

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.mojom:311: // Called when the Media
Route Manager receives a new list of sinks.
On 2016/03/16 17:20:12, amp wrote:
> Also as mentioned on the extension side change, this appears to be the same
call
> as OnSinksReceived above (in fact the comments are exactly the same).
> 
> I haven't followed through exactly why we would need a new method, there may
be
> a good reason I am missing.

As mentioned on the extension side, OnSinksReceived is doing two things
differently than OnSearchSinksReceived:

1. OnSinksReceived is tracking media sources, not search criteria (here
sink_id).
2. OnSinksReceived is expecting a complete list of sinks that are compatible
with a media source but search is only providing additional sinks.

To make this work with a single function, search would have to send a complete
sink list and something other than a media source would have to be used as the
tracking key. I don't think we can overload the string to have both key
meanings, so maybe we could add an enum that indicates which type of key we're
using? This would also let us still only send search results as additions
instead of complete lists.

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1049:
* @param {string} name Name of the new hangout.
On 2016/03/16 17:20:12, amp wrote:
> Can we include the domain here as well (it should have been passed in to the
> container.

Done.

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1092:
searchResultsToShow.unshift(this.createManualHangoutItem_(searchInputText));
On 2016/03/16 17:20:12, amp wrote:
> Do we need to tie this into the mrps better somehow?  Something like have the
> mrp's send back a signal that they support searching for sinks (maybe send
back
> a psuedo sink with the correct icon type etc) and then use that to add in a
> search result entry that would actually trigger the search?
> 
> This would definitely work, but it feels too tightly tied to Hangouts (which
the
> UI ideally would know nothing about).

I like this idea. I'm not sure there's a good way to add this within the current
sink structure though. I could see each MRP having a special sink id and/or
having a dummy cast mode.  I think these both have problems with forward
compatibility though. Do you have an idea of how best to include this?

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1319:
this.fire('add-hangout', {
On 2016/03/16 17:20:12, amp wrote:
> maybe use a more generic name instead of referring to hangouts specifically.

I like the idea of pseudo sinks from the MRP's, and that would also get rid of
this mechanism. If we stay with this mechanism, I will rename it though.

https://codereview.chromium.org/1805813002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1327:
this.showOrCreateRoute_(clickedSink);
On 2016/03/16 17:20:12, amp wrote:
> I don't see where we do the route creation for the newly added sink.  Is that
in
> a coming patch/change?
> 
> Ideally clicking on a one of these pending sinks should start the spinner
(same
> as if a route was being created) and keep spinning until 1. the sink has been
> searched with the provider and 2. the route from the source to this sink has
> been created.

That will depend on what happens with the rest of the API because as it stands
now, this doesn't seem to work for hangouts. The first one (or more?)
create-route events on the hangout, even once it makes it from the MRP to the
UI, fails silently.  The cloud MRP rejects routing the sink for a short period,
even though it effectively created it. I think other changes will help address
this but we can talk offline about whether anything needs to change in the
extension to support this.

[btolsch, 2016-04-05 21:28:07]

Description was changed from

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension.

BUG=565696
R=amp@chromium.org
==========

to

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org
==========

[btolsch, 2016-04-05 21:31:32]

I think I have a good first draft of the pseudo sink method that was discussed
in the design doc and offline. Additionally, route creation is now packaged in
the MRPM. PTAL, thanks!

[btolsch, 2016-04-06 17:32:30]

Description was changed from

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org
==========

to

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org
==========

[btolsch, 2016-04-06 17:32:30]

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

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

I haven't seen the MRP side changes yet, so mostly just resource/ comments. I'll
take a look at the other CL to get more context.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_metrics.h:43: SEARCH_SINKS =
16,
Add this to histograms.xml.

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

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:18:
var PseudoSinkSearchStateMachine = function(pseudoSink) {
Could this be pulled out into a separate file?

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:52:
* Whether we are done collecting information.
Please go into more detail on what information is being collected.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:489:
searchInProgress_: {
Rename this to refer to the PseudoSinkSearchStateMachine. "searchInProgress_" is
a bit confusing since we also have properties like "isUserSearching_".

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1604:
* doesn't have any cast modes set when this function is called, this function
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?

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1616:
if (this.searchInProgress_.isDone()) {
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();
}

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

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router.js:75: var detail =
event.detail;
nit: Add /** @type {{ ... }} */

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_ui_interface.js (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_ui_interface.js:378: * @param
{string} domain User's current domain.
Are pseudosinks guaranteed to have a domain specified?

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_ui_interface.js:378: * @param
{string} domain User's current domain.
Is this the user's current domain, or the pseudosink's current domain? Are these
guaranteed to be the same?

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

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:764:
"Aborting.";
return;

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

Looks promising.  I like the psuedo sink implementation, thanks for putting all
the parts together for this.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:120: // The user's current
account domain.
call this 'hosted domain' in the comments to line up with what the sign in
manager returns.  The actual property itself is fine as just 'domain'.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:241: if
(sink_id_callback_entry == search_callbacks_.end()) {
Is this scenario expected?  Is it possible to get back multiple responses for a
single search?

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:326: result =
RouteRequestResult::FromError(
Does this result in an issue being shown in the UI, or is it the responsibility
of the MRP to create an issue?

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:782: bool
insert_wil_succeed;
s/wil/will/

Unless this is some kind of style/naming convention I'm not familiar with. :)

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

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:18:
var PseudoSinkSearchStateMachine = function(pseudoSink) {
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).

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:52:
* Whether we are done collecting information.
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.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:64:
if (!sinkId) {
Should you check this before assigning realSinkId_, or does that not matter?

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:130:
* Returns whether we are still waiting for more information from the search
Maybe reword to 'Returns false if we are still waiting ...'

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1290:
searchResultsToShow = this.pseudoSinks_.filter(function(pseudoSink) {
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.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1335:
return sink.id.startsWith('pseudo:');
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?

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1335:
return sink.id.startsWith('pseudo:');
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.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1611:
onReceiveSearchResult: function(sinkId) {
Is it possible that this never gets called (the result gets dropped somehow)? 
Should we have a timer to cleanup searchInProgress in that case?

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1641:
if (!this.searchInProgress_) {
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?

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:363: bool
MediaRouterUI::GetCreateOrConnectRouteParameters(
Feels odd to have this return a bool when it says 'Get...Parameters' in the
name, but if that's an accepted c++ style feel free to ignore me.

[apacible, 2016-04-06 21:33:06]

Side note, are there screenshots and/or a screencast demo?

[btolsch, 2016-04-08 09:31:26]

I think I've addressed all the comments and I've also added tests. I also had to
add a stub for Android. Please let me know if SearchSinksAndCreateRoute() needs
to do something real on Android. PTAL, thanks!

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:120: // The user's current
account domain.
On 2016/04/06 21:29:36, amp wrote:
> call this 'hosted domain' in the comments to line up with what the sign in
> manager returns.  The actual property itself is fine as just 'domain'.

Done.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:241: if
(sink_id_callback_entry == search_callbacks_.end()) {
On 2016/04/06 21:29:36, amp wrote:
> Is this scenario expected?  Is it possible to get back multiple responses for
a
> single search?

This isn't expected, it is just a generic guard. I can remove it if that is
preferred.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:326: result =
RouteRequestResult::FromError(
On 2016/04/06 21:29:36, amp wrote:
> Does this result in an issue being shown in the UI, or is it the
responsibility
> of the MRP to create an issue?

This shows an issue in the UI like other route creation errors.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:782: bool
insert_wil_succeed;
On 2016/04/06 21:29:36, amp wrote:
> s/wil/will/
> 
> Unless this is some kind of style/naming convention I'm not familiar with. :)

Done.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_metrics.h:43: SEARCH_SINKS =
16,
On 2016/04/06 21:27:01, apacible wrote:
> Add this to histograms.xml.

Done.

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

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:18:
var PseudoSinkSearchStateMachine = function(pseudoSink) {
On 2016/04/06 21:27:01, apacible wrote:
> Could this be pulled out into a separate file?

Done.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:18:
var PseudoSinkSearchStateMachine = function(pseudoSink) {
On 2016/04/06 21:29:36, amp wrote:
> 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).

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.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:52:
* Whether we are done collecting information.
On 2016/04/06 21:29:36, amp wrote:
> 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.

Removed with the simplification.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:52:
* Whether we are done collecting information.
On 2016/04/06 21:27:02, apacible wrote:
> Please go into more detail on what information is being collected.

Removed with the simplification.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:64:
if (!sinkId) {
On 2016/04/06 21:29:36, amp wrote:
> Should you check this before assigning realSinkId_, or does that not matter?

Removed with the simplification.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:130:
* Returns whether we are still waiting for more information from the search
On 2016/04/06 21:29:36, amp wrote:
> Maybe reword to 'Returns false if we are still waiting ...'

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

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:489:
searchInProgress_: {
On 2016/04/06 21:27:02, apacible wrote:
> Rename this to refer to the PseudoSinkSearchStateMachine. "searchInProgress_"
is
> a bit confusing since we also have properties like "isUserSearching_".

Done.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1290:
searchResultsToShow = this.pseudoSinks_.filter(function(pseudoSink) {
On 2016/04/06 21:29:36, amp wrote:
> 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.

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.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1335:
return sink.id.startsWith('pseudo:');
On 2016/04/06 21:29:36, amp wrote:
> 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?

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.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1335:
return sink.id.startsWith('pseudo:');
On 2016/04/06 21:29:36, amp wrote:
> 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.

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.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1604:
* doesn't have any cast modes set when this function is called, this function
On 2016/04/06 21:27:01, apacible wrote:
> 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?

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.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1611:
onReceiveSearchResult: function(sinkId) {
On 2016/04/06 21:29:36, amp wrote:
> Is it possible that this never gets called (the result gets dropped somehow)? 
> Should we have a timer to cleanup searchInProgress in that case?

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

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1616:
if (this.searchInProgress_.isDone()) {
On 2016/04/06 21:27:02, apacible wrote:
> 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();
> }

This has been even further simplified by some other changes.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1641:
if (!this.searchInProgress_) {
On 2016/04/06 21:29:36, amp wrote:
> 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?

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.

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

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router.js:75: var detail =
event.detail;
On 2016/04/06 21:27:02, apacible wrote:
> nit: Add /** @type {{ ... }} */

Done.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_ui_interface.js (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_ui_interface.js:378: * @param
{string} domain User's current domain.
On 2016/04/06 21:27:02, apacible wrote:
> Is this the user's current domain, or the pseudosink's current domain? Are
these
> guaranteed to be the same?

The pseudo-sink will only have a domain if the MRP that created it cares about
the domain. So if an MRP doesn't set the domain then it will get passed back an
unset domain.

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:363: bool
MediaRouterUI::GetCreateOrConnectRouteParameters(
On 2016/04/06 21:29:36, amp wrote:
> Feels odd to have this return a bool when it says 'Get...Parameters' in the
> name, but if that's an accepted c++ style feel free to ignore me.

I don't know that it's not accepted and I didn't want to say
'CreateCreate...Parameters'. Another option is Make/Build...Parameters. I don't
prefer any of them over others.

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

https://codereview.chromium.org/1805813002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:764:
"Aborting.";
On 2016/04/06 21:27:02, apacible wrote:
> return;

Done.

[amp, 2016-04-08 17:12:46]

Looks alright to me for the parts I understand. :)

chrome/browser/ui/webui/media_router/media_router_ui.cc in particular is a
little confusing to me, but that's probably more my lack of experience in c++ so
someone else can comment on the changes there.

The simplification for the search state looks good and is easier to follow.

I assume the route creation cleanup logic addresses the bugs we saw in the demo,
but I added a comment to maybe add some tests for those scenarios if relevant.

https://codereview.chromium.org/1805813002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/media_router/media_router_container_search_tests.js
(right):

https://codereview.chromium.org/1805813002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_search_tests.js:297:
});
Do we need any tests for the error scenario's where issues were added but didn't
show up in the filter view?

Also I recall there was some case where the UI got into a state where the pseudo
sink was visible, but clicking on it didn't try to create a route (I think it
was after there was an error). Did we resolve that (and can we add tests for the
scenario that caused it)?

[apacible, 2016-04-08 21:02:12]

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

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1498:
this.pseudoSinkSearchState_.getCurrentLaunchingSinkId();
There's some metrics we keep track of under showOrCreateRoute_() --

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

A few things:
1. Searching takes some more time compared to just clicking a sink populated
from the list, so adding to that metric will probably skew the results.
2. We definitely want to report if this is the first user action -- START_LOCAL.

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1560:
var sinksToShow = this.allSinks.filter(function(sink) {
We previously discussed the scenario:
1. Go to search view
2. Search and click a pseudosink.
3. Route not resolved immediately
4. User switches to sink view

The pseudosink isn't shown in the sink view and there's also no other indication
that we're still waiting for the route to be resolved.

Could we also check whether |pseudoSinkSearchState_| is set, and show the
pseudosink if it is? rebuildSinksToShow() can be called after
|pseudoSinkSearchState_| is set so we keep the displayed sink list current.

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.h:238: bool
GetCreateOrConnectRouteParameters(
Rename this to be more general? Previously it was just used for CreateRoute()
and ConnectRoute().

[btolsch, 2016-04-08 21:55:04]

Hi Jennifer,

I just wanted some clarification on the comments. Thanks!

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

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1498:
this.pseudoSinkSearchState_.getCurrentLaunchingSinkId();
On 2016/04/08 21:02:12, apacible wrote:
> There's some metrics we keep track of under showOrCreateRoute_() --
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> A few things:
> 1. Searching takes some more time compared to just clicking a sink populated
> from the list, so adding to that metric will probably skew the results.
> 2. We definitely want to report if this is the first user action --
START_LOCAL.

Are you saying I should add maybeReportUserFirstAction() to the pseudo sink case
but not sink click time?

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1560:
var sinksToShow = this.allSinks.filter(function(sink) {
On 2016/04/08 21:02:12, apacible wrote:
> We previously discussed the scenario:
> 1. Go to search view
> 2. Search and click a pseudosink.
> 3. Route not resolved immediately
> 4. User switches to sink view
> 
> The pseudosink isn't shown in the sink view and there's also no other
indication
> that we're still waiting for the route to be resolved.
> 
> Could we also check whether |pseudoSinkSearchState_| is set, and show the
> pseudosink if it is? rebuildSinksToShow() can be called after
> |pseudoSinkSearchState_| is set so we keep the displayed sink list current.

Showing a sink in the sink list does not depend on route creation completely
quickly.

We could show the pseudo sink in the sink list while we're waiting for a
response, but if we think that case could happen then we should probably also
stop taking input for the pseudo sink name.

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.h:238: bool
GetCreateOrConnectRouteParameters(
On 2016/04/08 21:02:12, apacible wrote:
> Rename this to be more general? Previously it was just used for CreateRoute()
> and ConnectRoute().

The use by SearchSinksAndCreateRoute() is identical to the CreateRoute() case.
Is there something you think makes more sense?

https://codereview.chromium.org/1805813002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/media_router/media_router_container_search_tests.js
(right):

https://codereview.chromium.org/1805813002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_search_tests.js:297:
});
On 2016/04/08 17:12:46, amp wrote:
> Do we need any tests for the error scenario's where issues were added but
didn't
> show up in the filter view?
> 
> Also I recall there was some case where the UI got into a state where the
pseudo
> sink was visible, but clicking on it didn't try to create a route (I think it
> was after there was an error). Did we resolve that (and can we add tests for
the
> scenario that caused it)?

I added a test in the filter tests to make sure issues are visible in filter
view.

The UI issues are resolved and it's not possible to get into the states we saw
before with the simplification. I did add a test to make sure everything resets
when route creation fails though.

[apacible, 2016-04-08 22:07:47]

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

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1498:
this.pseudoSinkSearchState_.getCurrentLaunchingSinkId();
On 2016/04/08 21:55:04, btolsch wrote:
> On 2016/04/08 21:02:12, apacible wrote:
> > There's some metrics we keep track of under showOrCreateRoute_() --
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > 
> > A few things:
> > 1. Searching takes some more time compared to just clicking a sink populated
> > from the list, so adding to that metric will probably skew the results.
> > 2. We definitely want to report if this is the first user action --
> START_LOCAL.
> 
> Are you saying I should add maybeReportUserFirstAction() to the pseudo sink
case
> but not sink click time?

Oh, I didn't finish my thought here. :)

Yes, add the maybeReportUserFirstAction() here.

In a future patch, we may want to add similar metrics for search/filter if
there's something we want to measure - e.g. how often someone switches between
search->sink without selecting anything.

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1560:
var sinksToShow = this.allSinks.filter(function(sink) {
On 2016/04/08 21:55:04, btolsch wrote:
> On 2016/04/08 21:02:12, apacible wrote:
> > We previously discussed the scenario:
> > 1. Go to search view
> > 2. Search and click a pseudosink.
> > 3. Route not resolved immediately
> > 4. User switches to sink view
> > 
> > The pseudosink isn't shown in the sink view and there's also no other
> indication
> > that we're still waiting for the route to be resolved.
> > 
> > Could we also check whether |pseudoSinkSearchState_| is set, and show the
> > pseudosink if it is? rebuildSinksToShow() can be called after
> > |pseudoSinkSearchState_| is set so we keep the displayed sink list current.
> 
> Showing a sink in the sink list does not depend on route creation completely
> quickly.
I'm not following -- elaborate?

> We could show the pseudo sink in the sink list while we're waiting for a
> response, but if we think that case could happen then we should probably also
> stop taking input for the pseudo sink name.

Agree on not taking input for the pseudo sink name.

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.h:238: bool
GetCreateOrConnectRouteParameters(
On 2016/04/08 21:55:04, btolsch wrote:
> On 2016/04/08 21:02:12, apacible wrote:
> > Rename this to be more general? Previously it was just used for
CreateRoute()
> > and ConnectRoute().
> 
> The use by SearchSinksAndCreateRoute() is identical to the CreateRoute() case.
> Is there something you think makes more sense?

Something like SetRouteParamters()?

[btolsch, 2016-04-08 22:19:18]

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

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1560:
var sinksToShow = this.allSinks.filter(function(sink) {
On 2016/04/08 22:07:47, apacible wrote:
> On 2016/04/08 21:55:04, btolsch wrote:
> > On 2016/04/08 21:02:12, apacible wrote:
> > > We previously discussed the scenario:
> > > 1. Go to search view
> > > 2. Search and click a pseudosink.
> > > 3. Route not resolved immediately
> > > 4. User switches to sink view
> > > 
> > > The pseudosink isn't shown in the sink view and there's also no other
> > indication
> > > that we're still waiting for the route to be resolved.
> > > 
> > > Could we also check whether |pseudoSinkSearchState_| is set, and show the
> > > pseudosink if it is? rebuildSinksToShow() can be called after
> > > |pseudoSinkSearchState_| is set so we keep the displayed sink list
current.
> > 
> > Showing a sink in the sink list does not depend on route creation completely
> > quickly.
> I'm not following -- elaborate?
> 
> > We could show the pseudo sink in the sink list while we're waiting for a
> > response, but if we think that case could happen then we should probably
also
> > stop taking input for the pseudo sink name.
> 
> Agree on not taking input for the pseudo sink name.

The typo probably didn't help. What I meant is that route creation can take a
long time or not, but the real sink returned by search should always be fast
because it doesn't depend on any calls outside of MR/MRPM.

[apacible, 2016-04-08 22:48:40]

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

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1560:
var sinksToShow = this.allSinks.filter(function(sink) {
On 2016/04/08 22:19:17, btolsch wrote:
> On 2016/04/08 22:07:47, apacible wrote:
> > On 2016/04/08 21:55:04, btolsch wrote:
> > > On 2016/04/08 21:02:12, apacible wrote:
> > > > We previously discussed the scenario:
> > > > 1. Go to search view
> > > > 2. Search and click a pseudosink.
> > > > 3. Route not resolved immediately
> > > > 4. User switches to sink view
> > > > 
> > > > The pseudosink isn't shown in the sink view and there's also no other
> > > indication
> > > > that we're still waiting for the route to be resolved.
> > > > 
> > > > Could we also check whether |pseudoSinkSearchState_| is set, and show
the
> > > > pseudosink if it is? rebuildSinksToShow() can be called after
> > > > |pseudoSinkSearchState_| is set so we keep the displayed sink list
> current.
> > > 
> > > Showing a sink in the sink list does not depend on route creation
completely
> > > quickly.
> > I'm not following -- elaborate?
> > 
> > > We could show the pseudo sink in the sink list while we're waiting for a
> > > response, but if we think that case could happen then we should probably
> also
> > > stop taking input for the pseudo sink name.
> > 
> > Agree on not taking input for the pseudo sink name.
> 
> The typo probably didn't help. What I meant is that route creation can take a
> long time or not, but the real sink returned by search should always be fast
> because it doesn't depend on any calls outside of MR/MRPM.

Is the real sink always shown in the sink list after it's returned? (We talked
about this offline earlier but I must have switched what was returning
immediately -- sink vs route.)

As long as we:
- Have an indication that there's a currently launching sink in the sink list if
the user switches to the sink view,
- Disable allowing the user to launch another sink through the sink or filter
list

we're OK.

IIRC from a previous discussion, we+UX decided it was fine to allow the user to
keep typing in the search input as long as they couldn't launch another sink.
Let's keep it that way unless we find some issues with that experience.

[btolsch, 2016-04-11 08:17:26]

Description was changed from

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org
==========

to

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org,mfoltz@chromium...
==========

[btolsch, 2016-04-11 08:17:26]

btolsch@chromium.org changed reviewers:
+ isherman@chromium.org, mfoltz@chromium.org

[btolsch, 2016-04-11 08:19:36]

+mfoltz for c/b/m/android/router
+isherman for histograms.xml

Addressed comments and added a few more tests. PTAL, thanks!

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

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1498:
this.pseudoSinkSearchState_.getCurrentLaunchingSinkId();
On 2016/04/08 22:07:47, apacible wrote:
> On 2016/04/08 21:55:04, btolsch wrote:
> > On 2016/04/08 21:02:12, apacible wrote:
> > > There's some metrics we keep track of under showOrCreateRoute_() --
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > > 
> > > A few things:
> > > 1. Searching takes some more time compared to just clicking a sink
populated
> > > from the list, so adding to that metric will probably skew the results.
> > > 2. We definitely want to report if this is the first user action --
> > START_LOCAL.
> > 
> > Are you saying I should add maybeReportUserFirstAction() to the pseudo sink
> case
> > but not sink click time?
> 
> Oh, I didn't finish my thought here. :)
> 
> Yes, add the maybeReportUserFirstAction() here.
> 
> In a future patch, we may want to add similar metrics for search/filter if
> there's something we want to measure - e.g. how often someone switches between
> search->sink without selecting anything.

Done.

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1560:
var sinksToShow = this.allSinks.filter(function(sink) {
On 2016/04/08 22:48:40, apacible wrote:
> On 2016/04/08 22:19:17, btolsch wrote:
> > On 2016/04/08 22:07:47, apacible wrote:
> > > On 2016/04/08 21:55:04, btolsch wrote:
> > > > On 2016/04/08 21:02:12, apacible wrote:
> > > > > We previously discussed the scenario:
> > > > > 1. Go to search view
> > > > > 2. Search and click a pseudosink.
> > > > > 3. Route not resolved immediately
> > > > > 4. User switches to sink view
> > > > > 
> > > > > The pseudosink isn't shown in the sink view and there's also no other
> > > > indication
> > > > > that we're still waiting for the route to be resolved.
> > > > > 
> > > > > Could we also check whether |pseudoSinkSearchState_| is set, and show
> the
> > > > > pseudosink if it is? rebuildSinksToShow() can be called after
> > > > > |pseudoSinkSearchState_| is set so we keep the displayed sink list
> > current.
> > > > 
> > > > Showing a sink in the sink list does not depend on route creation
> completely
> > > > quickly.
> > > I'm not following -- elaborate?
> > > 
> > > > We could show the pseudo sink in the sink list while we're waiting for a
> > > > response, but if we think that case could happen then we should probably
> > also
> > > > stop taking input for the pseudo sink name.
> > > 
> > > Agree on not taking input for the pseudo sink name.
> > 
> > The typo probably didn't help. What I meant is that route creation can take
a
> > long time or not, but the real sink returned by search should always be fast
> > because it doesn't depend on any calls outside of MR/MRPM.
> 
> Is the real sink always shown in the sink list after it's returned? (We talked
> about this offline earlier but I must have switched what was returning
> immediately -- sink vs route.)
> 
> As long as we:
> - Have an indication that there's a currently launching sink in the sink list
if
> the user switches to the sink view,
> - Disable allowing the user to launch another sink through the sink or filter
> list
> 
> we're OK.
> 
> IIRC from a previous discussion, we+UX decided it was fine to allow the user
to
> keep typing in the search input as long as they couldn't launch another sink.
> Let's keep it that way unless we find some issues with that experience.

Done.

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/1805813002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.h:238: bool
GetCreateOrConnectRouteParameters(
On 2016/04/08 22:07:47, apacible wrote:
> On 2016/04/08 21:55:04, btolsch wrote:
> > On 2016/04/08 21:02:12, apacible wrote:
> > > Rename this to be more general? Previously it was just used for
> CreateRoute()
> > > and ConnectRoute().
> > 
> > The use by SearchSinksAndCreateRoute() is identical to the CreateRoute()
case.
> > Is there something you think makes more sense?
> 
> Something like SetRouteParamters()?

Done.

[Ilya Sherman, 2016-04-11 20:56:15]

histograms.xml lgtm

[apacible, 2016-04-11 21:30:49]

lgtm

[amp, 2016-04-15 00:31:16]

lgtm

[amp, 2016-04-18 17:20:40]

Ping.  Mark or Derek did you have any concerns with this?

As the extension side as already been submitted the UI will now show (as of the
extension release on 4/18) the pseudo sink in the regular sink list until this
change goes in.

[imcheng, 2016-04-18 23:15:47]

looks ok overall. Two high level comments:
1) The number of input parameters on CreateRoute family APIs is getting out of
control. We should have a cleanup patch to reduce it.
2) The changes in the UI are a bit complicated. It would be beneficial to have
some additional comments explaining the logic around pseudo sinks filtering, and
refactoring to make the code more readable.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:70: const
MediaSource::Id& source_id,
We would try to reduce the number of input parameters in a separate patch. We
could introduce something like a CreateRouteParameters struct with default
values for some fields.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:168: // Sends a SearchSinks request
to the MRPM using the given criteria.
nit on comment: MRPM is a detail of the mojo implementation of MediaRouter. I
would word it as "Searches for a MediaSink using |search_input| and |domain| as
criteria. Also please document the additional behavior of a route being created.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router.mojom:118: // Arbitrary string
input to the search method.
Instead of calling it arbitrary, I would say something like "Each Media Route
Provider may interpret this field differently."

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:313: void
MediaRouterMojoImpl::SearchAndCreateResponseReceived(
So this is very similar to RouteResponseReceived except the off_the_record bits.
What is the reason RouteResponseReceived cannot be reused for the new api?

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:782: bool
insert_will_succeed;
this variable is not used?

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:383:
search_callbacks_;
Is it possible that multiple callbacks could exist for the same sink id at the
same time?

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

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:312:
* @private {PseudoSinkSearchState}
should this be ?PseudoSinkSearchState ?

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:397:
* @private {!Array<!media_router.Sink>}
How does this pass closure compile? From the way it is used it doesn't look like
an array of sinks.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1477:
this.pseudoSinkSearchState_.receiveSinkResponse(sinkId);
can receiveSinkResponse and checkForRealSink be made into one call? In the one
place where they are not called together (L1514), the pseudo sink id is always
used.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1502:
this.fire('search-sinks-and-create-route', {
could you please refactor the logic here with the create route logic in
showOrCreateRoute_?

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1620:
this.pseudoSinkSearchState_.checkForRealSink(this.allSinks);
Can checkForRealSink and getCurrentLaunchingSinkId be combined into one call?

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:484:
std::move(route_response_callbacks),
Is std::move necessary here? You end up doing a copy when you do base::Bind
anyway.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:127:
sink_val->SetBoolean("isPseudoSink", sink.id().find("pseudo:") == 0);
Use base::StartsWith:
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

[btolsch, 2016-04-19 01:39:43]

Derek, there was one comment I didn't quite understand but I believe I've
addressed the rest. Also, is there anything specific that you find overly
complicated or difficult to read in the UI code relating to filtering and
searching?

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:70: const
MediaSource::Id& source_id,
On 2016/04/18 23:15:46, imcheng wrote:
> We would try to reduce the number of input parameters in a separate patch. We
> could introduce something like a CreateRouteParameters struct with default
> values for some fields.

Acknowledged.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:168: // Sends a SearchSinks request
to the MRPM using the given criteria.
On 2016/04/18 23:15:46, imcheng wrote:
> nit on comment: MRPM is a detail of the mojo implementation of MediaRouter. I
> would word it as "Searches for a MediaSink using |search_input| and |domain|
as
> criteria. Also please document the additional behavior of a route being
created.

Done.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router.mojom:118: // Arbitrary string
input to the search method.
On 2016/04/18 23:15:46, imcheng wrote:
> Instead of calling it arbitrary, I would say something like "Each Media Route
> Provider may interpret this field differently."

Done.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:313: void
MediaRouterMojoImpl::SearchAndCreateResponseReceived(
On 2016/04/18 23:15:46, imcheng wrote:
> So this is very similar to RouteResponseReceived except the off_the_record
bits.
> What is the reason RouteResponseReceived cannot be reused for the new api?

No, removed.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:782: bool
insert_will_succeed;
On 2016/04/18 23:15:46, imcheng wrote:
> this variable is not used?

This was convenient when |sink_callback_entry| was being used after the if. It's
unnecessary now. Removed.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:383:
search_callbacks_;
On 2016/04/18 23:15:46, imcheng wrote:
> Is it possible that multiple callbacks could exist for the same sink id at the
> same time?

One UI can't create that situation, but if two UIs could submit the search near
simultaneously then yes. I converted it to a list. Let me know if you'd rather
it stay as one entry and not handle this case.

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

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:312:
* @private {PseudoSinkSearchState}
On 2016/04/18 23:15:47, imcheng wrote:
> should this be ?PseudoSinkSearchState ?

As a proper object, using its name alone implies '?'.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:397:
* @private {!Array<!media_router.Sink>}
On 2016/04/18 23:15:47, imcheng wrote:
> How does this pass closure compile? From the way it is used it doesn't look
like
> an array of sinks.

I actually don't know how it passed the compile, but this was fixed by a
separate patch which is present in the rebase (patchset 7).

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1477:
this.pseudoSinkSearchState_.receiveSinkResponse(sinkId);
On 2016/04/18 23:15:47, imcheng wrote:
> can receiveSinkResponse and checkForRealSink be made into one call? In the one
> place where they are not called together (L1514), the pseudo sink id is always
> used.

I don't think I understand this comment. I assume you're referring to L1620
(L1547 in patchset 7) and not L1514 since that has nothing to do with
checkForRealSink(). But at L1620, I'm not sure what you mean by "the pseudo sink
id is always used." Could you explain?

It would be possible to make them one function if L1620 uses |null| to avoid
resetting an existing sink id, though I think that's slightly less readable.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1502:
this.fire('search-sinks-and-create-route', {
On 2016/04/18 23:15:47, imcheng wrote:
> could you please refactor the logic here with the create route logic in
> showOrCreateRoute_?

Done.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1620:
this.pseudoSinkSearchState_.checkForRealSink(this.allSinks);
On 2016/04/18 23:15:46, imcheng wrote:
> Can checkForRealSink and getCurrentLaunchingSinkId be combined into one call?

If the call at L1513 is removed and instead sets the launching sink id manually
to the pseudo sink id. I merged them for now so you can tell me what you think
about that line in the showOrCreateRoute_() refactor.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:484:
std::move(route_response_callbacks),
On 2016/04/18 23:15:47, imcheng wrote:
> Is std::move necessary here? You end up doing a copy when you do base::Bind
> anyway.

Actually, SearchSinksAndCreateRoute() accepts it by const ref so it's
unnecessary for that reason. Removed.

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:127:
sink_val->SetBoolean("isPseudoSink", sink.id().find("pseudo:") == 0);
On 2016/04/18 23:15:47, imcheng wrote:
> Use base::StartsWith:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

Done.

[imcheng, 2016-04-19 19:52:37]

lgtm

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

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1477:
this.pseudoSinkSearchState_.receiveSinkResponse(sinkId);
On 2016/04/19 01:39:43, btolsch wrote:
> On 2016/04/18 23:15:47, imcheng wrote:
> > can receiveSinkResponse and checkForRealSink be made into one call? In the
one
> > place where they are not called together (L1514), the pseudo sink id is
always
> > used.
> 
> I don't think I understand this comment. I assume you're referring to L1620
> (L1547 in patchset 7) and not L1514 since that has nothing to do with
> checkForRealSink(). But at L1620, I'm not sure what you mean by "the pseudo
sink
> id is always used." Could you explain?
> 
> It would be possible to make them one function if L1620 uses |null| to avoid
> resetting an existing sink id, though I think that's slightly less readable.
I meant L1514 in this patchset, where we are calling getCurrentLaunchingSinkId()
on a new PseudoSinkSearchState, which always return clickedSink.id. With the
exception of L1514, invocations of getCurrentLaunchingSinkId() is always
preceded with checkForRealSink(), so I asked if it's possible to merge the two
calls.

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

https://codereview.chromium.org/1805813002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1035:
// that should not be hidden, then we convert them to the match object
It took me a couple of passes to understand the logic for including a
pseudoSink. But I think it would be good to spell out in the comments the
criterias for including a pseudo sink in the search result.

[btolsch, 2016-04-20 04:20:23]

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

https://codereview.chromium.org/1805813002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1477:
this.pseudoSinkSearchState_.receiveSinkResponse(sinkId);
On 2016/04/19 19:52:36, imcheng wrote:
> On 2016/04/19 01:39:43, btolsch wrote:
> > On 2016/04/18 23:15:47, imcheng wrote:
> > > can receiveSinkResponse and checkForRealSink be made into one call? In the
> one
> > > place where they are not called together (L1514), the pseudo sink id is
> always
> > > used.
> > 
> > I don't think I understand this comment. I assume you're referring to L1620
> > (L1547 in patchset 7) and not L1514 since that has nothing to do with
> > checkForRealSink(). But at L1620, I'm not sure what you mean by "the pseudo
> sink
> > id is always used." Could you explain?
> > 
> > It would be possible to make them one function if L1620 uses |null| to avoid
> > resetting an existing sink id, though I think that's slightly less readable.
> I meant L1514 in this patchset, where we are calling
getCurrentLaunchingSinkId()
> on a new PseudoSinkSearchState, which always return clickedSink.id. With the
> exception of L1514, invocations of getCurrentLaunchingSinkId() is always
> preceded with checkForRealSink(), so I asked if it's possible to merge the two
> calls.

That makes sense, and I did merge them since you also asked that in another
comment. But here you mentioned receiveSinkResponse() and checkForRealSink()
which was confusing.

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

https://codereview.chromium.org/1805813002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1035:
// that should not be hidden, then we convert them to the match object
On 2016/04/19 19:52:36, imcheng wrote:
> It took me a couple of passes to understand the logic for including a
> pseudoSink. But I think it would be good to spell out in the comments the
> criterias for including a pseudo sink in the search result.

Done.

[btolsch, 2016-04-20 04:20:40]

The CQ bit was checked by btolsch@chromium.org

[btolsch, 2016-04-20 04:20:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, amp@chromium.org, apacible@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1805813002/#ps160001
(title: "Clarifying pseudo sink comments")

[commit-bot: I haz the power, 2016-04-20 04:20:51]

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

[commit-bot: I haz the power, 2016-04-20 04:29:04]

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

[commit-bot: I haz the power, 2016-04-20 04:29:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[btolsch, 2016-04-20 16:32:08]

Description was changed from

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org,mfoltz@chromium...
==========

to

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org,mfoltz@chromium...
==========

[btolsch, 2016-04-20 16:32:08]

btolsch@chromium.org changed reviewers:
+ avayvod@chromium.org

[btolsch, 2016-04-20 16:32:50]

+avayvod for c/b/m/android files.

[whywhat, 2016-04-20 19:47:19]

android lgtm

[btolsch, 2016-04-20 23:12:48]

The CQ bit was checked by btolsch@chromium.org

[commit-bot: I haz the power, 2016-04-20 23:13:55]

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

[commit-bot: I haz the power, 2016-04-20 23:25:53]

Description was changed from

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org,mfoltz@chromium...
==========

to

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org,mfoltz@chromium...
==========

[commit-bot: I haz the power, 2016-04-20 23:25:55]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-22 19:27:07]

Description was changed from

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org,mfoltz@chromium...
==========

to

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

This change adds the ability for the filter UI to present the user with the
option to create a new sink that is named with the currently entered search
text if any route providers support this. This change adds all the necessary
wiring for the UI to talk to the Media Router and for the Media Router to talk
to the MRPM extension. Additionally, a route will be created to the currently
selected media source if a sink is found.

BUG=565696
R=amp@chromium.org,imcheng@chromium.org,apacible@chromium.org,mfoltz@chromium...

Committed: https://crrev.com/2443442ccda2d3ce929704bdefb8965067c09045
Cr-Commit-Position: refs/heads/master@{#388591}
==========

[commit-bot: I haz the power, 2016-04-22 19:27:10]

Patchset 9 (id:??) landed as
https://crrev.com/2443442ccda2d3ce929704bdefb8965067c09045
Cr-Commit-Position: refs/heads/master@{#388591}