1197690 - [Presentation WebAPI] Support resuming a disconnected presentation session

Status: RESOLVED FIXED

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

Description

[Sean Lin [:seanlin] (inactive from Mar 2016)]

+++ This bug was initially created as a clone of Bug #1195605 +++

Support resuming a disconnected presentation session.

Comment 1

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

Corresponding requirement: https://github.com/w3c/presentation-api/blob/gh-pages/uc-req.md#req03-resuming-connection-to-presentation

https://github.com/w3c/presentation-api/issues/170 will define the finalized function name.

Comment 2

[Kershaw Chang [:kershaw]]

Attachment: Part1: Support reconnect command

WIP

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement reconnect

Comment 4

[Junior [inactive]]

Comment on attachment 8770881 [details] [diff] [review]
Part1: Support reconnect command

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

I have a quick scan of this patch, generally good but I'd like to see a rebased version.
Could we have a test in test_tcp_control_channel.js?

::: dom/presentation/interfaces/nsIPresentationControlService.idl
@@ +65,5 @@
> +   */
> +  void onReconnectRequest(in nsITCPDeviceInfo aDeviceInfo,
> +                          in DOMString url,
> +                          in DOMString aPresentationId,
> +                          in nsIPresentationControlChannel aControlChannel);

Please consider a well-organized order of APIs with nsIPresentationControlChannel.idl

::: dom/presentation/provider/LegacyPresentationControlService.js
@@ +238,5 @@
>    },
>  
> +  reconnect: function() {
> +    // Legacy protocol doesn't support extra reconnect protocol.
> +    // Simply close the transport channel.

Please refer to bug 1276378 comment 20

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +873,5 @@
>    return NS_OK;
>  }
>  
> +already_AddRefed<MulticastDNSDeviceProvider::Device>
> +MulticastDNSDeviceProvider::GetOrCreateDevice(nsITCPDeviceInfo* aDeviceInfo)

Annotate the criterion

