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?

(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

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.

> > @@ +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?

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

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

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.

Address the previous comment.

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.

rebase

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

https://hg.mozilla.org/mozilla-central/rev/733ebbaf1df6 https://hg.mozilla.org/mozilla-central/rev/593b94c92120

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

Hi Josh, Could you please approve the pull request in comment#19? Thanks.

Comment on attachment 8771230 [details] pull request for tv 2.6 Approved fro TV 2.6

Gary, please help to merge. Thanks.