1205219 - [Presentation WebAPI] Support terminate semantics

Status: RESOLVED FIXED

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

Description

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

Latest spec support |close| and |terminate| semantics to allow only closing session transport or terminating the session.

Comment 1

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

Hi Josh,

Could you help mark 2.5+ to this one?

Comment 2

[Josh Cheng [:josh]]

Hi Sean,
Can you provide update for the current status? Thanks!

Comment 3

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

There are two tasks for this bug:

1. for supporting terminate: We already implement the behavior in |PresentationSession.close| but need to modify the function name to |PresentationSession.terminate| in WebIDL.

2. for supporting close: Need to send control message between session end points over the app-to-app transport channel.

|close| semantic is an advanced functionality for power saving, but it's not planned to be used in any of the TV 2.5 user story. I'll suggest that we do task #1 in TV 2.5 scope and leave task #2 post 2.5.

Comment 4

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

Attachment: Part 1 - WebIDL & implementation changes, v1

According to comment 3, |close| hasn't been fully implemented in this patch. (NS_ERROR_NOT_IMPLEMENTED will be thrown when |close| gets called.)

Comment 5

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

Attachment: Part 2 - Tests, v1

Comment 6

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

Sorry about the delay. I'll try to review the patches tomorrow.

Comment 7

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

Comment on attachment 8668357 [details] [diff] [review]
Part 1 - WebIDL & implementation changes, v1

>+PresentationSession::Close(ErrorResult& aRv)
> {
>-  // Closing does nothing if the session is already terminated.
>-  if (NS_WARN_IF(mState == PresentationSessionState::Terminated)) {
>+  // It only works when the state is CONNECTED.
>+  if (NS_WARN_IF(mState != PresentationSessionState::Connected)) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return;
>+  }
This method should not throw here. Per spec it just queues a task, and even that task
"If the presentation connection state of connection is not connected, then abort these steps. "
So just return early if the state isn't connected.

>+PresentationSession::Terminate(ErrorResult& aRv)
>+{
>+  // It only works when the state is CONNECTED.
>+  if (NS_WARN_IF(mState != PresentationSessionState::Connected)) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>     return;
>   }
Same here.


>+++ b/dom/presentation/interfaces/nsIPresentationService.idl
>@@ -58,21 +58,28 @@ interface nsIPresentationService : nsISu
update uuid

>diff --git a/dom/webidl/PresentationSession.webidl b/dom/webidl/PresentationSession.webidl
>--- a/dom/webidl/PresentationSession.webidl
>+++ b/dom/webidl/PresentationSession.webidl
>@@ -5,65 +5,70 @@
>  */
> 
> enum PresentationSessionState
This seems to be PresentationConnectionState in the current spec and ..

> [Pref="dom.presentation.enabled",
>  CheckAnyPermissions="presentation"]
> interface PresentationSession : EventTarget {
... this interface is called PresentationConnection in the current spec.
Could you file a followup bug to change the name.



>   /*
>-   * Both the requesting page and the presenting page can close the session by
>-   * calling terminate(). Then, the session is destroyed and its state is
>-   * truned into "terminated". After getting into the state of "terminated",
>-   * resumeSession() is incapable of re-establishing the connection.
>+   * Both the controlling and receving browsing context can close the session.
>+   * Then, the session state should turn into "closed".
>    *
>-   * This function does nothing if the state has already been "terminated".
>+   * This function only works when the state is not "connected".
>    */
>+  [Throws]
>   void close();
Could we rather just comment this out for now so that we don't expose unimplemented functions to JS.
The C++ implementation can stay as it is.

Comment 8

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

Attachment: Part 1 - WebIDL & implementation changes, v2

Updating based on r+ comments.

Comment 9

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

Attachment: Part 2 - Tests, v1

Comment 10

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

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

Comment 11

[Carsten Book [:Tomcat]]

Hi Sean, i got:

remote: *************************** ERROR ***************************
remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIPresentationSessionListener in changeset 986ac78a1ea2
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIPresentationSessionListener
remote: 
remote: If you intentionally want to keep the current UUID, include
remote: 'IGNORE IDL' in the commit message.
remote: *************************************************************

when i tried to apply this. Is this expected ?

Comment 12

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

Attachment: Part 1 - WebIDL & implementation changes, v3

Updating UUID.

Comment 13

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

(In reply to Carsten Book [:Tomcat] from comment #11)
> Hi Sean, i got:
> 
> remote: *************************** ERROR ***************************
> remote: Push rejected because the following IDL interfaces were
> remote: modified without changing the UUID:
> remote:   - nsIPresentationSessionListener in changeset 986ac78a1ea2
> remote: 
> remote: To update the UUID for all of the above interfaces and their
> remote: descendants, run:
> remote:   ./mach update-uuids nsIPresentationSessionListener
> remote: 
> remote: If you intentionally want to keep the current UUID, include
> remote: 'IGNORE IDL' in the commit message.
> remote: *************************************************************
> 
> when i tried to apply this. Is this expected ?
Sorry, I just updated the UUID in the new patch.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7acef81a1e90
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb22083aaf0

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/7acef81a1e90
https://hg.mozilla.org/mozilla-central/rev/3cb22083aaf0