@@ +882,5 @@
>    RefPtr<Device> device;
>    uint32_t index;
>    if (FindDeviceByAddress(address, index)) {
>      device = mDevices[index];
>    } else {

Make sure that comment changes in bug 1276378 comment 19 is applied.

::: dom/presentation/provider/PresentationControlService.js
@@ +183,5 @@
>    },
>  
> +  onSessionReconnect: function(aDeviceInfo, aUrl, aPresentationId, aControlChannel) {
> +    DEBUG && log("TCPPresentationServer - onSessionReconnect: " +
> +                 aDeviceInfo.address + ":" + aDeviceInfo.port); //jshint ignore:line

nit: space between "//" and jshint

@@ +184,5 @@
>  
> +  onSessionReconnect: function(aDeviceInfo, aUrl, aPresentationId, aControlChannel) {
> +    DEBUG && log("TCPPresentationServer - onSessionReconnect: " +
> +                 aDeviceInfo.address + ":" + aDeviceInfo.port); //jshint ignore:line
> +    this.listener.onReconnectRequest(aDeviceInfo,

Bail out if this.listener is null. Please make sure that the control channel is appropriately release to avoid leakage.

@@ +413,5 @@
>      this._stateMachine.terminate(aPresentationId);
>    },
>  
> +  reconnect: function(aPresentationId, aUrl) {
> +    if (this._direction != "sender") {

Clearly annotate the limitation in idl

@@ +603,5 @@
>      this._listener.notifyOpened();
>    },
>  
> +  _notifyReconnected: function() {
> +    this._listener.notifyReconnected();

Is there any chance that this._listener is null?

@@ +687,5 @@
>  
>    notifyTerminate: function(presentationId) {
>      this._presentationService.onSessionTerminate(this._deviceInfo,
>                                                  presentationId,
>                                                  this);

Here's a nit. I know its not yours.

Comment 5

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

Comment on attachment 8770882 [details] [diff] [review]
Part2: Implement reconnect

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

This patch seems to have many missing part. I only check the reconnect algorithm with W3C spec.

::: dom/presentation/PresentationRequest.cpp
@@ +141,5 @@
>  
>  already_AddRefed<Promise>
> +PresentationRequest::Reconnect(const nsAString& aPresentationId,
> +                               ErrorResult& aRv)
> +{

missing the security check algorithm listed in spec.
please add a comment if you want to delay the implementation along with bug 1254488.

@@ +146,5 @@
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
> +  if (NS_WARN_IF(!global)) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }

First, need to search if there is any matching PresentationConnection in current browsing context. Need to return the same PresentationConnection object if found.

::: dom/presentation/PresentationService.cpp
@@ +709,5 @@
> +  RefPtr<PresentationSessionInfo> info = GetSessionInfo(aSessionId, aRole);
> +  if (NS_WARN_IF(!info)) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +

Need to check URL and state here as well.

::: dom/presentation/ipc/PresentationIPCService.h
@@ -68,5 @@
> -  // to retrieve the correspondent session ID. Besides, also keep the mapping
> -  // between the responding session ID and the window ID to help look up the
> -  // window ID.
> -  nsClassHashtable<nsUint64HashKey, nsString> mRespondingSessionIds;
> -  nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;

These changes are for DataChannel OOP, any particular reason you need to remove them?

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Part1: Support reconnect command - v2

Rebase and address previous comments.

Comment 7

[Junior [inactive]]

Comment on attachment 8771370 [details] [diff] [review]
Part1: Support reconnect command - v2

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

I'd like to hold the r+ mainly because I'd like to see the test again.

::: dom/presentation/interfaces/nsIPresentationControlChannel.idl
@@ +133,5 @@
> +   * Note that only controller is allowed to reconnect a session.
> +   * @param presentationId The Id for representing this session.
> +   * @param url The URL requested to open by remote device.
> +   * @throws NS_ERROR_FAILURE on failure
> +   */

Please let me confirm if the parameters are necessary or not. Could we change the presentationId and url in |reconnect| for a control channel?
If yes, what does the presenter behave?
If no, there's something wrong.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +872,5 @@
>  
>    return NS_OK;
>  }
>  
> +// Create a new device if can not find one by address.

Lack of subject in the "if" sentence. Please rephrase.

::: dom/presentation/provider/ReceiverStateMachine.jsm
@@ +74,5 @@
> +        stateMachine._sendCommand({
> +          type: CommandType.RECONNECT_ACK,
> +          presentationId: command.presentationId
> +        });
> +        break;

Please align with bug 1276378 comment 24

::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
@@ +479,5 @@
> +    },
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationControlChannelListener]),
> +  };
> +}
> +

The setup is dup with |loopOfferAnswer|. Any reason to do this? Could we just merge to loopOfferAnswer?

Comment 8

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #5)
> Comment on attachment 8770882 [details] [diff] [review]
> Part2: Implement reconnect
> 
> Review of attachment 8770882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch seems to have many missing part. I only check the reconnect
> algorithm with W3C spec.
> 

Yes, this is a not completed patch. My intention is to make sure I am on the right track.

