[Media Router] Set timeout for route creation.

[Media Router] Set timeout for route creation.

This sets a timeout in case of a route creation request that doesn't get a response from the provider.

We determined the timeout time, 20s, based on a couple of factors. The default cast timeout is 10s, but there is a way for third party apps to override the timeout, usually using 12s instead. This change also makes the WebUI handle a timeout on route creation.

This change also removes is_launching from the MediaSink. We determine which Sink is currently being launched by tracking the ID of the Sink that was clicked in the WebUI.

BUG=490372
Committed: https://crrev.com/05ed82a4c0bd71cd6ea3f39d922eb4a45bb39075
Cr-Commit-Position: refs/heads/master@{#352615}

[apacible, 2015-09-22 20:02:24]

Patchset #1 (id:1) has been deleted

[apacible, 2015-09-22 20:03:16]

Patchset #1 (id:20001) has been deleted

[imcheng, 2015-09-22 21:45:11]

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

[imcheng, 2015-09-22 21:45:12]

https://codereview.chromium.org/1363593003/diff/40001/chrome/app/media_router...
File chrome/app/media_router_strings.grdp (right):

https://codereview.chromium.org/1363593003/diff/40001/chrome/app/media_router...
chrome/app/media_router_strings.grdp:54: <message
name="IDS_MEDIA_ROUTER_ISSUE_CREATE_ROUTE_TIMEOUT" desc="Title of an issue to
show when the user attemps to create a route and the Media Router times out
while waiting for a route creation response.">
typo in "attempts"

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

https://codereview.chromium.org/1363593003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:41: static const int
kCreateRouteTimeoutSeconds = 20;
no need to be static since it's already in anonymous namespace

https://codereview.chromium.org/1363593003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:377: // Start the timer.
You should probably start the timer before CreateRoute, as it is possible for
the callbacks to be invoked immediately.

https://codereview.chromium.org/1363593003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:385: void
MediaRouterUI::RouteCreationTimeout() {
Hmm... two things. We will have to think about this carefully.

1) Looks like we need to reset has_pending_route_request_ and
requesting_route_for_default_source_, otherwise the dialog will be in a bad
state if we never get a response back. See OnRouteResponseReceived.
2) What if we do get response back from the router after the timeout? That will
likely put it into a bad state as well if we started another route request while
the previous one is coming back.

https://codereview.chromium.org/1363593003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:391: AddIssue(issue);
nit: no need to have |issue| local variable. But it's up to you if you think
separating it increases readability.

[apacible, 2015-09-22 22:37:19]

https://codereview.chromium.org/1363593003/diff/40001/chrome/app/media_router...
File chrome/app/media_router_strings.grdp (right):

https://codereview.chromium.org/1363593003/diff/40001/chrome/app/media_router...
chrome/app/media_router_strings.grdp:54: <message
name="IDS_MEDIA_ROUTER_ISSUE_CREATE_ROUTE_TIMEOUT" desc="Title of an issue to
show when the user attemps to create a route and the Media Router times out
while waiting for a route creation response.">
On 2015/09/22 21:45:11, imcheng1 wrote:
> typo in "attempts"

Done.

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

https://codereview.chromium.org/1363593003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:41: static const int
kCreateRouteTimeoutSeconds = 20;
On 2015/09/22 21:45:12, imcheng1 wrote:
> no need to be static since it's already in anonymous namespace

Done.

https://codereview.chromium.org/1363593003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:377: // Start the timer.
On 2015/09/22 21:45:12, imcheng1 wrote:
> You should probably start the timer before CreateRoute, as it is possible for
> the callbacks to be invoked immediately.

Done.

https://codereview.chromium.org/1363593003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:385: void
MediaRouterUI::RouteCreationTimeout() {
On 2015/09/22 21:45:12, imcheng1 wrote:
> Hmm... two things. We will have to think about this carefully.
> 
> 1) Looks like we need to reset has_pending_route_request_ and
> requesting_route_for_default_source_, otherwise the dialog will be in a bad
> state if we never get a response back. See OnRouteResponseReceived.
> 2) What if we do get response back from the router after the timeout? That
will
> likely put it into a bad state as well if we started another route request
while
> the previous one is coming back.

