From c2f678f863a10cb0b832cf2c7c034a302a9f40c0 Mon Sep 17 00:00:00 2001 From: Jennifer Apacible Date: Wed, 27 Jan 2016 17:38:47 -0800 Subject: [PATCH] [Media Router] Handle route creation where resolved route is not for display. This change handles route creation where the initially resolved route is not marked for display. This keeps track of the route id and checks if the route is updated to be ready for display. In the case where the route is never ready for display, route creation times out and fails. The MRP will send an issue to the Media Router if this happens. Updated reporting for relevant UMA metric. BUG=579138 Review URL: https://codereview.chromium.org/1605133002 Cr-Commit-Position: refs/heads/master@{#371385} (cherry picked from commit 3647a2e3c8476adfc7aba16eb12b7f3e13648ec6) Review URL: https://codereview.chromium.org/1646573003 . Cr-Commit-Position: refs/branch-heads/2623@{#171} Cr-Branched-From: 92d77538a86529ca35f9220bd3cd512cbea1f086-refs/heads/master@{#369907} --- .../media_router_container.js | 70 ++++++++++++++----- .../resources/media_router/media_router.js | 13 ++++ .../media_router/media_router_ui_interface.js | 18 +++-- .../ui/webui/media_router/media_router_ui.cc | 3 +- .../media_router_webui_message_handler.cc | 37 ++++++---- .../media_router_webui_message_handler.h | 3 +- ...a_router_webui_message_handler_unittest.cc | 21 ++---- 7 files changed, 108 insertions(+), 57 deletions(-) diff --git a/chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js b/chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js index 79b8d77e62ccf..886a53e77e0d3 100644 --- a/chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js +++ b/chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js @@ -189,6 +189,17 @@ Polymer({ value: false, }, + /** + * The ID of the route that is currently being created. This is set when + * route creation is resolved but not ready for its controls to be + * displayed. + * @private {string} + */ + pendingCreatedRouteId_: { + type: String, + value: '', + }, + /** * The time the sink list was shown and populated with at least one sink. * This is reset whenever the user switches views or there are no sinks @@ -851,26 +862,21 @@ Polymer({ * * @param {string} sinkId The ID of the sink to which the Media Route was * creating a route. - * @param {?media_router.Route} route The newly created route to the sink - * if succeeded; null otherwise. + * @param {string} routeId The ID of the newly created route for the sink if + * succeeded; empty otherwise. */ - onCreateRouteResponseReceived: function(sinkId, route) { - this.currentLaunchingSinkId_ = ''; - // The provider will handle sending an issue for a failed route request. - if (!route) + onCreateRouteResponseReceived: function(sinkId, routeId) { + // Check that |sinkId| exists and corresponds to |currentLaunchingSinkId_|. + if (!this.sinkMap_[sinkId] || this.currentLaunchingSinkId_ != sinkId) return; - // Check that |sinkId| exists. - if (!this.sinkMap_[sinkId]) + // The provider will handle sending an issue for a failed route request. + if (this.isEmptyOrWhitespace_(routeId)) { + this.resetRouteCreationProperties_(false); return; + } - // If there is an existing route associated with the same sink, its - // |sinkToRouteMap_| entry will be overwritten with that of the new route, - // which results in the correct sink to route mapping. - this.routeList.push(route); - this.showRouteDetails_(route); - - this.startTapTimer_(); + this.pendingCreatedRouteId_ = routeId; }, /** @@ -892,12 +898,10 @@ Polymer({ }, /** - * Handles timeout of previous create route attempt. Clearing - * |currentLaunchingSinkId_| hides the spinner indicating there is a route - * creation in progress and show the device icon instead. + * Handles timeout of previous create route attempt. */ onNotifyRouteCreationTimeout: function() { - this.currentLaunchingSinkId_ = ''; + this.resetRouteCreationProperties_(false); }, /** @@ -930,6 +934,19 @@ Polymer({ tempSinkToRouteMap[route.sinkId] = route; }, this); + // If there is route creation in progress, check if any of the route ids + // correspond to |pendingCreatedRouteId_|. If so, the newly created route + // is ready to be displayed; switch to route details view. + if (this.currentLaunchingSinkId_ != '' && + this.pendingCreatedRouteId_ != '') { + var route = tempSinkToRouteMap[this.currentLaunchingSinkId_]; + if (route && this.pendingCreatedRouteId_ == route.id) { + this.showRouteDetails_(route); + this.startTapTimer_(); + this.resetRouteCreationProperties_(true); + } + } + // If |currentRoute_| is no longer active, clear |currentRoute_|. Also // switch back to the SINK_PICKER view if the user is currently in the // ROUTE_DETAILS view. @@ -995,6 +1012,21 @@ Polymer({ this.rebuildSinksToShow_(); }, + /** + * Resets the properties relevant to creating a new route. Fires an event + * indicating whether or not route creation was successful. + * Clearing |currentLaunchingSinkId_| hides the spinner indicating there is + * a route creation in progress and show the device icon instead. + * + * @private + */ + resetRouteCreationProperties_: function(creationSuccess) { + this.currentLaunchingSinkId_ = ''; + this.pendingCreatedRouteId_ = ''; + + this.fire('report-route-creation', {success: creationSuccess}); + }, + /** * Updates the shown cast mode, and updates the header text fields * according to the cast mode. If |castMode| type is AUTO, then set diff --git a/chrome/browser/resources/media_router/media_router.js b/chrome/browser/resources/media_router/media_router.js index f358ce789670c..7a0535264b605 100644 --- a/chrome/browser/resources/media_router/media_router.js +++ b/chrome/browser/resources/media_router/media_router.js @@ -47,6 +47,7 @@ cr.define('media_router', function() { container.addEventListener('report-initial-action', onInitialAction); container.addEventListener('report-initial-action-close', onInitialActionClose); + container.addEventListener('report-route-creation', onReportRouteCreation); container.addEventListener('report-sink-click-time', onSinkClickTimeReported); container.addEventListener('report-sink-count', onSinkCountReported); @@ -218,6 +219,18 @@ cr.define('media_router', function() { media_router.MediaRouterView.SINK_LIST); } + /** + * Reports whether or not the route creation was successful. + * + * @param {!Event} event + * Parameters in |event|.detail: + * success - whether or not the route creation was successful. + */ + function onReportRouteCreation(event) { + var detail = event.detail; + media_router.browserApi.reportRouteCreation(detail.success); + } + /** * Reports the initial state of the dialog after it is opened. * Called after initial data is populated. diff --git a/chrome/browser/resources/media_router/media_router_ui_interface.js b/chrome/browser/resources/media_router/media_router_ui_interface.js index fb53eff22931c..4d22488bc15e2 100644 --- a/chrome/browser/resources/media_router/media_router_ui_interface.js +++ b/chrome/browser/resources/media_router/media_router_ui_interface.js @@ -22,11 +22,11 @@ cr.define('media_router.ui', function() { * * @param {string} sinkId The ID of the sink to which the Media Route was * creating a route. - * @param {?media_router.Route} route The newly create route to the sink - * if route creation succeeded; null otherwise + * @param {string} routeId The ID of the newly created route that corresponds + * to the sink if route creation succeeded; empty otherwise. */ - function onCreateRouteResponseReceived(sinkId, route) { - container.onCreateRouteResponseReceived(sinkId, route); + function onCreateRouteResponseReceived(sinkId, routeId) { + container.onCreateRouteResponseReceived(sinkId, routeId); } /** @@ -214,6 +214,15 @@ cr.define('media_router.browserApi', function() { chrome.send('reportNavigateToView', [view]); } + /** + * Reports whether or not a route was created successfully. + * + * @param {boolean} success + */ + function reportRouteCreation(success) { + chrome.send('reportRouteCreation', [success]); + } + /** * Reports the cast mode that the user selected. * @@ -284,6 +293,7 @@ cr.define('media_router.browserApi', function() { reportInitialState: reportInitialState, reportNavigateToView: reportNavigateToView, reportSelectedCastMode: reportSelectedCastMode, + reportRouteCreation: reportRouteCreation, reportSinkCount: reportSinkCount, reportTimeToClickSink: reportTimeToClickSink, reportTimeToInitialActionClose: reportTimeToInitialActionClose, 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 7814a68815f2c..9ceaded12d2a3 100644 --- a/chrome/browser/ui/webui/media_router/media_router_ui.cc +++ b/chrome/browser/ui/webui/media_router/media_router_ui.cc @@ -465,7 +465,8 @@ void MediaRouterUI::OnRouteResponseReceived(const int route_request_id, DVLOG(0) << "MediaRouteResponse returned error: " << error; } - handler_->OnCreateRouteResponseReceived(sink_id, route); + std::string route_id = route ? route->media_route_id() : std::string(); + handler_->OnCreateRouteResponseReceived(sink_id, route_id); current_route_request_id_ = -1; route_creation_timer_.Stop(); } diff --git a/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc b/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc index 558392be1ab90..a8e238fac7ee4 100644 --- a/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc +++ b/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc @@ -41,6 +41,7 @@ const char kReportClickedSinkIndex[] = "reportClickedSinkIndex"; const char kReportInitialAction[] = "reportInitialAction"; const char kReportInitialState[] = "reportInitialState"; const char kReportNavigateToView[] = "reportNavigateToView"; +const char kReportRouteCreation[] = "reportRouteCreation"; const char kReportSelectedCastMode[] = "reportSelectedCastMode"; const char kReportSinkCount[] = "reportSinkCount"; const char kReportTimeToClickSink[] = "reportTimeToClickSink"; @@ -222,22 +223,11 @@ void MediaRouterWebUIMessageHandler::UpdateCastModes( void MediaRouterWebUIMessageHandler::OnCreateRouteResponseReceived( const MediaSink::Id& sink_id, - const MediaRoute* route) { + const MediaRoute::Id& route_id) { DVLOG(2) << "OnCreateRouteResponseReceived"; - if (route) { - scoped_ptr route_value(RouteToValue(*route, false, - media_router_ui_->GetRouteProviderExtensionId())); - web_ui()->CallJavascriptFunction(kOnCreateRouteResponseReceived, - base::StringValue(sink_id), *route_value); - UMA_HISTOGRAM_BOOLEAN("MediaRouter.Ui.Action.StartLocalSessionSuccessful", - true); - } else { - web_ui()->CallJavascriptFunction(kOnCreateRouteResponseReceived, - base::StringValue(sink_id), - *base::Value::CreateNullValue()); - UMA_HISTOGRAM_BOOLEAN("MediaRouter.Ui.Action.StartLocalSessionSuccessful", - false); - } + web_ui()->CallJavascriptFunction(kOnCreateRouteResponseReceived, + base::StringValue(sink_id), + base::StringValue(route_id)); } void MediaRouterWebUIMessageHandler::UpdateIssue(const Issue* issue) { @@ -298,6 +288,10 @@ void MediaRouterWebUIMessageHandler::RegisterMessages() { kReportInitialAction, base::Bind(&MediaRouterWebUIMessageHandler::OnReportInitialAction, base::Unretained(this))); + web_ui()->RegisterMessageCallback( + kReportRouteCreation, + base::Bind(&MediaRouterWebUIMessageHandler::OnReportRouteCreation, + base::Unretained(this))); web_ui()->RegisterMessageCallback( kReportSelectedCastMode, base::Bind(&MediaRouterWebUIMessageHandler::OnReportSelectedCastMode, @@ -567,6 +561,19 @@ void MediaRouterWebUIMessageHandler::OnReportNavigateToView( } } +void MediaRouterWebUIMessageHandler::OnReportRouteCreation( + const base::ListValue* args) { + DVLOG(1) << "OnReportRouteCreation"; + bool route_created_successfully; + if (!args->GetBoolean(0, &route_created_successfully)) { + DVLOG(1) << "Unable to extract args."; + return; + } + + UMA_HISTOGRAM_BOOLEAN("MediaRouter.Ui.Action.StartLocalSessionSuccessful", + route_created_successfully); +} + void MediaRouterWebUIMessageHandler::OnReportSelectedCastMode( const base::ListValue* args) { DVLOG(1) << "OnReportSelectedCastMode"; diff --git a/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h b/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h index e5bec190eac8e..562ba43a09ee1 100644 --- a/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h +++ b/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h @@ -38,7 +38,7 @@ class MediaRouterWebUIMessageHandler : public content::WebUIMessageHandler { void UpdateCastModes(const CastModeSet& cast_modes, const std::string& source_host); void OnCreateRouteResponseReceived(const MediaSink::Id& sink_id, - const MediaRoute* route); + const MediaRoute::Id& route_id); // Does not take ownership of |issue|. Note that |issue| can be nullptr, when // there are no more issues. @@ -68,6 +68,7 @@ class MediaRouterWebUIMessageHandler : public content::WebUIMessageHandler { void OnReportInitialAction(const base::ListValue* args); void OnReportInitialState(const base::ListValue* args); void OnReportNavigateToView(const base::ListValue* args); + void OnReportRouteCreation(const base::ListValue* args); void OnReportSelectedCastMode(const base::ListValue* args); void OnReportSinkCount(const base::ListValue* args); void OnReportTimeToClickSink(const base::ListValue* args); diff --git a/chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc b/chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc index a936dd7c9a96a..39b534100368d 100644 --- a/chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc +++ b/chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc @@ -174,9 +174,7 @@ TEST_F(MediaRouterWebUIMessageHandlerTest, OnCreateRouteResponseReceived) { MediaRoute route(route_id, MediaSource("mediaSource"), sink_id, description, is_local, "", true); - EXPECT_CALL(*mock_media_router_ui_, GetRouteProviderExtensionId()).WillOnce( - ReturnRef(provider_extension_id())); - handler_->OnCreateRouteResponseReceived(sink_id, &route); + handler_->OnCreateRouteResponseReceived(sink_id, route.media_route_id()); EXPECT_EQ(1u, web_ui_->call_data().size()); const content::TestWebUI::CallData& call_data = *web_ui_->call_data()[0]; EXPECT_EQ("media_router.ui.onCreateRouteResponseReceived", @@ -187,20 +185,9 @@ TEST_F(MediaRouterWebUIMessageHandlerTest, OnCreateRouteResponseReceived) { EXPECT_EQ(sink_id, sink_id_value->GetString()); const base::Value* arg2 = call_data.arg2(); - const base::DictionaryValue* route_value = nullptr; - ASSERT_TRUE(arg2->GetAsDictionary(&route_value)); - - std::string value; - EXPECT_TRUE(route_value->GetString("id", &value)); - EXPECT_EQ(route_id, value); - EXPECT_TRUE(route_value->GetString("sinkId", &value)); - EXPECT_EQ(sink_id, value); - EXPECT_TRUE(route_value->GetString("description", &value)); - EXPECT_EQ(description, value); - - bool actual_is_local = false; - EXPECT_TRUE(route_value->GetBoolean("isLocal", &actual_is_local)); - EXPECT_EQ(is_local, actual_is_local); + const base::StringValue* route_id_value = nullptr; + ASSERT_TRUE(arg2->GetAsString(&route_id_value)); + EXPECT_EQ(route_id, route_id_value->GetString()); } TEST_F(MediaRouterWebUIMessageHandlerTest, UpdateIssue) {