[Media Router] MediaRouterUI + WebUI handler implementation.

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

Index: chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
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 183ae946b928fc18cb9b27fa64c8a80702ebba00..f6f1eeb988f332ee1ff5b9694c1df3b1959f7e0e 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
@@ -6,7 +6,9 @@
 
 #include "base/bind.h"
 #include "chrome/browser/ui/webui/media_router/media_router_ui.h"
+#include "chrome/grit/generated_resources.h"
 #include "content/public/browser/web_ui.h"
+#include "ui/base/l10n/l10n_util.h"
 
 namespace media_router {
 
@@ -19,7 +21,67 @@ const char kActOnIssue[] = "actOnIssue";
 const char kCloseRoute[] = "closeRoute";
 const char kCloseDialog[] = "closeDialog";
 
-// TODO(imcheng): Define JS function names here.
+// Callback names.

[Wez, 2015/05/20 17:51:47]

Are these actually callback names, or event/notification names?

[imcheng (use chromium acct), 2015/05/20 22:01:14]

These are JS function names.

+const char kSetInitialSettings[] = "media_router.setInitialSettings";
+const char kAddRoute[] = "media_router.ui.addRoute";
+const char kSetSinkList[] = "media_router.ui.setSinkList";
+const char kSetRouteList[] = "media_router.ui.setRouteList";
+const char kSetCastModeList[] = "media_router.ui.setCastModeList";
+
+void SetSinksListValue(

[Wez, 2015/05/20 17:51:47]

nit: As below, can you just have this accept the sinks and return a
scoped_ptr<base::ListValue>? It means one of your call-sites loses the stack
allocation optimization, but since the ListValue will be heap allocating stuff
internally, I doubt that that's a significant impact. (Famous last words...)

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Done.

+    const std::vector<MediaSinkWithCastModes>& sinks,
+    base::ListValue* value) {
+  value->Clear();
+
+  for (const MediaSinkWithCastModes& sink_with_cast_modes : sinks) {

[Wez, 2015/05/20 17:51:47]

nit: Suggest adding some blank lines in here to break the loop contents up into
logical chunks, for readability.

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Done.

+    const MediaSink& sink = sink_with_cast_modes.sink;
+    scoped_ptr<base::DictionaryValue> sink_val(new base::DictionaryValue);
+    sink_val->SetString("id", sink.id());
+    sink_val->SetString("name", sink.name());
+    scoped_ptr<base::ListValue> cast_modes_val(new base::ListValue);
+    for (MediaCastMode cast_mode : sink_with_cast_modes.cast_modes)
+      cast_modes_val->AppendInteger(cast_mode);
+    sink_val->Set("castModes", cast_modes_val.Pass());
+    value->Append(sink_val.release());
+  }
+}
+
+void SetMediaRouteDictionary(

[Wez, 2015/05/20 17:51:47]

This name's not very descriptive - suggest e.g. MediaRouteDetailsToValue() and
have it return the DictionaryValue rather than modifying an existing one, since
that should work fine with your two current call-sites.

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Renamed to RouteToValue and similarly for others.

+    const MediaRoute& route, base::DictionaryValue* dictionary) {
+  dictionary->SetString("id", route.media_route_id());
+  dictionary->SetString("sinkId", route.media_sink().id());
+  dictionary->SetString("title", route.description());
+  dictionary->SetBoolean("isLocal", route.is_local());
+}
+
+void SetRoutesListValue(
+    const std::vector<MediaRoute>& routes,
+    base::ListValue* value) {

[Wez, 2015/05/20 17:51:47]

See comments on preceding methods re naming and scoped_ptr<> result

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Done.

+  value->Clear();
+
+  for (const MediaRoute& route : routes) {
+    scoped_ptr<base::DictionaryValue> route_val(new base::DictionaryValue);
+    SetMediaRouteDictionary(route, route_val.get());
+    value->Append(route_val.release());
+  }
+}
+
+void SetCastModesListValue(
+    const CastModeSet& cast_modes,
+    const std::string& source_host,
+    base::ListValue* value) {

[Wez, 2015/05/20 17:51:47]

Same here.

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Done.

+  value->Clear();
+
+  for (const MediaCastMode& cast_mode : cast_modes) {
+    scoped_ptr<base::DictionaryValue> cast_mode_val(new base::DictionaryValue);
+    cast_mode_val->SetInteger("type", cast_mode);
+    cast_mode_val->SetString(
+        "title", MediaCastModeToTitle(cast_mode, source_host));
+    cast_mode_val->SetString(
+        "description", MediaCastModeToDescription(cast_mode, source_host));
+    value->Append(cast_mode_val.release());
+  }
+}
 
 }  // namespace
 
