[MediaRouter] Add implementation of PresentationServiceDelegate

chrome/browser/media/router/presentation_service_delegate_impl.h

Index: chrome/browser/media/router/presentation_service_delegate_impl.h
diff --git a/chrome/browser/media/router/presentation_service_delegate_impl.h b/chrome/browser/media/router/presentation_service_delegate_impl.h
new file mode 100644
index 0000000000000000000000000000000000000000..7a5e478a3742ba5d358acb77227347cf440d6f17
--- /dev/null
+++ b/chrome/browser/media/router/presentation_service_delegate_impl.h
@@ -0,0 +1,190 @@
+// 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_PRESENTATION_SERVICE_DELEGATE_IMPL_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_PRESENTATION_SERVICE_DELEGATE_IMPL_H_
+
+#include <map>
+#include <string>
+#include <utility>
+
+#include "base/gtest_prod_util.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "chrome/browser/media/router/media_router.h"
+#include "chrome/browser/media/router/media_source.h"
+#include "content/public/browser/presentation_service_delegate.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+namespace content {
+class RenderFrameHost;
+class PresentationScreenAvailabilityListener;
+class WebContents;
+struct PresentationSessionInfo;
+struct PresentationSessionMessage;
+}

[Wez, 2015/05/27 22:37:53]

nit: This namespace is just long enough that I'd recommend adding the trailing
comment.

[haibinlu, 2015/05/28 21:44:05]

Done.

