[Media Router] MediaRouterUI + WebUI handler implementation.

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

Index: chrome/browser/ui/webui/media_router/media_router_ui.h
diff --git a/chrome/browser/ui/webui/media_router/media_router_ui.h b/chrome/browser/ui/webui/media_router/media_router_ui.h
index 6fcb3040300d9805fec477a81a70434ba8f6a607..8dbc972b4e1a355a93d1c4fe4fab6816c47d2dc8 100644
--- a/chrome/browser/ui/webui/media_router/media_router_ui.h
+++ b/chrome/browser/ui/webui/media_router/media_router_ui.h
@@ -6,28 +6,111 @@
 #define CHROME_BROWSER_UI_WEBUI_MEDIA_ROUTER_MEDIA_ROUTER_UI_H_
 
 #include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/media/router/issue.h"
 #include "chrome/browser/ui/webui/constrained_web_dialog_ui.h"
+#include "chrome/browser/ui/webui/media_router/media_cast_mode.h"
+#include "chrome/browser/ui/webui/media_router/media_sink_with_cast_modes.h"
+#include "chrome/browser/ui/webui/media_router/query_result_manager.h"
 #include "content/public/browser/web_ui_data_source.h"
 
 namespace media_router {
 
+class IssuesObserver;
 class MediaRouterWebUIMessageHandler;
+class MediaRoutesObserver;
 
 // Implements the chrome://media-router user interface.
-class MediaRouterUI : public ConstrainedWebDialogUI {
+class MediaRouterUI : public ConstrainedWebDialogUI,

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

nit: MediaRouterUi would be the preferred name according to style-guide, though
I notice that Chromium is pretty inconsistent where UI is concerned; feel free
to ignore my UI->Ui comments elsewhere. :-/

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

Acknowledged.

+                      public QueryResultManager::Observer {
  public:
   // |web_ui| owns this object and is used to initialize the base class.
   explicit MediaRouterUI(content::WebUI* web_ui);
   ~MediaRouterUI() override;
 
+  // QueryResultManager::Observer
+  void OnResultsUpdated(const std::vector<MediaSinkWithCastModes>& sinks)
+      override;

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

Do we expect callers to invoke this? If not then consider moving it to the
private section; although the interface inheritance must always be public, it's
OK to "hide" interface methods that callers shouldn't need to think about.

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

Done.

+
+  // Called by |routes_observer_| when the set of active routes has changed.

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

Would it make sense for this to be private and for the observer helper
sub-component to be a friend of this class?

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

Done.

+  void OnRoutesUpdated(const std::vector<MediaRoute>& routes);
+
   // Closes the media router UI.
   void Close();
 
+// Notifies that the UI has been initialized.

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

nit: Indentation?

(BTW, you know about git cl format, right?)

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

Not clear from this comment who/what is being notified?

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

Done.

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

Done.

+  void UIInitialized();

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

nit: CamelCase applies even to acronyms & abbreviations, so UI->Ui here.

OnUiInitialized?

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

Acknowledged.

+
+  // Submits a route request for using MediaSink |sink_id| and source
+  // determined by the preferred MediaCastMode.

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

It's unclear in this context what "using MediaSink |sink_id|" means - do you
mean that we create a route from source content to |sink_id|, using the
preferred MediaCastMode (i.e. fling, tab-mirroring, etc)?

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

Done.

+  // The preferred cast mode is determined from the set of currently supported
+  // cast modes in |cast_modes_|.
+  // Returns true if a route request is successfully submitted.

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

Since a "success" result only indicates that the request has been made, not that
it's successfully completed, suggest rephrases this to "Returns false if unable
to request the route." then briefly describe what happens when the route request
completes.

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

Done.

+  // One of the Init* functions must have been called before.

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

There are no Init* functions...?

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

Init* functions are not upstreamed yet. I will remove this comment for now.

+  bool CreateRoute(const MediaSinkId& sink_id);
+
+  // Submits a route request for using MediaSink |sink_id| and source
+  // determined by |cast_mode_override|.

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

There seems to be some confusion between "mode" and "source" terminology here.
:(

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

A cast mode maps to a source in the context of a MediaRouterUI instance. I
changed the comments here.

+  // Returns true if a route request is successfully submitted.
+  // One of the Init* functions must have been called before.

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

See above re Init* functions?

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

Done.

+  bool CreateRouteWithCastModeOverride(
+      const MediaSinkId& sink_id, MediaCastMode cast_mode_override);
+
+  // Calls MediaRouter to close the given route.
+  void CloseRoute(const MediaRouteId& route_id);
+
+  // If the UI is already initialized, notifies |handler_| to update the UI.

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

nit: What does it do if the UI is not initialized? Suggest rewording, e.g.
"Updates the UI to display the new |issue|. Ignored if the UI is not yet
initialized."

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

Done.

+  void SetIssue(const Issue* issue);
+
+  // Calls MediaRouter to clear the given issue.

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

nit: What does it mean to "clear" the issue? Is that documented elsewhere?

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

Yes, the MediaRouter API that's called by this is here:
https://codereview.chromium.org/1143603004/diff/60001/chrome/browser/media/ro...

It means action has been taken to resolve the issue and can be removed from the
issue reporter's (i.e., component extension) record.

+  void ClearIssue(const Issue::IssueId& issue_id);
+
+  // Returns the header text that should be displayed in the UI when it is
+  // initially loaded. (i.e., user has not selected a cast mode override yet)

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

nit: You mean "e.g.", not "i.e.", I think?  "e.g." means "for example", where
"i.e." means "in other words".

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

It's really neither. So I will just reword it.

+  std::string GetInitialHeaderText() const;
+
+  bool has_pending_route_request() const { return has_pending_route_request_; }
+  const std::string& source_host() const { return source_host_; }
+  const std::vector<MediaSinkWithCastModes>& sinks() const { return sinks_; }
+  const std::vector<MediaRoute>& routes() const { return routes_; }
+  const std::set<MediaCastMode>& cast_modes() const { return cast_modes_; }
+
  private:
+  // Callback from Media Router for a route creation request.

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

nit: "Callback passed to MediaRouter to receive response to route creation
requests."

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

Done.

+  void OnRouteResponseReceived(scoped_ptr<MediaRoute> route,
+                               const std::string& error);
+
+  bool DoCreateRoute(const MediaSinkId& sink_id, MediaCastMode cast_mode);

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

Do we need this? Why can't CreateRoute() just call CreateRouteWithCastMode()
with the preferred route, and have that contain the implementation?

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

I think it's a bit more explicit this way. In the future we may also pass in a
flag to the create route request that specifies a cast mode override was chosen.

+
   // Owned by the |web_ui| passed in the ctor, and guaranteed to be deleted
   // only after it has deleted |this|.
   MediaRouterWebUIMessageHandler* handler_;
 
+  scoped_ptr<IssuesObserver> issues_observer_;

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

nit: Add a comment to this block of members to explain that they are non-null
while we're registered to receive notifications from those components.

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

Done.

+  scoped_ptr<MediaRoutesObserver> routes_observer_;
+
+  // Keeps track of whether initial settings have been requested.

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

initial settings for what?
requested by whom?

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

Done.

+  bool ui_initialized_;

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

nit: is_initialized_?

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

There are a couple of things to keep track of initialized here: this instance
and the ui. This tracks the latter.

+
+  // Set to |true| if there is a pending route request for this UI.

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

Does this basically mean it's true when we're trying to connect to the device to
start an activity?

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

Yes. Since this class interfaces with media router, the documentation here uses
media router terminology.

+  bool has_pending_route_request_;
+
+  std::vector<MediaSinkWithCastModes> sinks_;
+  std::vector<MediaRoute> routes_;
+  CastModeSet cast_modes_;
+  // TODO(imcheng): Set |source_host_| once DEFAULT mode is supported.
+  std::string source_host_;

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

Do we need this variable if DEFAULT mode is not yet supported? Or can we add in
in the later CL that adds that mode?  Do we have a bug # for that work?

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

I can remove this for now. DEFAULT mode is already supported in private repo. I
just wanted to upstream that separately. It uses the same bug # as this one
since it's just part of the MediaRouterUI implementation.

+
+  scoped_ptr<QueryResultManager> query_result_manager_;
+
+  // Does not own this object.

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

This comment's pretty ambiguous - are you saying |router_| doesn't own |this|,
or |this| doesn't own |router_|, or something else? Isn't this just a bare
reference back to the global MediaRouter instance? Is the guarantee simply that
that will never be torn down until the browser shuts down? What guarantees that
|this| will be torn down before that point?

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

How about "Not owned by this instance."?

MediaRouter is a browser context keyed service. This instance is backed by a
WebContents. Thus this will be destroyed before MediaRouter.

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

I'd suggest "Cached pointer to the MediaRouter for our BrowserContext.?" - WDYT?

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

Ok. But I replaced "our" with "this instance's".

+  MediaRouter* router_;
+
+  // NOTE: Weak pointers must be invalidated before all other member variables.
+  // Therefore |weak_factory_| must be placed at the end.
+  base::WeakPtrFactory<MediaRouterUI> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(MediaRouterUI);
 };