Closed Bug 1197690 Opened 4 years ago Closed 3 years ago

[Presentation WebAPI] Support resuming a disconnected presentation session

Categories

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

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
FxOS-S1 (26Jun)
blocking-b2g 2.6+
Tracking Status
b2g-v2.6 --- fixed
firefox51 --- fixed

People

(Reporter: selin, Assigned: kershaw)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [ft:conndevices] [ETA 7/15])

Attachments

(4 files, 11 obsolete files)

40.87 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
72.64 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
42.30 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
48 bytes, text/x-github-pull-request
Details | Review
+++ This bug was initially created as a clone of Bug #1195605 +++

Support resuming a disconnected presentation session.
No longer depends on: 1195605
Summary: [Presentation WebAPI] Support many-to-one session → [Presentation WebAPI] Support resuming a disconnected presentation session
blocking-b2g: --- → 2.6?
blocking-b2g: 2.6? → 2.6+
Whiteboard: [ft:conndevices] → [ft:conndevices] [ETA 6/30]
Target Milestone: --- → FxOS-S1 (26Jun)
Whiteboard: [ft:conndevices] [ETA 6/30] → [ft:conndevices] [ETA 7/15]
Attached patch Part1: Support reconnect command (obsolete) — Splinter Review
WIP
Assignee: nobody → kechang
Attachment #8770881 - Flags: feedback?(juhsu)
Attached patch Part2: Implement reconnect (obsolete) — Splinter Review
Attachment #8770882 - Flags: feedback?(schien)
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.
Attachment #8770881 - Flags: feedback?(juhsu)
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?
Attachment #8770882 - Flags: feedback?(schien)
Rebase and address previous comments.
Attachment #8770881 - Attachment is obsolete: true
Attachment #8771370 - Flags: review?(juhsu)
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?
Attachment #8771370 - Flags: review?(juhsu)
(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.
(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.
Address previous comments.
Attachment #8771370 - Attachment is obsolete: true
Attachment #8772298 - Flags: review?(juhsu)
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.
Attachment #8772298 - Flags: review?(juhsu) → review+
Attached patch Part2: Implement reconnect - v2 (obsolete) — Splinter Review
Attachment #8770882 - Attachment is obsolete: true
Attachment #8772767 - Flags: feedback?(schien)
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 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.
Attachment #8772767 - Flags: feedback?(schien) → feedback+
(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.
(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!
Attached patch Part2: Implement reconnect - v3 (obsolete) — Splinter Review
1. Address previous comments
2. Add url in PresentationConnection
3. Fix some bugs found in test cases
Attachment #8772767 - Attachment is obsolete: true
Attachment #8775004 - Flags: feedback?(schien)
Attached patch Interdiff for part2 (obsolete) — Splinter Review
Interdiff of v2 and v3.
Attached patch Part3: test case (obsolete) — Splinter Review
Tests.
Attachment #8775006 - Flags: feedback?(schien)
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.
Attachment #8775004 - Flags: feedback?(schien) → feedback+
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.
Attachment #8775006 - Flags: feedback?(schien) → feedback+
Attached patch Part2: Implement reconnect - v4 (obsolete) — Splinter Review
Address the previous comments.
Attachment #8775004 - Attachment is obsolete: true
Attachment #8775005 - Attachment is obsolete: true
Attachment #8775006 - Attachment is obsolete: true
Attachment #8775525 - Flags: review?(bugs)
Attached patch Part3: test case- v2 (obsolete) — Splinter Review
Attachment #8775526 - Flags: review?(bugs)
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.
Attachment #8775525 - Flags: review?(bugs) → review-
Comment on attachment 8775526 [details] [diff] [review]
Part3: test case- v2

mostly rs+
Attachment #8775526 - Flags: review?(bugs) → review+
(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.
Attached patch Part2: Implement reconnect - v5 (obsolete) — Splinter Review
This only fixes the previous comments.

Please take a look again. Thanks.
Attachment #8775525 - Attachment is obsolete: true
Attachment #8776434 - Flags: review?(bugs)
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+
Attachment #8776434 - Flags: review?(bugs) → review+
(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+
Rebase and carry r+.
Attachment #8772298 - Attachment is obsolete: true
Attachment #8775526 - Attachment is obsolete: true
Attachment #8776434 - Attachment is obsolete: true
Attachment #8777173 - Flags: review+
Fix nits and carry r+.
Attachment #8777174 - Flags: review+
Carry r+.
Attachment #8777175 - Flags: review+
Keywords: checkin-needed
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
Attachment #8779164 - Flags: approval-mozilla-b2g48?
(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.
Flags: needinfo?(jocheng)
Comment on attachment 8779164 [details] [review]
pull request for tv 2.6

Approve for TV 2.6
Flags: needinfo?(jocheng)
Attachment #8779164 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.