Closed Bug 1205219 Opened 4 years ago Closed 4 years ago

[Presentation WebAPI] Support terminate semantics

Categories

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

defect

Tracking

()

RESOLVED FIXED
FxOS-S9 (16Oct)
feature-b2g 2.5+
Tracking Status
firefox44 --- fixed

People

(Reporter: schien, Assigned: selin)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed, Whiteboard: [ft:conndevices][partner-blocker])

Attachments

(2 files, 3 obsolete files)

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

Could you help mark 2.5+ to this one?
Flags: needinfo?(jocheng)
feature-b2g: --- → 2.5+
Flags: needinfo?(jocheng)
Priority: -- → P1
Assignee: nobody → selin
Target Milestone: --- → FxOS-S9 (16Oct)
Status: NEW → ASSIGNED
Whiteboard: [ft:conndevices] → [ft:conndevices][partner-blocker]
Hi Sean,
Can you provide update for the current status? Thanks!
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.
Summary: [Presentation WebAPI] Support close/terminate semantics → [Presentation WebAPI] Support terminate semantics
According to comment 3, |close| hasn't been fully implemented in this patch. (NS_ERROR_NOT_IMPLEMENTED will be thrown when |close| gets called.)
Attachment #8668357 - Flags: review?(bugs)
Attached patch Part 2 - Tests, v1 (obsolete) — Splinter Review
Attachment #8670086 - Flags: review?(bugs)
Sorry about the delay. I'll try to review the patches tomorrow.
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.
Attachment #8668357 - Flags: review?(bugs) → review+
Attachment #8670086 - Flags: review?(bugs) → review+
Updating based on r+ comments.
Attachment #8668357 - Attachment is obsolete: true
Attachment #8671140 - Flags: review+
Attachment #8670086 - Attachment is obsolete: true
Attachment #8671141 - Flags: review+
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 ?
Flags: needinfo?(selin)
Updating UUID.
Attachment #8671140 - Attachment is obsolete: true
Attachment #8671288 - Flags: review+
(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.
Flags: needinfo?(selin)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.