Skip to content

Commit

Permalink
[Media Router] Support custom timeout for route requests.
Browse files Browse the repository at this point in the history
Add base::TimeDelta parameter to CreateRoute/JoinRoute/
ConnectRouteByRouteId APIs. This allows us to specify different timeout
amounts depending on the source used in the route creation.

This timeout will be provided to MRPM to be enforced. Note that if
timeout is not provided, the MRPM will still use a default timeout in
the interest of not keeping the extension alive indefinitely.

In addition, a result code is added to the route response. Currently
we have a TIMED_OUT code for timed out requests. In the future we can
add more codes to differentiate error conditions in order to surface
to pages that use Presentation API.

Refactored the data of a route response into RouteRequestResult class.

Modified the desktop UI code to pass in timeout amounts to CreateRoute
and ConnectRouteByRouteId depending on source / cast mode. Also
modified / simplified route request timeout logic in UI.

BUG=581818,567368

Review URL: https://codereview.chromium.org/1639053003

Cr-Commit-Position: refs/heads/master@{#373708}
  • Loading branch information
imcheng authored and Commit bot committed Feb 5, 2016
1 parent 600b32e commit 20f3f90
Show file tree
Hide file tree
Showing 32 changed files with 661 additions and 335 deletions.
32 changes: 23 additions & 9 deletions chrome/browser/media/android/router/media_router_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "chrome/browser/media/router/media_routes_observer.h"
#include "chrome/browser/media/router/media_sinks_observer.h"
#include "chrome/browser/media/router/presentation_session_messages_observer.h"
#include "chrome/browser/media/router/route_request_result.h"
#include "jni/ChromeMediaRouter_jni.h"
#include "url/gurl.h"

Expand Down Expand Up @@ -71,10 +72,14 @@ void MediaRouterAndroid::CreateRoute(
const MediaSink::Id& sink_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks) {
const std::vector<MediaRouteResponseCallback>& callbacks,
base::TimeDelta timeout) {
// TODO(avayvod): Implement timeouts (crbug.com/583036).
if (!origin.is_valid()) {
scoped_ptr<RouteRequestResult> result = RouteRequestResult::FromError(
"Invalid origin", RouteRequestResult::INVALID_ORIGIN);
for (const MediaRouteResponseCallback& callback : callbacks)
callback.Run(nullptr, "", "Invalid origin");
callback.Run(*result);
return;
}

Expand Down Expand Up @@ -121,7 +126,8 @@ void MediaRouterAndroid::ConnectRouteByRouteId(
const MediaRoute::Id& route_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks) {
const std::vector<MediaRouteResponseCallback>& callbacks,
base::TimeDelta timeout) {
NOTIMPLEMENTED();
}

Expand All @@ -130,10 +136,14 @@ void MediaRouterAndroid::JoinRoute(
const std::string& presentation_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks) {
const std::vector<MediaRouteResponseCallback>& callbacks,
base::TimeDelta timeout) {
// TODO(avayvod): Implement timeouts (crbug.com/583036).
if (!origin.is_valid()) {
scoped_ptr<RouteRequestResult> result = RouteRequestResult::FromError(
"Invalid origin", RouteRequestResult::INVALID_ORIGIN);
for (const MediaRouteResponseCallback& callback : callbacks)
callback.Run(nullptr, "", "Invalid origin");
callback.Run(*result);
return;
}

Expand Down Expand Up @@ -365,8 +375,10 @@ void MediaRouterAndroid::OnRouteCreated(
jis_local, std::string(),
true); // TODO(avayvod): Populate for_display.

scoped_ptr<RouteRequestResult> result = RouteRequestResult::FromSuccess(
make_scoped_ptr(new MediaRoute(route)), request->presentation_id);
for (const MediaRouteResponseCallback& callback : request->callbacks)
callback.Run(&route, request->presentation_id, std::string());
callback.Run(*result);

route_requests_.Remove(jroute_request_id);

Expand All @@ -384,10 +396,12 @@ void MediaRouterAndroid::OnRouteRequestError(
if (!request)
return;

std::string error_text = ConvertJavaStringToUTF8(env, jerror_text);

// TODO(imcheng): Provide a more specific result code.
scoped_ptr<RouteRequestResult> result =
RouteRequestResult::FromError(ConvertJavaStringToUTF8(env, jerror_text),
RouteRequestResult::UNKNOWN_ERROR);
for (const MediaRouteResponseCallback& callback : request->callbacks)
callback.Run(nullptr, std::string(), error_text);
callback.Run(*result);

route_requests_.Remove(jroute_request_id);
}
Expand Down
27 changes: 14 additions & 13 deletions chrome/browser/media/android/router/media_router_android.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,25 @@ class MediaRouterAndroid : public MediaRouterBase {
const MediaRoute* FindRouteBySource(const MediaSource::Id& source_id) const;

// MediaRouter implementation.
void CreateRoute(
const MediaSource::Id& source_id,
const MediaSink::Id& sink_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks) override;
void JoinRoute(
const MediaSource::Id& source,
const std::string& presentation_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks) override;
void CreateRoute(const MediaSource::Id& source_id,
const MediaSink::Id& sink_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks,
base::TimeDelta timeout) override;
void JoinRoute(const MediaSource::Id& source,
const std::string& presentation_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks,
base::TimeDelta timeout) override;
void ConnectRouteByRouteId(
const MediaSource::Id& source,
const MediaRoute::Id& route_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks) override;
const std::vector<MediaRouteResponseCallback>& callbacks,
base::TimeDelta timeout) override;
void DetachRoute(const MediaRoute::Id& route_id) override;
void TerminateRoute(const MediaRoute::Id& route_id) override;
void SendRouteMessage(const MediaRoute::Id& route_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,8 @@ void MediaRouterDialogControllerAndroid::OnSinkSelected(

MediaRouter* router = MediaRouterFactory::GetApiForBrowserContext(
initiator()->GetBrowserContext());
router->CreateRoute(
source_id,
ConvertJavaStringToUTF8(env, jsink_id),
origin,
initiator(),
route_response_callbacks);
router->CreateRoute(source_id, ConvertJavaStringToUTF8(env, jsink_id), origin,
initiator(), route_response_callbacks, base::TimeDelta());
}

void MediaRouterDialogControllerAndroid::OnRouteClosed(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "chrome/browser/media/router/create_presentation_connection_request.h"

#include "chrome/browser/media/router/media_source_helper.h"
#include "chrome/browser/media/router/route_request_result.h"

using content::PresentationSessionInfo;
using content::PresentationError;
Expand Down Expand Up @@ -57,15 +58,13 @@ void CreatePresentationConnectionRequest::InvokeErrorCallback(
// static
void CreatePresentationConnectionRequest::HandleRouteResponse(
scoped_ptr<CreatePresentationConnectionRequest> presentation_request,
const MediaRoute* route,
const std::string& presentation_id,
const std::string& error) {
if (!route) {
presentation_request->InvokeErrorCallback(
content::PresentationError(content::PRESENTATION_ERROR_UNKNOWN, error));
const RouteRequestResult& result) {
if (!result.route()) {
presentation_request->InvokeErrorCallback(content::PresentationError(
content::PRESENTATION_ERROR_UNKNOWN, result.error()));
} else {
presentation_request->InvokeSuccessCallback(presentation_id,
route->media_route_id());
presentation_request->InvokeSuccessCallback(
result.presentation_id(), result.route()->media_route_id());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ struct PresentationSessionInfo;

namespace media_router {

class RouteRequestResult;

// Holds parameters for creating a presentation session.
// A request object is created by presentation_service_delegate_impl when it
// gets create-session request. The object is then passed to and owned by the
Expand Down Expand Up @@ -62,9 +64,7 @@ class CreatePresentationConnectionRequest {
// Handle route creation/joining response by invoking the right callback.
static void HandleRouteResponse(
scoped_ptr<CreatePresentationConnectionRequest> presentation_request,
const MediaRoute* route,
const std::string& presentation_id,
const std::string& error);
const RouteRequestResult& result);

private:
const PresentationRequest presentation_request_;
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/media/router/media_router.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
'presentation_session_messages_observer.cc',
'presentation_session_messages_observer.h',
'render_frame_host_id.h',
'route_request_result.cc',
'route_request_result.h',
],
# Files that are only needed on desktop builds
'media_router_non_android_sources': [
Expand Down
37 changes: 18 additions & 19 deletions chrome/browser/media/router/media_router.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "base/callback.h"
#include "base/callback_list.h"
#include "base/memory/scoped_vector.h"
#include "base/time/time.h"
#include "chrome/browser/media/router/issue.h"
#include "chrome/browser/media/router/media_route.h"
#include "chrome/browser/media/router/media_sink.h"
Expand All @@ -32,25 +33,13 @@ class MediaRoutesObserver;
class MediaSinksObserver;
class PresentationConnectionStateObserver;
class PresentationSessionMessagesObserver;
class RouteRequestResult;

// Type of callback used in |CreateRoute()|, |JoinRoute()|, and
// |ConnectRouteByRouteId()|. Callback is
// invoked when the route request either succeeded or failed.
// On success:
// |route|: The route created or joined.
// |presentation_id|:
// The presentation ID of the route created or joined. In the case of
// |CreateRoute()|, the ID is generated by MediaRouter and is guaranteed to
// be unique.
// |error|: Empty string.
// On failure:
// |route|: nullptr
// |presentation_id|: Empty string.
// |error|: Non-empty string describing the error.
// |ConnectRouteByRouteId()|. Callback is invoked when the route request either
// succeeded or failed.
using MediaRouteResponseCallback =
base::Callback<void(const MediaRoute* route,
const std::string& presentation_id,
const std::string& error)>;
base::Callback<void(const RouteRequestResult& result)>;

// Subscription object returned by calling
// |AddPresentationConnectionStateChangedCallback|. See the method comments for
Expand All @@ -62,6 +51,7 @@ using PresentationConnectionStateSubscription = base::CallbackList<void(
// Responsible for registering observers for receiving sink availability
// updates, handling route requests/responses, and operating on routes (e.g.
// posting messages or closing).
// TODO(imcheng): Reduce number of parameters by putting them into structs.
class MediaRouter : public KeyedService {
public:
using PresentationSessionMessageCallback = base::Callback<void(
Expand All @@ -80,12 +70,15 @@ class MediaRouter : public KeyedService {
// The caller may pass in nullptr for |web_contents| if tab is not applicable.
// Each callback in |callbacks| is invoked with a response indicating
// success or failure, in the order they are listed.
// If |timeout| is positive, then any un-invoked |callbacks| will be invoked
// with a timeout error after the timeout expires.
virtual void CreateRoute(
const MediaSource::Id& source_id,
const MediaSink::Id& sink_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks) = 0;
const std::vector<MediaRouteResponseCallback>& callbacks,
base::TimeDelta timeout) = 0;

// Creates a route and connects it to an existing route identified by
// |route_id|. |route_id| must refer to a non-local route, unnassociated with
Expand All @@ -97,12 +90,15 @@ class MediaRouter : public KeyedService {
// (See CreateRoute documentation).
// Each callback in |callbacks| is invoked with a response indicating
// success or failure, in the order they are listed.
// If |timeout| is positive, then any un-invoked |callbacks| will be invoked
// with a timeout error after the timeout expires.
virtual void ConnectRouteByRouteId(
const MediaSource::Id& source_id,
const MediaRoute::Id& route_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks) = 0;
const std::vector<MediaRouteResponseCallback>& callbacks,
base::TimeDelta timeout) = 0;

// Joins an existing route identified by |presentation_id|.
// |source|: The source to route to the existing route.
Expand All @@ -112,12 +108,15 @@ class MediaRouter : public KeyedService {
// (See CreateRoute documentation).
// Each callback in |callbacks| is invoked with a response indicating
// success or failure, in the order they are listed.
// If |timeout| is positive, then any un-invoked |callbacks| will be invoked
// with a timeout error after the timeout expires.
virtual void JoinRoute(
const MediaSource::Id& source,
const std::string& presentation_id,
const GURL& origin,
content::WebContents* web_contents,
const std::vector<MediaRouteResponseCallback>& callbacks) = 0;
const std::vector<MediaRouteResponseCallback>& callbacks,
base::TimeDelta timeout) = 0;

// Terminates the media route specified by |route_id|.
virtual void TerminateRoute(const MediaRoute::Id& route_id) = 0;
Expand Down
37 changes: 33 additions & 4 deletions chrome/browser/media/router/media_router.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,14 @@ struct RouteMessage {
array<uint8>? data;
};

// Maps to a ResultCode value in route_request_result.h
// The enum defined here is a subset of those defined in route_request_result.h.
enum RouteRequestResultCode {
UNKNOWN_ERROR,
OK,
TIMED_OUT
};

// Modeled after the MediaRouter interface defined in
// chrome/browser/media/router/media_router.h
interface MediaRouteProvider {
Expand All @@ -122,45 +130,66 @@ interface MediaRouteProvider {
// same-tab scopes.
// Since int types cannot be made optional, use -1 as |tab_id| in cases where
// it is not applicable.
// If |timeout_millis| is positive, it will be used in place of the default
// timeout defined by Media Route Provider Manager.
// If the operation was successful, |route| will be defined and
// |error_text| will be null.
// If the operation failed, |route| will be null and |error_text|
// will be set.
// |result| will be set to OK if successful, or an error code if an error
// occurred.
CreateRoute(string media_source,
string sink_id,
string original_presentation_id,
string origin,
int32 tab_id) => (MediaRoute? route, string? error_text);
int32 tab_id,
int64 timeout_millis) => (MediaRoute? route,
string? error_text,
RouteRequestResultCode result_code);

// Joins an established route given by |presentation_id|
// with |media_source|.
// |origin| and |tab_id| are used for validating same-origin/tab scopes.
// (See CreateRoute for additional documentation)
// If |timeout_millis| is positive, it will be used in place of the default
// timeout defined by Media Route Provider Manager.
// If the operation was successful, |route| will be defined and
// |error_text| will be null.
// If the operation failed, |route| will be null and |error_text|
// will be set.
// |result| will be set to OK if successful, or an error code if an error
// occurred.
JoinRoute(string media_source,
string presentation_id,
string origin,
int32 tab_id) => (MediaRoute? route, string? error_text);
int32 tab_id,
int64 timeout_millis) => (MediaRoute? route,
string? error_text,
RouteRequestResultCode result_code);

// Connects an established route given by |route_id| with
// |media_source|. The presentation ID of the route created will be
// |presentation_id|, but it may be overridden by a provider implementation.
// The presentation ID will be used by the presentation API to refer to the
// created route. |origin| and |tab_id| are used for validating
// same-origin/tab scopes.
// If |timeout_millis| is positive, it will be used in place of the default
// timeout defined by Media Route Provider Manager.
// (See CreateRoute for additional documentation)
// If the operation was successful, |route| will be defined and
// |error_text| will be null. If the operation failed, |route| will be null
// and |error_text| will be set.
// |result| will be set to OK if successful, or an error code if an error
// occurred.
ConnectRouteByRouteId(string media_source,
string route_id,
string presentation_id,
string origin,
int32 tab_id) =>
(MediaRoute? route, string? error_text);
int32 tab_id,
int64 timeout_millis) =>
(MediaRoute? route,
string? error_text,
RouteRequestResultCode result_code);

// Terminates the route specified by |route_id|.
TerminateRoute(string route_id);
Expand Down
Loading

0 comments on commit 20f3f90

Please sign in to comment.