[Media Router WebUI] Clear pending route creation properties on error.

[Media Router WebUI] Clear pending route creation properties on error.

Sometimes, routes are created but initially "not for display". These are later resolved as "for display", then updated with an updated route list from the MRP to the WebUI.

Currently, when a route fails to resolve "for display", an issue is sent to the WebUI but no indication that the route creation had failed. This causes the sink that is currently "pending route launch" to show the spinner forever, rendering the rest of the sink list unable to create a route.

This change corresponds with a change in the MRP that makes use of the |routeId| field in an incoming issue and clears any pending route creation properties if the error corresponds to |pendingCreatedRouteId_|. This also renames |mediaRouteId| to |routeId| to be uniform with other references.

BUG=592081
Committed: https://crrev.com/813d605e717ff246ea1bf65e11db6fbb4cf8679e
Cr-Commit-Position: refs/heads/master@{#382640}

[apacible, 2016-03-17 23:26:08]

Description was changed from

==========
asdf

BUG=
==========

to

==========
[Media Router WebUI] Clear pending route creation properties on error.



BUG=592081
==========

[apacible, 2016-03-17 23:36:17]

Description was changed from

==========
[Media Router WebUI] Clear pending route creation properties on error.



BUG=592081
==========

to

==========
[Media Router WebUI] Clear pending route creation properties on error.

Sometimes, routes are created but initially "not for display". These are later
resolved as "for display", then updated with an updated route list from the MRP
to the WebUI.

Currently, when a route fails to resolve "for display", an issue is sent to the
WebUI but no indication that the route creation had failed. This causes the sink
that is currently "pending route launch" to show the spinner forever, rendering
the rest of the sink list unable to create a route.

This change corresponds with a change in the MRP that makes use of the
|mediaRouteId| field in an incoming issue and clears any pending route creation
properties if the error corresponds to |pendingCreatedRouteId_|.

BUG=592081
==========

[apacible, 2016-03-17 23:38:49]

Patchset #1 (id:1) has been deleted

[apacible, 2016-03-21 16:41:06]

Patchset #1 (id:20001) has been deleted

[apacible, 2016-03-21 16:42:13]

Description was changed from

==========
[Media Router WebUI] Clear pending route creation properties on error.

Sometimes, routes are created but initially "not for display". These are later
resolved as "for display", then updated with an updated route list from the MRP
to the WebUI.

Currently, when a route fails to resolve "for display", an issue is sent to the
WebUI but no indication that the route creation had failed. This causes the sink
that is currently "pending route launch" to show the spinner forever, rendering
the rest of the sink list unable to create a route.

This change corresponds with a change in the MRP that makes use of the
|mediaRouteId| field in an incoming issue and clears any pending route creation
properties if the error corresponds to |pendingCreatedRouteId_|.

BUG=592081
==========

to

==========
[Media Router WebUI] Clear pending route creation properties on error.

Sometimes, routes are created but initially "not for display". These are later
resolved as "for display", then updated with an updated route list from the MRP
to the WebUI.

Currently, when a route fails to resolve "for display", an issue is sent to the
WebUI but no indication that the route creation had failed. This causes the sink
that is currently "pending route launch" to show the spinner forever, rendering
the rest of the sink list unable to create a route.

This change corresponds with a change in the MRP that makes use of the |routeId|
field in an incoming issue and clears any pending route creation properties if
the error corresponds to |pendingCreatedRouteId_|. This also renames
|mediaRouteId| to |routeId| to be uniform with other references.

BUG=592081
==========

[apacible, 2016-03-21 16:42:14]

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

[apacible, 2016-03-21 16:43:40]

Patchset #1 (id:40001) has been deleted

[apacible, 2016-03-21 16:44:02]

PTAL, thanks!

[mark a. foltz, 2016-03-21 20:41:21]

I think there is a somewhat more fundamental issue, which is that the MRP is
resolving to a route immediately on user action and then using an Issue to
signal the failure to create a route.

Also, do you know why the MRP is returning a not-for-display route immediately?

I recall discussing using something like the presentation connection states
('connecting', 'connected', 'closed', 'error') to track of the state of entire
routes as well.  That might be a cleaner way to handle these situations, but I
am not sure how well that would map onto the current behavior.

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

https://codereview.chromium.org/1807223003/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1211:
if (this.pendingCreatedRouteId_ != '' && !!issue && issue.routeId &&
!!this.pendingCreatedRouteId_

also the issue.routeId check should not be needed, since it can only ==
this.pendingCreatedRouteId_ when it is set.

https://codereview.chromium.org/1807223003/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1213:
this.resetRouteCreationProperties_(false);
Is this only something we want to do for blocking issues?

[apacible, 2016-03-21 23:17:28]

> I think there is a somewhat more fundamental issue, which is that the MRP is
> resolving to a route immediately on user action and then using an Issue to
> signal the failure to create a route.
> 
> Also, do you know why the MRP is returning a not-for-display route
immediately?
Sometimes the route is resolved before it's ready for display, so switching to
the route details would yield a mostly empty view with no controls.

> I recall discussing using something like the presentation connection states
> ('connecting', 'connected', 'closed', 'error') to track of the state of entire
> routes as well.  That might be a cleaner way to handle these situations, but I
> am not sure how well that would map onto the current behavior.
Acknowledged. It may be a good idea to revisit how we structure the data since
we've done additions in the past (e.g. for_display flag for routes, checks for
pending routes) that we could consolidate.

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

https://codereview.chromium.org/1807223003/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1211:
if (this.pendingCreatedRouteId_ != '' && !!issue && issue.routeId &&
On 2016/03/21 20:41:21, mark a. foltz wrote:
> !!this.pendingCreatedRouteId_
> 
> also the issue.routeId check should not be needed, since it can only ==
> this.pendingCreatedRouteId_ when it is set.

Done.

https://codereview.chromium.org/1807223003/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1213:
this.resetRouteCreationProperties_(false);
On 2016/03/21 20:41:21, mark a. foltz wrote:
> Is this only something we want to do for blocking issues?

No, the route creation failure issues are non-blocking.

[mark a. foltz, 2016-03-22 17:02:05]

lgtm

[apacible, 2016-03-22 18:13:17]

The CQ bit was checked by apacible@chromium.org

[commit-bot: I haz the power, 2016-03-22 18:13:36]

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

[commit-bot: I haz the power, 2016-03-22 19:36:10]

Description was changed from

==========
[Media Router WebUI] Clear pending route creation properties on error.

Sometimes, routes are created but initially "not for display". These are later
resolved as "for display", then updated with an updated route list from the MRP
to the WebUI.

Currently, when a route fails to resolve "for display", an issue is sent to the
WebUI but no indication that the route creation had failed. This causes the sink
that is currently "pending route launch" to show the spinner forever, rendering
the rest of the sink list unable to create a route.

This change corresponds with a change in the MRP that makes use of the |routeId|
field in an incoming issue and clears any pending route creation properties if
the error corresponds to |pendingCreatedRouteId_|. This also renames
|mediaRouteId| to |routeId| to be uniform with other references.

BUG=592081
==========

to

==========
[Media Router WebUI] Clear pending route creation properties on error.

Sometimes, routes are created but initially "not for display". These are later
resolved as "for display", then updated with an updated route list from the MRP
to the WebUI.

Currently, when a route fails to resolve "for display", an issue is sent to the
WebUI but no indication that the route creation had failed. This causes the sink
that is currently "pending route launch" to show the spinner forever, rendering
the rest of the sink list unable to create a route.

This change corresponds with a change in the MRP that makes use of the |routeId|
field in an incoming issue and clears any pending route creation properties if
the error corresponds to |pendingCreatedRouteId_|. This also renames
|mediaRouteId| to |routeId| to be uniform with other references.

BUG=592081
==========

[commit-bot: I haz the power, 2016-03-22 19:36:11]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-03-22 19:37:46]

Description was changed from

==========
[Media Router WebUI] Clear pending route creation properties on error.

Sometimes, routes are created but initially "not for display". These are later
resolved as "for display", then updated with an updated route list from the MRP
to the WebUI.

Currently, when a route fails to resolve "for display", an issue is sent to the
WebUI but no indication that the route creation had failed. This causes the sink
that is currently "pending route launch" to show the spinner forever, rendering
the rest of the sink list unable to create a route.

This change corresponds with a change in the MRP that makes use of the |routeId|
field in an incoming issue and clears any pending route creation properties if
the error corresponds to |pendingCreatedRouteId_|. This also renames
|mediaRouteId| to |routeId| to be uniform with other references.

BUG=592081
==========

to

==========
[Media Router WebUI] Clear pending route creation properties on error.

Sometimes, routes are created but initially "not for display". These are later
resolved as "for display", then updated with an updated route list from the MRP
to the WebUI.

Currently, when a route fails to resolve "for display", an issue is sent to the
WebUI but no indication that the route creation had failed. This causes the sink
that is currently "pending route launch" to show the spinner forever, rendering
the rest of the sink list unable to create a route.

This change corresponds with a change in the MRP that makes use of the |routeId|
field in an incoming issue and clears any pending route creation properties if
the error corresponds to |pendingCreatedRouteId_|. This also renames
|mediaRouteId| to |routeId| to be uniform with other references.

BUG=592081

Committed: https://crrev.com/813d605e717ff246ea1bf65e11db6fbb4cf8679e
Cr-Commit-Position: refs/heads/master@{#382640}
==========

[commit-bot: I haz the power, 2016-03-22 19:37:47]

Patchset 2 (id:??) landed as
https://crrev.com/813d605e717ff246ea1bf65e11db6fbb4cf8679e
Cr-Commit-Position: refs/heads/master@{#382640}