> ::: dom/presentation/PresentationRequest.cpp
> @@ +141,5 @@
> >  
> >  already_AddRefed<Promise>
> > +PresentationRequest::Reconnect(const nsAString& aPresentationId,
> > +                               ErrorResult& aRv)
> > +{
> 
> missing the security check algorithm listed in spec.
> please add a comment if you want to delay the implementation along with bug
> 1254488.
> 

Will add a comment. 

> @@ +146,5 @@
> > +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
> > +  if (NS_WARN_IF(!global)) {
> > +    aRv.Throw(NS_ERROR_UNEXPECTED);
> > +    return nullptr;
> > +  }
> 
> First, need to search if there is any matching PresentationConnection in
> current browsing context. Need to return the same PresentationConnection
> object if found.
> 

Yes, I missed that part. It seems that we don't have a list to save all presentation connections in controller. I will think how to do this. 

> ::: dom/presentation/PresentationService.cpp
> @@ +709,5 @@
> > +  RefPtr<PresentationSessionInfo> info = GetSessionInfo(aSessionId, aRole);
> > +  if (NS_WARN_IF(!info)) {
> > +    return NS_ERROR_NOT_AVAILABLE;
> > +  }
> > +
> 
> Need to check URL and state here as well.
> 

This function should be only invoked at controller side. I think we can just use the URL in the session info and don't have to check it again.

> ::: dom/presentation/ipc/PresentationIPCService.h
> @@ -68,5 @@
> > -  // to retrieve the correspondent session ID. Besides, also keep the mapping
> > -  // between the responding session ID and the window ID to help look up the
> > -  // window ID.
> > -  nsClassHashtable<nsUint64HashKey, nsString> mRespondingSessionIds;
> > -  nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;
> 
> These changes are for DataChannel OOP, any particular reason you need to
> remove them?

I just found it seems not used here. I will check again.

Comment 9

[Kershaw Chang [:kershaw]]

(In reply to Junior [:junior] from comment #7)
> Comment on attachment 8771370 [details] [diff] [review]
> Part1: Support reconnect command - v2
> 
> Review of attachment 8771370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to hold the r+ mainly because I'd like to see the test again.
> 
> ::: dom/presentation/interfaces/nsIPresentationControlChannel.idl
> @@ +133,5 @@
> > +   * Note that only controller is allowed to reconnect a session.
> > +   * @param presentationId The Id for representing this session.
> > +   * @param url The URL requested to open by remote device.
> > +   * @throws NS_ERROR_FAILURE on failure
> > +   */
> 
> Please let me confirm if the parameters are necessary or not. Could we
> change the presentationId and url in |reconnect| for a control channel?
> If yes, what does the presenter behave?
> If no, there's something wrong.
> 

Since we create a new control channel every time we try to reconnect, I think we have to pass the presentationId and url. Receiver will check whether the presentationId and url are matched. If not, the control channel will be closed.

> ::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
> @@ +872,5 @@
> >  
> >    return NS_OK;
> >  }
> >  
> > +// Create a new device if can not find one by address.
> 
> Lack of subject in the "if" sentence. Please rephrase.
> 

Will do.

> ::: dom/presentation/provider/ReceiverStateMachine.jsm
> @@ +74,5 @@
> > +        stateMachine._sendCommand({
> > +          type: CommandType.RECONNECT_ACK,
> > +          presentationId: command.presentationId
> > +        });
> > +        break;
> 
> Please align with bug 1276378 comment 24
> 

Got it.

> ::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
> @@ +479,5 @@
> > +    },
> > +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationControlChannelListener]),
> > +  };
> > +}
> > +
> 
> The setup is dup with |loopOfferAnswer|. Any reason to do this? Could we
> just merge to loopOfferAnswer?

Yes, I can merge it.

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Part1: Support reconnect command - v3

Address previous comments.

Comment 11

[Junior [inactive]]

Comment on attachment 8772298 [details] [diff] [review]
Part1: Support reconnect command - v3

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

