[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..96e4ca0a0e0092982c6b05768d6ed1eb5d6ebc29 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,10 +22,12 @@ PresentationServiceImpl::PresentationServiceImpl(
     PresentationServiceDelegate* delegate)
     : WebContentsObserver(web_contents),
       render_frame_host_(render_frame_host),
-      delegate_(delegate) {
+      delegate_(delegate),
+      start_session_in_progress_(false),
+      weak_factory_(this) {
   DCHECK(render_frame_host_);
   DCHECK(web_contents);
-  VLOG(2) << "PresentationServiceImpl: "
+  DVLOG(2) << "PresentationServiceImpl: "
           << render_frame_host_->GetProcess()->GetID() << ", "

[mark a. foltz, 2015/03/13 01:39:13]

Indentation of this line and the one following needs to be fixed

[imcheng, 2015/03/13 23:36:46]

Done.

           << render_frame_host_->GetRoutingID();
   if (delegate_)
@@ -40,7 +43,7 @@ PresentationServiceImpl::~PresentationServiceImpl() {
 void PresentationServiceImpl::CreateMojoService(
     RenderFrameHost* render_frame_host,
     mojo::InterfaceRequest<presentation::PresentationService> request) {
-  VLOG(2) << "PresentationServiceImpl::CreateService";
+  DVLOG(2) << "CreateMojoService";
   WebContents* web_contents =
       WebContents::FromRenderFrameHost(render_frame_host);
   DCHECK(web_contents);
@@ -55,7 +58,7 @@ void PresentationServiceImpl::CreateMojoService(
 }
 
 void PresentationServiceImpl::OnConnectionError() {
-  VLOG(1) << "PresentationServiceImpl::OnConnectionError: "
+  DVLOG(1) << "OnConnectionError: "
           << render_frame_host_->GetProcess()->GetID() << ", "

[mark a. foltz, 2015/03/13 01:39:13]

Indentation

[imcheng, 2015/03/13 23:36:46]

Done.

           << render_frame_host_->GetRoutingID();
 }
@@ -63,61 +66,206 @@ void PresentationServiceImpl::OnConnectionError() {
 void PresentationServiceImpl::GetScreenAvailability(
     const mojo::String& presentation_url,
     const ScreenAvailabilityMojoCallback& callback) {
-  VLOG(2) << "PresentationServiceImpl::GetScreenAvailability";
+  DVLOG(2) << "GetScreenAvailability";
   if (!delegate_)
     return;
 
-  const std::string& presentation_url_str = !presentation_url.is_null() ?
-      presentation_url.get() : default_presentation_url_;
-
-  // GetScreenAvailability() is called with no URL and there is no default
-  // Presentation URL.
-  if (presentation_url_str.empty())
-    return;
-
+  const std::string& presentation_url_str = presentation_url.get();
   auto it = availability_contexts_.find(presentation_url_str);

[mark a. foltz, 2015/03/13 01:39:13]

Can you factor out a method GetOrCreaeteAvailabiltyContext to make this method
shorter and more readable?

[imcheng, 2015/03/13 23:36:46]

Done.

   if (it == availability_contexts_.end()) {
     linked_ptr<ScreenAvailabilityContext> context(
         new ScreenAvailabilityContext(presentation_url_str));
-
     if (!delegate_->AddScreenAvailabilityListener(
         render_frame_host_->GetProcess()->GetID(),
         render_frame_host_->GetRoutingID(),
         context.get())) {
-      VLOG(1) << "AddScreenAvailabilityListener failed. Ignoring request.";
+      DVLOG(1) << "AddScreenAvailabilityListener failed. Ignoring request.";
       return;
     }
-
     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();
+void PresentationServiceImpl::OnScreenAvailabilityListenerRemoved(
+    const mojo::String& presentation_url) {
+  DVLOG(2) << "OnScreenAvailabilityListenerRemoved";
+  if (!delegate_)
+    return;
+
+  const std::string& presentation_url_str = presentation_url.get();
+  auto it = availability_contexts_.find(presentation_url_str);

[mark a. foltz, 2015/03/13 01:39:13]

Factor out method GetAvailabilityContext(str)?

[imcheng, 2015/03/13 23:36:46]

Actually, I need the iterator here since erase by iterator is more efficient.

+  if (it == availability_contexts_.end())
+    return;
+
+  delegate_->RemoveScreenAvailabilityListener(

[mark a. foltz, 2015/03/13 01:39:13]

Is the delegate_ unique to this instance of the PresentationServiceImpl?  If so,
can it keep the RFH's process and routing ids as members instead of passing them
every time?

[imcheng, 2015/03/13 23:36:46]

It is not. It's actually per tab (see line 55-56) so delegate_ is shared for
instances of PresentationServiceImpl (per frame) in the same tab. Chatted
offline. We will keep it as it is for now.

+      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();
+  DVLOG(2) << "StartSession";
+  if (!delegate_)
+    return;
+
+  if (!start_session_in_progress_) {

[mark a. foltz, 2015/03/13 01:39:12]

Is this test the same as queued_start_session_requests_.empty() ?

[whywhat, 2015/03/13 17:44:35]

I think one can have one session being started while the queue is empty.
I'd also like to get rid of the flag though: maybe queue every session in
StartSession/JoinSession and remove the front one in
OnStartOrJoinSessionCallback?

[imcheng, 2015/03/13 23:36:46]

Ok, I will make the change as you suggest to get rid of the boolean. Though, in
a way this is more error prone, since we can potentially have a mismatch between
head of queue and the request just processed.

+    DoStartSession(presentation_url, presentation_id, callback);
+  } else {
+    queued_start_session_requests_.push_back(make_linked_ptr(
+        new StartSessionRequest(presentation_url, presentation_id, callback)));
+  }
 }
 
 void PresentationServiceImpl::JoinSession(
     const mojo::String& presentation_url,
     const mojo::String& presentation_id,
     const NewSessionMojoCallback& callback) {
-  NOTIMPLEMENTED();
+  DVLOG(2) << "JoinSession";
+  if (!delegate_)
+    return;
+
+  delegate_->JoinSession(
+      render_frame_host_->GetProcess()->GetID(),
+      render_frame_host_->GetRoutingID(),
+      presentation_url,
+      presentation_id,
+      base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionSucceeded,
+                 weak_factory_.GetWeakPtr(), false, callback),
+      base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionError,
+                 weak_factory_.GetWeakPtr(), false, callback));
+}
+
+void PresentationServiceImpl::HandleQueuedStartSessionRequests() {
+  if (queued_start_session_requests_.empty()) {

[mark a. foltz, 2015/03/13 01:39:13]

Ditto

[imcheng, 2015/03/13 23:36:46]

It wasn't the same because there could be a request in progress while queue is
empty.

+    start_session_in_progress_ = false;
+  } else {
+    linked_ptr<StartSessionRequest> request =
+        queued_start_session_requests_.front();
+    queued_start_session_requests_.pop_front();
+    DoStartSession(request->presentation_url,
+                   request->presentation_id,
+                   request->callback);
+  }
+}
+
+void PresentationServiceImpl::DoStartSession(
+    const std::string& presentation_url,
+    const std::string& presentation_id,
+    const NewSessionMojoCallback& callback) {
+  start_session_in_progress_ = true;

[whywhat, 2015/03/13 17:44:35]

if called from HandleQueueStartSessionRequests, it's already true, right?

[imcheng, 2015/03/13 23:36:46]

Yes.

+  delegate_->StartSession(
+      render_frame_host_->GetProcess()->GetID(),
+      render_frame_host_->GetRoutingID(),
+      presentation_url,
+      presentation_id,
+      base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionSucceeded,
+                 weak_factory_.GetWeakPtr(), true, callback),
+      base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionError,
+                 weak_factory_.GetWeakPtr(), true, callback));
+}
+
+void PresentationServiceImpl::OnStartOrJoinSessionSucceeded(
+    bool is_start_session,
+    const NewSessionMojoCallback& callback,
+    const PresentationSessionInfo& session_info) {
+  callback.Run(
+      presentation::PresentationSessionInfo::From(session_info),
+      presentation::PresentationErrorPtr());
+  if (is_start_session) {

[whywhat, 2015/03/13 17:44:35]

No need for braces for one line if branches.

[imcheng, 2015/03/13 23:36:46]

Done.

+    HandleQueuedStartSessionRequests();
+  }
+}
+
+void PresentationServiceImpl::OnStartOrJoinSessionError(
+    bool is_start_session,
+    const NewSessionMojoCallback& callback,
+    const PresentationError& error) {
+  callback.Run(
+      presentation::PresentationSessionInfoPtr(),
+      presentation::PresentationError::From(error));
+  if (is_start_session) {
+    HandleQueuedStartSessionRequests();

[whywhat, 2015/03/13 17:44:35]

ditto

[imcheng, 2015/03/13 23:36:46]

Done.

+  }
+}
+
+void PresentationServiceImpl::DoSetDefaultPresentationUrl(
+    const std::string& default_presentation_url,
+    const std::string& default_presentation_id) {
+  DCHECK(delegate_);
+  delegate_->SetDefaultPresentationUrl(
+      render_frame_host_->GetProcess()->GetID(),
+      render_frame_host_->GetRoutingID(),
+      default_presentation_url,
+      default_presentation_id);
+  default_presentation_url_ = default_presentation_url;
+  default_presentation_id_ = default_presentation_id;
+}
+
+void PresentationServiceImpl::SetDefaultPresentationUrl(
+    const mojo::String& default_presentation_url,
+    const mojo::String& default_presentation_id) {
+  DVLOG(2) << "SetDefaultPresentationUrl";
+  if (!delegate_)
+    return;
+
+  std::string old_default_url(default_presentation_url_);

[mark a. foltz, 2015/03/13 01:39:13]

const std::string& ... .get() like you did above?   

[imcheng, 2015/03/13 23:36:46]

Done.

+  std::string new_default_url(default_presentation_url);
+
+  // Don't call delegate if nothing changed.
+  if (old_default_url == new_default_url &&
+      default_presentation_id_ == default_presentation_id) {
+    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, default_presentation_id);
+    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.
+  ScreenAvailabilityMojoCallback* old_callback = old_it->second->callback();
+  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())) {
+      DVLOG(1) << "AddScreenAvailabilityListener failed. Ignoring request.";
+      return;
+    }
+    new_it = availability_contexts_.insert(
+        std::make_pair(context->GetPresentationUrl(), context)).first;

[mark a. foltz, 2015/03/13 01:39:13]

The method I suggested above to get or create the context could be used here too

[imcheng, 2015/03/13 23:36:46]

Done.

+  }
+  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, default_presentation_id);
 }
 
 void PresentationServiceImpl::DidNavigateAnyFrame(
     content::RenderFrameHost* render_frame_host,
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {
-  VLOG(2) << "PresentationServiceImpl::DidNavigateAnyFrame";
+  DVLOG(2) << "PresentationServiceImpl::DidNavigateAnyFrame";
   if (render_frame_host_ != render_frame_host)
     return;
 
@@ -129,42 +277,47 @@ void PresentationServiceImpl::DidNavigateAnyFrame(
   bool in_page_navigation = details.is_in_page ||
       details.type == content::NAVIGATION_TYPE_IN_PAGE;
 
-  VLOG(2) << "DidNavigateAnyFrame: "
+  DVLOG(2) << "DidNavigateAnyFrame: "
           << "prev host: " << prev_url_host << ", curr host: " << curr_url_host
           << ", in_page_navigation: " << in_page_navigation;
 
   if (in_page_navigation)
     return;
 
-  // Unregister all sources if the frame actually navigated.
-  RemoveAllListeners();
+  // Reset if the frame actually navigated.
+  Reset();
 }
 
 void PresentationServiceImpl::RenderFrameDeleted(
     content::RenderFrameHost* render_frame_host) {
-  VLOG(2) << "PresentationServiceImpl::RenderFrameDeleted";
+  DVLOG(2) << "PresentationServiceImpl::RenderFrameDeleted";
   if (render_frame_host_ != render_frame_host)
     return;
 
   // RenderFrameDeleted means this object is getting deleted soon.
-  RemoveAllListeners();
+  Reset();
 }
 
-void PresentationServiceImpl::RemoveAllListeners() {
-  VLOG(2) << "PresentationServiceImpl::RemoveAllListeners";
-  if (!delegate_)
-    return;
-
-  delegate_->RemoveAllScreenAvailabilityListeners(
+void PresentationServiceImpl::Reset() {
+  DVLOG(2) << "PresentationServiceImpl::Reset";
+  if (delegate_) {
+    delegate_->RemoveAllScreenAvailabilityListeners(
         render_frame_host_->GetProcess()->GetID(),
         render_frame_host_->GetRoutingID());
 
+    // Clear default presentation URL.
+    DoSetDefaultPresentationUrl(std::string(), std::string());
+  }
+
   availability_contexts_.clear();
+  start_session_in_progress_ = false;
+  queued_start_session_requests_.clear();
 }
 
 void PresentationServiceImpl::OnDelegateDestroyed() {
-  VLOG(2) << "PresentationServiceImpl::OnDelegateDestroyed";
+  DVLOG(2) << "PresentationServiceImpl::OnDelegateDestroyed";
   delegate_ = nullptr;
+  Reset();
 }
 
 PresentationServiceImpl::ScreenAvailabilityContext::ScreenAvailabilityContext(
@@ -186,7 +339,7 @@ void PresentationServiceImpl::ScreenAvailabilityContext::CallbackReceived(
     // Run callback now, reset result.
     // There shouldn't be any callbacks stored in this scenario.
     DCHECK(!callback_ptr_);
-    callback.Run(*available_ptr_);
+    callback.Run(presentation_url_, *available_ptr_);
     Reset();
   }
 }
@@ -210,9 +363,21 @@ void PresentationServiceImpl::ScreenAvailabilityContext
     // Invoke callback and erase it.
     // There shouldn't be any result stored in this scenario.
     DCHECK(!available_ptr_);
-    callback_ptr_->Run(available);
+    callback_ptr_->Run(presentation_url_, available);
     Reset();
   }
 }
 
+PresentationServiceImpl::StartSessionRequest::StartSessionRequest(
+    const std::string& presentation_url,
+    const std::string& presentation_id,
+    const NewSessionMojoCallback& callback)
+    : presentation_url(presentation_url),
+      presentation_id(presentation_id),
+      callback(callback) {
+}
+
+PresentationServiceImpl::StartSessionRequest::~StartSessionRequest() {
+}
+
 }  // namespace content