[Presentation API] Additional plumbing for PresentationServiceImpl.

content/browser/presentation/presentation_service_impl.cc

Index: content/browser/presentation/presentation_service_impl.cc
diff --git a/content/browser/presentation/presentation_service_impl.cc b/content/browser/presentation/presentation_service_impl.cc
index 4a1db13ea7a41d8e8d79981ecfe079f8b731a41a..522e854498446abb84432215a9b65dae27815a4b 100644
--- a/content/browser/presentation/presentation_service_impl.cc
+++ b/content/browser/presentation/presentation_service_impl.cc
@@ -5,6 +5,7 @@
 #include "content/browser/presentation/presentation_service_impl.h"
 
 #include "base/logging.h"
+#include "content/browser/presentation/presentation_type_converters.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/render_frame_host.h"
@@ -21,7 +22,8 @@ PresentationServiceImpl::PresentationServiceImpl(
     PresentationServiceDelegate* delegate)
     : WebContentsObserver(web_contents),
       render_frame_host_(render_frame_host),
-      delegate_(delegate) {
+      delegate_(delegate),
+      weak_factory_(this) {
   DCHECK(render_frame_host_);
   DCHECK(web_contents);
   VLOG(2) << "PresentationServiceImpl: "

[mark a. foltz, 2015/03/11 01:08:18]

Is VLOG() necessary for these messages, or would DVLOG() be sufficient?   They
look like they are for developer debugging.

[imcheng, 2015/03/12 20:03:41]

Converted to DVLOG().

@@ -40,7 +42,7 @@ PresentationServiceImpl::~PresentationServiceImpl() {
 void PresentationServiceImpl::CreateMojoService(
     RenderFrameHost* render_frame_host,
     mojo::InterfaceRequest<presentation::PresentationService> request) {
-  VLOG(2) << "PresentationServiceImpl::CreateService";
+  VLOG(2) << "CreateMojoService";
   WebContents* web_contents =
       WebContents::FromRenderFrameHost(render_frame_host);
   DCHECK(web_contents);
@@ -55,7 +57,7 @@ void PresentationServiceImpl::CreateMojoService(
 }
 
 void PresentationServiceImpl::OnConnectionError() {
-  VLOG(1) << "PresentationServiceImpl::OnConnectionError: "
+  VLOG(1) << "OnConnectionError: "
           << render_frame_host_->GetProcess()->GetID() << ", "
           << render_frame_host_->GetRoutingID();
 }
@@ -63,7 +65,7 @@ void PresentationServiceImpl::OnConnectionError() {
 void PresentationServiceImpl::GetScreenAvailability(
     const mojo::String& presentation_url,
     const ScreenAvailabilityMojoCallback& callback) {
-  VLOG(2) << "PresentationServiceImpl::GetScreenAvailability";
+  VLOG(2) << "GetScreenAvailability";
   if (!delegate_)
     return;
 
@@ -72,8 +74,10 @@ void PresentationServiceImpl::GetScreenAvailability(
 
   // GetScreenAvailability() is called with no URL and there is no default
   // Presentation URL.

[mark a. foltz, 2015/03/11 01:08:18]

In this case we will be falling back on "1-UA" mode, i.e. offscreen tab
rendering.

Since the media source patch is landing soon, maybe we can define a default
media source for offscreen content to populate the screen availability query.

[imcheng, 2015/03/12 20:03:41]

Ok. In that case, we can pass an empty string to PSDelegate which will then use
the default media source.

-  if (presentation_url_str.empty())
+  if (presentation_url_str.empty()) {
+    VLOG(1) << "No valid presentation URL.";
     return;
+  }
 
   auto it = availability_contexts_.find(presentation_url_str);
   if (it == availability_contexts_.end()) {
@@ -89,28 +93,151 @@ void PresentationServiceImpl::GetScreenAvailability(
     }
 
     it = availability_contexts_.insert(
-        std::make_pair(presentation_url_str, context)).first;
+        std::make_pair(context->GetPresentationUrl(), context)).first;
   }
 
   it->second->CallbackReceived(callback);
 }
 
 void PresentationServiceImpl::OnScreenAvailabilityListenerRemoved() {
-  NOTIMPLEMENTED();
+  VLOG(2) << "OnScreenAvailabilityListenerRemoved";
+  if (!delegate_)

[mark a. foltz, 2015/03/11 23:55:57]

When could this be null?  Can we enforce a non-null delegate_ in the ctor?

It seems like there are null-checks for delegate_ throughout the file, but it's
inconsistent.  It would be simpler not to have them at all.

[imcheng, 2015/03/12 20:03:41]

We should have these null checks because we don't expect all embedders to
implement PSDelegate and hook it up here. I did forget to add null checks in new
 functions defined, so I will add them back.

Another reason is that we don't assume the lifetime of PSDelegate interface.
It's possible for the PSDelegate to be destroyed before this class, in that
case, OnDelegateDestroyed() will be called and delegate_ will be set to nullptr.

+    return;
+
+  auto it = availability_contexts_.find(default_presentation_url_);
+  if (it == availability_contexts_.end())
+    return;
+
+  delegate_->RemoveScreenAvailabilityListener(
+      render_frame_host_->GetProcess()->GetID(),
+      render_frame_host_->GetRoutingID(),
+      it->second.get());
+  availability_contexts_.erase(it);
 }
 
 void PresentationServiceImpl::StartSession(
     const mojo::String& presentation_url,
     const mojo::String& presentation_id,
     const NewSessionMojoCallback& callback) {
-  NOTIMPLEMENTED();
+  VLOG(2) << "StartSession";
+  if (start_or_join_session_cb_.get())
+    return;
+
+  delegate_->StartSession(
+      render_frame_host_->GetProcess()->GetID(),
+      render_frame_host_->GetRoutingID(),
+      presentation_url,
+      presentation_id,
+      base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionSucceeded,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionError,
+                 weak_factory_.GetWeakPtr()));
+  start_or_join_session_cb_.reset(new NewSessionMojoCallback(callback));
 }
 
 void PresentationServiceImpl::JoinSession(
     const mojo::String& presentation_url,
     const mojo::String& presentation_id,
     const NewSessionMojoCallback& callback) {
-  NOTIMPLEMENTED();
+  VLOG(2) << "JoinSession";
+  if (start_or_join_session_cb_.get())
+    return;
+
+  delegate_->JoinSession(
+      render_frame_host_->GetProcess()->GetID(),
+      render_frame_host_->GetRoutingID(),
+      presentation_url,
+      presentation_id,
+      base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionSucceeded,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionError,
+                 weak_factory_.GetWeakPtr()));
+  start_or_join_session_cb_.reset(new NewSessionMojoCallback(callback));

[mark a. foltz, 2015/03/11 23:55:57]

All the two bound callbacks above do is invoke the start_or_join_session_cb_.

What if we just new'ed up a NewSessionMojoCallback and/or ErrorMojoCallback and
passed ownership to delegate_, who would run them directly (instead of tracking
them via a member in the PresentationServiceImpl)?

[imcheng, 2015/03/12 20:03:41]

The problem is that embedders do not have access to content implementation.
Presentation service mojom is a content implementation detail and therefore
cannot be exposed to PSDelegate (which lives in content/public).

+}
+
+void PresentationServiceImpl::OnStartOrJoinSessionSucceeded(
+    const PresentationSessionInfo& session_info) {
+  CHECK(start_or_join_session_cb_.get());
+  start_or_join_session_cb_->Run(
+      presentation::PresentationSessionInfo::From(session_info),
+      presentation::PresentationErrorPtr());
+  start_or_join_session_cb_.reset();
+}
+
+void PresentationServiceImpl::OnStartOrJoinSessionError(
+    const PresentationError& error) {
+  CHECK(start_or_join_session_cb_.get());
+  start_or_join_session_cb_->Run(
+      presentation::PresentationSessionInfoPtr(),
+      presentation::PresentationError::From(error));
+  start_or_join_session_cb_.reset();
+}
+

[mark a. foltz, 2015/03/11 23:55:57]

extra newline

[imcheng, 2015/03/12 20:03:41]

Done.

+
+void PresentationServiceImpl::DoSetDefaultPresentationUrl(
+    const std::string& presentation_url) {
+  delegate_->SetDefaultPresentationUrl(
+      render_frame_host_->GetProcess()->GetID(),
+      render_frame_host_->GetRoutingID(),
+      presentation_url);
+  default_presentation_url_ = presentation_url;

[mark a. foltz, 2015/03/11 23:55:57]

Does this class also need to know the default_presentation_url_?

As I read this it's hard to know whose job it is to keep track - this object or
the delegate.

[imcheng, 2015/03/12 20:03:41]

Yes, this class needs to know the DPU. Both this class and delegate holds a copy
of the current DPU. I thought letting the delegate hold the DPU and exposing a
GetDPU() in PSDelegate interface, but I think it's a pretty awkward interface
since PSImpl is the one that sets DPU on the delegate. And it's also strange to
let this class hold the DPU and expose GetDPU() in PSDelegate::Observer.
Additionally, I want to avoid introducing unnecessary interfaces to PSDelegate.

+}
+
+void PresentationServiceImpl::SetDefaultPresentationUrl(
+    const mojo::String& presentation_url) {
+  VLOG(2) << "SetDefaultPresentationUrl";
+  if (!delegate_)
+    return;
+
+  std::string old_default_url = default_presentation_url_;
+  std::string new_default_url = presentation_url;
+
+  if (old_default_url == new_default_url)
+    return;
+
+  if (old_default_url.empty()) {
+    DoSetDefaultPresentationUrl(new_default_url);
+    return;
+  }
+
+  auto old_it = availability_contexts_.find(old_default_url);
+  // Haven't started listening yet.
+  if (old_it == availability_contexts_.end()) {
+    DoSetDefaultPresentationUrl(new_default_url);
+    return;
+  }
+
+  // Have already started listening. Create a listener for the new URL and
+  // transfer the callback from the old listener, if any.
+  // This is done so that a listener added before default URL is changed
+  // will continue to work.

[mark a. foltz, 2015/03/11 23:55:57]

Are we sure that the callback that registered for the old URL wants to be
invoked for the new URL?

[imcheng, 2015/03/12 20:03:41]

That would depend on the blink implementation. I believe the callback is not
bound to any particular url so it should be safe to transfer. And then I also
want to verify that this transferring logic matches the behavior defined in the
spec. I asked the question to Anton regarding blink implementation in the other
comment.

+  ScreenAvailabilityMojoCallback* old_callback = old_it->second->callback();
+  if (!new_default_url.empty()) {
+    auto new_it = availability_contexts_.find(new_default_url);
+    if (new_it == availability_contexts_.end()) {
+      linked_ptr<ScreenAvailabilityContext> context(
+          new ScreenAvailabilityContext(new_default_url));
+      if (!delegate_->AddScreenAvailabilityListener(
+          render_frame_host_->GetProcess()->GetID(),
+          render_frame_host_->GetRoutingID(),
+          context.get())) {
+        VLOG(1) << "AddScreenAvailabilityListener failed. Ignoring request.";
+        return;
+      }
+      new_it = availability_contexts_.insert(
+          std::make_pair(context->GetPresentationUrl(), context)).first;
+    }
+    if (old_callback)
+      new_it->second->CallbackReceived(*old_callback);
+  }
+
+  // Remove listener for old default presentation URL.
+  delegate_->RemoveScreenAvailabilityListener(
+      render_frame_host_->GetProcess()->GetID(),
+      render_frame_host_->GetRoutingID(),
+      old_it->second.get());
+  availability_contexts_.erase(old_it);
+  DoSetDefaultPresentationUrl(new_default_url);
 }
 
 void PresentationServiceImpl::DidNavigateAnyFrame(
@@ -136,8 +263,8 @@ void PresentationServiceImpl::DidNavigateAnyFrame(
   if (in_page_navigation)
     return;
 
-  // Unregister all sources if the frame actually navigated.
-  RemoveAllListeners();
+  // Reset if the frame actually navigated.
+  Reset();
 }
 
 void PresentationServiceImpl::RenderFrameDeleted(
@@ -147,24 +274,30 @@ void PresentationServiceImpl::RenderFrameDeleted(
     return;
 
   // RenderFrameDeleted means this object is getting deleted soon.
-  RemoveAllListeners();
+  Reset();
 }
 
-void PresentationServiceImpl::RemoveAllListeners() {
-  VLOG(2) << "PresentationServiceImpl::RemoveAllListeners";
+void PresentationServiceImpl::Reset() {
+  VLOG(2) << "PresentationServiceImpl::Reset";
   if (!delegate_)
     return;
 
+  // Unregister all sources.
   delegate_->RemoveAllScreenAvailabilityListeners(
         render_frame_host_->GetProcess()->GetID(),
         render_frame_host_->GetRoutingID());
-
   availability_contexts_.clear();
+
+  // Clear default presentation URL.
+  DoSetDefaultPresentationUrl(std::string());
+
+  start_or_join_session_cb_.reset();
 }
 
 void PresentationServiceImpl::OnDelegateDestroyed() {
   VLOG(2) << "PresentationServiceImpl::OnDelegateDestroyed";
   delegate_ = nullptr;
+  start_or_join_session_cb_.reset();
 }
 
 PresentationServiceImpl::ScreenAvailabilityContext::ScreenAvailabilityContext(