[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 29e0f0229ffd9ad023675ce58cdd24e3926d2cd4..63950ee76cad3d253e56aa48a80af41fa7a008ea 100644
--- a/content/renderer/presentation/presentation_dispatcher.cc
+++ b/content/renderer/presentation/presentation_dispatcher.cc
@@ -92,7 +92,6 @@ GetWebPresentationConnectionCloseReasonFromMojo(
       return blink::WebPresentationConnectionCloseReason::Error;
   }
 }
-
 }  // namespace
 
 namespace content {
@@ -262,57 +261,83 @@ 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];
+    std::unique_ptr<blink::WebPresentationAvailabilityCallbacks> callback) {
+  DCHECK(!availabilityUrls.isEmpty());
+
+  std::vector<GURL> urls;
+  for (const auto& availability_url : availabilityUrls)
+    urls.push_back(availability_url);
+
+  auto screen_availability = GetScreenAvailability(urls);
+  // Resolve promise if we know some URLs' status.

[mark a. foltz, 2017/01/11 00:06:02]

// Reject Promise if screen availability is unsupported for all URLs.

[zhaobin, 2017/01/12 03:02:12]

Done.

+  if (screen_availability == ScreenAvailability::UNSUPPORTED) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &blink::WebPresentationAvailabilityCallbacks::onError,
+            base::Passed(&callback),
+            blink::WebPresentationError(
+                blink::WebPresentationError::ErrorTypeAvailabilityNotSupported,
+                "Screen availability monitoring not supported")));
+    // Do not listen to urls if we reject the promise.
+    return;
+  }
+
   AvailabilityStatus* status = nullptr;

[mark a. foltz, 2017/01/11 00:06:02]

Consider using std::find_if instead of looping.

[zhaobin, 2017/01/12 03:02:12]

Done.

-  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();
+  for (const auto& availability : availability_set_) {
+    if (availability->urls == urls) {
+      status = availability.get();
+      break;
+    }
   }
-  DCHECK(status);
 
-  if (status->listening_state == ListeningState::ACTIVE) {
+  if (!status) {
+    status = new AvailabilityStatus(urls);
+    availability_set_.insert(base::WrapUnique(status));

[mark a. foltz, 2017/01/11 00:06:02]

When are entries removed from |availability_set_|?

[zhaobin, 2017/01/12 03:02:12]

We do not. |availability_set_| will finally be cleared in dtor. Current single
url implementation does not remove |AvailabilityStatus| from
|availability_status_|.

We may remove an entry when there is no observers or no callbacks for a set of
urls and add it back when new observer or callback becomes available.

+  }
+
+  if (screen_availability != ScreenAvailability::UNKNOWN) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(&blink::WebPresentationAvailabilityCallbacks::onSuccess,
-                   base::Passed(&callbacks), status->last_known_availability));
-    return;
+                   base::Passed(&callback),
+                   screen_availability == ScreenAvailability::AVAILABLE));
+  } else {
+    status->availability_callbacks.Add(std::move(callback));
   }
 
-  status->availability_callbacks.Add(std::move(callbacks));
-  UpdateListeningState(status);
+  for (const auto& availabilityUrl : urls)
+    StartListening(availabilityUrl);
 }
 
 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;
+  std::vector<GURL> urls;
+  for (const auto& url : observer->urls())
+    urls.push_back(url);
+
+  for (auto& status : availability_set_) {
+    if (status->urls == urls)
+      status->availability_observers.insert(observer);

[mark a. foltz, 2017/01/11 00:06:02]

Do you need to break the loop after inserting the observer (there should only be
one inserted)?

[zhaobin, 2017/01/12 03:02:12]

Done.

   }

[mark a. foltz, 2017/01/11 00:06:02]

Can you add a DCHECK() that one observer was actually inserted at the end of the
loop?

[zhaobin, 2017/01/12 03:02:12]

Done.

-  status_it->second->availability_observers.insert(observer);
-  UpdateListeningState(status_it->second.get());
+
+  for (const auto& availabilityUrl : urls)
+    StartListening(availabilityUrl);
 }
 
 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;
+  std::vector<GURL> urls;
+  for (const auto& url : observer->urls())
+    urls.push_back(url);
+
+  for (auto& status : availability_set_) {
+    if (status->urls == urls)
+      status->availability_observers.erase(observer);

[mark a. foltz, 2017/01/11 00:06:02]

Do you need to break after finding the matching status?

[zhaobin, 2017/01/12 03:02:12]

Done.

   }

[mark a. foltz, 2017/01/11 00:06:02]

Can you add a DCHECK that exactly one observer was erased?

[zhaobin, 2017/01/12 03:02:12]

Done.

