1258602 - [Presentation WebAPI] implement PresentationConnectionList

Status: RESOLVED FIXED

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

Description

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

PresentationConnectionList is introduced for handling connection list enumeration and change notification.

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: WebIDL changes

S.C., could you take a look at this change?
Not sure if this is the correct track to go.

Comment 2

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

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

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionList

S.C, Could you please take a quick look at this patch?
Basically, I just moved codes from PresentationReceiver to PresentationConnectionList.

Thanks.

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Part2: Test case changes

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionList

Fix comment2.

Comment 6

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

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.

Comment 7

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

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.

Comment 8

[Kershaw Chang [:kershaw]]

(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

Comment 9

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

(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.

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionList - v2

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.

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: 0002-Bug-1258602-Part2-Test-case-changes.patch

Comment 12

[Kershaw Chang [:kershaw]]

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 13

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

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.

Comment 14

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

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 15

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

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.

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionList - v3

Based on the latest spec.

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Part2: Test case changes - v3

Comment 18

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

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.

Comment 19

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

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.

Comment 20

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionList - v4

Based on the sequence diagram on wiki.

Comment 21

[Kershaw Chang [:kershaw]]

Attachment: Part2: Test case changes - v4

Rebase and address previous comment.

Comment 22

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

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.

Comment 23

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

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.

Comment 24

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionList - v5

Comment 25

[Kershaw Chang [:kershaw]]

Attachment: Interdiff for v4 and v5 of part1.

Comment 26

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

Comment on attachment 8751080 [details] [diff] [review]
Part1: Add PresentationConnectionList - v5

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

lgtm

Comment 27

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

Reviews are coming. Sorry about delay. Trying to get to this and other presentation API patches tomorrow.

Comment 28

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

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)

Comment 29

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

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+

Comment 30

[Kershaw Chang [:kershaw]]

(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.

Comment 31

[Kershaw Chang [:kershaw]]

(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

Comment 32

[Kershaw Chang [:kershaw]]

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

Comment 33

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9458a5255e7c

Comment 34

[Kershaw Chang [:kershaw]]

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

Comment 35

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f24da41cf66

Comment 36

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=995861b03656

Comment 38

[Kershaw Chang [:kershaw]]

Attachment: Part3: Changes for making the initial state to “connecting”

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.

Comment 39

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

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.

Comment 40

[Kershaw Chang [:kershaw]]

> @@ +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.

Comment 41

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

(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+.

Comment 42

[Kershaw Chang [:kershaw]]

Thanks S.C. and smaug for reviewing.

Test result looks ok.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39791f67757a

Comment 43

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionList, r=smaug

Rebase and carry r+.

Comment 44

[Kershaw Chang [:kershaw]]

Attachment: Part2: Test case changes, r=smaug

Comment 45

[Kershaw Chang [:kershaw]]

Attachment: Part3: Changes for making the initial state to “connecting”, r=smaug, schien

Comment 46

[Kershaw Chang [:kershaw]]

Attachment: Part3: Changes for making the initial state to “connecting”, r=smaug

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54ca84566554
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f15af55efb
https://hg.mozilla.org/integration/mozilla-inbound/rev/3da545fd691f

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54ca84566554
https://hg.mozilla.org/mozilla-central/rev/e0f15af55efb
https://hg.mozilla.org/mozilla-central/rev/3da545fd691f

Comment 49

[Kershaw Chang [:kershaw]]

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

Comment 50

[Josh Cheng [:josh]]

Comment on attachment 8757750 [details] [diff] [review]
Part1: Add PresentationConnectionList, r=smaug

Approve for TV 2.6

Comment 51

[Josh Cheng [:josh]]

Comment on attachment 8757751 [details] [diff] [review]
Part2: Test case changes, r=smaug

Approve for TV 2.6

Comment 52

[Josh Cheng [:josh]]

Comment on attachment 8757766 [details] [diff] [review]
Part3: Changes for making the initial state to “connecting”, r=smaug

Approve for TV 2.6

Comment 53

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/8f0ee7349702af19342f6c172f4f371852e4bec5

Comment 54

[Boris Zbarsky [:bzbarsky]]

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?

Comment 55

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

Yes: http://w3c.github.io/presentation-api/#interface-presentationreceiver

Comment 56

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

Or no, it did actually come from a spec :)

Comment 57

[Boris Zbarsky [:bzbarsky]]

r=me to add the link to the idl... ;)

Comment 58

[Kershaw Chang [:kershaw]]

(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.