[Media Router] Set timeout for route creation.

chrome/browser/ui/webui/media_router/media_router_ui.cc

Index: chrome/browser/ui/webui/media_router/media_router_ui.cc
diff --git a/chrome/browser/ui/webui/media_router/media_router_ui.cc b/chrome/browser/ui/webui/media_router/media_router_ui.cc
index 296124c1f59c59f22c0c1b7be8b843fb45a79392..70c9ce64908ecdb92015f1135024dfb631a89d80 100644
--- a/chrome/browser/ui/webui/media_router/media_router_ui.cc
+++ b/chrome/browser/ui/webui/media_router/media_router_ui.cc
@@ -26,15 +26,20 @@
 #include "chrome/browser/ui/webui/media_router/media_router_resources_provider.h"
 #include "chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/grit/generated_resources.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_data_source.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/web_dialogs/web_dialog_delegate.h"
 
 namespace media_router {
 
 namespace {
 
+// The amount of time to wait for a response when creating a new route.
+static const int kCreateRouteTimeoutSeconds = 20;

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

no need to be static since it's already in anonymous namespace

[apacible, 2015/09/22 22:37:18]

Done.

+
 std::string GetHostFromURL(const GURL& gurl) {
   if (gurl.is_empty())
     return std::string();
@@ -302,6 +307,7 @@ void MediaRouterUI::OnRouteResponseReceived(const MediaSink::Id& sink_id,
   handler_->OnCreateRouteResponseReceived(sink_id, route);
   has_pending_route_request_ = false;
   requesting_route_for_default_source_ = false;
+  route_creation_timer_.Stop();
 }
 
 bool MediaRouterUI::DoCreateRoute(const MediaSink::Id& sink_id,
@@ -367,9 +373,24 @@ bool MediaRouterUI::DoCreateRoute(const MediaSink::Id& sink_id,
   router_->CreateRoute(source.id(), sink_id, origin,
                        SessionTabHelper::IdForTab(initiator_),
                        route_response_callbacks);
+
+  // Start the timer.

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

You should probably start the timer before CreateRoute, as it is possible for
the callbacks to be invoked immediately.

[apacible, 2015/09/22 22:37:18]

Done.

+  route_creation_timer_.Start(
+      FROM_HERE, base::TimeDelta::FromSeconds(kCreateRouteTimeoutSeconds),
+      this, &MediaRouterUI::RouteCreationTimeout);
+
   return true;
 }
 
+void MediaRouterUI::RouteCreationTimeout() {

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

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.

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

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?

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

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.

+  Issue issue(
+      l10n_util::GetStringUTF8(IDS_MEDIA_ROUTER_ISSUE_CREATE_ROUTE_TIMEOUT),
+      std::string(), IssueAction(IssueAction::TYPE_DISMISS),
+      std::vector<IssueAction>(), std::string(), Issue::NOTIFICATION,
+      false, std::string());
+  AddIssue(issue);

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

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]

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

+}
+
 std::string MediaRouterUI::GetFrameURLHost() const {
   return GetHostFromURL(frame_url_);
 }