1) Done.
2) Hmm, some questions:
- Can we tell the provider to cancel any/all route requests? (presumably if we
time out)
- Can the provider handle >1 route request?

https://codereview.chromium.org/1363593003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:391: AddIssue(issue);
On 2015/09/22 21:45:12, imcheng1 wrote:
> nit: no need to have |issue| local variable. But it's up to you if you think
> separating it increases readability.

Keeping as is; I also did this for readability in the other issue we send from
Media Router.

[apacible, 2015-09-23 21:08:00]

Patchset #3 (id:80001) has been deleted

[imcheng, 2015-09-25 17:43:18]

looks like you will need to rebase? There was a timer patch that landed
recently.

[apacible, 2015-09-25 18:36:03]

On 2015/09/25 17:43:18, imcheng1 wrote:
> looks like you will need to rebase? There was a timer patch that landed
> recently.

Yep, I will rebase this afternoon.

[apacible, 2015-09-29 13:53:28]

Friendly ping!

[imcheng, 2015-09-29 19:08:53]

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

https://codereview.chromium.org/1363593003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:385: void
MediaRouterUI::RouteCreationTimeout() {
On 2015/09/22 22:37:19, apacible wrote:
> On 2015/09/22 21:45:12, imcheng1 wrote:
> > Hmm... two things. We will have to think about this carefully.
> > 
> > 1) Looks like we need to reset has_pending_route_request_ and
> > requesting_route_for_default_source_, otherwise the dialog will be in a bad
> > state if we never get a response back. See OnRouteResponseReceived.
> > 2) What if we do get response back from the router after the timeout? That
> will
> > likely put it into a bad state as well if we started another route request
> while
> > the previous one is coming back.
> 
> 1) Done.
> 2) Hmm, some questions:
> - Can we tell the provider to cancel any/all route requests? (presumably if we
> time out)
> - Can the provider handle >1 route request?

Re #2: In general, we don't have a way to cancel a route request. The provider
is able to handle >1 route requests (for example, you can send route requests in
separate tabs) but how it is handled is up to the provider.

As discussed offline, MediaRouterUI should ignore requests that arrive after the
timeout (i.e., it should not cause the dialog to navigate to route details or
reset the state of this class). One way to do this is to generate and bind a
unique id to MediaRouterUI::OnRouteResponseReceived, and perform a match against
the current request id when it is invoked.

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

https://codereview.chromium.org/1363593003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:412: AddIssue(issue);
We can also consider doing away with the timer on the WebUI side. When this
method is called, we could send a command to WebUI to add the issue and reset
the sink picker to original state.

[apacible, 2015-09-30 18:05:46]

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

https://codereview.chromium.org/1363593003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:412: AddIssue(issue);
On 2015/09/29 19:08:53, imcheng1 wrote:
> We can also consider doing away with the timer on the WebUI side. When this
> method is called, we could send a command to WebUI to add the issue and reset
> the sink picker to original state.

We actually don't have a timer on the WebUI side, so if the provider never
responds the spinner just keeps spinning until the sinks are updated where the
clicked sink no longer has "isLaunching" set. This also leaves the WebUI in a
weird state if a sink is left "launching".

