[Presentation API] Handle multiple Presentation URLs in PresentationRequest::getAvailability()

content/renderer/presentation/presentation_dispatcher.cc

 // 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.
 
 #include "content/renderer/presentation/presentation_dispatcher.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
@@ skipping 57 matching lines @@
       return blink::WebPresentationConnectionCloseReason::Error;
     case blink::mojom::PresentationConnectionCloseReason::CLOSED:
       return blink::WebPresentationConnectionCloseReason::Closed;
     case blink::mojom::PresentationConnectionCloseReason::WENT_AWAY:
       return blink::WebPresentationConnectionCloseReason::WentAway;
     default:
       NOTREACHED();
       return blink::WebPresentationConnectionCloseReason::Error;
   }
 }
-
 }  // namespace
 
 namespace content {
 
 PresentationDispatcher::PresentationDispatcher(RenderFrame* render_frame)
     : RenderFrameObserver(render_frame),
       controller_(nullptr),
       binding_(this) {
 }
 
@@ skipping 149 matching lines @@
 
 void PresentationDispatcher::terminateSession(
     const blink::WebURL& presentationUrl,
     const blink::WebString& presentationId) {
   ConnectToPresentationServiceIfNeeded();
   presentation_service_->Terminate(presentationUrl, presentationId.utf8());
 }
 
 void PresentationDispatcher::getAvailability(
     const blink::WebVector<blink::WebURL>& availabilityUrls,
-    std::unique_ptr<blink::WebPresentationAvailabilityCallbacks> callbacks) {
-  // TODO(mfoltz): Pass all URLs to PresentationService. See crbug.com/627655.
-  const blink::WebURL& availabilityUrl = availabilityUrls[0];
-  AvailabilityStatus* status = nullptr;
-  auto status_it = availability_status_.find(availabilityUrl);
-  if (status_it == availability_status_.end()) {
-    status = new AvailabilityStatus(availabilityUrl);
-    availability_status_[availabilityUrl] = base::WrapUnique(status);
-  } else {
-    status = status_it->second.get();
-  }
-  DCHECK(status);
+    std::unique_ptr<blink::WebPresentationAvailabilityCallbacks> callback) {
+  DCHECK(!availabilityUrls.isEmpty());
 
-  if (status->listening_state == ListeningState::ACTIVE) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(&blink::WebPresentationAvailabilityCallbacks::onSuccess,
-                   base::Passed(&callbacks), status->last_known_availability));
-    return;
+  auto availability_callback = base::MakeUnique<AvailabilityCallback>(
+      availabilityUrls, std::move(callback));
+
+  for (const auto& availabilityUrl : availabilityUrls) {
+    AvailabilityStatus* status = nullptr;
+    auto status_it = availability_status_.find(availabilityUrl);
+    if (status_it == availability_status_.end()) {
+      status = new AvailabilityStatus(availabilityUrl);
+      availability_status_[availabilityUrl] = base::WrapUnique(status);
+    } else {
+      status = status_it->second.get();
+    }
+    DCHECK(status);
+    status->availability_callbacks.insert(availability_callback.get());
   }
 
-  status->availability_callbacks.Add(std::move(callbacks));
-  UpdateListeningState(status);
+  for (const auto& availabilityUrl : availabilityUrls) {
+    auto status_it = availability_status_.find(availabilityUrl);
+    DCHECK(status_it != availability_status_.end());

[mlamouri (slow - plz ping), 2017/01/04 16:15:00]

I think you can use DCHECK_NE here.

[zhaobin, 2017/01/06 01:55:21]

Code removed.

+    AvailabilityStatus* status = status_it->second.get();
+
+    UpdateAvailabilityObserversAndCallbacks(status);

[mark a. foltz, 2017/01/03 21:47:13]

Can this loop be combined with the previous one?  Or do you need all status
objects updated first?

[zhaobin, 2017/01/06 01:55:21]

Code removed.

+    UpdateListeningState(status);
+  }
+
+  availability_callbacks_.push_back(std::move(availability_callback));
 }
 
 void PresentationDispatcher::startListening(
     blink::WebPresentationAvailabilityObserver* observer) {
-  // TODO(mfoltz): Pass all URLs to PresentationService. See crbug.com/627655.
-  const blink::WebURL& availabilityUrl = observer->urls()[0];
-  auto status_it = availability_status_.find(availabilityUrl);
-  if (status_it == availability_status_.end()) {
-    DLOG(WARNING) << "Start listening for availability for unknown URL "
-                  << GURL(availabilityUrl);
-    return;
+  for (const auto& availabilityUrl : observer->urls()) {
+    auto status_it = availability_status_.find(availabilityUrl);
+    if (status_it == availability_status_.end()) {
+      DLOG(WARNING) << "Start listening for availability for unknown URL "
+                    << GURL(availabilityUrl);
+      return;
+    }
+    status_it->second->availability_observers.insert(observer);
+    UpdateListeningState(status_it->second.get());
   }
-  status_it->second->availability_observers.insert(observer);
-  UpdateListeningState(status_it->second.get());
 }
 
 void PresentationDispatcher::stopListening(
     blink::WebPresentationAvailabilityObserver* observer) {
-  // TODO(mfoltz): Pass all URLs to PresentationService. See crbug.com/627655.
-  const blink::WebURL& availabilityUrl = observer->urls()[0];
-  auto status_it = availability_status_.find(availabilityUrl);
-  if (status_it == availability_status_.end()) {
-    DLOG(WARNING) << "Stop listening for availability for unknown URL "
-                  << GURL(availabilityUrl);
-    return;
+  for (const auto& availabilityUrl : observer->urls()) {
+    auto status_it = availability_status_.find(availabilityUrl);
+    if (status_it == availability_status_.end()) {
+      DLOG(WARNING) << "Stop listening for availability for unknown URL "
+                    << GURL(availabilityUrl);
+      return;
+    }
+    status_it->second->availability_observers.erase(observer);
+    UpdateListeningState(status_it->second.get());
   }
-  status_it->second->availability_observers.erase(observer);
-  UpdateListeningState(status_it->second.get());
 }
 
 void PresentationDispatcher::setDefaultPresentationUrls(
     const blink::WebVector<blink::WebURL>& presentationUrls) {
   ConnectToPresentationServiceIfNeeded();
 
   std::vector<GURL> urls;
   for (const auto& url : presentationUrls)
     urls.push_back(url);
 
@@ skipping 27 matching lines @@
                                                          bool available) {
   auto status_it = availability_status_.find(url);
   if (status_it == availability_status_.end())
     return;
   AvailabilityStatus* status = status_it->second.get();
   DCHECK(status);
 
   if (status->listening_state == ListeningState::WAITING)
     status->listening_state = ListeningState::ACTIVE;
 
-  for (auto* observer : status->availability_observers)
-    observer->availabilityChanged(available);
-
-  for (AvailabilityCallbacksMap::iterator iter(&status->availability_callbacks);
-       !iter.IsAtEnd(); iter.Advance()) {
-    iter.GetCurrentValue()->onSuccess(available);
-  }
-  status->last_known_availability = available;
-  status->availability_callbacks.Clear();
+  status->last_known_availability = available ? ScreenAvailability::AVAILABLE
+                                              : ScreenAvailability::UNAVAILABLE;
+  UpdateAvailabilityObserversAndCallbacks(status);

[mark a. foltz, 2017/01/03 21:47:13]

This only needs to be called if last_known_availability has changed, right?

[zhaobin, 2017/01/06 01:55:20]

Done.

   UpdateListeningState(status);
 }
 
 void PresentationDispatcher::OnScreenAvailabilityNotSupported(const GURL& url) {
   auto status_it = availability_status_.find(url);
   if (status_it == availability_status_.end())
     return;
   AvailabilityStatus* status = status_it->second.get();
   DCHECK(status);
   DCHECK(status->listening_state == ListeningState::WAITING);
 
-  const blink::WebString& not_supported_error = blink::WebString::fromUTF8(
-      "getAvailability() isn't supported at the moment. It can be due to "
-      "a permanent or temporary system limitation. It is recommended to "
-      "try to blindly start a session in that case.");

[mlamouri (slow - plz ping), 2017/01/04 16:15:00]

Why are you removing the entire error message?

-  for (AvailabilityCallbacksMap::iterator iter(&status->availability_callbacks);
-       !iter.IsAtEnd(); iter.Advance()) {
-    iter.GetCurrentValue()->onError(blink::WebPresentationError(
-        blink::WebPresentationError::ErrorTypeAvailabilityNotSupported,
-        not_supported_error));
+  status->last_known_availability = ScreenAvailability::UNSUPPORTED;
+  UpdateAvailabilityObserversAndCallbacks(status);

[mark a. foltz, 2017/01/03 21:47:12]

This only needs to be called if last_known_availability has changed, right?

[zhaobin, 2017/01/06 01:55:20]

Done.

+  UpdateListeningState(status);
+}
+
+void PresentationDispatcher::UpdateAvailabilityObserversAndCallbacks(

[mark a. foltz, 2017/01/03 21:47:13]

Shorter as UpdateAvailabilityStatus ?

[zhaobin, 2017/01/06 01:55:20]

Done.

+    AvailabilityStatus* status) {

[mark a. foltz, 2017/01/03 21:47:13]

Could this be a method on AvailabilityStatus()?

[zhaobin, 2017/01/06 01:55:20]

Done.

+  for (auto* observer : status->availability_observers) {
+    auto availability = GetScreenAvailability(observer->urls());
+    if (availability != ScreenAvailability::UNKNOWN)
+      observer->availabilityChanged(availability ==

[mark a. foltz, 2017/01/03 21:47:13]

Where do you check that the availability has changed from the previous value
that was passed to the observer?

[zhaobin, 2017/01/06 01:55:20]

No check in PresentationDispatcher. PresentationRequest::availabilityChanged has
the check logic, only fire event if availability value has changed. Do we need
to add the check in PresentationDispatcher as well?

+                                    ScreenAvailability::AVAILABLE);
   }
-  status->availability_callbacks.Clear();
-  UpdateListeningState(status);
+
+  AvailabilityCallbacksSet callbacks_to_be_removed;
+  for (auto* availability_callback : status->availability_callbacks) {
+    auto availability = GetScreenAvailability(availability_callback->urls);
+    if (availability == ScreenAvailability::UNKNOWN) {

[mark a. foltz, 2017/01/03 21:47:13]

Nit: This could be a switch statement with a NOTREACHED default case.

[zhaobin, 2017/01/06 01:55:20]

Code removed.

+      continue;
+    } else if (availability == ScreenAvailability::UNSUPPORTED) {
+      availability_callback->callback->onError(blink::WebPresentationError(
+          blink::WebPresentationError::ErrorTypeAvailabilityNotSupported,
+          "Screen availability monitoring not supported"));
+    } else {
+      bool is_available = availability == ScreenAvailability::AVAILABLE;
+      availability_callback->callback->onSuccess(is_available);

[mlamouri (slow - plz ping), 2017/01/04 16:15:00]

nit: merge in one line

[zhaobin, 2017/01/06 01:55:20]

Done.

+    }
+
+    callbacks_to_be_removed.insert(availability_callback);
+  }
+
+  // Clean up callbacks from |availability_status_|.
+  for (auto* callback_to_be_removed : callbacks_to_be_removed) {
+    for (auto& status_it : availability_status_) {
+      auto status = status_it.second.get();
+      if (!base::ContainsValue(status->availability_callbacks,
+                               callback_to_be_removed))
+        continue;

[mlamouri (slow - plz ping), 2017/01/04 16:15:00]

style: { }

[zhaobin, 2017/01/06 01:55:20]

Code removed.

+      status->availability_callbacks.erase(callback_to_be_removed);
+      UpdateListeningState(status);
+    }
+  }
+
+  // Clean up callbacks from |availability_callbacks_|.
+  std::remove_if(availability_callbacks_.begin(), availability_callbacks_.end(),
+                 [&](const std::unique_ptr<AvailabilityCallback>& callback) {
+                   return base::ContainsValue(callbacks_to_be_removed,
+                                              callback.get());
+                 });
 }
 
 void PresentationDispatcher::OnDefaultSessionStarted(
     blink::mojom::PresentationSessionInfoPtr session_info) {
   if (!controller_)
     return;
 
   if (!session_info.is_null()) {
     presentation_service_->ListenForConnectionMessages(session_info.Clone());
     controller_->didStartDefaultSession(
@@ skipping 89 matching lines @@
 }
 
 void PresentationDispatcher::ConnectToPresentationServiceIfNeeded() {
   if (presentation_service_.get())
     return;
 
   render_frame()->GetRemoteInterfaces()->GetInterface(&presentation_service_);
   presentation_service_->SetClient(binding_.CreateInterfacePtrAndBind());
 }
 
+void PresentationDispatcher::SetPresentationServiceForTest(
+    blink::mojom::PresentationServicePtr presentation_service) {
+  presentation_service_ = std::move(presentation_service);
+}
+
 void PresentationDispatcher::UpdateListeningState(AvailabilityStatus* status) {
-  bool should_listen = !status->availability_callbacks.IsEmpty() ||
+  bool should_listen = !status->availability_callbacks.empty() ||
                        !status->availability_observers.empty();
   bool is_listening = status->listening_state != ListeningState::INACTIVE;
 
   if (should_listen == is_listening)
     return;
 
   ConnectToPresentationServiceIfNeeded();
   if (should_listen) {
     status->listening_state = ListeningState::WAITING;
     presentation_service_->ListenForScreenAvailability(status->url);
   } else {
     status->listening_state = ListeningState::INACTIVE;
     presentation_service_->StopListeningForScreenAvailability(status->url);
   }
 }
 
+PresentationDispatcher::ScreenAvailability
+PresentationDispatcher::GetScreenAvailability(
+    const blink::WebVector<blink::WebURL>& urls) {
+  size_t available_cnt = 0;
+  size_t unavailable_cnt = 0;
+  size_t unsupport_cnt = 0;
+  size_t unknown_cnt = 0;

[mlamouri (slow - plz ping), 2017/01/04 16:15:00]

style: s/cnt/count/. the abreviation doesn't seem needed here.

[zhaobin, 2017/01/06 01:55:20]

Code removed.

+
+  for (const auto& url : urls) {
+    if (!base::ContainsKey(availability_status_, url))
+      return ScreenAvailability::UNKNOWN;
+
+    auto url_availability = availability_status_[url]->last_known_availability;
+    switch (url_availability) {
+      case ScreenAvailability::AVAILABLE:
+        available_cnt++;
+        break;
+      case ScreenAvailability::UNAVAILABLE:
+        unavailable_cnt++;
+        break;
+      case ScreenAvailability::UNSUPPORTED:
+        unsupport_cnt++;
+        break;
+      case ScreenAvailability::UNKNOWN:
+        unknown_cnt++;
+        break;
+    }
+  }

[mlamouri (slow - plz ping), 2017/01/04 16:15:00]

FWIW, you can simplify this by having a map that associates the enum to an int
so you don't need to write the whole switch but just
`counters[url_availability_]++;` (or something similar)

[zhaobin, 2017/01/06 01:55:20]

Done.

+
+  if (unknown_cnt > 0)
+    return ScreenAvailability::UNKNOWN;

[mark a. foltz, 2017/01/03 21:47:13]

IMO, we should only return UNKNOWN if all URLs are UNKNOWN (i.e. we have
absolutely no information about the status).  Otherwise we can return the best
information we have now for some URLs (AVAILABLE/UNAVAILABLE/UNSUPPORTED).

[zhaobin, 2017/01/06 01:55:20]

Done.

+  if (unsupport_cnt > 0)
+    return ScreenAvailability::UNSUPPORTED;
+  if (available_cnt > 0)

[mark a. foltz, 2017/01/03 21:47:13]

This case should go first: if any URL is available, then the request is
available.

[zhaobin, 2017/01/06 01:55:20]

Done.

+    return ScreenAvailability::AVAILABLE;
+
+  DCHECK(unavailable_cnt == urls.size());

[mlamouri (slow - plz ping), 2017/01/04 16:15:00]

DCHECK_EQ

[zhaobin, 2017/01/06 01:55:20]

Done.

+  return ScreenAvailability::UNAVAILABLE;

[mark a. foltz, 2017/01/03 21:47:13]

This case should go third, when some URLs are unavailable and some are unknown,
then report UNAVAILABLE.

[zhaobin, 2017/01/06 01:55:20]

Done.

+}
+
 PresentationDispatcher::SendMessageRequest::SendMessageRequest(
     blink::mojom::PresentationSessionInfoPtr session_info,
     blink::mojom::ConnectionMessagePtr message)
     : session_info(std::move(session_info)), message(std::move(message)) {}
 
 PresentationDispatcher::SendMessageRequest::~SendMessageRequest() {}
 
 // static
 PresentationDispatcher::SendMessageRequest*
 PresentationDispatcher::CreateSendTextMessageRequest(
@@ skipping 27 matching lines @@
   session_info->id = presentationId.utf8();
 
   blink::mojom::ConnectionMessagePtr session_message =
       blink::mojom::ConnectionMessage::New();
   session_message->type = type;
   session_message->data = std::vector<uint8_t>(data, data + length);
   return new SendMessageRequest(std::move(session_info),
                                 std::move(session_message));
 }
 
+PresentationDispatcher::AvailabilityCallback::AvailabilityCallback(
+    const blink::WebVector<blink::WebURL>& availability_urls,
+    std::unique_ptr<blink::WebPresentationAvailabilityCallbacks>
+        availability_callback)
+    : urls(availability_urls), callback(std::move(availability_callback)) {}
+
+PresentationDispatcher::AvailabilityCallback::~AvailabilityCallback() {}
+
 PresentationDispatcher::AvailabilityStatus::AvailabilityStatus(
     const GURL& availability_url)
     : url(availability_url),
-      last_known_availability(false),
+      last_known_availability(ScreenAvailability::UNKNOWN),
       listening_state(ListeningState::INACTIVE) {}
 
 PresentationDispatcher::AvailabilityStatus::~AvailabilityStatus() {
 }
 
 }  // namespace content