Add Media Router Mojo impl code.

chrome/browser/media/router/media_router_mojo_impl.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTER_MOJO_IMPL_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTER_MOJO_IMPL_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/containers/hash_tables.h"
+#include "base/containers/scoped_ptr_hash_map.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop_proxy.h"
+#include "base/observer_list.h"
+#include "base/threading/thread_checker.h"
+#include "chrome/browser/media/router/issue.h"
+#include "chrome/browser/media/router/media_router.h"
+#include "chrome/browser/media/router/media_router.mojom.h"
+#include "components/keyed_service/core/keyed_service.h"
+
+namespace content {
+class BrowserContext;
+}  // namespace content

[Wez, 2015/05/20 23:37:49]

nit: This namespace is sufficiently short that you can omit the comment.

[imcheng (use chromium acct), 2015/05/21 19:28:51]

Done.

+
+namespace extensions {
+class EventPageTracker;
+}  // namespace extensions
+
+namespace media_router {
+
+class MediaRouterMojoTest;
+
+// Implementation of the Mojo-based service which bridges the Media Router
+// Provider Manager and the Media Router.

[Wez, 2015/05/20 23:37:49]

Would it be clearer to describe this as the Mojo service exposing MediaRouter
functionality to the Media Router Provider Manager?

[imcheng (use chromium acct), 2015/05/21 19:28:51]

Done.

+class MediaRouterMojoImpl : public MediaRouter,
+                            public interfaces::MediaRouterObserver,
+                            public mojo::ErrorHandler,
+                            public KeyedService {
+ public:
+  ~MediaRouterMojoImpl() override;
+
+  // Binds the MediaRouterMojoImpl BrowserContext service to a Mojo request.
+  // Called by the Mojo module registry.
+  // |extension_id|: The ID of the component extension, used for querying
+  //     suspension state.
+  // |context|: The BrowserContext which owns the extension process.
+  // |request|: The Mojo connection request used for binding.
+  static void BindToRequest(
+      const std::string& extension_id,
+      content::BrowserContext* context,
+      mojo::InterfaceRequest<interfaces::MediaRouterObserver> request);
+
+  // mojo::ErrorHandler

[Wez, 2015/05/20 23:37:49]

You mean "mojo::ErrorHandler interface."?

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Changed to "mojo::ErrorHandler implementation."

+  void OnConnectionError() override;
+
+  // interfaces::MediaRouter implementation.

[Wez, 2015/05/20 23:37:48]

This class doesn't seem to inherit from interfaces::MediaRouter?

[imcheng (use chromium acct), 2015/05/21 19:28:51]

Done.

+  // Registers a Mojo MediaRouter proxy object from the extension.

[Wez, 2015/05/20 23:37:49]

Why does this method deserve a comment, but none of the On*() ones do? Isn't the
purpose of ProvideMediaRouter() defined in the interfaces::MediaRouter
interface?

[imcheng (use chromium acct), 2015/05/21 19:28:52]

removed.

+  void ProvideMediaRouter(
+      interfaces::MediaRouterPtr media_router_ptr,
+      const interfaces::MediaRouterObserver::ProvideMediaRouterCallback&
+          callback) override;
+  void OnMessage(const mojo::String& route_id,
+                 const mojo::String& message) override;
+  void OnIssue(interfaces::IssuePtr issue) override;
+  void OnSinksReceived(const mojo::String& media_source,
+                       mojo::Array<interfaces::MediaSinkPtr> sinks) override;
+  void OnRoutesUpdated(mojo::Array<interfaces::MediaRoutePtr> routes) override;
+
+  // MediaRouter implementation.
+  // For messages sent to the extension.

[Wez, 2015/05/20 23:37:48]

?

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Removed.

+  // Execution of the requests is delegated to the Do* methods, which can be
+  // enqueued for later use if the extension is temporarily suspended.
+  void CreateRoute(const MediaSourceId& source_id,
+                   const MediaSinkId& sink_id,
+                   const MediaRouteResponseCallback& callback) override;
+  void CloseRoute(const MediaRouteId& route_id) override;
+  void PostMessage(const MediaRouteId& route_id,
+                   const std::string& message) override;
+  void ClearIssue(const Issue::IssueId& issue_id) override;
+  // These methods send messages to the extension upon first registration
+  // and last unregistration.

[Wez, 2015/05/20 23:37:49]

nit: Blank line above this comment, to make it more clearly a distinct block of
methods to which the comment applies?

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Removed since it is no longer true.

+  bool RegisterMediaSinksObserver(MediaSinksObserver* observer) override;
+  void UnregisterMediaSinksObserver(MediaSinksObserver* observer) override;
+  bool RegisterMediaRoutesObserver(MediaRoutesObserver* observer) override;
+  void UnregisterMediaRoutesObserver(MediaRoutesObserver* observer) override;
+  void AddIssuesObserver(IssuesObserver* observer) override;
+  void RemoveIssuesObserver(IssuesObserver* observer) override;
+
+ private:
+  friend class MediaRouterMojoImplFactory;
+  friend class MediaRouterMojoTest;
+
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoExtensionTest,
+                           DeferredBindingAndSuspension);

[Wez, 2015/05/20 23:37:49]

What's the DeferredBindingAndSuspension thing about?

[imcheng (use chromium acct), 2015/05/21 19:28:51]

It's a test that's disabled until "issues with extension system teardown are
addressed." I am not too familiar with the details, but it looks like a test
that tries to call MediaRouterMojoImpl from the browser before it's hooked up to
the component extension. Filed a bug to reenable it.

+
+  // New instances should be constructed through
+  // MediaRouterMojoImplFactory::GetApiForBrowserContext.

[Wez, 2015/05/20 23:37:49]

Suggest "Standard constructor, used by ..."

[imcheng (use chromium acct), 2015/05/21 19:28:51]

Done.

+  MediaRouterMojoImpl();
+
+  // Used for testing.

[Wez, 2015/05/20 23:37:48]

Suggest "Constructor used only for testing."

Consider instead having a static method CreateForTest() - the ForTest() in the
name will cause presubmit checks to fail if anyone mistakenly uses the method in
production code. You could then safely leave that method in the public API
section and avoid some unnecessary friending, potentially.

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Changed comment. The MediaRouterMojoTest setup code need to access to private
methods, so I will leave it for now.

+  MediaRouterMojoImpl(
+      const std::string& mojo_media_router_ext_id,
+      extensions::EventPageTracker* event_page_tracker_for_test);
+
+  // Binds |this| to a Mojo interface request, so that clients can acquire a
+  // handle to a MediaRouterMojoImpl instance via the Mojo service connector.
+  void Bind(mojo::InterfaceRequest<interfaces::MediaRouterObserver> request);
+
+  // Enqueues a closure for later execution by ExecutePendingRequests().
+  void EnqueueTask(const base::Closure& closure);
+
+  // Starts monitoring the suspend state of |extension_id|.
+  void MonitorExtension(const std::string& extension_id,
+                        content::BrowserContext* context);

[Wez, 2015/05/20 23:37:48]

Clarify the role of |context| in this API; I assume it's a bare reference -
what's it used for?

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Done.

+
+  // Runs a closure if the extension monitored by |extension_monitor_| is
+  // active, or defers it for later execution if the extension is suspended.
+  void RunOrDefer(const base::Closure& request_cb);
+
+  // Runs pending requests in |pending_requests_|.

[Wez, 2015/05/20 23:37:49]

Suggest "Runs the closures queued in |pending_requests_|."

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Done.

+  inline void ExecutePendingRequests();

[Wez, 2015/05/20 23:37:48]

Remove inline from here.

[imcheng (use chromium acct), 2015/05/21 19:28:51]

Done.

+
+  // These calls invoke methods in the MRPM via Mojo.
+  void DoCreateRoute(const MediaSourceId& source_id,
+                     const MediaSinkId& sink_id,
+                     const MediaRouteResponseCallback& callback);
+  void DoCloseRoute(const MediaRouteId& route_id);
+  void DoPostMessage(const MediaRouteId& route_id, const std::string& message);
+  void DoClearIssue(const Issue::IssueId& issue_id);
+  void DoStartObservingMediaSinks(const std::string& source_id);
+  void DoStopObservingMediaSinks(const std::string& source_id);
+  void DoStartObservingMediaRoutes();
+  void DoStopObservingMediaRoutes();
+  void DoStartObservingIssues();
+  void DoStopObservingIssues();
+
+  // Pending requests queued to be executed once MRPM becomes ready.
+  std::vector<base::Closure> pending_requests_;
+
+  // Multimap of sinks observer objects, keyed by media source ID.

[Wez, 2015/05/20 23:37:49]

nit: This comment doesn't scan; you mean it's a Multi-map of MediaSink observers
keyed my MediaSource Id.

In any case, this comment describes what I can tell my reading the code - what
it doesn't say is why we need this member. e.g. "Multimap of observers
registered via StartObservingMediaSinks."

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Removed comment.

+  base::ScopedPtrHashMap<MediaSourceId,
+                         scoped_ptr<ObserverList<MediaSinksObserver>>>
+      sinks_observers_;
+
+  // List of route observer objects.

[Wez, 2015/05/20 23:37:49]

Similarly, this comment doesn't tell me what this method is for - suggest either
removing it or updating the comment to provide that info.

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Done.

+  ObserverList<MediaRoutesObserver> routes_observers_;
+
+  // Binds |this| to a Mojo MediaRouterObserver interface.
+  // The binding is released when binding_ is deleted.

[Wez, 2015/05/20 23:37:49]

Again, this comment doesn't describe _why_ this member exists - what is it for?

If I had to guess it looks like it tells Mojo the service implementation to
route requests for that interface to? But does it bind an individual client
request, or cope with multiple clients, or what?

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Done.

+  scoped_ptr<mojo::Binding<interfaces::MediaRouterObserver>> binding_;
+
+  // Mojo proxy object for the Provider Manager.

[Wez, 2015/05/20 23:37:49]

Is there a possibility that this may be "null" if the extension isn't currently
running, for example?

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Yes. Clarified in comments.

+  interfaces::MediaRouterPtr mojo_media_router_;

[Wez, 2015/05/20 23:37:49]

I'm confused... why is the interface to the Provider Manager called
MediaRouter[Ptr]? Shouldn't that be the name for the MediaRouter interface that
the Provider Manager uses...?

[imcheng (use chromium acct), 2015/05/21 19:28:51]

There was a discussion with Kevin and xhwang@ on the naming. The idea, I think,
is that the provider manager is a mojo implementation of the MediaRouter. This
class is an implementation that delegates the calls to it.

Please see:
https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro...

[Wez, 2015/05/21 23:32:47]

That doesn't sound right. The MediaRouter is the component in Chrome, and the
MediaRouteProviderManager is the extension-side component that the
MediaRouteProviders integrate with.

Or is the ideal that the MediaRouteProviders all talk via Mojo, through a
MediaRouter interface, to the MediaRouteProviderManager, which then talks to the
C++ MediaRouter?

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

Hmm. I think it makes sense to me. From Chrome's perspective, it does not need
to know about provider manager (this means we should update the comments here to
omit 'Provider Manager'). All it knows as that there is a component extension
that acts as a MediaRouter because it implements provides an implementation of
the MediaRouter interface. We are using MediaRouter to describe the set of APIs
for routing media, and not just the component in Chrome. That might be the
source of confusion?

+
+  // ID of the component extension. Used for managing its suspend/wake state

[Wez, 2015/05/20 23:37:49]

nit: ID->Id.

[imcheng (use chromium acct), 2015/05/21 19:28:51]

Done.

+  // via event_page_tracker_.
+  std::string mojo_media_router_extension_id_;
+
+  // Provides querying and waking functionality for the component extension's
+  // suspend state.

[Wez, 2015/05/20 23:37:49]

Suggest: "Allows the extension to be monitor for suspend, and woken."

Also suggest clarifying who owns the EventPageTracker, e.g. is this a cached
pointer to a singleton?

[imcheng (use chromium acct), 2015/05/21 19:28:51]

Done.

+  extensions::EventPageTracker* event_page_tracker_;
+
+  // GUID of this object's instance.
+  // Used by the component extension to determine if its persistent cache
+  // is stale.

[Wez, 2015/05/20 23:37:49]

Probably needs elaborating upon; suggest something like:
"GUID unique to each browser run. Component extension uses this to detect when
its persisted state was written by an older browser instance, and is therefore
stale."

[imcheng (use chromium acct), 2015/05/21 19:28:52]

Done.

+  std::string instance_id_;
+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaRouterMojoImpl);
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTER_MOJO_IMPL_H_