Closed Bug 1210340 Opened 4 years ago Closed 4 years ago

[Presentation WebAPI] Support close semantics

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
blocking-b2g 2.6+
Tracking Status
b2g-v2.6 --- fixed
firefox50 --- fixed

People

(Reporter: selin, Assigned: kershaw)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [ft:conndevices] [ETA 6/30])

Attachments

(3 files, 4 obsolete files)

As per bug 1205219 comment 3, supporting |close| method is an advanced functionality for power saving, but it's not planned to be used in any of the TV 2.5 user story.

Besides, it needs extra efforts to send control message between session end points over the app-to-app transport channel.
In latest spec (Nov 13th 2015), |connection.close()| will only close connection but |connection.terminate()| will also close the receiver app. The implementation of |terminate| in m-c is actually the |close| is spec. We need to swap the implementation to align with latest spec.

https://w3c.github.io/presentation-api/#closing-a-presentationconnection
What is the use case for .close() in the spec?
blocking-b2g: --- → 2.6?
blocking-b2g: 2.6? → 2.6+
Whiteboard: [ft:conndevices] → [ft:conndevices] [ETA 6/30]
(In reply to Olli Pettay [:smaug] from comment #2)
> What is the use case for .close() in the spec?

1. Controller page on mobile device can close the communication channel for energy saving.
2. For many-to-one session it allows the controller to leave the session without closing the receiver page.
Two things need to be covered in this bug.
1. trigger SessionTransport.close
2. address bug 1258600 comment #17
Note that there are somethings need to be done, but not covered in this patch.
1. Add connection back into loadgroup when the connection is resumed.
2. Sending the close reason to the other side for indicating the session is closed or terminated [1]. Not sure if this should be covered in this bug.

[1] https://wiki.mozilla.org/WebAPI/PresentationAPI:Protocol_Draft#Close_Connection
Assignee: nobody → kechang
Attachment #8765723 - Flags: feedback?(schien)
Comment on attachment 8765723 [details] [diff] [review]
Implement PresentationConnection.close

Review of attachment 8765723 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/PresentationConnection.cpp
@@ +158,5 @@
>    }
>  
>    nsresult rv = service->SendSessionMessage(mId, mRole, aData);
>    if(NS_WARN_IF(NS_FAILED(rv))) {
> +    aRv.Throw(rv);

Directly throwing low-level error to WebAPI seems odd to me.

@@ +204,5 @@
>  
> +  nsresult rv = service->TerminateSession(mId, mRole);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    aRv.Throw(rv);
> +  }

IMO we should not throw exception at this point. Browser should take care of any low-level exception happened during termination procedure.
Attachment #8765723 - Flags: feedback?(schien)
> 
> @@ +204,5 @@
> >  
> > +  nsresult rv = service->TerminateSession(mId, mRole);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    aRv.Throw(rv);
> > +  }
> 
> IMO we should not throw exception at this point. Browser should take care of
> any low-level exception happened during termination procedure.

So, we should just ignore the result here?
What do we have to do if UA fails to terminate a session?
Flags: needinfo?(schien)
(In reply to Kershaw Chang [:kershaw] from comment #7)
> > 
> > @@ +204,5 @@
> > >  
> > > +  nsresult rv = service->TerminateSession(mId, mRole);
> > > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > > +    aRv.Throw(rv);
> > > +  }
> > 
> > IMO we should not throw exception at this point. Browser should take care of
> > any low-level exception happened during termination procedure.
> 
> So, we should just ignore the result here?
> What do we have to do if UA fails to terminate a session?

We can guarantee as least local session will be terminate no matter what happened.
Flags: needinfo?(schien)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #8)
> (In reply to Kershaw Chang [:kershaw] from comment #7)
> > > 
> > > @@ +204,5 @@
> > > >  
> > > > +  nsresult rv = service->TerminateSession(mId, mRole);
> > > > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > > > +    aRv.Throw(rv);
> > > > +  }
> > > 
> > > IMO we should not throw exception at this point. Browser should take care of
> > > any low-level exception happened during termination procedure.
> > 
> > So, we should just ignore the result here?
> > What do we have to do if UA fails to terminate a session?
> 
> We can guarantee as least local session will be terminate no matter what
> happened.

PresentationSessionInfo::Close basically always returns NS_OK, so I think we can just ignore the return value from PresentationService::TerminateSession.
Address the previous comment.
Attachment #8765723 - Attachment is obsolete: true
Attachment #8767891 - Flags: feedback?(schien)
Comment on attachment 8767891 [details] [diff] [review]
Implement PresentationConnection.close - V2

Review of attachment 8767891 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/PresentationConnection.cpp
@@ +262,5 @@
> +      nsresult rv = RemoveFromLoadGroup();
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }
> +

We still want to fire corresponding event even if RemoveFromLoadGroup is failed. Either use assertion or adjust the execute order.
Attachment #8767891 - Flags: feedback?(schien) → feedback+
Address the previous comment.
Attachment #8767891 - Attachment is obsolete: true
Attachment #8768285 - Flags: review?(bugs)
Attachment #8768285 - Flags: review?(bugs) → review+
Sorry for the delay of tests.

This patch changes to test onclose in all test cases.
Test for terminating a session will be covered in bug 1276378.
Attachment #8769554 - Flags: review?(bugs)
Attachment #8769554 - Flags: review?(bugs) → review+
rebase
Attachment #8768285 - Attachment is obsolete: true
Attachment #8769554 - Attachment is obsolete: true
Attachment #8770428 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/733ebbaf1df6
Implement PresentationConnection.close(), r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/593b94c92120
Replace onterminate with onclose in test cases, r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/733ebbaf1df6
https://hg.mozilla.org/mozilla-central/rev/593b94c92120
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): presentation api
User impact if declined: n/a
Testing completed: manual test
Risk to taking this patch (and alternatives if risky): n/a
String or UUID changes made by this patch:n/a
Attachment #8771230 - Flags: approval-mozilla-b2g48?
Hi Josh,

Could you please approve the pull request in comment#19?

Thanks.
Flags: needinfo?(jocheng)
Comment on attachment 8771230 [details] [review]
pull request for tv 2.6

Approved fro TV 2.6
Flags: needinfo?(jocheng)
Attachment #8771230 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Gary, please help to merge. Thanks.
Flags: needinfo?(xeonchen)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.