@@ -53,33 +115,139 @@ void MediaRouterWebUIMessageHandler::RegisterMessages() {
                  base::Unretained(this)));
 }
 
+void MediaRouterWebUIMessageHandler::UpdateSinks(
+      const std::vector<MediaSinkWithCastModes>& sinks) {
+  base::ListValue sinks_val;
+  SetSinksListValue(sinks, &sinks_val);
+  web_ui()->CallJavascriptFunction(kSetSinkList, sinks_val);
+}
+
+void MediaRouterWebUIMessageHandler::UpdateRoutes(
+    const std::vector<MediaRoute>& routes) {
+  base::ListValue routes_val;
+  SetRoutesListValue(routes, &routes_val);
+  web_ui()->CallJavascriptFunction(kSetRouteList, routes_val);
+}
+
+void MediaRouterWebUIMessageHandler::UpdateCastModes(
+    const CastModeSet& cast_modes, const std::string& source_host) {
+  base::ListValue cast_modes_val;
+  SetCastModesListValue(cast_modes, source_host, &cast_modes_val);
+

[Wez, 2015/05/20 17:51:47]

nit: Why the blank line here - you don't have one in the preceding methods?

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Done.

+  web_ui()->CallJavascriptFunction(kSetCastModeList, cast_modes_val);
+}
+
+void MediaRouterWebUIMessageHandler::AddRoute(const MediaRoute& route) {
+  DVLOG(1) << "AddRoute";

[Wez, 2015/05/20 17:51:47]

nit: Why does AddRoute deserve a DVLOG but not the other updates?

[imcheng (use chromium acct), 2015/05/20 22:01:13]

Added DVLOG(2) to public methods.

[Wez, 2015/05/21 22:58:37]

Acknowledged.

+
+  base::DictionaryValue route_value;
+  SetMediaRouteDictionary(route, &route_value);
+
+  web_ui()->CallJavascriptFunction(kAddRoute, route_value);
+}
+
+void MediaRouterWebUIMessageHandler::UpdateIssue(const Issue* issue) {
+  // TODO(imcheng): Implement conversion from Issue to dictionary object.
+  NOTIMPLEMENTED();
+}
+
 void MediaRouterWebUIMessageHandler::OnGetInitialSettings(
     const base::ListValue* args) {
-  // TODO(imcheng): Implement.
-  NOTIMPLEMENTED();
+  MediaRouterUI* media_router_ui = GetMediaRouterUI();
+
+  base::DictionaryValue initial_settings;
+
+  initial_settings.SetString("headerText",
+                             media_router_ui->GetInitialHeaderText());
+
+  scoped_ptr<base::ListValue> sinks(new base::ListValue);
+  SetSinksListValue(media_router_ui->sinks(), sinks.get());
+  initial_settings.Set("sinks", sinks.release());
+
+  scoped_ptr<base::ListValue> routes(new base::ListValue);
+  SetRoutesListValue(media_router_ui->routes(), routes.get());
+  initial_settings.Set("routes", routes.release());
+
+  scoped_ptr<base::ListValue> cast_modes(new base::ListValue);
+  SetCastModesListValue(
+      media_router_ui->cast_modes(),
+      media_router_ui->source_host(),
+      cast_modes.get());
+  initial_settings.Set("castModes", cast_modes.release());
+
+  web_ui()->CallJavascriptFunction(kSetInitialSettings, initial_settings);
+  media_router_ui->UIInitialized();
 }
 
 void MediaRouterWebUIMessageHandler::OnCreateRoute(
     const base::ListValue* args) {
-  // TODO(imcheng): Implement.
-  NOTIMPLEMENTED();
+  // Note that it is possible for this source to be different from

[Wez, 2015/05/20 17:51:47]

What is "this source"?

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Removed comment as it is duplicated with MRUI.

+  // the MediaSource in MediaRouterUI if an update occurred between the user
+  // click and invocation of this function. But this is a rare edge case since
+  // sources don't change in practice.
+  std::string sink_id;
+  int cast_mode_num;
+  if (!args->GetString(0, &sink_id) || !args->GetInteger(1, &cast_mode_num)) {
+    LOG(ERROR) << "Unable to extract args.";
+    return;
+  }
+
+  if (sink_id.empty()) {
+    LOG(ERROR) << "Media Route Provider Manager did not respond with a "
+                 << "valid sink ID. Aborting.";

[Wez, 2015/05/20 17:51:47]

nit: Consider folding this into the "Unable to extract args" check.

Is there also a restriction on valid values for |cast_mode_num|?

[imcheng (use chromium acct), 2015/05/20 22:01:13]

Yes. It is checked below.

+    return;
+  }
+
+  MediaRouterUI* media_router_ui =
+      static_cast<MediaRouterUI*>(web_ui()->GetController());
+  if (media_router_ui->has_pending_route_request()) {
+    LOG(ERROR) << "UI already has pending route request. Ignoring.";
+    return;
+  }
+
+  DVLOG(1) << "sink id: " << sink_id << ", cast mode: " << cast_mode_num;
+
+  // TODO(haibinlu): add parameters to media source according to the cast mode
+  // before sending to MRPM, e.g. low-fps-mirror, user-override.

[Wez, 2015/05/20 17:51:47]

Can you re-word this comment to be clearer - I think you're siggesting that we
should add configuration of mode-specific parameters here...?

Do we have a bug # for this follow-up work?

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Done. Added tracking bug.

+  bool success = false;
+  if (IsValidCastModeNum(cast_mode_num)) {
+    // Users explicitly selected cast mode.

[Wez, 2015/05/20 17:51:47]

nit: Users->User

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Done.

+    DVLOG(1) << "Cast mode override: " << cast_mode_num;
+    success = media_router_ui->CreateRouteWithCastModeOverride(
+        sink_id, static_cast<MediaCastMode>(cast_mode_num));
+  } else {
+    success = media_router_ui->CreateRoute(sink_id);
+  }
+
+  if (!success) {
+    // TODO(imcheng): Display error in UI.

[Wez, 2015/05/20 17:51:47]

Do we have a bug filed for this follow-up work?

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Done.

+    LOG(ERROR) << "Error initiating route request.";
+  }
 }
 
 void MediaRouterWebUIMessageHandler::OnActOnIssue(
     const base::ListValue* args) {
-  // TODO(imcheng): Implement.
-  NOTIMPLEMENTED();
+

[Wez, 2015/05/20 17:51:47]

Still doesn't look to be implemented..?

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Reverted.

 }
 
 void MediaRouterWebUIMessageHandler::OnCloseRoute(
     const base::ListValue* args) {
   // TODO(imcheng): Implement.
   NOTIMPLEMENTED();

[Wez, 2015/05/20 17:51:47]

Looks like this should be removed?

[imcheng (use chromium acct), 2015/05/20 22:01:14]

Done.

+  DVLOG(1) << "OnCloseRoute";
+  std::string route_id;
+  if (!args->GetString(0, &route_id)) {
+    DVLOG(1) << "Unable to extract args.";
+    return;
+  }
+  GetMediaRouterUI()->CloseRoute(route_id);
 }
 
 void MediaRouterWebUIMessageHandler::OnCloseDialog(
     const base::ListValue* args) {
-  CHECK(!dialog_closing_);
+  if (dialog_closing_)

[Wez, 2015/05/20 17:51:47]

How can we get OnCloseDialog if it's already closing?

[imcheng (use chromium acct), 2015/05/20 22:01:14]

If we can manage to call it again before it is actually closed. One example is
navigating to chrome://media-router and clicking on the close button. While it's
not meaningful to do that, it's not really worth it to (and not sure if we can)
handle this special case.

[Wez, 2015/05/21 22:58:37]

Acknowledged.

+    return;
+
   dialog_closing_ = true;
   GetMediaRouterUI()->Close();
 }