[Presentation WebAPI] support PresentationRequest / PresentationAvailability / getSession(s)

RESOLVED FIXED in Firefox 43

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: schien, Assigned: selin)

Tracking

(Blocks 3 bugs, {dev-doc-needed})

unspecified
FxOS-S8 (02Oct)
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.5+, firefox43 fixed)

Details

(Whiteboard: [ft:conndevices], )

Attachments

(5 attachments, 11 obsolete attachments)

15.81 KB, patch
smaug
: review+
Details | Diff | Splinter Review
20.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
28.71 KB, patch
smaug
: review+
Details | Diff | Splinter Review
45.30 KB, patch
selin
: review+
Details | Diff | Splinter Review
48.37 KB, patch
selin
: review+
Details | Diff | Splinter Review
This bug is for supporting PresentationRequest / PresentationAvailability / getSession(s) in the latest editor's draft.
feature-b2g: --- → 2.5+
Target Milestone: --- → FxOS-S8 (02Oct)
Status: NEW → ASSIGNED
Posted patch Part 1 - WebIDL Bindings, v1 (obsolete) — Splinter Review
Updating the API according to the latest spec (except for |join()| since it's left to bug 1195605).
http://w3c.github.io/presentation-api/
Attachment #8651620 - Attachment is obsolete: true
Attachment #8651621 - Attachment is obsolete: true
Attachment #8651622 - Attachment is obsolete: true
Attachment #8651623 - Attachment is obsolete: true
Attachment #8654722 - Flags: review?(bugs)
Comment on attachment 8654722 [details] [diff] [review]
Part 1 - WebIDL Bindings, v1




(I'm not a fan of PresentationRequest interface. Odd mix of Promise and event handling. I may file some spec bugs)





>+[Constructor(DOMString url),
>+ Pref="dom.presentation.enabled",
>+ CheckAnyPermissions="presentation",
>+ AvailableIn="PrivilegedApps"]
>+interface PresentationRequest : EventTarget {
>+  /*
>+   * A requesting page use start() to start a new session, and the session will
>+   * be returned with the promise. UA may show a prompt box with a list of
>+   * available devices and ask the user to grant permission, choose a device, or
>+   * cancel the operation.
>+   *
>+   * The promise is resolved when the presenting page is successfully loaded and
>+   * the communication channel is established, i.e., the session state is
>+   * "connected".
>+   *
>+   * The promise may be rejected duo to one of the following reasons:
>+   * - "OperationError": Unexpected error occurs.
>+   * - "NotFoundError":  No available device.
>+   * - "AbortError":     User dismiss/cancel the device prompt box.
>+   * - "NetworkError":   Failed to establish the control channel or data channel.
>+   * - "TimeoutError":   Presenting page takes too long to load.
>+   */
>+  [Throws]
>+  Promise<PresentationSession> start();
>+
>+ /*
>+  * UA triggers device discovery mechanism periodically and monitor device
>+  * availability.
>+  *
>+  * The promise may be rejected duo to one of the following reasons:
>+  * - "NotSupportedError": Unable to continuously monitor the availability.
>+  */
>+  [Throws]
>+  Promise<PresentationAvailability> getAvailability();
>+
>+  /*
>+   * It is called when a session associated with a PresentationRequest is created.
>+   * The event is fired for all sessions that are created for the controller.
>+   */
>+  attribute EventHandler onsessionconnect;
>+};
So we're still missing reconnect(). Will that be added in a followup bug?



>+[Constructor(DOMString type,
>+             optional PresentationSessionConnectEventInit eventInitDict),
>+ Pref="dom.presentation.enabled",
>+ CheckAnyPermissions="presentation",
>+ AvailableIn="PrivilegedApps"]
>+interface PresentationSessionConnectEvent : Event
>+{
>+  [SameObject]
>+  readonly attribute PresentationSession session;
>+};
>+
>+dictionary PresentationSessionConnectEventInit : EventInit
>+{
>+  PresentationSession? session = null;
>+};
So the spec has wrong kind of dictionary and/or interface here.
Make session on the interface nullable too and this should be fine.
We can switch to using required later if the spec is changed to that.




This has some TODO, so can't land as such, but I assume TODO are fixed in the followup patches.
Attachment #8654722 - Flags: review?(bugs) → review+
Hmm, is Shutdown handling enough here? The objects which have Shutdown() method are unlinked only if there are no unknown strong references to them. Is that guaranteed? Should we _also_ call Shutdown()
when DOMEventTargetHelper DisconnectFromOwner() is called? (So, override that method and call
Shutdown(); and DOMEventTargetHelper::DisconnectFromOwner()).

Have you tested this with debug builds and XPCOM_MEM_LEAK_LOG environment variable set?

I'm mostly worried about PresentationAvailability::Shutdown() which calls 
service->UnregisterAvailabilityListener(this); and mAvailabilityListeners keeps a strong reference 
to PresentationAvailability. So as far as I see, that leaks.
Comment on attachment 8654723 [details] [diff] [review]
Part 2 - Change notification & event dispatching, v1


>+NS_IMETHODIMP
>+PresentationAvailability::NotifyAvailableChange(bool aIsAvailable)
>+{
>+  mIsAvailable = aIsAvailable;
>+
>+  nsRefPtr<AsyncEventDispatcher> asyncDispatcher =
>+    new AsyncEventDispatcher(this, NS_LITERAL_STRING("change"), false);
>+  return asyncDispatcher->PostDOMEvent();
>+}
Per spec "change" is fired synchronously right after changing the value of
PresentationAvailability.value. Here you do it async.
So, you should have a runnable which changes the value and then synchronously dispatch the event.



>@@ -64,27 +65,27 @@ interface nsIPresentationService : nsISu
update the uuid of this interface 




r-, because of the memory leak and event dispatching, but please upload a diff on top of this patch. Would be easier to review :)
Attachment #8654723 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #8)
> So we're still missing reconnect(). Will that be added in a followup bug?
Yep. We'll do it in bug 1197690.
Posted patch Part 1 - WebIDL Bindings, v2 (obsolete) — Splinter Review
Updating based on r+ comments.
Attachment #8654722 - Attachment is obsolete: true
Attachment #8656472 - Flags: review+
(and happy to review updated patch here. ping me on IRC or send email, or wait until next week when I'll open review queue again.)
Attachment #8656978 - Flags: review?(bugs)
Posted patch Part 1 - WebIDL Bindings, v3 (obsolete) — Splinter Review
Use 'required' for PresentationSessionConnectEventInit.
https://github.com/w3c/presentation-api/issues/184
Attachment #8656472 - Attachment is obsolete: true
Attachment #8657669 - Flags: review+
Attachment #8656978 - Flags: review?(bugs) → review+
The full version of r+ Part 2 patch.
Attachment #8656978 - Attachment is obsolete: true
Attachment #8658126 - Flags: review+
Posted patch Part 3 - Adjust errors, v1 (obsolete) — Splinter Review
Attachment #8658128 - Flags: review?(bugs)
Comment on attachment 8658128 [details] [diff] [review]
Part 3 - Adjust errors, v1

Why NS_ERROR_DOM_NETWORK_ERR anywhere?
I don't see the spec using it.
Would be clearer, IMO, to use NS_ERROR_DOM_* in cases where the error propagates to JS code, and that means using only the error defined in the spec.
Or does the spec miss some error cases? (I wouldn't be surprised)
Attachment #8658128 - Flags: review?(bugs) → review-
Prevent to use NS_ERROR_DOM_NETWORK_ERR which is not mentioned in the spec.
Attachment #8658128 - Attachment is obsolete: true
Attachment #8658618 - Flags: review?(bugs)
Use more spec-like terminology "controlling browsing context" and "presenting browsing context" instead of "requester" and "responder" for class names to reduce potential confusion.
Attachment #8658619 - Flags: review?(bugs)
Attachment #8658618 - Flags: review?(bugs) → review+
Comment on attachment 8658619 [details] [diff] [review]
Part 4 - Rename PresentationSessionInfo relevant classes

Thanks, I think this makes the code a tad easier to follow.

There are still some cases where receiver is used though.
Feel free to fix those in some other bug or while doing other changes.
Attachment #8658619 - Flags: review?(bugs) → review+
Attachment #8658621 - Flags: review?(bugs) → review+
Updating with the latest code base.
Attachment #8657669 - Attachment is obsolete: true
Attachment #8660549 - Flags: review+
Updating with the latest code base.
Attachment #8658126 - Attachment is obsolete: true
Attachment #8660551 - Flags: review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.