1301259 - [Presentation WebAPI] close the existing transport first before reconnect while in connected state.

Status: RESOLVED FIXED

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

Description

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

https://w3c.github.io/presentation-api/#reconnecting-to-a-presentation

We should close the existing session transport before initiating reconnect procedure while the session info is already in connected state.

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: patch - v1

Summary:
1. Close the connected connection during reconnect phase
2. Introduce PresentationSessionTransportIPC to close the data channel in the content process.
3. Separate |mSessionInfos| in PresentationIPCService into two tables for controller and receiver.

Please have a look. Thanks.

Comment 2

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

Comment on attachment 8791111 [details] [diff] [review]
patch - v1

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

::: dom/presentation/PresentationCallbacks.cpp
@@ +112,5 @@
>    if (mConnection) {
> +    mConnection->NotifyStateChange(
> +      mSessionId,
> +      nsIPresentationSessionListener::STATE_CONNECTING,
> +      NS_OK);

Need update state to |CLOSED| in NotifyError.

::: dom/presentation/PresentationService.h
@@ +26,5 @@
>  
> +class PresentationService final
> +  : public nsIPresentationService
> +  , public nsIObserver
> +  , public PresentationServiceBase<PresentationSessionInfo>

nit: align the longest one to column 80.

::: dom/presentation/PresentationServiceBase.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_PresentationServiceBase_h
>  #define mozilla_dom_PresentationServiceBase_h
>  
> +#include "nsIPresentationService.h"

nit: follow alphabetical order of file name.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +522,5 @@
>    if (mState != nsIPresentationSessionListener::STATE_CONNECTING) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if (NS_WARN_IF(!transport)) {

s/transport/aTransport

@@ +1042,5 @@
> +  // If |mState| is connecting, just close the connection and start
> +  // doing reconnect.
> +  if (mState == nsIPresentationSessionListener::STATE_CONNECTING) {
> +    NS_WARN_IF(NS_FAILED(
> +      Close(NS_OK, nsIPresentationSessionListener::STATE_CLOSED)));

You'll need to pending reconnect procedure until |NotifyDisconnect|.

@@ +1253,5 @@
>      return rv;
>    }
>  
>    // The session transport is managed by content process
> +  if (NS_WARN_IF(!transport)) {

s/transport/aTransport

::: dom/presentation/ipc/PresentationBuilderParent.cpp
@@ +252,3 @@
>  
> +  if (callback) {
> +    callback->NotifyTransportReady();

|NotifyTransportReady| should be called in PresentationSessionTransportIPC::SetCallback.

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: patch - v2

Address the previous comments.

Please take a look. Thanks.

Comment 4

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

Comment on attachment 8794738 [details] [diff] [review]
patch - v2

Would have been nice to split this kind of patch to two. One doing the code move to the base class thingie, and then another patch making functionality changes.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] (reviewing overload) from comment #4)
> Comment on attachment 8794738 [details] [diff] [review]
> patch - v2
> 
> Would have been nice to split this kind of patch to two. One doing the code
> move to the base class thingie, and then another patch making functionality
> changes.

Thanks for the review.
I'll split the patch as you described.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Part1: Refactor of PresentationServiceBase, r=smaug

Carry reviewer's name.

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: Part2: Close the connected connection before reconnecting, r=smaug

Comment 8

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cc10782187c

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c572919bea35
Part1: Move session info structures to PresentationServiceBase class, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8797c707348
Part2: Close the connected/connecting connection before starting reconnecting, r=smaug

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c572919bea35
https://hg.mozilla.org/mozilla-central/rev/f8797c707348