::: dom/presentation/PresentationDeviceManager.cpp
@@ +276,5 @@
> +
> +  RefPtr<PresentationSessionRequest> request =
> +    new PresentationSessionRequest(aDevice, aUrl, aPresentationId, aControlChannel);
> +  obs->NotifyObservers(request,
> +                       PRESENTATION_RECONNECT_REQUEST_TOPIC,

Update the description in PresentationSessionRequest.idl

::: dom/presentation/interfaces/nsIPresentationControlChannel.idl
@@ +129,5 @@
>    void disconnect(in nsresult reason);
> +
> +  /*
> +   * Reconnect a presentation on remote endpoint.
> +   * Note that only controller is allowed to reconnect a session.

|ControlService.connect| and |ControlChannel.disconnect| are about the control channel. However, |ControlChannel.reconnect| is about a presentation session, so does |launch| and |terminate|.

That's the reason (|re|dis)connect make me confused here, and the bad thing here is the word |reconnect| in the spec.

To reduce the ambiguity, please find another bug for the naming

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +872,5 @@
>  
>    return NS_OK;
>  }
>  
> +// Create a new device if we were unable to find it with the address.

s/it/one/

::: dom/presentation/provider/PresentationControlService.js
@@ +194,5 @@
>    },
>  
> +  onSessionReconnect: function(aDeviceInfo, aUrl, aPresentationId, aControlChannel) {
> +    DEBUG && log("TCPPresentationServer - onSessionReconnect: " +
> +                 aDeviceInfo.address + ":" + aDeviceInfo.port); //jshint ignore:line

nit: space after "//"

::: dom/presentation/provider/ReceiverStateMachine.jsm
@@ +121,5 @@
>      }
>    },
>  
> +  reconnect: function _reconnect() {
> +    // presentation session can only be reconnected by controlling UA.

Debug message is informative enough.

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement reconnect - v2

Comment 13

[Kershaw Chang [:kershaw]]

Comment on attachment 8772767 [details] [diff] [review]
Part2: Implement reconnect - v2

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

::: dom/presentation/PresentationCallbacks.h
@@ -29,5 @@
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIPRESENTATIONSERVICECALLBACK
>  
>    PresentationRequesterCallback(PresentationRequest* aRequest,
> -                                const nsAString& aUrl,

It seems that aUrl is not used here, so remove it.

::: dom/presentation/ipc/PresentationIPCService.h
@@ -68,5 @@
> -  // to retrieve the correspondent session ID. Besides, also keep the mapping
> -  // between the responding session ID and the window ID to help look up the
> -  // window ID.
> -  nsClassHashtable<nsUint64HashKey, nsString> mRespondingSessionIds;
> -  nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;

Remove these tables since they are already defined in PresentationServiceBase.

Comment 14

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

Comment on attachment 8772767 [details] [diff] [review]
Part2: Implement reconnect - v2

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

Need add test for following scenarios:
1. reconnect a connecting / connected session in the same browser context
2. reconnect a closed session in other browser context
3. reconnect a connecting / connected session in other browser context
4. error handling for NotFoundError

::: dom/presentation/PresentationCallbacks.h
@@ -29,5 @@
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIPRESENTATIONSERVICECALLBACK
>  
>    PresentationRequesterCallback(PresentationRequest* aRequest,
> -                                const nsAString& aUrl,

I think we are going to use it while implementing multiple URL in one PresentationRequest.

::: dom/presentation/PresentationConnectionCollection.h
@@ +23,5 @@
> +{
> +public:
> +  static PresentationConnectionCollection* GetSingleton();
> +
> +  ~PresentationConnectionCollection();

make it private and virtual.

@@ +30,5 @@
> +
> +  void RemoveConnection(PresentationConnection* aConnection);
> +
> +  already_AddRefed<PresentationConnection>
> +  FindConnection(nsPIDOMWindowInner* aWindow, const nsAString& aId);

why not simply pass the windowId?

::: dom/presentation/PresentationRequest.cpp
@@ +201,5 @@
> +PresentationRequest::FindOrCreatePresentationConnection(
> +  const nsAString& aPresentationId,
> +  Promise* aPromise)
> +{
> +  if (NS_WARN_IF(!aPromise)) {

use MOZ_ASSERT?

@@ +222,5 @@
> +        aPromise->MaybeResolve(connection);
> +        break;
> +      case PresentationConnectionState::Connecting:
> +      case PresentationConnectionState::Connected:
> +        aPromise->MaybeResolve(connection);

In spec step 7.4 it says connection state should be switch to "connecting" and then fire the "onconnect" event.

@@ +253,5 @@
> +                              nsIPresentationService::ROLE_CONTROLLER,
> +                              callback);
> +  if (rv == NS_ERROR_NOT_AVAILABLE) {
> +    aPromise->MaybeReject(NS_ERROR_DOM_NOT_FOUND_ERR);
> +  }

We should also reject the promise if getting other failure result.

::: dom/presentation/PresentationService.cpp
@@ +374,5 @@
>      return NS_OK;
>    }
>  #endif
>  
>    // Create or reuse session info.

We should add more description about how we reuse session info.

@@ +723,5 @@
> +
> +  if (NS_WARN_IF(!info->GetUrl().Equals(aUrl))) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +

We'll need to update the windowID <-> presentationId mapping as well, otherwise all the following event will be deliver to the old window.
Also need to notify IPC service to update the mapping for data channel scenario.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +1272,5 @@
>  
>  NS_IMETHODIMP
> +PresentationPresentingInfo::NotifyReconnected()
> +{
> +  // Do nothing.

Maybe assert here since we are pretty sure it won't happen.

::: dom/webidl/PresentationRequest.webidl
@@ +36,5 @@
> +   * The connection state is "connecting".
> +   *
> +   * The promise may be rejected duo to one of the following reasons:
> +   * - "OperationError": Unexpected error occurs.
> +   * - "NotFoundError":  Can not find a presentation connection with the presentationId.

add comment for SecurityError as well.

Comment 15

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #14)
> Comment on attachment 8772767 [details] [diff] [review]
> Part2: Implement reconnect - v2
> 
> Review of attachment 8772767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Need add test for following scenarios:
> 1. reconnect a connecting / connected session in the same browser context
> 2. reconnect a closed session in other browser context
> 3. reconnect a connecting / connected session in other browser context
> 4. error handling for NotFoundError
> 

Sure.

> ::: dom/presentation/PresentationCallbacks.h
> @@ -29,5 @@
> >    NS_DECL_ISUPPORTS
> >    NS_DECL_NSIPRESENTATIONSERVICECALLBACK
> >  
> >    PresentationRequesterCallback(PresentationRequest* aRequest,
> > -                                const nsAString& aUrl,
> 
> I think we are going to use it while implementing multiple URL in one
> PresentationRequest.
> 

I'll add it back in next version.

> ::: dom/presentation/PresentationConnectionCollection.h
> @@ +23,5 @@
> > +{
> > +public:
> > +  static PresentationConnectionCollection* GetSingleton();
> > +
> > +  ~PresentationConnectionCollection();
> 
> make it private and virtual.
> 

Ok.

> @@ +30,5 @@
> > +
> > +  void RemoveConnection(PresentationConnection* aConnection);
> > +
> > +  already_AddRefed<PresentationConnection>
> > +  FindConnection(nsPIDOMWindowInner* aWindow, const nsAString& aId);
> 
> why not simply pass the windowId?
> 

Yes, it would be simpler. I'll do it.

> ::: dom/presentation/PresentationRequest.cpp
> @@ +201,5 @@
> > +PresentationRequest::FindOrCreatePresentationConnection(
> > +  const nsAString& aPresentationId,
> > +  Promise* aPromise)
> > +{
> > +  if (NS_WARN_IF(!aPromise)) {
> 
> use MOZ_ASSERT?
> 

OK.

> @@ +222,5 @@
> > +        aPromise->MaybeResolve(connection);
> > +        break;
> > +      case PresentationConnectionState::Connecting:
> > +      case PresentationConnectionState::Connected:
> > +        aPromise->MaybeResolve(connection);
> 
> In spec step 7.4 it says connection state should be switch to "connecting"
> and then fire the "onconnect" event.
> 

In step 7.3, it says we should abort all remaining steps. So, I think this should be fine.

> @@ +253,5 @@
> > +                              nsIPresentationService::ROLE_CONTROLLER,
> > +                              callback);
> > +  if (rv == NS_ERROR_NOT_AVAILABLE) {
> > +    aPromise->MaybeReject(NS_ERROR_DOM_NOT_FOUND_ERR);
> > +  }
> 
> We should also reject the promise if getting other failure result.
> 
> ::: dom/presentation/PresentationService.cpp
> @@ +374,5 @@
> >      return NS_OK;
> >    }
> >  #endif
> >  
> >    // Create or reuse session info.
> 
> We should add more description about how we reuse session info.
> 

OK.

> @@ +723,5 @@
> > +
> > +  if (NS_WARN_IF(!info->GetUrl().Equals(aUrl))) {
> > +    return NS_ERROR_INVALID_ARG;
> > +  }
> > +
> 
> We'll need to update the windowID <-> presentationId mapping as well,
> otherwise all the following event will be deliver to the old window.
> Also need to notify IPC service to update the mapping for data channel
> scenario.
> 

Got it.

> ::: dom/presentation/PresentationSessionInfo.cpp
> @@ +1272,5 @@
> >  
> >  NS_IMETHODIMP
> > +PresentationPresentingInfo::NotifyReconnected()
> > +{
> > +  // Do nothing.
> 
> Maybe assert here since we are pretty sure it won't happen.
> 

OK.

> ::: dom/webidl/PresentationRequest.webidl
> @@ +36,5 @@
> > +   * The connection state is "connecting".
> > +   *
> > +   * The promise may be rejected duo to one of the following reasons:
> > +   * - "OperationError": Unexpected error occurs.
> > +   * - "NotFoundError":  Can not find a presentation connection with the presentationId.
> 
> add comment for SecurityError as well.

Will do.

Comment 16

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

(In reply to Kershaw Chang [:kershaw] from comment #15)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > @@ +222,5 @@
> > > +        aPromise->MaybeResolve(connection);
> > > +        break;
> > > +      case PresentationConnectionState::Connecting:
> > > +      case PresentationConnectionState::Connected:
> > > +        aPromise->MaybeResolve(connection);
> > 
> > In spec step 7.4 it says connection state should be switch to "connecting"
> > and then fire the "onconnect" event.
> > 
> 
> In step 7.3, it says we should abort all remaining steps. So, I think this
> should be fine.
> 

I misread the spec, thanks for the correction!

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement reconnect - v3

1. Address previous comments
2. Add url in PresentationConnection
3. Fix some bugs found in test cases

Comment 18

[Kershaw Chang [:kershaw]]

Attachment: Interdiff for part2

Interdiff of v2 and v3.

Comment 19

[Kershaw Chang [:kershaw]]

Attachment: Part3: test case

Tests.

Comment 20

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

Comment on attachment 8775004 [details] [diff] [review]
Part2: Implement reconnect - v3

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

::: dom/presentation/Presentation.cpp
@@ +53,5 @@
> +  if (!docShell) {
> +    return false;
> +  }
> +
> +  nsContentUtils::GetPresentationURL(docShell, mPresentationUrl);

storing an extra attribute that only for receiver side that only used once seems a waste to me.

::: dom/presentation/PresentationReceiver.cpp
@@ +31,5 @@
>  NS_INTERFACE_MAP_END
>  
>  /* static */ already_AddRefed<PresentationReceiver>
> +PresentationReceiver::Create(nsPIDOMWindowInner* aWindow,
> +                             const nsAString& aUrl)

I'll prefer PresentationReceiver get the presentation URL by itself instead of passing from somewhere.

::: dom/presentation/PresentationRequest.cpp
@@ +185,5 @@
> +
> +  RefPtr<PresentationRequest> self = this;
> +  nsString presentationId = nsString(aPresentationId);
> +  nsCOMPtr<nsIRunnable> r =
> +    NS_NewRunnableFunction([self, presentationId, promise] () -> void {

We might prefer using NewRunnableMethod since bug 1268313 is landed.
*reminder: need switch to NS_NewRunnableFunction on tv 2.6 branch.

::: dom/presentation/ipc/PresentationParent.cpp
@@ +250,5 @@
>  
>  NS_IMETHODIMP
> +PresentationParent::NotifyReplaced(const nsAString& aSessionId)
> +{
> +  return NS_OK;

Add a comment to explain that PresentationConnection replace procedure is done in PresentationIPCService::RegisterSessionListener.

Comment 21

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

Comment on attachment 8775006 [details] [diff] [review]
Part3: test case

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

::: dom/presentation/tests/mochitest/mochitest.ini
@@ +55,5 @@
>  skip-if = (e10s || toolkit == 'gonk' || toolkit == 'android')
>  [test_presentation_sender_on_terminate_request.html]
>  skip-if = toolkit == 'android'
>  [test_presentation_sandboxed_presentation.html]
> +[test_presentation_reconnect_connection.html]

how about test_presentation_reconnect.html?

::: dom/presentation/tests/mochitest/test_presentation_reconnect_connection.html
@@ +221,5 @@
> +    iframe.contentWindow.postMessage("startConnection", "*");
> +    gScript.sendAsyncMessage('save-control-channel-listener');
> +    addEventListener("message", function listener(event) {
> +      var message = event.data;
> +      if (/^COMMAND /.exec(message)) {

You can simply use a map to store the command handler.

@@ +234,5 @@
> +
> +          var request1 = new PresentationRequest("http://example1.com");
> +          request1.reconnect(command.id).then(
> +            function(aConnection) {
> +              is(aConnection.state, "connecting", "The state should be connecting.");

You can use Promise.all() to wait both onconnect in this page and onclose in iframe. Also, send a message to make sure we reroute the message correctly.

Comment 22

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement reconnect - v4

Address the previous comments.

Comment 23

[Kershaw Chang [:kershaw]]

Attachment: Part3: test case- v2

Comment 24

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

Comment on attachment 8775525 [details] [diff] [review]
Part2: Implement reconnect - v4

>+  // We found a matched connection with the same window ID, URL, and
>+  // the session ID. Resolve the promise with this connection and dispatch
>+  // the event.
>+  if (mConnection) {
>+    mPromise->MaybeResolve(mConnection);
>+    rv = mRequest->DispatchConnectionAvailableEvent(mConnection);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+  }
>+  else {
nit, 
} else {
Same also elsewhere


>+PresentationConnection::NotifyReplaced(const nsAString& aSessionId)
>+{
>+  return NotifyStateChange(mId,
>+                           nsIPresentationSessionListener::STATE_CLOSED,
>+                           NS_OK);
>+}
So why NotifyReplaced takes sessionId as param, when no NotifyReplaced uses it for anything?

>+++ b/dom/presentation/ipc/PresentationIPCService.h
>@@ -57,24 +57,16 @@ private:
>                        const PresentationIPCRequest& aRequest);
> 
>   nsTObserverArray<nsCOMPtr<nsIPresentationAvailabilityListener> > mAvailabilityListeners;
>   nsRefPtrHashtable<nsStringHashKey,
>                     nsIPresentationSessionListener> mSessionListeners;
>   nsRefPtrHashtable<nsUint64HashKey,
>                     nsIPresentationRespondingListener> mRespondingListeners;
>   RefPtr<PresentationResponderLoadingCallback> mCallback;
>-
>-  // Store the mapping between the window ID of the OOP page (in this process)
>-  // and the ID of the responding session. It's used for an OOP receiver page
>-  // to retrieve the correspondent session ID. Besides, also keep the mapping
>-  // between the responding session ID and the window ID to help look up the
>-  // window ID.
>-  nsClassHashtable<nsUint64HashKey, nsString> mRespondingSessionIds;
>-  nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;
Hmm, what happens to these.


This is so massive patch that I'll need to look at it couple of times.
Could you explain/fix those small things I mentioned, and I'll take another look.

Comment 25

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

Comment on attachment 8775526 [details] [diff] [review]
Part3: test case- v2

mostly rs+

Comment 26

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8775525 [details] [diff] [review]
> Part2: Implement reconnect - v4
> 
> >+  // We found a matched connection with the same window ID, URL, and
> >+  // the session ID. Resolve the promise with this connection and dispatch
> >+  // the event.
> >+  if (mConnection) {
> >+    mPromise->MaybeResolve(mConnection);
> >+    rv = mRequest->DispatchConnectionAvailableEvent(mConnection);
> >+    if (NS_WARN_IF(NS_FAILED(rv))) {
> >+      return rv;
> >+    }
> >+  }
> >+  else {
> nit, 
> } else {
> Same also elsewhere
> 

Will fix in next version.

> 
> >+PresentationConnection::NotifyReplaced(const nsAString& aSessionId)
> >+{
> >+  return NotifyStateChange(mId,
> >+                           nsIPresentationSessionListener::STATE_CLOSED,
> >+                           NS_OK);
> >+}
> So why NotifyReplaced takes sessionId as param, when no NotifyReplaced uses
> it for anything?
> 

Great catch. We really don't need this parameter.

> >+++ b/dom/presentation/ipc/PresentationIPCService.h
> >@@ -57,24 +57,16 @@ private:
> >                        const PresentationIPCRequest& aRequest);
> > 
> >   nsTObserverArray<nsCOMPtr<nsIPresentationAvailabilityListener> > mAvailabilityListeners;
> >   nsRefPtrHashtable<nsStringHashKey,
> >                     nsIPresentationSessionListener> mSessionListeners;
> >   nsRefPtrHashtable<nsUint64HashKey,
> >                     nsIPresentationRespondingListener> mRespondingListeners;
> >   RefPtr<PresentationResponderLoadingCallback> mCallback;
> >-
> >-  // Store the mapping between the window ID of the OOP page (in this process)
> >-  // and the ID of the responding session. It's used for an OOP receiver page
> >-  // to retrieve the correspondent session ID. Besides, also keep the mapping
> >-  // between the responding session ID and the window ID to help look up the
> >-  // window ID.
> >-  nsClassHashtable<nsUint64HashKey, nsString> mRespondingSessionIds;
> >-  nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;
> Hmm, what happens to these.
> 

These two tables are actually redundant and useless. We already defined the same tables in http://searchfox.org/mozilla-central/source/dom/presentation/PresentationServiceBase.h#50.

> 
> This is so massive patch that I'll need to look at it couple of times.
> Could you explain/fix those small things I mentioned, and I'll take another
> look.

Thanks for the review. Please let me know if there is anything not clear.

Comment 27

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement reconnect - v5

This only fixes the previous comments.

Please take a look again. Thanks.

Comment 28

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

Comment on attachment 8776434 [details] [diff] [review]
Part2: Implement reconnect - v5


>+  if (mRole == nsIPresentationService::ROLE_CONTROLLER) {
>+    PresentationConnectionCollection::GetSingleton()->RemoveConnection(this);
>+  }
> }
Why this can be done only when role is controller?
If PresentationConnectionCollection::AddConnection/RemoveConnection/FindConnection are supposed to be used for controller only, they should have
assertions about the role, and PresentationConnectionCollection could perhaps have different name hinting that it is only about controller.
Perhaps ControllerConnectionCollection ?



>+
>+class PresentationConnectionCollection final
So this class could have different name and methods some assertions about the role


>+  /*
>+   * A requesting page can use reconnect(presentationId) to reopen a
>+   * closed presentation connection.
Hmm, is it really only about that, since there can be existing connection being opened.


those nits fixed, r+

Comment 29

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] from comment #28)
> Comment on attachment 8776434 [details] [diff] [review]
> Part2: Implement reconnect - v5
> 
> 
> >+  if (mRole == nsIPresentationService::ROLE_CONTROLLER) {
> >+    PresentationConnectionCollection::GetSingleton()->RemoveConnection(this);
> >+  }
> > }
> Why this can be done only when role is controller?

According to the spec, we have to search a connection that has the matched id, browsing context, and the URL when trying to start reconnecting at controller side. So, we need to track all connections only at controller side.

> If
> PresentationConnectionCollection::AddConnection/RemoveConnection/
> FindConnection are supposed to be used for controller only, they should have
> assertions about the role, and PresentationConnectionCollection could
> perhaps have different name hinting that it is only about controller.
> Perhaps ControllerConnectionCollection ?

Nice suggestion! That would be more clear.

> 
> 
> 
> >+
> >+class PresentationConnectionCollection final
> So this class could have different name and methods some assertions about
> the role
> 

Will do.

> 
> >+  /*
> >+   * A requesting page can use reconnect(presentationId) to reopen a
> >+   * closed presentation connection.
> Hmm, is it really only about that, since there can be existing connection
> being opened.
> 

Yes, you are right. I'll update the comment.

> 
> those nits fixed, r+

Comment 30

[Kershaw Chang [:kershaw]]

Attachment: Part1: Support reconnect command, r=junior

Rebase and carry r+.

Comment 31

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement reconnect, r=smaug

Fix nits and carry r+.

Comment 32

[Kershaw Chang [:kershaw]]

Attachment: Part3: Tests, r=smaug

Carry r+.

Comment 33

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=247b023931cb

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c30312cf569
Part1: Support reconnect command, r=junior
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6bb6caf7039
Part2: Implement reconnect, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42d7d3067a8
Part3: Tests, r=smaug

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c30312cf569
https://hg.mozilla.org/mozilla-central/rev/e6bb6caf7039
https://hg.mozilla.org/mozilla-central/rev/e42d7d3067a8

Comment 36

[Kershaw Chang [:kershaw]]

Attachment: pull request for tv 2.6

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: Unable to reconnect to a presentation
Testing completed: Local test.
Risk to taking this patch (and alternatives if risky): N/A
String or UUID changes made by this patch: N/A

Comment 37

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #36)
> Created attachment 8779164 [details] [review]
> pull request for tv 2.6
> 
> 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: Unable to reconnect to a presentation
> Testing completed: Local 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 approve this request? Thanks.

Comment 38

[Josh Cheng [:josh]]

Comment on attachment 8779164 [details] [review]
pull request for tv 2.6

Approve for TV 2.6

Comment 39

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/9db666adbe8066750ecd2c2f15184560a4ed2897