PresentationConnectionClosedEvent is introduced to identify the reason of connection closing.

Looks like the event interface could be implemented using event codegen. Just add the .webidl interface and then the file to GENERATED_EVENTS_WEBIDL_FILES list in moz.build. After building .cpp file will be in <objdir>/dom/bindings/<the_new_event>.cpp and .h file in <objdir>/dist/include/mozilla/dom/<the_new_event>.h Filed https://github.com/w3c/presentation-api/issues/273 I assume .message should have default value "" in dictionary.

(I'm marking this as "fixlater" but feel free to change that if you're going to work on this sooner (or later :), SC)

First version. Please take a quick look. Thanks.

Comment on attachment 8752026 [details] [diff] [review] Implement PresentationConnectionClosedEvent - v1 Review of attachment 8752026 [details] [diff] [review]: ----------------------------------------------------------------- We'll need to send the close reason to remote device to implement the wentaway behavior. If it is not going to be done in this bug, you'll need to define the scope in bugzilla comment and file a dependent bug. ::: dom/presentation/PresentationConnection.cpp @@ +110,5 @@ > + mState = PresentationConnectionState::Closed; > + DispatchConnectionClosedEvent(PresentationConnectionClosedReason::Wentaway, > + EmptyString()); > + } > + This modification doesn't matched with https://w3c.github.io/presentation-api/#closing-a-presentationconnection. For page that navigated away it should shutdown the connection with reason "wentaway" (so that remote page will receive close event with reason "wentaway") but not dispatch close event locally. @@ +217,5 @@ > } > > mState = state; > > + if (mState == PresentationConnectionState::Connected) { We can move the entire section into another function. @@ +237,5 @@ > + message))) { > + mozilla::GetErrorName(aReason, message); > + message.InsertLiteral("Internal error: ", 0); > + } > + CopyASCIItoUTF16(message, errorMsg); should use |CopyUTF8toUTF16|. @@ +291,5 @@ > return DispatchMessageEvent(jsData); > } > > nsresult > +PresentationConnection::DispatchStateChangeEvent(const nsAString& aEventType) Should either change the function name to something meaningful or use AsyncEventDispatcher directly without this helper function.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4) > Comment on attachment 8752026 [details] [diff] [review] > Implement PresentationConnectionClosedEvent - v1 > > Review of attachment 8752026 [details] [diff] [review]: > ----------------------------------------------------------------- > > We'll need to send the close reason to remote device to implement the > wentaway behavior. If it is not going to be done in this bug, you'll need to > define the scope in bugzilla comment and file a dependent bug. > According to [1], when receiving a signal from the destination browsing context that a connection is going to be closed, the close reason can be either closed or wentaway. So, I plan to just close the transport channel when the PresentationConnection is discarded by browser. At the other side, the destination browser can only close the connection with "closed" reason. [1] https://w3c.github.io/presentation-api/#interface-presentationconnection

Rebase on bug 1258602.

1. Replace |onstatechange| for current test cases. 2. Add a new test for testing when a connection is went away.

