Closed Bug 1258602 Opened 4 years ago Closed 4 years ago

[Presentation WebAPI] implement PresentationConnectionList

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
blocking-b2g 2.6+
Tracking Status
firefox49 --- fixed
b2g-v2.6 --- fixed

People

(Reporter: schien, Assigned: kershaw)

References

(Depends on 1 open bug, )

Details

(Whiteboard: btpp-active [ETA 5/26][ft:conndevices])

Attachments

(3 files, 14 obsolete files)

34.32 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
9.08 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
34.08 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
PresentationConnectionList is introduced for handling connection list enumeration and change notification.
Attached patch WebIDL changes (obsolete) — Splinter Review
S.C., could you take a look at this change?
Not sure if this is the correct track to go.
Assignee: nobody → kechang
Attachment #8738915 - Flags: feedback?(schien)
blocking-b2g: --- → 2.6?
Comment on attachment 8738915 [details] [diff] [review]
WebIDL changes

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

Yes, that's what I expect. Please leave comment like in [1] so people can know where to fix after FrozenArray is available.
[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/RTCTrackEvent.webidl#22
Attachment #8738915 - Flags: feedback?(schien) → feedback+
Whiteboard: btpp-active
blocking-b2g: 2.6? → 2.6+
S.C, Could you please take a quick look at this patch?
Basically, I just moved codes from PresentationReceiver to PresentationConnectionList.

Thanks.
Attachment #8738915 - Attachment is obsolete: true
Attachment #8741664 - Flags: feedback?(schien)
Attached patch Part2: Test case changes (obsolete) — Splinter Review
Attachment #8741665 - Flags: feedback?(schien)
Fix comment2.
Attachment #8741664 - Attachment is obsolete: true
Attachment #8741664 - Flags: feedback?(schien)
Attachment #8741668 - Flags: feedback?(schien)
Comment on attachment 8741668 [details] [diff] [review]
Part1: Add PresentationConnectionList

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

::: dom/presentation/PresentationConnectionList.cpp
@@ +12,5 @@
> +#include "PresentationConnection.h"
> +#include "PresentationConnectionList.h"
> +
> +using namespace mozilla;
> +using namespace mozilla::dom;

replace it with since we don't want to import the entire namespace via |using namespace|.

namespace mozilla {
namespace dom {

...

} // namespace dom
} // namespace mozilla

@@ +23,5 @@
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(PresentationConnectionList,
> +                                                DOMEventTargetHelper)
> +  tmp->Shutdown();

We'll end up calling |Shutdown| multiple times after object being cycle collected, is that expected?

@@ +131,5 @@
> +  if (NS_WARN_IF(!GetOwner())) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (NS_WARN_IF(aWindowId != GetOwner()->WindowID())) {

use |mWindowID| here?

@@ +150,5 @@
> +nsresult
> +PresentationConnectionList::DispatchConnectionAvailableEvent()
> +{
> +  RefPtr<AsyncEventDispatcher> asyncDispatcher =
> +    new AsyncEventDispatcher(this, NS_LITERAL_STRING("connectionavailable"), false);

You'll need to create a PresentationConnectionAvailableEvent by following https://w3c.github.io/presentation-api/#monitoring-incoming-presentation-connections

::: dom/presentation/PresentationConnectionList.h
@@ +38,5 @@
> +
> +private:
> +  PresentationConnectionList(nsPIDOMWindowInner* aWindow);
> +
> +  ~PresentationConnectionList();

nit: add |virtual|

::: dom/presentation/PresentationReceiver.cpp
@@ +11,1 @@
>  #include "PresentationReceiver.h"

nit: move "PresentationReceiver.h" to first.

@@ +24,1 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

use NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(PresentationReceiver, ...), see reference https://dxr.mozilla.org/mozilla-central/source/dom/base/DOMError.cpp#15

@@ +27,1 @@
>    tmp->Shutdown();

We don't need special shutdown handling after this change.

@@ +104,2 @@
>  
> +    mPendingGetConnectionListPromises.AppendElement(promise);

we should store and return the same promise object to reflect [SameObject] in WebIDL.

::: dom/presentation/PresentationReceiver.h
@@ +48,4 @@
>  
> +  nsCOMPtr<nsPIDOMWindowInner> mOwner;
> +  nsString mSessionId;
> +  nsTArray<RefPtr<Promise>> mPendingGetConnectionListPromises;

Do we need an array here? In spec we have [SameObject] for connectionList attribute.
May we name it |mGetConnectionListPromise| since this is the promise object we return every time "receiver.connectionList" is used?

::: dom/presentation/moz.build
@@ +17,5 @@
>      'Presentation.h',
>      'PresentationAvailability.h',
>      'PresentationCallbacks.h',
>      'PresentationConnection.h',
> +    'PresentationConnectionList.h',

PresentationConnectionList is only used in dom/presentation, no need to expose this header file.

::: dom/webidl/PresentationConnectionList.webidl
@@ +16,5 @@
> +  [Frozen, Cached, Pure]
> +  readonly attribute sequence<PresentationConnection> connections;
> +
> +  /*
> +   * It is called when an incoming connection is connecting.

s/connecting/connected
From spec this event is fired when PresentationConnection is connected.
Attachment #8741668 - Flags: feedback?(schien)
Comment on attachment 8741665 [details] [diff] [review]
Part2: Test case changes

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

lgtm. We'll need to add test for event handling after connection resumption is implemented on bug Bug 1197690.
Attachment #8741665 - Flags: feedback?(schien) → feedback+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #6)
> Comment on attachment 8741668 [details] [diff] [review]
> Part1: Add PresentationConnectionList
> 
> Review of attachment 8741668 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for all the comments.

> ::: dom/presentation/PresentationConnectionList.cpp
> @@ +12,5 @@
> > +#include "PresentationConnection.h"
> > +#include "PresentationConnectionList.h"
> > +
> > +using namespace mozilla;
> > +using namespace mozilla::dom;
> 
> replace it with since we don't want to import the entire namespace via
> |using namespace|.
> 
> namespace mozilla {
> namespace dom {
> 
> ...
> 
> } // namespace dom
> } // namespace mozilla
> 

It seems that all cpp files in dom/presentation are importing all namespace via using. Maybe we have to file another bug to fix other files?

> @@ +23,5 @@
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> > +
> > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(PresentationConnectionList,
> > +                                                DOMEventTargetHelper)
> > +  tmp->Shutdown();
> 
> We'll end up calling |Shutdown| multiple times after object being cycle
> collected, is that expected?

It looks correct to me. I also found other places that |Shutdown| has been called in destructor and unlinking.
https://dxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelAgent.cpp#22
https://dxr.mozilla.org/mozilla-central/source/dom/base/Console.cpp#854
(In reply to Kershaw Chang [:kershaw] from comment #8)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #6)
> > Comment on attachment 8741668 [details] [diff] [review]
> > Part1: Add PresentationConnectionList
> > 
> > Review of attachment 8741668 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> Thanks for all the comments.
> 
> > ::: dom/presentation/PresentationConnectionList.cpp
> > @@ +12,5 @@
> > > +#include "PresentationConnection.h"
> > > +#include "PresentationConnectionList.h"
> > > +
> > > +using namespace mozilla;
> > > +using namespace mozilla::dom;
> > 
> > replace it with since we don't want to import the entire namespace via
> > |using namespace|.
> > 
> > namespace mozilla {
> > namespace dom {
> > 
> > ...
> > 
> > } // namespace dom
> > } // namespace mozilla
> > 
> 
> It seems that all cpp files in dom/presentation are importing all namespace
> via using. Maybe we have to file another bug to fix other files?
Yeah that's something we should do and it should be a good-first bug.
> 
> > @@ +23,5 @@
> > > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> > > +
> > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(PresentationConnectionList,
> > > +                                                DOMEventTargetHelper)
> > > +  tmp->Shutdown();
> > 
> > We'll end up calling |Shutdown| multiple times after object being cycle
> > collected, is that expected?
> 
> It looks correct to me. I also found other places that |Shutdown| has been
> called in destructor and unlinking.
> https://dxr.mozilla.org/mozilla-central/source/dom/audiochannel/
> AudioChannelAgent.cpp#22
> https://dxr.mozilla.org/mozilla-central/source/dom/base/Console.cpp#854
Calling Shutdown multiple times is not that bad but we need to make sure PresentationConnectionList::Shutdown is safe to be called multiple times.
Address previous comments.

Please let me know if you need interdiff for this.

> ::: dom/presentation/moz.build
> @@ +17,5 @@
> >      'Presentation.h',
> >      'PresentationAvailability.h',
> >      'PresentationCallbacks.h',
> >      'PresentationConnection.h',
> > +    'PresentationConnectionList.h',
> 
> PresentationConnectionList is only used in dom/presentation, no need to
> expose this header file.

|mozilla/dom/PresentationConnectionList.h| is needed in PresentationConnectionListBinding.cpp. It seems that we still need to expose PresentationConnectionList.h.
Attachment #8741668 - Attachment is obsolete: true
Attachment #8742755 - Flags: feedback?(schien)
Attachment #8741665 - Attachment is obsolete: true
Attachment #8742756 - Flags: feedback?(schien)
Comment on attachment 8742755 [details] [diff] [review]
Part1: Add PresentationConnectionList - v2

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

::: dom/presentation/PresentationConnectionList.cpp
@@ +185,5 @@
> +    else {
> +      NS_WARN_IF(!self->mConnections.RemoveElement(connection));
> +    }
> +  });
> +  NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(r)));

Delay clearing the cached connections to make sure the web page can get the updated data.
Comment on attachment 8742755 [details] [diff] [review]
Part1: Add PresentationConnectionList - v2

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

::: dom/presentation/PresentationConnectionList.cpp
@@ +131,5 @@
> +  }
> +
> +  RefPtr<PresentationConnection> connection =
> +    PresentationConnection::Create(GetOwner(), aSessionId,
> +                                   PresentationConnectionState::Closed, this);

Logically session Id (or presentation Id) is associated with PresentationConnection but not PresentationConnectionList. Passing the session Id all the way from PresentationConnectionList seems weird to me. Maybe we can move the PresentationConnection creation code back to PresentationReceiver and only use the state notification for updating connection list.

@@ +182,5 @@
> +      self->mConnections.AppendElement(connection);
> +      self->DispatchConnectionAvailableEvent(connection);
> +    }
> +    else {
> +      NS_WARN_IF(!self->mConnections.RemoveElement(connection));

only remove terminated connection but not connection in "connecting" or "closed" state.

@@ +185,5 @@
> +    else {
> +      NS_WARN_IF(!self->mConnections.RemoveElement(connection));
> +    }
> +  });
> +  NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(r)));

That's really weird since PresentationConnection and PresentationConnectionList doesn't have [Cached] attributes. We'll need to check the behavior of the generated binding code to understand the root cause.

::: dom/presentation/PresentationConnectionList.h
@@ +51,5 @@
> +
> +  nsresult DispatchConnectionAvailableEvent(PresentationConnection* aConnection);
> +
> +  void NotifyStateChange(const nsAString& aSessionId,
> +                         PresentationConnectionState aState);

Simply make this function public so you can unfriend with PresentationConnection.

@@ +57,5 @@
> +  // Store the inner window ID for |UnregisterRespondingListener| call in
> +  // |Shutdown| since the inner window may not exist at that moment.
> +  uint64_t mWindowId;
> +
> +  nsRefPtrHashtable<nsStringHashKey, PresentationConnection> mInitedConnections;

The number of connection in the list should be small. Using a hashtable might be too much overhead according to MDN ( https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Hashtables#When_Should_I_Use_a_Hashtable.3F ). Using an array and do linear search is enough.

::: dom/presentation/PresentationReceiver.cpp
@@ +73,4 @@
>    }
>  
> +  if (mConnectionList) {
> +    mGetConnectionListPromise->MaybeResolve(mConnectionList);

You don't need to call MaybeResolve multiple times. You can simply return the Promise without doing anything once it is resolved.
Attachment #8742755 - Flags: feedback?(schien)
I also discovery a issue in spec about PresentationReceiver, file a spec bug to track it. https://github.com/w3c/presentation-api/issues/281
Comment on attachment 8742756 [details] [diff] [review]
0002-Bug-1258602-Part2-Test-case-changes.patch

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

::: dom/presentation/tests/mochitest/file_presentation_receiver.html
@@ +38,5 @@
>      ok(navigator.presentation.receiver, "navigator.presentation.receiver should be available.");
>  
> +    navigator.presentation.receiver.connectionList.then(
> +      function(aList) {
> +        aList.onconnectionavailable = function(aEvent) {

check if aList.length equals to 0 before onconnectionavailable.

::: dom/presentation/tests/mochitest/file_presentation_receiver_establish_connection_error.html
@@ +45,5 @@
> +          ok(connection.id, "Connection ID should be set: " + connection.id);
> +          is(connection.state, "connected", "Connection state at receiver side should be connected.");
> +          aResolve();
> +        }
> +        command({ name: 'trigger-incoming-offer' });

should not do 'trigger-incoming-offer' here because this test case is for testing ctrl channel close before offer/answer. There should be no element in connection list and no 'connectionavailable' / 'statechange' event fired.
We should do something if fail to establish connection at this stage, file a spec bug for defining the behavior. https://github.com/w3c/presentation-api/issues/283

::: dom/presentation/tests/mochitest/file_presentation_receiver_oop.html
@@ +38,5 @@
>      ok(navigator.presentation.receiver, "navigator.presentation.receiver should be available in OOP receiving pages.");
>  
> +    navigator.presentation.receiver.connectionList.then(
> +      function(aList) {
> +        aList.onconnectionavailable = function(aEvent) {

check if aList.length equals to 0 before onconnectionavailable.
Attachment #8742756 - Flags: feedback?(schien)
Based on the latest spec.
Attachment #8742755 - Attachment is obsolete: true
Attachment #8744834 - Flags: feedback?(schien)
Attached patch Part2: Test case changes - v3 (obsolete) — Splinter Review
Attachment #8742756 - Attachment is obsolete: true
Attachment #8744835 - Flags: feedback?(schien)
Comment on attachment 8744834 [details] [diff] [review]
Part1: Add PresentationConnectionList - v3

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

Per our discussion the procedure of creating connection list should follow https://wiki.mozilla.org/WebAPI/PresentationAPI:WebIDL_Implementation#Get_Connection_List .
You can file a separated bug for the NotifySessionConnect changes.

::: dom/presentation/PresentationConnection.h
@@ +27,5 @@
>  
> +  static already_AddRefed<PresentationConnection> Create(
> +    nsPIDOMWindowInner* aWindow,
> +    const nsAString& aId,
> +    PresentationConnectionState aState);

static already_addRefed<PresentationConnection>
Create(nsPIDOMWindowInner* aWindow,
       const nsAString& aId,
       PresentationConnectionState aState);

@@ +68,5 @@
>    nsresult DispatchMessageEvent(JS::Handle<JS::Value> aData);
>  
>    nsString mId;
>    PresentationConnectionState mState;
> +  RefPtr<PresentationConnectionList> mConnectionList;

maybe call it "mOwningConnectionList"?

::: dom/presentation/PresentationConnectionList.h
@@ +53,5 @@
> +  // This array stores all connections no matter what the state is.
> +  ConnectionArray mAllConnections;
> +
> +  // This array stores only non-terminsted connections.
> +  ConnectionArray mNonTerminatedConnections;

terminated connections should be dropped from connection list, so there is no point having two ConnectionArray.

::: dom/presentation/PresentationReceiver.cpp
@@ +117,5 @@
> +      self->CreateConnectionList();
> +    }));
> +  }
> +
> +  mConnectionList->AddConnection(connection);

By following the sequence diagram on wiki. We can simply associate the connection list and connection via PresentationConnection ctor.
Attachment #8744834 - Flags: feedback?(schien)
Comment on attachment 8744835 [details] [diff] [review]
Part2: Test case changes - v3

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

::: dom/presentation/tests/mochitest/file_presentation_receiver.html
@@ +36,5 @@
>    return new Promise(function(aResolve, aReject) {
>      ok(navigator.presentation, "navigator.presentation should be available.");
>      ok(navigator.presentation.receiver, "navigator.presentation.receiver should be available.");
>  
> +    navigator.presentation.receiver.connectionList.then(

same as in OOP test case.

::: dom/presentation/tests/mochitest/file_presentation_receiver_establish_connection_error.html
@@ +34,5 @@
>    return new Promise(function(aResolve, aReject) {
>      ok(navigator.presentation, "navigator.presentation should be available.");
>      ok(navigator.presentation.receiver, "navigator.presentation.receiver should be available.");
>  
> +    navigator.presentation.receiver.connectionList.then(

According to latest spec, connectionList will never be resolved if fail to establish connection. You may use |Promise.race()| to test if connectionList is not be resolved or be rejected at certain amount of time. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/race

::: dom/presentation/tests/mochitest/file_presentation_receiver_oop.html
@@ +38,5 @@
>      ok(navigator.presentation.receiver, "navigator.presentation.receiver should be available in OOP receiving pages.");
>  
> +    navigator.presentation.receiver.connectionList.then(
> +      function(aList) {
> +        is(aList.connections.length, 0, "The connection list should be empty before onconnectionavailable event.");

According to the latest spec, there will be one connected connection available after promise resolved.

@@ +77,5 @@
>      aResolve();
>    });
>  }
>  
> +function testGetConnection() {

we can change this to |testConnectionListSameObject| and test if promise is the same and can be resolved correctly.
Attachment #8744835 - Flags: feedback?(schien)
Depends on: 1267965
Whiteboard: btpp-active → btpp-active [ETA 5/26]
Based on the sequence diagram on wiki.
Attachment #8744834 - Attachment is obsolete: true
Attachment #8744835 - Attachment is obsolete: true
Attachment #8749598 - Flags: feedback?(schien)
Attached patch Part2: Test case changes - v4 (obsolete) — Splinter Review
Rebase and address previous comment.
Attachment #8749599 - Flags: feedback?(schien)
Comment on attachment 8749598 [details] [diff] [review]
Part1: Add PresentationConnectionList - v4

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

Getting closed!

::: dom/presentation/PresentationConnection.cpp
@@ +44,3 @@
>    : DOMEventTargetHelper(aWindow)
>    , mId(aId)
>    , mState(aState)

mState(PresentationConnectionState::Connecting) should be enough.

@@ +236,1 @@
>    return DispatchStateChangeEvent();

According to https://wiki.mozilla.org/WebAPI/PresentationAPI:WebIDL_Implementation, we should do |DispatchStateChangeEvent| before |NotifyStateChange|.

::: dom/presentation/PresentationConnection.h
@@ +28,5 @@
> +  static already_AddRefed<PresentationConnection>
> +  Create(nsPIDOMWindowInner* aWindow,
> +         const nsAString& aId,
> +         const uint8_t aRole,
> +         PresentationConnectionState aState,

PresentationConnection is always created with 'connecting' state, we can remove it from |Create| and ctor.

::: dom/presentation/PresentationConnectionList.cpp
@@ +46,5 @@
> +}
> +
> +/* virtual */ void
> +PresentationConnectionList::DisconnectFromOwner()
> +{

|DisconnectFromOwner| might be called if user navigate to other web page. We shouldn't remove the connections since user might come back via history.

@@ +109,5 @@
> +                                              PresentationConnection* aConnection)
> +{
> +  if (!aConnection) {
> +    return;
> +  }

MOZ_ASSERT(aConnection)?

@@ +130,5 @@
> +      break;
> +    case PresentationConnectionState::Closed:
> +      if (!connectionFound) {
> +        mConnections.AppendElement(aConnection);
> +      }

If a PresentationConnection never become connected, then we shouldn't expose it to connection list.

@@ +133,5 @@
> +        mConnections.AppendElement(aConnection);
> +      }
> +      break;
> +    case PresentationConnectionState::Terminated:
> +        mConnections.RemoveElement(aConnection);

We can use |connectionFound| to reduce one list scan.

::: dom/presentation/PresentationReceiver.cpp
@@ +35,4 @@
>  {
> +  RefPtr<PresentationReceiver> receiver =
> +    new PresentationReceiver(aWindow);
> +  return NS_WARN_IF(!receiver->Init()) ? nullptr : receiver.forget();

I'm also modifying this function in bug 1234128. Patch checked-in later will need to resolve the conflict.
Attachment #8749598 - Flags: feedback?(schien)
Comment on attachment 8749599 [details] [diff] [review]
Part2: Test case changes - v4

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

I'll merge the file_presentation_receiver_oop.html and file_presentation_1ua_receiver_oop.html into their corresponding non-oop file in bug 1234128. You'll only need to modify one place after my patch landed.
Attachment #8749599 - Flags: feedback?(schien) → feedback+
Attachment #8749598 - Attachment is obsolete: true
Attachment #8751080 - Flags: feedback?(schien)
Comment on attachment 8751080 [details] [diff] [review]
Part1: Add PresentationConnectionList - v5

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

lgtm
Attachment #8751080 - Flags: feedback?(schien) → feedback+
Attachment #8751080 - Flags: review?(bugs)
Attachment #8749599 - Flags: review?(bugs)
Reviews are coming. Sorry about delay. Trying to get to this and other presentation API patches tomorrow.
Comment on attachment 8751080 [details] [diff] [review]
Part1: Add PresentationConnectionList - v5

>@@ -215,17 +222,26 @@ PresentationConnection::NotifyStateChange(const nsAString& aSessionId,
>     }
> 
>     nsresult rv = service->UnregisterSessionListener(mId, mRole);
>     if(NS_WARN_IF(NS_FAILED(rv))) {
>       return rv;
>     }
>   }
> 
>-  return DispatchStateChangeEvent();
>+  nsresult rv = DispatchStateChangeEvent();
Otherwise the patch looks ok, but I don't see statechange anywhere in the latest spec.
But looks like our PresentationConnection interface doesn't follow the latest spec exactly anyhow.
So, fine, but please file a followup to make us follow the spec (or file a spec bug to make it follow what we do, whatever is the right thing)
Attachment #8751080 - Flags: review?(bugs) → review+
Comment on attachment 8749599 [details] [diff] [review]
Part2: Test case changes - v4

> function testConnectionReady() {
>   return new Promise(function(aResolve, aReject) {
>     info('Receiver: --- testConnectionReady ---');
>     connection.onstatechange = function() {
>       connection.onstatechange = null;
>-      is(connection.state, "connected", "Receiver: Connection state should become connected.");
>-      aResolve();
>+      ok(false, "Should not get |onstatechange| event.")
>+      aReject();
>     };
Could you explain this. Since we do still dispatch statechange event.
with that, r+
Attachment #8749599 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #28)
> Comment on attachment 8751080 [details] [diff] [review]
> Part1: Add PresentationConnectionList - v5
> 
> >@@ -215,17 +222,26 @@ PresentationConnection::NotifyStateChange(const nsAString& aSessionId,
> >     }
> > 
> >     nsresult rv = service->UnregisterSessionListener(mId, mRole);
> >     if(NS_WARN_IF(NS_FAILED(rv))) {
> >       return rv;
> >     }
> >   }
> > 
> >-  return DispatchStateChangeEvent();
> >+  nsresult rv = DispatchStateChangeEvent();
> Otherwise the patch looks ok, but I don't see statechange anywhere in the
> latest spec.
> But looks like our PresentationConnection interface doesn't follow the
> latest spec exactly anyhow.
> So, fine, but please file a followup to make us follow the spec (or file a
> spec bug to make it follow what we do, whatever is the right thing)

Thanks for reviewing.

Bug 1258600 is created for PresentationConnectionClosedEvent and also making PresentationConnection to be aligned with spec.
(In reply to Olli Pettay [:smaug] from comment #29)
> Comment on attachment 8749599 [details] [diff] [review]
> Part2: Test case changes - v4
> 
> > function testConnectionReady() {
> >   return new Promise(function(aResolve, aReject) {
> >     info('Receiver: --- testConnectionReady ---');
> >     connection.onstatechange = function() {
> >       connection.onstatechange = null;
> >-      is(connection.state, "connected", "Receiver: Connection state should become connected.");
> >-      aResolve();
> >+      ok(false, "Should not get |onstatechange| event.")
> >+      aReject();
> >     };
> Could you explain this. Since we do still dispatch statechange event.
> with that, r+

Please see the flow diagram in [1]. The first incoming PresentationConnection object can be retrieved is when the state became connected and the promise for getting the connection list is resolved. So, it is not possible to get the first state change event (connecting -> connected).

[1] https://wiki.mozilla.org/WebAPI/PresentationAPI:WebIDL_Implementation#Get_Connection_List
Duplicate of this bug: 1254489
According to the spec, the promise of starting the session should be resolved when a available presentation display is selected. At this moment, the state of the PresentationConnection should be connecting.

So, this patch adjusts the flow of starting session and the corresponding test cases to follow the spec.
Attachment #8757326 - Flags: review?(schien)
Attachment #8757326 - Flags: review?(bugs)
Comment on attachment 8757326 [details] [diff] [review]
Part3: Changes for making the initial state to “connecting”

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

r- because the semantic change for calling callback->NotifyError.

::: dom/presentation/PresentationService.cpp
@@ +66,5 @@
> +  const nsAString& aRequestUrl,
> +  const nsAString& aId,
> +  const nsAString& aOrigin,
> +  uint64_t aWindowId,
> +  nsIPresentationServiceCallback* aCallback)

I'll prefer style like https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsChild.cpp#884

@@ +146,5 @@
>  
>  NS_IMETHODIMP
>  PresentationDeviceRequest::Cancel()
>  {
> +  return mCallback->NotifyError(NS_ERROR_DOM_ABORT_ERR);

PresentationSessionInfo::ReplyError does more than calling callback->NotifyError but also remove itself from the table in PresentationService.

We'll have to do the same thing, otherwise those dead session info will stay referenced by PresentationService.

::: dom/presentation/tests/mochitest/test_presentation_1ua_sender_and_receiver.html
@@ +128,5 @@
>        connection = aConnection;
>        ok(connection, "Sender: Connection should be available.");
>        ok(connection.id, "Sender: Connection ID should be set.");
> +      is(connection.state, "connecting", "The initial state should be connecting.");
> +      connection.onstatechange = function() {

Just a reminder, this need to be changed if bug 1258600 lands first and so does in other test cases.
Attachment #8757326 - Flags: review?(schien) → review-
> @@ +146,5 @@
> >  
> >  NS_IMETHODIMP
> >  PresentationDeviceRequest::Cancel()
> >  {
> > +  return mCallback->NotifyError(NS_ERROR_DOM_ABORT_ERR);
> 
> PresentationSessionInfo::ReplyError does more than calling
> callback->NotifyError but also remove itself from the table in
> PresentationService.
> 
> We'll have to do the same thing, otherwise those dead session info will stay
> referenced by PresentationService.
> 

In this patch, the controlling session info is created in PresentationDeviceRequest::Select, so I think it should be fine to only call mCallback->NotifyError in PresentationDeviceRequest::Cancel. There will be no dead session info in PresentationService.

> :::
> dom/presentation/tests/mochitest/test_presentation_1ua_sender_and_receiver.
> html
> @@ +128,5 @@
> >        connection = aConnection;
> >        ok(connection, "Sender: Connection should be available.");
> >        ok(connection.id, "Sender: Connection ID should be set.");
> > +      is(connection.state, "connecting", "The initial state should be connecting.");
> > +      connection.onstatechange = function() {
> 
> Just a reminder, this need to be changed if bug 1258600 lands first and so
> does in other test cases.
Will do.
Flags: needinfo?(schien)
(In reply to Kershaw Chang [:kershaw] from comment #40)
> > @@ +146,5 @@
> > >  
> > >  NS_IMETHODIMP
> > >  PresentationDeviceRequest::Cancel()
> > >  {
> > > +  return mCallback->NotifyError(NS_ERROR_DOM_ABORT_ERR);
> > 
> > PresentationSessionInfo::ReplyError does more than calling
> > callback->NotifyError but also remove itself from the table in
> > PresentationService.
> > 
> > We'll have to do the same thing, otherwise those dead session info will stay
> > referenced by PresentationService.
> > 
> 
> In this patch, the controlling session info is created in
> PresentationDeviceRequest::Select, so I think it should be fine to only call
> mCallback->NotifyError in PresentationDeviceRequest::Cancel. There will be
> no dead session info in PresentationService.
Thanks for the explanation, the timing that session info created has been delayed until the very end of device selection procedure and I misread your patch.
Switch to r+.
Flags: needinfo?(schien)
Attachment #8757326 - Flags: review- → review+
Attachment #8757326 - Flags: review?(bugs) → review+
Thanks S.C. and smaug for reviewing.

Test result looks ok.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39791f67757a
Rebase and carry r+.
Attachment #8749599 - Attachment is obsolete: true
Attachment #8751080 - Attachment is obsolete: true
Attachment #8751081 - Attachment is obsolete: true
Attachment #8757326 - Attachment is obsolete: true
Attachment #8757750 - Flags: review+
Keywords: checkin-needed
Attachment #8757750 - Flags: approval-mozilla-b2g48?
Attachment #8757751 - Flags: approval-mozilla-b2g48?
Attachment #8757766 - Flags: approval-mozilla-b2g48?
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: PresentationConnnectionList 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
Flags: needinfo?(jocheng)
Whiteboard: btpp-active [ETA 5/26] → btpp-active [ETA 5/26][ft:conndevices]
Comment on attachment 8757750 [details] [diff] [review]
Part1: Add PresentationConnectionList, r=smaug

Approve for TV 2.6
Flags: needinfo?(jocheng)
Attachment #8757750 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Comment on attachment 8757751 [details] [diff] [review]
Part2: Test case changes, r=smaug

Approve for TV 2.6
Attachment #8757751 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Comment on attachment 8757766 [details] [diff] [review]
Part3: Changes for making the initial state to “connecting”, r=smaug

Approve for TV 2.6
Attachment #8757766 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
PresentationReceiver.webidl doesn't have a link to the spec that the IDL came from (which is what we normally do for IDL from specs).  Did it not come from a spec?
Flags: needinfo?(kechang)
Or no, it did actually come from a spec :)
r=me to add the link to the idl... ;)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #57)
> r=me to add the link to the idl... ;)

I've filed bug 1310607 for this.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.