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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

This bug is for supporting PresentationRequest / PresentationAvailability / getSession(s) in the latest editor's draft.

Comment 1

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings. (WIP)

Comment 2

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Change notification & event dispatching. (WIP)

Comment 3

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 3 - Adjust errors. (WIP)

Comment 4

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 4 - Tests. (WIP)

Comment 5

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings, v1

Updating the API according to the latest spec (except for |join()| since it's left to bug 1195605).
http://w3c.github.io/presentation-api/

Comment 6

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Change notification & event dispatching, v1

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Filed https://github.com/w3c/presentation-api/issues/184

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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 10

[Olli Pettay [:smaug][[e-mail]]]

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 :)

Comment 11

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(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.

Comment 12

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings, v2

Updating based on r+ comments.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(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.)

Comment 14

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Change notification & event dispatching, diff-v1-v2

Comment 15

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings, v3

Use 'required' for PresentationSessionConnectEventInit.
https://github.com/w3c/presentation-api/issues/184

Comment 16

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Change notification & event dispatching, v2

The full version of r+ Part 2 patch.

Comment 17

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 3 - Adjust errors, v1

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 19

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 3 - Adjust errors, v2

Prevent to use NS_ERROR_DOM_NETWORK_ERR which is not mentioned in the spec.

Comment 20

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 4 - Rename PresentationSessionInfo relevant classes

Use more spec-like terminology "controlling browsing context" and "presenting browsing context" instead of "requester" and "responder" for class names to reduce potential confusion.

Comment 21

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 5 - Tests, v1

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 23

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings, v4

Updating with the latest code base.

Comment 24

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Change notification & event dispatching, v3

Updating with the latest code base.

Comment 25

[Sean Lin [:seanlin] (inactive from Mar 2016)]

The try-run. FYI.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d773c0192152

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b07b414b045
https://hg.mozilla.org/integration/mozilla-inbound/rev/4de2bdb67cd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e3a179f4f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe2431a5f08
https://hg.mozilla.org/integration/mozilla-inbound/rev/967ff76a23a3

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0b07b414b045
https://hg.mozilla.org/mozilla-central/rev/4de2bdb67cd1
https://hg.mozilla.org/mozilla-central/rev/c1e3a179f4f3
https://hg.mozilla.org/mozilla-central/rev/2fe2431a5f08
https://hg.mozilla.org/mozilla-central/rev/967ff76a23a3