[Media Router] Set timeout for route creation.

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/media_router/media_router_ui.h"
 
 #include <string>
 
 #include "base/strings/string_util.h"
 #include "chrome/browser/media/router/create_presentation_session_request.h"
 #include "chrome/browser/media/router/issue.h"
 #include "chrome/browser/media/router/issues_observer.h"
 #include "chrome/browser/media/router/media_route.h"
 #include "chrome/browser/media/router/media_router.h"
 #include "chrome/browser/media/router/media_router_factory.h"
 #include "chrome/browser/media/router/media_router_mojo_impl.h"
 #include "chrome/browser/media/router/media_routes_observer.h"
 #include "chrome/browser/media/router/media_sink.h"
 #include "chrome/browser/media/router/media_sinks_observer.h"
 #include "chrome/browser/media/router/media_source.h"
 #include "chrome/browser/media/router/media_source_helper.h"
 #include "chrome/browser/media/router/presentation_service_delegate_impl.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/webui/media_router/media_router_localized_strings_provider.h"
 #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();
   std::string host = gurl.host();
   if (base::StartsWith(host, "www.", base::CompareCase::INSENSITIVE_ASCII))
     host = host.substr(4);
   return host;
 }
 
 }  // namespace
@@ skipping 247 matching lines @@
                                             const std::string& error) {
   DVLOG(1) << "OnRouteResponseReceived";
   if (!route) {
     // The provider will handle sending an issue for a failed route request.
     DVLOG(0) << "MediaRouteResponse returned error: " << error;
   }
 
   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,
                                   MediaCastMode cast_mode) {
   DCHECK(query_result_manager_.get());
   DCHECK(initiator_);
 
   // Note that there is a rarely-encountered bug, where the MediaCastMode to
   // MediaSource mapping could have been updated, between when the user
   // clicked on the UI to start a create route request,
@@ skipping 45 matching lines @@
     } else if (presentation_service_delegate_) {
       route_response_callbacks.push_back(
           base::Bind(&PresentationServiceDelegateImpl::OnRouteResponse,
                      presentation_service_delegate_));
     }
   }
 
   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_);
 }
 
 const std::string& MediaRouterUI::GetRouteProviderExtensionId() const {
   return router_->media_route_provider_extension_id();
 }
 
 }  // namespace media_router