+
+namespace media_router {
+
+class MediaRoute;
+class MediaSinksObserver;
+class PresentationFrameMap;
+
+using RenderFrameHostId = std::pair<int, int>;

[Wez, 2015/05/27 22:37:52]

Since this alias is used only internally in the PresentationServiceDelegateImpl,
let's move it to the private section of the class.

Why is this using rather than typedef? Doesn't seem to be any advantage to
using, and typedef is more familiar syntax, surely?

[haibinlu, 2015/05/28 21:44:04]

They are the same. mfoltz suggested using type aliases. 
Some private functions use "const RenderFrameHostId&"

+
+// Implementation of PresentationServiceDelegate that that interfaces an
+// instance of WebContents with the Chrome Media Router.

[Wez, 2015/05/27 22:37:52]

This comment doesn't really help me understand what this class does.

In particular, be explicit about where this fits into the overall implementation
- my guess is that the generic Presentation API glue in WebContents delegates to
this interface to actually service requests, and that this implementation
services them using the Media Router, for example - is that correct?

[haibinlu, 2015/05/28 21:44:05]

Correct.Updated comments.

+// In addition to fulfilling Presentation requests, it also
+// provide tab-level information that is required for creating

[Wez, 2015/05/27 22:37:52]

s/provide/provides

[haibinlu, 2015/05/28 21:44:04]

Done.

+// browser-initiated sessions.

[Wez, 2015/05/27 22:37:52]

Again, it's not clear what "provides tab-level information..." means - who does
it provide that information to, and what does "tab-level information" even mean?

[haibinlu, 2015/05/28 21:44:04]

Updated to "default presentation URL"

+class PresentationServiceDelegateImpl
+    : public content::WebContentsUserData<PresentationServiceDelegateImpl>,

[Wez, 2015/05/27 22:37:53]

Suggest clarifying why we need to derive from this - are instances essentially
"user data" from WebContents' point of view, for example? Is that how ownership
of the instance is managed - does the WebContents own it and tear it down when
it goes out of scope?

[haibinlu, 2015/05/28 21:44:04]

Done.

+      public content::PresentationServiceDelegate {
+ public:
+  // Maximum number of sources that can be registered at a time.
+  static const int kMaxNumSources = 256;

[Wez, 2015/05/27 22:37:52]

Where is this used by callers, if at all? If it's not then move it into private
section.

[haibinlu, 2015/05/28 21:44:04]

moved to private. it is used by unit tests. 

+
+  ~PresentationServiceDelegateImpl() override;
+
+  // content::PresentationServiceDelegate implementation.
+  // PresentationScreenAvailabilityListener is one per
+  // (frame, presentation URL), while PresentationServiceDelegate::Observer
+  // is one per frame.

[Wez, 2015/05/27 22:37:52]

Surely this detail is described in the PresentationServiceDelegate interface
comments, so you don't need it here?

[haibinlu, 2015/05/28 21:44:05]

removed

+  void AddObserver(
+      int render_process_id,
+      int render_frame_id,
+      content::PresentationServiceDelegate::Observer* observer) override;
+  void RemoveObserver(int render_process_id, int render_frame_id) override;
+  bool AddScreenAvailabilityListener(
+      int render_process_id,
+      int render_frame_id,
+      content::PresentationScreenAvailabilityListener* listener) override;
+  void RemoveScreenAvailabilityListener(
+      int render_process_id,
+      int render_frame_id,
+      content::PresentationScreenAvailabilityListener* listener) override;
+  void Reset(int render_process_id, int render_frame_id) override;
+  void SetDefaultPresentationUrl(
+      int render_process_id,
+      int render_frame_id,
+      const std::string& default_presentation_url,
+      const std::string& default_presentation_id) override;
+  void StartSession(int render_process_id,
+                    int render_frame_id,
+                    const std::string& presentation_url,
+                    const std::string& presentation_id,
+                    const PresentationSessionSuccessCallback& success_cb,
+                    const PresentationSessionErrorCallback& error_cb) override;
+  void JoinSession(int render_process_id,
+                   int render_frame_id,
+                   const std::string& presentation_url,
+                   const std::string& presentation_id,
+                   const PresentationSessionSuccessCallback& success_cb,
+                   const PresentationSessionErrorCallback& error_cb) override;
+  void ListenForSessionMessages(
+      int render_process_id,
+      int render_frame_id,
+      const PresentationSessionMessageCallback& message_cb) override;
+  void SendMessage(
+      int render_process_id,
+      int render_frame_id,
+      scoped_ptr<content::PresentationSessionMessage> message_request,
+      const SendMessageCallback& send_message_cb) override;
+
+  // Notifies this object that |route| has been created or joined outside of a
+  // Presentation API request. The route could be due to
+  // browser action (e.g., browser initiated media router dialog) or
+  // a media route provider (e.g., autojoin).
+  void OnRouteCreated(const MediaRoute& route);

[Wez, 2015/05/27 22:37:53]

Does this need to be part of the class' public API?

Rather than "Notifies this object...", suggest "Callback invoked when a |route|
has been created or joined...", since it's not clear what it means for the
function to "notify this".

[haibinlu, 2015/05/28 21:44:05]

It does. Callers to be added in another patch.
When MR calls this method,
PresentationServiceDelegate.Observer.OnDefaultPresentationStarted will be
invoked.

+
+  // Returns the default MediaSource for this tab if there is one.
+  // Returns an empty MediaSource otherwise.
+  MediaSource default_source() const { return default_source_; }

[Wez, 2015/05/27 22:37:53]

This and the default-media-source-observer APIs: this class is the
PresentationServiceDelegate implementation - who is it that will call these
additional APIs that aren't available through the PresentationServiceDelegate
interface?

+
+  // Observer interface for listening to default MediaSource changes for the
+  // WebContents.
+  class DefaultMediaSourceObserver {
+   public:
+    virtual ~DefaultMediaSourceObserver() {}
+
+    // Called when default media source for the corresponding WebContents has
+    // changed.
+    // |source|: New default MediaSource, or empty if default was removed.
+    // |source_host|: Host name of the frame that contains the default media
+    // source, or empty if there is no default media source.
+    virtual void OnDefaultMediaSourceChanged(
+        const MediaSource& source,
+        const std::string& source_host) = 0;
+  };
+
+  // Adds / removes an observer for listening to default MediaSource changes.
+  void AddDefaultMediaSourceObserver(DefaultMediaSourceObserver* observer);
+  void RemoveDefaultMediaSourceObserver(DefaultMediaSourceObserver* observer);
+
+  base::WeakPtr<PresentationServiceDelegateImpl> GetWeakPtr();

[Wez, 2015/05/27 22:37:52]

What is this required for?

+
+ private:
+  explicit PresentationServiceDelegateImpl(content::WebContents* web_contents);
+  friend class content::WebContentsUserData<PresentationServiceDelegateImpl>;
+
+  friend class PresentationServiceDelegateImplTest;
+  friend class PresentationServiceDelegateImplBrowserTest;
+  FRIEND_TEST_ALL_PREFIXES(PresentationServiceDelegateImplTest,
+                           AddScreenAvailabilityListener);
+  FRIEND_TEST_ALL_PREFIXES(PresentationServiceDelegateImplTest,
+                           MaxNumObservers);
+  FRIEND_TEST_ALL_PREFIXES(PresentationServiceDelegateImplTest,
+                           AddListenerSameUrlTwice);
+  FRIEND_TEST_ALL_PREFIXES(PresentationServiceDelegateImplTest, Reset);
+  FRIEND_TEST_ALL_PREFIXES(PresentationServiceDelegateImplTest,
+                           DelegateObservers);
+  FRIEND_TEST_ALL_PREFIXES(PresentationServiceDelegateImplBrowserTest,
+                           RemoveObserversOnNavigation);

[Wez, 2015/05/27 22:37:52]

Do these tests actually need to access this class' internals? Looking at the
tests, the only APIs they seem to use are the two *ForTest() ones, which means
you can just make those public - PRESUBMIT will then catch anyone trying to
(ab)use them in non-test code.

+
+  // Returns |listener|'s presentation URL as a MediaSource. If |listener| does
+  // not have a persentation URL, returns the tab mirroring MediaSource.
+  MediaSource GetMediaSourceFromListener(
+      content::PresentationScreenAvailabilityListener* listener);
+
+  // Returns |true| if frame |rfh_id| is the main frame of |web_contents_|.
+  bool IsMainFrame(const RenderFrameHostId& rfh_id) const;

[Wez, 2015/05/27 22:37:52]

nit: rfh_id -> render_frame_host_id

+
+  // Updates tab-level default MediaSource and source host name. If either

[Wez, 2015/05/27 22:37:53]

What is the "source host name"? Do you mean the source's URL, or something else?
e.g. what if the source is a mirrored tab, or the desktop?

+  // changed, notify the observers.

[Wez, 2015/05/27 22:37:52]

nit: Why is this method not SetDefault...()?

+  void UpdateDefaultMediaSourceAndNotifyObservers(
+      const MediaSource& new_default_source,

[Wez, 2015/05/27 22:37:52]

nit: No need for |new_| prefix, since it's implicit in the purpose of this
method.

+      const std::string& new_default_source_host);
+
+  void SetMediaRouterForTest(MediaRouter* router);
+
+  bool HasScreenAvailabilityListenerForTest(
+      const RenderFrameHostId& rfh_id,
+      const MediaSourceId& source_id) const;
+
+  // Manages frames that are using presentation API.

[Wez, 2015/05/27 22:37:53]

This comment doesn't really add anything - in what way does it "manage" them? Is
it basically just a store for those details? What does it map from and to?

+  scoped_ptr<PresentationFrameMap> frame_map_;
+
+  MediaSource default_source_;
+  std::string default_source_host_;
+
+  // Observers listening for changes to default media source. Not owned by
+  // this class.

[Wez, 2015/05/27 22:37:52]

What does "not owned by this class" mean here?

[haibinlu, 2015/05/28 21:44:05]

Done.

+  ObserverList<DefaultMediaSourceObserver> default_media_source_observers_;
+
+  // Does not own these objects.

[Wez, 2015/05/27 22:37:52]

Suggest something like "References to the WebContents that owns this instance,
and associated browser profile's MediaRouter instance."

[haibinlu, 2015/05/28 21:44:05]

Done.

+  content::WebContents* web_contents_;
+  MediaRouter* router_;
+
+  // NOTE: All WeakPtrs must be invalidated before member variables.

[Wez, 2015/05/27 22:37:52]

nit: You don't need this comment; it's documented with WeakPtrFactory.

[haibinlu, 2015/05/28 21:44:04]

Done.

+  base::WeakPtrFactory<PresentationServiceDelegateImpl> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(PresentationServiceDelegateImpl);
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_PRESENTATION_SERVICE_DELEGATE_IMPL_H_