-  status_it->second->availability_observers.erase(observer);
-  UpdateListeningState(status_it->second.get());
+
+  for (const auto& availabilityUrl : urls)
+    StopListening(availabilityUrl);
 }
 
 void PresentationDispatcher::setDefaultPresentationUrls(
@@ -351,47 +376,81 @@ void PresentationDispatcher::OnDestruct() {
 
 void PresentationDispatcher::OnScreenAvailabilityUpdated(const GURL& url,
                                                          bool available) {
-  auto status_it = availability_status_.find(url);
-  if (status_it == availability_status_.end())
+  auto* listening_status = GetListeningStatus(url);
+  if (!listening_status)
     return;
-  AvailabilityStatus* status = status_it->second.get();
-  DCHECK(status);
 
-  if (status->listening_state == ListeningState::WAITING)
-    status->listening_state = ListeningState::ACTIVE;
+  if (listening_status->listening_state == ListeningState::WAITING)
+    listening_status->listening_state = ListeningState::ACTIVE;
+
+  auto new_screen_availability = available ? ScreenAvailability::AVAILABLE
+                                           : ScreenAvailability::UNAVAILABLE;
+  if (listening_status->last_known_availability == new_screen_availability)
+    return;
 
-  for (auto* observer : status->availability_observers)
-    observer->availabilityChanged(available);
+  listening_status->last_known_availability = new_screen_availability;
 
-  for (AvailabilityCallbacksMap::iterator iter(&status->availability_callbacks);
-       !iter.IsAtEnd(); iter.Advance()) {
-    iter.GetCurrentValue()->onSuccess(available);
+  for (auto& status : availability_set_) {
+    if (!base::ContainsValue(status->urls, url))
+      continue;
+    auto screen_availability = GetScreenAvailability(status->urls);
+    // screen_availability cannot be UNKNOWN or UNSUPPORTED here

[mark a. foltz, 2017/01/11 00:06:02]

DCHECK(availability == AVAILABLE || availability == UNAVAILABLE)

[zhaobin, 2017/01/12 03:02:12]

Done.

+    for (auto* observer : status->availability_observers) {
+      observer->availabilityChanged(screen_availability ==
+                                    ScreenAvailability::AVAILABLE);
+    }
+
+    for (AvailabilityCallbacksMap::iterator iter(
+             &status->availability_callbacks);
+         !iter.IsAtEnd(); iter.Advance()) {
+      iter.GetCurrentValue()->onSuccess(screen_availability ==
+                                        ScreenAvailability::AVAILABLE);
+    }
+    status->availability_callbacks.Clear();
   }
-  status->last_known_availability = available;
-  status->availability_callbacks.Clear();
-  UpdateListeningState(status);
 }
 
 void PresentationDispatcher::OnScreenAvailabilityNotSupported(const GURL& url) {
-  auto status_it = availability_status_.find(url);
-  if (status_it == availability_status_.end())
+  auto* listening_status = GetListeningStatus(url);
+  if (!listening_status)
     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.");
-  for (AvailabilityCallbacksMap::iterator iter(&status->availability_callbacks);
-       !iter.IsAtEnd(); iter.Advance()) {
-    iter.GetCurrentValue()->onError(blink::WebPresentationError(
-        blink::WebPresentationError::ErrorTypeAvailabilityNotSupported,
-        not_supported_error));
+
+  if (listening_status->listening_state == ListeningState::WAITING)
+    listening_status->listening_state = ListeningState::ACTIVE;
+
+  if (listening_status->last_known_availability ==
+      ScreenAvailability::UNSUPPORTED)
+    return;
+
+  listening_status->last_known_availability = ScreenAvailability::UNSUPPORTED;
+
+  for (auto& status : availability_set_) {
+    if (!base::ContainsValue(status->urls, url))
+      continue;
+
+    // ScreenAvailabilityNotSupported should be a browser side setting, which
+    // means all urls in PresentationAvailability should report NotSupported.
+    // It is not possible to change listening status from Available or
+    // Unavailable to NotSupported. No need to update observer.
+    auto screen_availability = GetScreenAvailability(status->urls);
+    DCHECK_EQ(screen_availability, ScreenAvailability::UNSUPPORTED);

[mark a. foltz, 2017/01/11 00:06:02]

What if a different URL in |status->urls| was already AVAILABLE?

[zhaobin, 2017/01/12 03:02:12]

Is it possible to have some URLs report AVAILABLE and some report UNSUPPORTED?
If so, shall we wait for all status before rejecting promise as UNSUPPORTED?

+
+    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.");
+    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();
+
+    for (const auto& availability_url : status->urls)
+      StopListening(availability_url);
   }
-  status->availability_callbacks.Clear();
-  UpdateListeningState(status);
 }
 
 void PresentationDispatcher::OnDefaultSessionStarted(
@@ -499,22 +558,80 @@ void PresentationDispatcher::ConnectToPresentationServiceIfNeeded() {
   presentation_service_->SetClient(binding_.CreateInterfacePtrAndBind());
 }
 
-void PresentationDispatcher::UpdateListeningState(AvailabilityStatus* status) {
-  bool should_listen = !status->availability_callbacks.IsEmpty() ||
-                       !status->availability_observers.empty();
-  bool is_listening = status->listening_state != ListeningState::INACTIVE;
+void PresentationDispatcher::SetPresentationServiceForTest(
+    blink::mojom::PresentationServicePtr presentation_service) {
+  presentation_service_ = std::move(presentation_service);
+}
+
+void PresentationDispatcher::StartListening(const GURL& url) {

[mark a. foltz, 2017/01/11 00:06:02]

It's potentially confusing to have startListening and StartListening methods.
Since the ones above are determined by the WebPresentationClient API, can you
rename these two?

[zhaobin, 2017/01/12 03:02:12]

Done.

+  auto* listening_status = GetListeningStatus(url);
+  if (!listening_status) {
+    listening_status = new ListeningStatus(url);
+    listening_status_[url] = base::WrapUnique(listening_status);

[mark a. foltz, 2017/01/11 00:06:02]

Nit: Use .insert() to avoid an extra allocation.

When are entries removed from listening_status_?

[zhaobin, 2017/01/12 03:02:12]

Done.

+  }
 
-  if (should_listen == is_listening)
+  // Already listening.
+  if (listening_status->listening_state != ListeningState::INACTIVE)
     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);
+  listening_status->listening_state = ListeningState::WAITING;
+  presentation_service_->ListenForScreenAvailability(url);
+}
+
+void PresentationDispatcher::StopListening(const GURL& url) {
+  for (const auto& status : availability_set_) {
+    if (!base::ContainsValue(status->urls, url))
+      continue;
+
+    // URL is still observed by some availability object.
+    if (!status->availability_callbacks.IsEmpty() ||
+        !status->availability_observers.empty())
+      return;
   }
+
+  auto* listening_status = GetListeningStatus(url);
+  DCHECK(listening_status);

[mark a. foltz, 2017/01/11 00:06:02]

This means that StopListening was called on a URL that StartListening was never
called on, or was called twice on the same URL, correct?

[zhaobin, 2017/01/12 03:02:12]

It means StartListening was never called on. Calling StopListening twice is
fine, since we do not remove url listening status after StopListening.

+
+  if (listening_status->listening_state == ListeningState::INACTIVE)
+    return;
+
+  ConnectToPresentationServiceIfNeeded();
+  listening_status->listening_state = ListeningState::INACTIVE;
+  presentation_service_->StopListeningForScreenAvailability(url);
+}
+
+PresentationDispatcher::ListeningStatus*
+PresentationDispatcher::GetListeningStatus(const GURL& url) {
+  ListeningStatus* status = nullptr;
+  auto status_it = listening_status_.find(url);
+  if (status_it != listening_status_.end())
+    status = status_it->second.get();
+  return status;
+}
+
+PresentationDispatcher::ScreenAvailability
+PresentationDispatcher::GetScreenAvailability(const std::vector<GURL>& urls) {
+  std::map<ScreenAvailability, size_t> counters;

[mark a. foltz, 2017/01/11 00:06:02]

Does this initialize the values to zero?

[zhaobin, 2017/01/12 03:02:12]

Yes.

+
+  for (const auto& url : urls) {
+    if (!base::ContainsKey(listening_status_, url)) {
+      counters[ScreenAvailability::UNKNOWN]++;
+    } else {
+      auto url_availability = listening_status_[url]->last_known_availability;
+      counters[url_availability]++;
+    }
+  }
+
+  if (counters[ScreenAvailability::AVAILABLE] > 0)
+    return ScreenAvailability::AVAILABLE;
+  if (counters[ScreenAvailability::UNSUPPORTED] > 0)
+    return ScreenAvailability::UNSUPPORTED;
+  if (counters[ScreenAvailability::UNAVAILABLE] > 0)
+    return ScreenAvailability::UNAVAILABLE;
+
+  DCHECK_EQ(counters[ScreenAvailability::UNKNOWN], urls.size());
+  return ScreenAvailability::UNKNOWN;
 }
 
 PresentationDispatcher::SendMessageRequest::SendMessageRequest(
@@ -565,12 +682,17 @@ PresentationDispatcher::CreateSendBinaryMessageRequest(
 }
 
 PresentationDispatcher::AvailabilityStatus::AvailabilityStatus(
+    const std::vector<GURL>& availability_urls)
+    : urls(availability_urls) {}
+
+PresentationDispatcher::AvailabilityStatus::~AvailabilityStatus() {}
+
+PresentationDispatcher::ListeningStatus::ListeningStatus(
     const GURL& availability_url)
     : url(availability_url),
-      last_known_availability(false),
+      last_known_availability(ScreenAvailability::UNKNOWN),
       listening_state(ListeningState::INACTIVE) {}
 
-PresentationDispatcher::AvailabilityStatus::~AvailabilityStatus() {
-}
+PresentationDispatcher::ListeningStatus::~ListeningStatus() {}
 
 }  // namespace content