[imcheng, 2015-10-02 21:57:30]

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

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:411:
computeSinkIsLaunching_: function(isLaunching, isSinkLaunching) {
why can't we check only isSinkLaunching ? If isSinkLaunching is true then
isLauching is going to be true as well, right?

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/media_router/media_router_ui_interface.js (right):

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/media_router_ui_interface.js:14:
container.isLaunching = false;
do we also need to set the launch state for the sink back to false?

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/media_router_ui_interface.js:51: *        
headerTextTooltip: string,-
extra - at the end

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

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:423: AddIssue(issue);
Does that mean this issue will be shown in dialogs in other tabs (since they
also have IssueObservers)?

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:173: const std::string&
route_guid,
s/route_guid/route_request_id. I would also put this as first arg.

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:212: // Set to a random
GUID when expecting a route creation, or empty otherwise.
suggestion: Set to non-empty if tracking a pending route request.

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:213: // It's possible for
a route request to time out in Media Router, then the
not sure if this comment is needed. Remove?

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:216: std::string
expected_route_guid_;
I would just name this current_route_request_id_. The fact that it is a guid is
an implementation detail. Also, can we just use a simple sequential counter?

[apacible, 2015-10-02 23:45:00]

Patchset #8 (id:200001) has been deleted

[apacible, 2015-10-02 23:45:17]

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

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:411:
computeSinkIsLaunching_: function(isLaunching, isSinkLaunching) {
On 2015/10/02 21:57:30, imcheng1 wrote:
> why can't we check only isSinkLaunching ? If isSinkLaunching is true then
> isLauching is going to be true as well, right?

Not necessarily. 

The sink list isn't updated on timeout (likely because we expect the primary
reason for timeout is that the provider isn't responding). I considered setting
|sink.isLaunching| for all sinks back to false (where you commented in
media_router_ui_interface.js), but in the case where the routes suddenly update
to the WebUI where isLaunching hasn't been updated yet, we'd be in a weird state
where a sink thinks it's still launching. Using an overall |isLaunching|
overrides any of that.

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/media_router/media_router_ui_interface.js (right):

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/media_router_ui_interface.js:14:
container.isLaunching = false;
On 2015/10/02 21:57:30, imcheng1 wrote:
> do we also need to set the launch state for the sink back to false?

See thread in media_router_container.js.

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/media_router_ui_interface.js:51: *        
headerTextTooltip: string,-
On 2015/10/02 21:57:30, imcheng1 wrote:
> extra - at the end

Done.

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

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:423: AddIssue(issue);
On 2015/10/02 21:57:30, imcheng1 wrote:
> Does that mean this issue will be shown in dialogs in other tabs (since they
> also have IssueObservers)?

Correct.

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:173: const std::string&
route_guid,
On 2015/10/02 21:57:30, imcheng1 wrote:
> s/route_guid/route_request_id. I would also put this as first arg.

Done.

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:212: // Set to a random
GUID when expecting a route creation, or empty otherwise.
On 2015/10/02 21:57:30, imcheng1 wrote:
> suggestion: Set to non-empty if tracking a pending route request.

Done.

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:213: // It's possible for
a route request to time out in Media Router, then the
On 2015/10/02 21:57:30, imcheng1 wrote:
> not sure if this comment is needed. Remove?

Done.

https://codereview.chromium.org/1363593003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:216: std::string
expected_route_guid_;
On 2015/10/02 21:57:30, imcheng1 wrote:
> I would just name this current_route_request_id_. The fact that it is a guid
is
> an implementation detail. Also, can we just use a simple sequential counter?

Renamed. Switched to sequential counter.

[apacible, 2015-10-05 19:04:29]

Updated per offline conversation:
- Removed is_launching from the MediaSink.
- Removed isLaunching from media-router-container.
- Keeps track of the currently launching MediaSink with the ID in the
media-router-container. It is empty if no Sink is currently launching.

[imcheng, 2015-10-06 01:28:29]

lgtm

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

https://codereview.chromium.org/1363593003/diff/220001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:28:
disabled$="[[isLaunching]]" id="arrow-drop-icon"
does this need to be updated?

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

https://codereview.chromium.org/1363593003/diff/220001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:496:
// this.setLaunchState_(sinkId, false);
remove commented out line

[apacible, 2015-10-06 01:50:59]

Patchset #9 (id:240001) has been deleted

[apacible, 2015-10-06 01:51:34]

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

https://codereview.chromium.org/1363593003/diff/220001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:28:
disabled$="[[isLaunching]]" id="arrow-drop-icon"
On 2015/10/06 01:28:29, imcheng1 wrote:
> does this need to be updated?

Done.

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

https://codereview.chromium.org/1363593003/diff/220001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:496:
// this.setLaunchState_(sinkId, false);
On 2015/10/06 01:28:29, imcheng1 wrote:
> remove commented out line

Done.

[apacible, 2015-10-06 01:52:05]

Patchset #10 (id:280001) has been deleted

[apacible, 2015-10-06 16:16:39]

The CQ bit was checked by apacible@chromium.org

[apacible, 2015-10-06 16:16:40]

The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1363593003/#ps260001
(title: "Changes per imcheng@'s comments.")

[commit-bot: I haz the power, 2015-10-06 16:17:37]

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

[commit-bot: I haz the power, 2015-10-06 16:25:40]

Committed patchset #9 (id:260001)

[commit-bot: I haz the power, 2015-10-06 16:26:25]

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