Comment on attachment 8754333 [details] [diff] [review] Part3: Test case changes Review of attachment 8754333 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/presentation/tests/mochitest/test_presentation_1ua_connection_wentaway.js @@ +138,5 @@ > + aResolve(); > + }; > + gScript.addMessageListener('ready-to-remove-receiverFrame', function onReadyToRemove() { > + gScript.removeMessageListener('ready-to-remove-receiverFrame', onReadyToRemove); > + document.body.removeChild(receiverIframe); It seems this is the only way to trigger |DisconnectFromOwner| at receiver side. I tried to modify receiverFrame.src, but not working.

Comment on attachment 8754332 [details] [diff] [review] Part2: Implement onconnect, onclose and onterminate event handlers - v2 Review of attachment 8754332 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/presentation/PresentationConnection.cpp @@ +116,5 @@ > + > + nsresult rv = service->CloseSession( > + mId, mRole, nsIPresentationService::CLOSED_REASON_WENTAWAY); > + NS_WARN_IF(NS_FAILED(rv)); > + According to spec, when the browsing context was navigated or was discarded, we should close the connection with the reason wentaway. Is DisconnectFromOwner a good place for closing the connection? Or should I listen to OnLocationChange?

Do we somewhere block pages which use presentation API to enter bfache? If not, then DisconnectFromOwner certainly won't catch "browsing context was navigated" case. Could presentation session extend nsIRequest and add itself to relevant loadgroup, and then when loadgroup is "stopped", it would get a notification.

Comment on attachment 8754332 [details] [diff] [review] Part2: Implement onconnect, onclose and onterminate event handlers - v2 So I think nsIRequest approach probably the easiest for this stuff. It also deals with bfcache automatically, http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=1fcf8a5711ed&mark=8888-8888,8908-8943#8888 so that if there are requests in the loadgroup, we don't enter to bfcache.

Make PresentationConnection to extend nsIRequest and add itself to the load group.

Change receiver frame's src to test wentaway.

Comment on attachment 8754737 [details] [diff] [review] Part2: Implement onconnect, onclose and onterminate event handlers - v3 >+NS_IMETHODIMP >+PresentationConnection::Cancel(nsresult aStatus) >+{ >+ return ProcessConnectionWentAway(); Could we process ProcessConnectionWentAway asynchronously. Documentation for nsIRequest::Cancel is pretty strict that nothing unexpected should happen inside the method. So, nsCOMPtr<nsIRunnable> event = NewRunnableMethod(this, &PresentationConnection::ProcessConnectionWentAway); NS_DispatchToCurrentThread(ev); Ok, I see PresentationConnection::Close() hasn't been implemented yet, since I'd expect that to close the connection properly and remove the connection from loadgroup. I guess that will happen in some other bug.

Comment on attachment 8754738 [details] [diff] [review] Part3: Test case changes Mostly rs+

Not necessarily about this bug, but asking anyhow (since the loadgroup->AddRequest reminded me) Is it defined somewhere what kind of lifetime management PresentationConnection should have? I mean, if it doesn't have any event listeners and JS doesn't have a reference to it, should the connection be closed automatically and GC/CC collect the object? Loadgroup does keep a strong reference to the requests, so the patches do effectively mean that connecion stays alive as long as the page is alive. Maybe that is ok?

(In reply to Olli Pettay [:smaug] from comment #19) > Not necessarily about this bug, but asking anyhow (since the > loadgroup->AddRequest reminded me) > Is it defined somewhere what kind of lifetime management > PresentationConnection should have? > I mean, if it doesn't have any event listeners and JS doesn't have a > reference to it, should the connection be closed automatically and GC/CC > collect the object? > It seems that the lifetime of PresentationConnection is not defined in spec or any other places. Maybe S.C. knows? > Loadgroup does keep a strong reference to the requests, so the patches do > effectively mean that connecion stays alive as long as the page is alive. > Maybe that is ok? I think if the PresentationConnection is not used anywhere in js, it should be released automatically. Would it be better if we do loadgroup->AddRequest when the connection becomes closed or terminated?

I think there are two things to consider to do. These are not either-or, but separate things. (1) WebSocket for example keeps itself alive if there are event listeners for message or close events (you may want to read what HTML spec says about it). See also WebSocket::UpdateMustKeepAlive(). PresentationConnection could do something similar. I think the same setup is being used also elsewhere... was it in WebRTC or WebAudio or where... (2) To not depend on loadgroup's strong references, I would call AddRequest when the connection is created, but to effectively make the loadgroug->connection edge "weak", in Traverse method do something like NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "self"); cb.NoteXPCOMChild(tmp); whenever we haven't yet called RemoveRequest. That way cycle collector wouldn't need to know about the loadgroup->connection edge, since it has this artificial "self" edge.

(In reply to Olli Pettay [:smaug] from comment #21) > I think there are two things to consider to do. These are not either-or, but > separate things. > > (1) WebSocket for example keeps itself alive if there are event listeners > for message or close events (you may want to read what HTML spec says about > it). See also WebSocket::UpdateMustKeepAlive(). PresentationConnection could > do something similar. > I think the same setup is being used also elsewhere... was it in WebRTC or > WebAudio or where... > > > (2) To not depend on loadgroup's strong references, I would call AddRequest > when the connection is created, but to effectively make the > loadgroug->connection edge "weak", in Traverse method > do something like > NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "self"); > cb.NoteXPCOMChild(tmp); > whenever we haven't yet called RemoveRequest. > That way cycle collector wouldn't need to know about the > loadgroup->connection edge, since it has this artificial "self" edge. Thanks for the suggestions. If you agree, I would like to file a new bug for doing this.

The lifecycle of PresentationConnection might be different between controller and receiver. For controller, the lifecycle should be similar to WebSocket since it is initiated by controller page. Closing connection if no JS reference to it sounds reasonable. For receiver, the connection is associated with a global `receiver` object. Therefore, connection should be kept as long as the page is alive.

Rebase and carry r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/47a6b93fbd16 https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3fef6d6d9c https://hg.mozilla.org/integration/mozilla-inbound/rev/b7836822d084

https://hg.mozilla.org/mozilla-central/rev/47a6b93fbd16 https://hg.mozilla.org/mozilla-central/rev/3f3fef6d6d9c https://hg.mozilla.org/mozilla-central/rev/b7836822d084

Please help to approve. Thanks. 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: PresentationConnnectionClosedEvent would not be available. Testing completed: with automation test Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: n/a

Comment on attachment 8757977 [details] [diff] [review] Part1: Add PresentationConnectionClosedEvent, r=smaug Approve for TV 2.6

Comment on attachment 8757978 [details] [diff] [review] Part2: Implement onconnect, onclose and onterminate event handlers, r=smaug Approve for TV 2.6

Comment on attachment 8757979 [details] [diff] [review] Part3: Test case changes, r=smaug Approve for TV 2.6