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

content/renderer/presentation/presentation_dispatcher.h

Index: content/renderer/presentation/presentation_dispatcher.h
diff --git a/content/renderer/presentation/presentation_dispatcher.h b/content/renderer/presentation/presentation_dispatcher.h
index 0693f1363f1731366f0e61ec02b49fc63746b415..8c66c1ccc073055a0ec5a2f2e80652ee79424979 100644
--- a/content/renderer/presentation/presentation_dispatcher.h
+++ b/content/renderer/presentation/presentation_dispatcher.h
@@ -47,6 +47,7 @@ class CONTENT_EXPORT PresentationDispatcher
   ~PresentationDispatcher() override;
 
  private:
+  friend class PresentationDispatcherTest;

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

Takumi is also landing a unit test, so please coordinate with him on the best
way to do that; maybe land this unit test separately after rebasing on his
patch?

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

Sure.

   struct SendMessageRequest {
     SendMessageRequest(blink::mojom::PresentationSessionInfoPtr session_info,
                        blink::mojom::ConnectionMessagePtr message);
@@ -96,7 +97,7 @@ class CONTENT_EXPORT PresentationDispatcher
   void terminateSession(const blink::WebURL& presentationUrl,
                         const blink::WebString& presentationId) override;
   void getAvailability(
-      const blink::WebVector<blink::WebURL>& availabilityUrl,
+      const blink::WebVector<blink::WebURL>& availabilityUrls,
       std::unique_ptr<blink::WebPresentationAvailabilityCallbacks> callbacks)
       override;
   void startListening(blink::WebPresentationAvailabilityObserver*) override;
@@ -140,6 +141,8 @@ class CONTENT_EXPORT PresentationDispatcher
   void HandleSendMessageRequests(bool success);
 
   void ConnectToPresentationServiceIfNeeded();
+  void SetPresentationServiceForTest(
+      blink::mojom::PresentationServicePtr presentation_service);
 
   void UpdateListeningState();
 
@@ -160,20 +163,39 @@ class CONTENT_EXPORT PresentationDispatcher
     ACTIVE,
   };
 
-  using AvailabilityCallbacksMap =
-      IDMap<std::unique_ptr<blink::WebPresentationAvailabilityCallbacks>>;
+  enum class ScreenAvailability {

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

It's pretty much a nit but why not using AvailabilityStatus instead of
ScreenAvailability? It's a bit odd to see "Screen" mentioned as it's not really
mentioned elsewhere. Feel free to ignore :)

[mark a. foltz, 2017/01/05 22:40:49]

There's already an AvailabilityStatus struct declared in this class (@L191).  I
think ScreenAvailability is okay since there needs to be some thing that's
available or not.

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

ScreenAvailability represents if there is any screen (display) available for a
given url. AvailabilityStatus is the status of PresentationAvailability, which
has multiple urls.

Naming is a little confusing, please let me know if you have better suggestions
:)

+    UNKNOWN,
+    AVAILABLE,
+    UNAVAILABLE,
+    UNSUPPORTED
+  };
+
+  struct AvailabilityCallback {
+    AvailabilityCallback(
+        const blink::WebVector<blink::WebURL>& availability_urls,

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

Can this use std::vector<GURL> to be consistent with AvailabilityStatus and the
rest of the implementation in this file?

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

Done.

+        std::unique_ptr<blink::WebPresentationAvailabilityCallbacks>
+            availability_callback);
+    ~AvailabilityCallback();
+
+    blink::WebVector<blink::WebURL> urls;
+    std::unique_ptr<blink::WebPresentationAvailabilityCallbacks> callback;
+  };
+
+  using AvailabilityCallbacksVector =
+      std::vector<std::unique_ptr<AvailabilityCallback>>;
+  using AvailabilityCallbacksSet = std::set<AvailabilityCallback*>;
   using AvailabilityObserversSet =
       std::set<blink::WebPresentationAvailabilityObserver*>;
 
   // Tracks status of presentation displays availability for |availability_url|.
   struct AvailabilityStatus {
-    explicit AvailabilityStatus(const GURL& availability_url);
+    explicit AvailabilityStatus(const GURL& availability_urls);

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

availability_url

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

Done.

     ~AvailabilityStatus();
 
     const GURL url;
-    bool last_known_availability;
+    ScreenAvailability last_known_availability;
     ListeningState listening_state;
-    AvailabilityCallbacksMap availability_callbacks;
+    AvailabilityCallbacksSet availability_callbacks;

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

It's confusing in the code to have both availability_callbacks and
availability_callbacks_.

Can these be renamed status_callbacks and status_observers?

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

Code removed.

     AvailabilityObserversSet availability_observers;
   };
 
@@ -181,10 +203,27 @@ class CONTENT_EXPORT PresentationDispatcher
   std::map<GURL, std::unique_ptr<AvailabilityStatus>>
       availability_status_;
 
+  // Map of AvailabilityCallbacks.
+  AvailabilityCallbacksVector availability_callbacks_;

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

Is this for ownership purposes (i.e. to make sure all callbacks are deleted)?

Can you add a comment explaining why this is tracking callbacks both here and in
AvailabilityStatus?

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

Code removed.

+
   // Updates the listening state of availability for |status| and notifies the
   // client.
   void UpdateListeningState(AvailabilityStatus* status);
 
+  // Called when |status|'s |last_known_availability|, |availability_callbacks|,
+  // or |availability_observers| changes.
+  void UpdateAvailabilityObserversAndCallbacks(AvailabilityStatus* status);
+
+  // Returns UNKNOWN if any url in |urls| has screen availability UNKNOWN;
+  // Returns UNSUPPORTED if any url in |urls| have screen availability
+  // UNSUPPORTED, and no url has screen availability UNKNOWN;
+  // Returns AVAILABLE if at least one url in |urls| has screen availability
+  // AVAILABLE, and no url has screen availability UNKNOWN or UNSUPPORTED;
+  // Returns UNAVAILABLE if all urls in |urls| have screen availability
+  // UNAVAILABLE.
+  PresentationDispatcher::ScreenAvailability GetScreenAvailability(

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

GetAvailabilityStatus()?

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

Cannot use GetAvailabilityStatus since AvailabilityStatus is representing sth
else...

+      const blink::WebVector<blink::WebURL>& urls);

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

std::vector<GURL>

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

Done.

+
   DISALLOW_COPY_AND_ASSIGN(PresentationDispatcher);
 };