[Media Router] 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 extensions {
+class EventPageTracker;
+}
+
+namespace media_router {
+
+// Mojo service exposing Media Router functionality to the Media Route Provider
+// Manager.
+class MediaRouterMojoImpl : public MediaRouter,

[Wez, 2015/05/22 00:05:38]

Sorry, I meant to mention this fundamental question earlier...

Why is MediaRouterMojoImpl a MediaRouter, rather than _containing_ a MediaRouter
instance? It seems that the latter gives the right separation between the roles
- this class is really only intended to expose MediaRouter functions to the
component extension via Mojo, right?

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

I think "really only intended to expose MediaRouter functions to the
component extension via Mojo" is correct. However, the callers of Media Router
within Chrome (PresentationServiceDelegate, WebUI) does not know/care about this
detail. All they know is that there is a MediaRouter in Chrome that they talk
to. Which is why this implements MediaRouter.

[Wez, 2015/05/23 00:45:46]

Right - thinking about this last night, I realised that I've got myself confused
about what this class is - I'd seen it as a component that sits alongside a core
MediaRouter impl, to expose that to the extension. But it actually is _the_
MediaRouter in Chrome, and all it really does is adapt between the C++ Media
Router interface and delegate calls on to the extension - and inside the
extension the Media Route Provider Manager presents an MRPM interface to the
MRPs, and the MR interface to this class, hence the naming.

So... we need to make sure that the comments in this header accurately describe
the role this class plays - the most fundamental thing is for the comment on the
class to describe the fact that it delegates the MR API calls to an extension
and copes with suspend/wakeup of that extension.

With regard to the MediaRouter C++ interface vs the Mojo MediaRouter interface,
it's really strange and confusing to have them named the same thing - it begs
the question of why you don't just replace the C++ MediaRouter with the Mojo
version - aren't they basically the same thing? If they are not then is that
because the Mojo version is really an internal implementation detail of the
MediaRouterMojoImpl <-> Media Route Provider extension? If so then consider
renaming the Mojo MediaRouter to e.g. MediaRouteProvider, so that the
distinction is clearer?

[imcheng (use chromium acct), 2015/05/26 17:58:31]

ok, I changed the class level comments as you suggested. Re naming: I think in a
way it makes sense because the two (C++ and mojom) provide almost identical
interfaces. The way it stands now, the component extension is an implementation
of Media Router, and MediaRouterMojoImpl is a delegating / adaptation layer for
component extension wakeup/suspension. I would suggest we revisit the naming
issue when Kevin comes back. He had a long discussion with xhwang@ in his last
patch and will be able to provide some insight as to why they settled with this
name.

+                            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 implementation.
+  void OnConnectionError() override;
+
+  // interfaces::MediaRouterObserver implementation.
+  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.
+  // 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;
+  void RegisterMediaSinksObserver(MediaSinksObserver* observer) override;
+  void UnregisterMediaSinksObserver(MediaSinksObserver* observer) override;
+  void 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);
+
+  // Standard constructor, used by
+  // MediaRouterMojoImplFactory::GetApiForBrowserContext.
+  MediaRouterMojoImpl();
+
+  // Constructor used only for testing.
+  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);

[Wez, 2015/05/22 00:05:39]

I think you're right that there's no real value in breaking this and
InitExtensionInfo out of BindToRequest - you basically just want a non-static
equivalent of BindToRequest on this class, for BindToRequest to call.

Or, since BindToRequest is a member of this class, it could reasonably set the
fields in the Impl directly, removing the need for a non-static helper.

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

When I tried to change it I realized why it was structured this way. It's
because there is test setup code that only calls Bind() but set extension info
via other means, and then there are unit tests that call them both but where
using the static method isn't an option. So the cleanest way would be to have
these 2 member functions separately.

[Wez, 2015/05/23 00:45:46]

That's fine, but in that case you need to make it clear in the comment why it is
that they can't be folded together. And ideally you should have the API that
tests call be a public method with a *ForTest name, so that pre-submit will
catch if it's (ab)used elsewhere.

[imcheng (use chromium acct), 2015/05/26 17:58:31]

Both are used in production code, so *ForTest isn't really an option. But I
updated comments.

+
+  // Stores the |extension_id| of the component extension in this instance.
+  // Also obtains a reference to the extension's ProcessManager using
+  // |context|, which is a reference to the BrowserContext that this
+  // instance and extension are keyed on.

[Wez, 2015/05/22 00:05:38]

Does "keyed on" mean "belong to", in effect?

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

Yes

[Wez, 2015/05/23 00:45:46]

OK - in that case I'd suggest using "belong to", since that conveys the object
ownership implicitly.

[imcheng (use chromium acct), 2015/05/26 17:58:31]

Done.

+  void InitExtensionInfo(const std::string& extension_id,
+                         content::BrowserContext* context);
+
+  // Enqueues a closure for later execution by ExecutePendingRequests().
+  void EnqueueTask(const base::Closure& closure);
+
+  // 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);
+
+  // Runs the closures queued in |pending_requests_|.

[Wez, 2015/05/22 00:05:38]

nit: Suggest "Dispatches the Mojo requests queued in |pending_requests_|."

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

Done.

+  void ExecutePendingRequests();
+
+  // 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_;
+
+  base::ScopedPtrHashMap<MediaSourceId,
+                         scoped_ptr<ObserverList<MediaSinksObserver>>>
+      sinks_observers_;
+
+  ObserverList<MediaRoutesObserver> routes_observers_;
+
+  // Binds |this| to a Mojo MediaRouterObserver interface.
+  // This binding exists so that MediaRouterObserver calls from the

[Wez, 2015/05/22 00:05:38]

That is fairly intuitive from the class names; the question is what it achieves
- what _are_ MediaRouterObserver calls? Is MediaRouterObserver the API the
extension uses to watch for requests from MediaRouter, for example?

I'd expect a comment like:
"Binds |this| to service Mojo requests for <some sort of purpose> from the
component extension."

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

Yes. The component extension is a MediaRouter. Just as this class delegates
requests to the component extension MediaRouter, the component extension
MediaRouter sends responses back to this class via the MediaRouterObserver API.

[Wez, 2015/05/23 00:45:47]

See above re the class level comment :)

+  // component extensions are routed here.
+  // The binding is released when binding_ is deleted.
+  scoped_ptr<mojo::Binding<interfaces::MediaRouterObserver>> binding_;
+
+  // Mojo proxy object for the Provider Manager.
+  // Set to null initially, and set to the proxy to the component extension
+  // on |ProvideMediaRouter()|. This is set to null again when the component
+  // extension becomes suspended or if there is a connection error.

[Wez, 2015/05/22 00:05:38]

So this is basically the API through which we send requests to the Media Route
Provider, in which case I still don't understand the MediaRouter naming
convention. :(

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

We should really get rid of any references to the Provider Manager here. All
this class knows is that it is talking to a component extension implementation
of the Media Router API via mojo.

[Wez, 2015/05/23 00:45:46]

Acknowledged.

+  interfaces::MediaRouterPtr mojo_media_router_;
+
+  // Id of the component extension. Used for managing its suspend/wake state
+  // via event_page_tracker_.
+  std::string mojo_media_router_extension_id_;
+
+  // Allows the extension to be monitored for suspend, and woken.
+  // This is a reference to a BrowserContext keyed service that outlives this
+  // instance.
+  extensions::EventPageTracker* event_page_tracker_;
+
+  // 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.
+  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_