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

content/renderer/presentation/presentation_dispatcher.cc

Index: content/renderer/presentation/presentation_dispatcher.cc
diff --git a/content/renderer/presentation/presentation_dispatcher.cc b/content/renderer/presentation/presentation_dispatcher.cc
index d11943b79e404df99222a41641dd1e3910598231..1694c27812be5f634f48e6b63fe1379ac3729a2d 100644
--- a/content/renderer/presentation/presentation_dispatcher.cc
+++ b/content/renderer/presentation/presentation_dispatcher.cc
@@ -75,7 +75,6 @@ GetWebPresentationConnectionCloseReasonFromMojo(
       return blink::WebPresentationConnectionCloseReason::Error;
   }
 }
-
 }  // namespace
 
 namespace content {
@@ -245,57 +244,63 @@ void PresentationDispatcher::terminateSession(
 
 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();
+    std::unique_ptr<blink::WebPresentationAvailabilityCallbacks> callback) {
+  DCHECK(!availabilityUrls.isEmpty());
+
+  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());
   }
-  DCHECK(status);
 
-  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;
+  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);
   }
 
-  status->availability_callbacks.Add(std::move(callbacks));
-  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(
@@ -343,15 +348,9 @@ void PresentationDispatcher::OnScreenAvailabilityUpdated(const GURL& url,
   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);
 }
 
@@ -363,20 +362,57 @@ void PresentationDispatcher::OnScreenAvailabilityNotSupported(const GURL& url) {
   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->availability_callbacks.Clear();
+  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);
+  }
+
+  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_)
@@ -484,8 +520,13 @@ void PresentationDispatcher::ConnectToPresentationServiceIfNeeded() {
   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;
 
@@ -502,6 +543,46 @@ void PresentationDispatcher::UpdateListeningState(AvailabilityStatus* status) {
   }
 }
 
+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)
@@ -549,10 +630,18 @@ PresentationDispatcher::CreateSendBinaryMessageRequest(
                                 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() {