[Presentation API] (alternative) 1-UA: send message between controller and receiver page

chrome/browser/media/router/offscreen_presentation_manager.h

+// Copyright 2016 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_OFFSCREEN_PRESENTATION_MANAGER_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_OFFSCREEN_PRESENTATION_MANAGER_H_
+
+#include <map>
+#include <memory>
+#include <string>
+
+#include "base/macros.h"
+#include "chrome/browser/media/router/render_frame_host_id.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "content/public/browser/presentation_service_delegate.h"
+
+namespace media_router {
+
+// Instances of this class manages all offscreen presentations started in the
+// associated Profile and facilitates communication between the controllers and
+// the receiver of an offscreen presentation.
+//
+// Example usage:
+//
+// Receiver is created to host the offscreen presentation and registers itself
+// so that controller frames can connect to it:
+//
+//   OffscreenPresentationManager* manager =
+//       OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext(
+//           context);

[mark a. foltz, 2016/10/06 03:13:55]

Is |context| the one that has the controller frames (i.e. started the
presentation) or the receiver WebContents?

[zhaobin, 2016/10/07 01:07:03]

context is browserContext (profile).

offscreen_presentation_manager_(
          OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext(
              Profile::FromBrowserContext(web_contents_->GetBrowserContext())
                  ->GetOriginalProfile()))

+//   manager->RegisterOffscreenPresentationReceiver(presentation_id,
+//       base::Bind(&OnReceiverConnectionAvailable));
+//   ...
+//   void OnReceiverConnectionAvailable(

[mark a. foltz, 2016/10/06 03:13:55]

Is this called when a connection is available to the receiver or to the
controller or both?

[zhaobin, 2016/10/07 01:07:04]

receiver PSImpl::onReceiverConnectionAvailable() is invoked
when registerReceiver() is called for the first time (during receiver PSImpl
initialization), to notify all pending controllers 
or 
when registerController() is called (controller starts a presentation) and there
is an existing receiver for that presentation.

+//       const content::PresentationSessionInfo& session,
+//       OffscreenPresentationClient* controller) {
+//     [Connects controller PSImpl with receiver PSImpl to send/receive
+//      messages in response to send()]
+//     controller->SetOffscreenPresentationClient(receiver);
+//     receiver->SetOffscreenPresentationClient(controller);
+//   }
+//
+// Controller frame establishes connection with the receiver side, resulting
+// in a session with the two endpoints being the controller frame's PSImpl
+// (OffscreenPresentationClient) and the receiver frame's PSImpl
+// (OffscreenPresentationClient). Note calling this will trigger
+// |OnReceiverConnectionAvailable| on the receiver side.
+//
+//   manager->RegisterOffscreenPresentationController(
+//           presentation_id, controller_frame_id, controller_ptr);
+//
+// To send message from controller to receiver:

[mark a. foltz, 2016/10/06 03:13:54]

It looks like this is the same for the other direction.  Maybe just mention
that?

[zhaobin, 2016/10/07 01:07:04]

Done.

+//
+//   In controll's PSImpl::SendSessionMessage() {

[mark a. foltz, 2016/10/06 03:13:55]

contoller's

[zhaobin, 2016/10/07 01:07:04]

Done.

+//     receiver.OnSessionMessage();
+//   }
+//
+// A controller or receiver leaves the offscreen presentation (e.g.,
+// due to navigation) by unregistering themselves from
+// OffscreenPresentationConnection object.

[mark a. foltz, 2016/10/06 03:13:54]

If the receiver navigates, the offscreen tab should be closed.  I think the
WebContentsObserver for the receiver WebContents could take care of
unregistering the receiver from the controllers' PSImpls?

[zhaobin, 2016/10/07 01:07:04]

UnregisterReceiver() called in ReceiverPSDImpl::Reset() (for navigates or
offscreen tab closed).

+//
+// When the receiver is no longer associated with an offscreen presentation, it

[mark a. foltz, 2016/10/06 03:13:55]

I believe there are five ways to terminate an offscreen presentation:

1. Presentation attempts to navigate
2. Presentation calls window.close()
3. Presentation calls connection.terminate()
4. Controller calls connection.terminate()
5. Error in tab capture or streaming, or user stops the media route in the
dialog

For #1, #2 the WebContentsObserver can call PSImpl::Reset
For #3 this can be done inside PSImpl?
For #4 this is done here (in the manager)?
For #5 the component will close the capture stream, which closes the WebContents
and then is handled like #1 or #2.

[zhaobin, 2016/10/07 01:07:04]

(Not implemented yet) 
for #4, it will close the capture stream as in #5, and can be handled as #5.
for #3, we can make receiver's connection.terminate() call its controller's
connection.terminate() and handle it as #4.

need to fire onterminate() event correctly.

+// shall remove itself from associated controllers and then unregister itself
+// with OffscreenPresentationManager. Unregistration will prevent additional
+// controllers from establishing a connection with the receiver:
+//
+//   In receiver's PSImpl::Reset() {
+//     for (controller : offscreen_presentation_observers_)
+//       controller->RemoveOffscreenPresentationClient(receiver);
+//   }
+//   manager->UnregisterOffscreenPresentationReceiver(presentation_id);
+//
+// This class is not thread safe. All functions must be invoked on the UI

[mark a. foltz, 2016/10/06 03:13:54]

Is this called by another thread or code outside of chrome/browser/media/router?

If so, it would be a good idea to have a ThreadChecker member and DCHECK() for
thread affinity in the public API.

[zhaobin, 2016/10/07 01:07:04]

Done.

+// thread. All callbacks passed into this class will also be invoked on UI
+// thread.
+class OffscreenPresentationManager : public KeyedService {
+ public:
+  ~OffscreenPresentationManager() override;
+
+  // Register controller PSImpl to presentation with |presentation_id|,

[mark a. foltz, 2016/10/06 03:13:55]

Nit: Registers, Creates, Unregisters, etc. in function comments, here and below.

Also, should this be "Registers controller PresentationConnection for
|presentation_id|" since you are not passing a PSImpl?

[zhaobin, 2016/10/07 01:07:03]

Done.

+  // |render_frame_id|.
+  // Create a new presentation if no presentation with |presentation_id| exists.
+  // |controller| Not owned by this class.
+  void RegisterOffscreenPresentationController(
+      const std::string& presentation_id,
+      const std::string& presentation_url,

[mark a. foltz, 2016/10/06 03:13:54]

const GURL&

[zhaobin, 2016/10/07 01:07:03]

Merge GURL changes after https://codereview.chromium.org/2386633003/ lands

+      const RenderFrameHostId& render_frame_id,
+      content::PresentationConnectionPtr controller);
+
+  // Unregister controller PSImpl to presentation with |presentation_id|,
+  // |render_frame_id|.
+  void UnregisterOffscreenPresentationController(
+      const std::string& presentation_id,
+      const RenderFrameHostId& render_frame_id);

[mark a. foltz, 2016/10/06 03:13:55]

Should this take a content::PresentationConnectionPtr instead of a
render_frame_id?

[zhaobin, 2016/10/07 01:07:03]

content::PresentationConnectionPtr disallows copy and copy constructor (works as
unique ptr). It is a little complicated to pass in
content::PresentatioNConnectionPtr in unregisterController() as parameter.

+
+  // Register receiver PSImpl to presentation with |presentation_id|.

[mark a. foltz, 2016/10/06 03:13:54]

This function doesn't have a PSImpl parameter.

[zhaobin, 2016/10/07 01:07:03]

Done.

+  void RegisterOffscreenPresentationReceiver(

[mark a. foltz, 2016/10/06 03:13:55]

OnOffscreenPresentationReceiverCreated(id, url, callback)?  Since I think this
is called when the receiver WebContents has been created?

[zhaobin, 2016/10/07 01:07:04]

Done.

+      const std::string& presentation_id,
+      const std::string& presentation_url,

[mark a. foltz, 2016/10/06 03:13:54]

const GURL&

[zhaobin, 2016/10/07 01:07:04]

Done.

+      const content::ReceiverConnectionAvailableCallback& receiver_callback);
+
+  void UnregisterOffscreenPresentationReceiver(

[mark a. foltz, 2016/10/06 03:13:54]

OnOffscreenPresentationReceiverTerminated(presentation_id)?  Since I believe
this will be called only when the receiver has been terminated.

[zhaobin, 2016/10/07 01:07:03]

Done.

[zhaobin, 2016/10/07 01:07:03]

Done.

+      const std::string& presentation_id);
+
+ private:
+  // Represents an offscreen presentation registered with
+  // OffscreenPresentationManager.
+  // Contains callback to the receiver to inform it of new sessions

[mark a. foltz, 2016/10/06 03:13:55]

nit: s/sessions/connections/

[zhaobin, 2016/10/07 01:07:04]

Done.

+  // established from a controller.
+  // Contains set of controllers registered to OffscreenPresentationManager
+  // before corresponding receiver.
+  class OffscreenPresentation {
+   public:
+    explicit OffscreenPresentation(const std::string& presentation_id,
+                                   const std::string& presentation_url);

[mark a. foltz, 2016/10/06 03:13:55]

const GURL&

[zhaobin, 2016/10/07 01:07:03]

Done.

+    ~OffscreenPresentation();
+
+    void RegisterController(const RenderFrameHostId& render_frame_id,
+                            content::PresentationConnectionPtr controller);
+
+    void UnregisterController(const RenderFrameHostId& render_frame_id);
+
+    void RegisterReceiver(
+        const content::ReceiverConnectionAvailableCallback& receiver_callback);
+
+   private:
+    friend class OffscreenPresentationManagerTest;
+    friend class OffscreenPresentationManager;
+
+    // presentation_id for current presentation

[mark a. foltz, 2016/10/06 03:13:55]

Nit: comments should end in period, here and below.

[zhaobin, 2016/10/07 01:07:04]

Done.

+    const std::string presentation_id_;
+
+    // URL for current presentation
+    const std::string presentation_url_;

[mark a. foltz, 2016/10/06 03:13:55]

const GURL

[zhaobin, 2016/10/07 01:07:04]

Done.

+
+    // proxy to receiver PSImpl

[mark a. foltz, 2016/10/06 03:13:54]

I don't think this comment matches the code; the member is a callback not a
proxy?

[zhaobin, 2016/10/07 01:07:04]

Done.

+    content::ReceiverConnectionAvailableCallback receiver_callback_;
+
+    // proxy to controller PSImpl
+    // It only contains controllers registered before receiver_callback_
+    // is set. This map will be cleared in RegisterReceiver().
+    std::unordered_map<RenderFrameHostId,

[mark a. foltz, 2016/10/06 03:13:55]

Is it necessary to be a map, or would a vector<PendingControllerConnection>
work, where PendingControllerConnection is a struct holding RenderFrameHostId,
content::PresentationConnection be okay?

I guess using the keyed map makes it easier to check for duplicate registrations
by RenderFrameHostId.

[zhaobin, 2016/10/07 01:07:04]

It needs to be a map. We cannot pass content::PresentationConnectionPtr (mojo
ptr acts as unique_ptr) to UnregisterController(). So we need to map to
UnregisterController.

+                       content::PresentationConnectionPtr,
+                       RenderFrameHostIdHasher>
+        pending_controllers_;
+
+    DISALLOW_COPY_AND_ASSIGN(OffscreenPresentation);
+  };
+
+ private:
+  friend class OffscreenPresentationManagerFactory;
+  friend class OffscreenPresentationManagerTest;
+
+  // Used by OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext.
+  OffscreenPresentationManager();
+
+  // Maps from presentation ID to OffscreenPresentation.
+  std::map<std::string, std::unique_ptr<OffscreenPresentation>>
+      offscreen_presentations_;
+
+  DISALLOW_COPY_AND_ASSIGN(OffscreenPresentationManager);
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_OFFSCREEN_PRESENTATION_MANAGER_H_