[Media Router] MediaRouterUI + WebUI handler implementation.

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 893dc733c1a379b0c38d8c86110519efa20d23c4..c1faae7b0f8e561375a0d9cd59b519344f7ae90a 100644
--- a/chrome/browser/ui/webui/media_router/media_router_ui.cc
+++ b/chrome/browser/ui/webui/media_router/media_router_ui.cc
@@ -6,34 +6,111 @@
 
 #include <string>
 
+#include "chrome/browser/media/router/issues_observer.h"
+#include "chrome/browser/media/router/media_router.h"
 #include "chrome/browser/profiles/profile.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 "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_data_source.h"
 #include "ui/web_dialogs/web_dialog_delegate.h"
 
 namespace media_router {
 
+namespace {
+
+// Observes when issue to show in UI has been updated.

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

nit: Can we word this to be clearer; this class calls to refresh the UI when any
Issue is updated?

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

Done according to the document in IssuesObserver.

+class UIIssuesObserver : public IssuesObserver {

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

nit: UiIssuesObserver

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

Acknowledged.

+ public:
+  explicit UIIssuesObserver(MediaRouterUI* ui) : ui_(ui) {
+    DCHECK(ui);
+  }
+
+  ~UIIssuesObserver() override {}
+
+  // IssuesObserver implementation.
+  void OnIssueUpdated(const Issue* issue) override {
+    ui_->SetIssue(issue);
+  }
+
+ private:
+  // Not owned by this class.

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

Rather than "not owned by" I'd suggest something like "Reference back to the
owning MediaRouterUI instance." here and below.

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

Done.

+  MediaRouterUI* ui_;
+
+  DISALLOW_COPY_AND_ASSIGN(UIIssuesObserver);
+};
+
+class UIMediaRoutesObserver : public MediaRoutesObserver {
+ public:
+  UIMediaRoutesObserver(MediaRouter* router, MediaRouterUI* ui)
+    : MediaRoutesObserver(router), ui_(ui) {
+    DCHECK(ui_);
+  }
+
+  void OnRoutesUpdated(const std::vector<MediaRoute>& routes) override {
+    ui_->OnRoutesUpdated(routes);
+  }
+
+ private:
+  // Not owned by this class.
+  MediaRouterUI* ui_;
+
+  DISALLOW_COPY_AND_ASSIGN(UIMediaRoutesObserver);
+};
+
+}  // namespace
+
 MediaRouterUI::MediaRouterUI(content::WebUI* web_ui)
     : ConstrainedWebDialogUI(web_ui),
-      handler_(new MediaRouterWebUIMessageHandler()) {
+      handler_(new MediaRouterWebUIMessageHandler()),
+      ui_initialized_(false),

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

nit: is_initialized_?

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

See other comment.

+      has_pending_route_request_(false),
+      router_(nullptr),
+      weak_factory_(this) {
   // Create a WebUIDataSource containing the chrome://media-router page's
   // content.
   scoped_ptr<content::WebUIDataSource> html_source(
       content::WebUIDataSource::Create(chrome::kChromeUIMediaRouterHost));
   AddLocalizedStrings(html_source.get());
   AddMediaRouterUIResources(html_source.get());
-  Profile* profile = Profile::FromWebUI(web_ui);
   // Ownership of |html_source| is transferred to the BrowserContext.
-  content::WebUIDataSource::Add(profile, html_source.release());
+  content::WebUIDataSource::Add(Profile::FromWebUI(web_ui),
+                                html_source.release());

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

nit: Blank line after this, for consistency.

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

Done.

   // Ownership of |handler_| is transferred to |web_ui|.
   web_ui->AddMessageHandler(handler_);
+
+  content::WebContents* wc = web_ui->GetWebContents();
+  DCHECK(wc);
+
+  /* TODO(imcheng): Uncomment once Kevin's MediaRouterMojoImpl patch landed.

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

What's the bug # for that patch?

Does this class make sense with a null |router_|..?

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

If it's null then it will just crash at some point (DCHECK at the observers). It
should be fine since nobody is using it yet. Alternatively I can wait for
https://codereview.chromium.org/1143603004/ to land first and then uncomment
this before landing this patch.

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

Acknowledged.

+  router_ = MediaRouterMojoImplFactory::GetApiForBrowserContext(
+      wc->GetBrowserContext());
+  DCHECK(router_);
+  */
+
+  issues_observer_.reset(new UIIssuesObserver(this));
+  routes_observer_.reset(new UIMediaRoutesObserver(router_, this));

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

nit: Suggest a comment on this block "Register for Issue and MediaRoute
updates."?

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

Done.

 }
 
 MediaRouterUI::~MediaRouterUI() {
+  if (query_result_manager_.get())
+    query_result_manager_->RemoveObserver(this);
+}
+
+void MediaRouterUI::OnResultsUpdated(
+    const std::vector<MediaSinkWithCastModes>& sinks) {
+  sinks_ = sinks;
+  if (ui_initialized_)
+    handler_->UpdateSinks(sinks_);
+}
+
+void MediaRouterUI::OnRoutesUpdated(const std::vector<MediaRoute>& routes) {
+  routes_ = routes;
+  if (ui_initialized_)
+    handler_->UpdateRoutes(routes_);
 }
 
 void MediaRouterUI::Close() {
@@ -44,5 +121,73 @@ void MediaRouterUI::Close() {
   }
 }
 
+void MediaRouterUI::UIInitialized() {
+  ui_initialized_ = true;
+}
+
+bool MediaRouterUI::CreateRoute(const MediaSinkId& sink_id) {
+  return DoCreateRoute(sink_id, GetPreferredCastMode(cast_modes_));

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

Where does cast_modes_ ever get set..?

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

In the Init* functions to be upstreamed separately.

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

Acknowledged.

+}
+
+bool MediaRouterUI::CreateRouteWithCastModeOverride(

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

Why do we need the word "override"? i.e. not CreateRouteWithCastMode?

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

See other comment.

+    const MediaSinkId& sink_id, MediaCastMode cast_mode_override) {
+  // NOTE: It's actually not an override if
+  // |cast_mode_override| == |GetPreferredCastMode(cast_modes_)|.

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

Not sure what this comment is intended to achieve?

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

It's for later when we pass in an override flag to the CreateRoute request. If
user explicitly chose a cast mode and it is equal to the preferred cast mode,
then we don't consider it as an override.

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

Is the distinction important?

[imcheng (use chromium acct), 2015/05/22 00:02:00]

Probably. We don't want to pass in that flag if the user didn't actually mean to
override the source to be routed.

+  return DoCreateRoute(sink_id, cast_mode_override);
+}
+
+void MediaRouterUI::CloseRoute(const MediaRouteId& route_id) {
+  router_->CloseRoute(route_id);
+}
+
+void MediaRouterUI::SetIssue(const Issue* issue) {
+  if (ui_initialized_)
+    handler_->UpdateIssue(issue);
+}
+
+void MediaRouterUI::ClearIssue(const std::string& issue_id) {
+  // TODO(imcheng): Uncomment once Kevin's MediaRouterMojoImpl patch landed.

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

See above re bug #.

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

Done.

+  // router_->ClearIssue(issue_id);
+}
+
+std::string MediaRouterUI::GetInitialHeaderText() const {
+  if (cast_modes_.empty())
+    return std::string();
+
+  return MediaCastModeToTitle(GetPreferredCastMode(cast_modes_), source_host_);
+}
+
+void MediaRouterUI::OnRouteResponseReceived(scoped_ptr<MediaRoute> route,
+                                            const std::string& error) {
+  DVLOG(1) << "OnRouteResponseReceived";
+  if (!route)
+    LOG(ERROR) << "MediaRouteResponse returned error: " << error;
+  else
+    handler_->AddRoute(*route);
+
+  has_pending_route_request_ = false;
+}
+
+bool MediaRouterUI::DoCreateRoute(
+    const MediaSinkId& sink_id, MediaCastMode cast_mode) {
+  DCHECK(query_result_manager_.get());
+
+  // Note that it is possible for this source to be different from
+  // the MediaSource in MediaRouterUI if an update occurred between the user

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

It's not clear what "the MediaSource in MediaRouterUI" means - do you mean "the
source details displayed in the UI" or something else?

Why is the fact that the source may differ relevant to this code - are you just
trying to explain the reason for calling GetSourceForCastMode(), for example?

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

This describes a case that is theoretically possible but occurs very rarely in
practice, where the MediaSource intended by the user is different than the
MediaSource selected here. This is because the mapping from MediaCastMode to
MediaSource is not static over the lifetime of the UI.

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

OK, so I think you need to make clear that what you're describing is basically a
rarely-encountered bug that will mean the user doesn't get the behaviour they
expected.

[imcheng (use chromium acct), 2015/05/22 00:02:00]

Done.

+  // click and invocation of this function.
+  MediaSource source = query_result_manager_->GetSourceForCastMode(cast_mode);
+  if (source.Empty()) {
+    LOG(ERROR) << "No corresponding MediaSource for cast mode " << cast_mode;
+    return false;
+  }
+
+  router_->CreateRoute(source.id(), sink_id,
+                        base::Bind(&MediaRouterUI::OnRouteResponseReceived,

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

nit: Indentation - consider git cl format. :)

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

Done.

+                                   weak_factory_.GetWeakPtr()));
+
+  has_pending_route_request_ = true;
+  return true;
+}
+
 }  // namespace media_router