Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PresentationReceiver: rename getConnection() and getConnections() #201

Closed
mounirlamouri opened this issue Oct 6, 2015 · 30 comments
Closed
Assignees
Labels

Comments

@mounirlamouri
Copy link
Member

CC @avayvod @mfoltzgoogle @sicking

I have looked at the receiver side of the API fairly rarely and every time I am to remember that getConnection() is actually waiting for a connection while getConnection**s** return the list of active connections. I would suggest to rename them to:

interface PresentationReceiver {
  Promise<PresentationConnection> waitForConnection();
  Promise<sequence<PresentationConnection>> connections;
  attribute EventHandler onconnectionavailable;
};

I'm dubious about waitForConnection because it's basically the same as the connectionavailable event or did I miss the subtlety here?

@mounirlamouri
Copy link
Member Author

@avayvod and I talked to @jakearchibald and we came up with this proposition:

interface PresentationReceiver {
  Promise<> ready;
  sequence<PresentationConnection> connections;
  attribute EventHandler onconnectionavailable;
};

It made me wonder why we couldn't simply do:

interface PresentationReceiver {
  sequence<PresentationConnection> connections;
  attribute EventHandler onconnectionavailable;
};

It seems that if we accept that connections can be returned synchronously and the event will be fired when there is a new connection, pages could have code like:

function handshake(c) {
   // ...
}

function onloadhandler() {
  navigator.presentation.receiver.onconnectionavailable = handshake;
  navigator.presentation.receiver.connections.forEach(handshake);
}

WDYT?

@sicking
Copy link

sicking commented Oct 6, 2015

I don't have a strong opinion either way. But if we do have a synchronous .connections property, we need to define when exactly the connection which launched the page on the TV shows up there. More specifically, is .connections.length==1 when the page on the TV is first launched?

@schien should probably comment on how much work it would be to make the initial connection synchronously available.

@avayvod
Copy link
Contributor

avayvod commented Oct 6, 2015

The F2F meeting notes about the issue 19 that led to the current spec: http://www.w3.org/2015/05/19-webscreens-minutes.html#item12

The argument against having an array property was that the prototype can be changed by the web developers. It doesn't apply to Sequence though afaik.
The other argument against the proposal was about supporting a simple use case of one connection - then the page wouldn't need an event handler or care about a sequence of the connections. This is why we left getSession() method.

@sicking I think the timing doesn't really matter in the Mounir's example - if the connection is already initialized, it could be populated by the time the page accesses the sequence but if it's not, then the page will receive an event.

I think the question now is about the easiness to support the simple use case - one presentation, one connection.

We also need to keep in mind a case when the page wants to designate itself as a presentation, that's what Promise<> ready property could be used for. But as long as it's a backward compatible change, we could add it later. See issue #32

@sicking
Copy link

sicking commented Oct 6, 2015

Yes, a readonly attribute sequence<PresentationConnection> connections wouldn't be mutable by the webpage.

The timing matters because we need to define if something like the following should work:

<html>
<head>
<script>
navigator.presentation.receiver.connections[0].send("I'm ready to rock");
</script>
<body>
...

@mounirlamouri
Copy link
Member Author

I think the best think the spec could say is that the code above has an undefined behaviour. Maybe the connection will be set, maybe not. The code is clearly wrong, assuming that connections has one element which might not be the case obviously. What should be done is to try to use the current connections and listen for new ones, don't you think?

Regarding synchronous-ity, my understanding is that the main process will notify the child process that a connection was created. Having a synchronous array with an event should make the design implementatable in that case. I'm not sure what situation would make the synchronous array not implementation.

@sicking
Copy link

sicking commented Oct 6, 2015

If the behavior of the example is undefined, then I'd rather stick to using an asynchronous promise-based API.

@mfoltzgoogle
Copy link
Contributor

The Promise-based API is to avoid race conditions when the page attempts to use the first connection before it's ready, or adds an event handler after the first connection is ready. In some sense there is a "stream" or queue of incoming connections provided asynchronously to the page, and it's crucial that the presentation be able to easily consume the first as well as subsequent elements.

The desire to have a simple API for the common case of a single controller motivated the addition of the getConnection() function - I believe @anssiko advocated for this feature.

I actually like the following proposal, which is more parallel to how screen availability is handled (promise for initial state + event/attribute for updates):

interface PresentationReceiver {
  Promise<PresentationConnection> waitForConnection();
  readonly attribute sequence<PresentationConnection> connections;
  attribute EventHandler onconnectionavailable;
};

The simple case is handled by calling waitForConnection which will resolve on the initial (starting) controller connecting. Multi-controller applications can add an event handler for connectionavailable in the resolver to be notified of subsequent connections, but this is optional.

@sicking
Copy link

sicking commented Oct 6, 2015

I agree with @mfoltzgoogle regarding preferred API. I think the synchronous API adds more risk of races in the page.

@mounirlamouri
Copy link
Member Author

The reason why I started to think about another proposal than the initial one I made is because there are a few issues with it. We could solve these issues by specifying behaviour in the specification but I think it will still allow developers to shot themselves in the foot.

  • What should be the behaviour of waitForConnection() if there is already one connection? Two? If the first connection has been disconnected?
  • Given that connections is asynchronous and I expect that we will pass a PresentationConnection in the connectionavailable event, there might be some race conditions here. For example: a call to connections is made, connectionavailable fires, connections promise is resolved. What does the sequence<PresentationConnection> contain? All the connections including the new one or not including the new one? I think it will be easy for implementers and authors to overlook that.

Also, I'm not really convinced that having an asynchronous connections property solve all the misusage of the API. For example, one could write this:

<html>
<head>
<script>
navigator.presentation.receiver.connections.then(c => c[0].send("I'm ready to rock"););
</script>
<body>
...

Which would be as wrong as the previous code from Jonas above.

@avayvod
Copy link
Contributor

avayvod commented Oct 7, 2015

I don't think we need to couple the events "the presentation is loaded and ready" with "there's a connection made".
The following would be more similar to how the availability is handled:

navigator.presentation.receiver.then(function (receiver) {
  if (!receiver.connections.empty) receiver.connections[0].send("I'm ready to rock.");

So receiver would be a promise property that resolves once the page is loaded and/or is a presentation. It would have the standard sync value and an onchange event interface for connections.

I don't think we should distinguish the first connection from the rest given that:
a) there might not be any connections when the page is ready (e.g. if it becomes a presentation rather than it's launched from the controlling page)
b) the initial connection can always disconnect and the presentation might want to wait for another one
c) there might be more than one connection made to the receiving context before the page has loaded and asked for connection(s)

Ideally we would have a way for the controller to specify if the presentation only supports one connection and then the presentation could use some simpler path but as of today, there's no way for the presentation or controller to require one persistent connection so it's up to the implementation.

@mounirlamouri
Copy link
Member Author

Having receiver has a promise isn't that different from connections as a promise and will make it more complex for a website to know if it is being run as a receiver (ie. checks if navigator.presentation.receiver is null.

If we follow Anton's lead and not distinguish first connections from the others but still want to keep the one connection UC easy, what about having something as simple as:

interface PresentationReceiver {
  Promise<PresentationConnection> waitForNextConnection();
};

It would basically behave like a stream and will resolve the promise when a PresentationConnection is available. It solves a couple of issues like having to deal with whether we should show closed connections in connections. The website would have to take care of handling all the connections itself.
(Anton pointed to me that he suggested something similar in the past so it must definitely be a great idea ;))

WDYT?

@mfoltzgoogle
Copy link
Contributor

@mounirlamouri Thanks for reminding me of #19 where this API was hashed out before.

It looks like I was not a big fan of exposing the full sequence of connections to the presenting page (see #19 (comment)).

From what I can tell, the arguments for exposing the sequence of connections were to allow the presenting page to implement a cap on the number of controllers, and possibly to simplify broadcast messaging. Either can be implemented without the sequence however. It means presentation has to manage its own Array of connections for the multi-controller case. As we are already under the assumption that the multi-controller case is uncommon, I am not too concerned. And it can be added later if needed.

In the spirit of moving forward with the minimal viable API, let's go with the proposal in #201 (comment). We would need to nail down the following behaviors:

  1. Controller connects while there is no unresolved Promise from waitForNextConnection.
  2. Presentation creates multiple unresolved Promises via waitForNextConnection while there is no pending connection.
  3. When does the connection change state to "connected" on the controller? When it's used to resolve the Promise, or earlier?

For #1 and #2 the presenting UA would maintain a queue of incoming connections and Promises and resolve them in order. For #3 we need to either specify a more complete connection handshake, or mandate buffering until the presentation is actually to receive messages.

@sicking
Copy link

sicking commented Oct 8, 2015

I'm not sure I understand the proposed behavior of waitForNextConnection. What qualifies as "next". I.e. if a connection is established, and 5 seconds later the page for the first time calls waitForNextConnection, will that wait for the second connection?

Does each call to waitForNextConnection "consume" a connection from the queue? And so the scenario above is guaranteed to always return the first connection.

What happens if you have two parts of the page which both needs access to the connection. Does that mean that the second caller of waitForNextConnection will wait for a second connection which is never coming?

I think we should think about what we're trying to archive with this API. My goals have been:

  • Make it possible to get a list of connections and get notified whenever that list changes.
  • If possible, create a simpler syntax sugar API for the common case of only having a single connection. (Have we added the API to make multiple connections opt-in yet?)
  • Reduce the risk of races.

I think the API in #201 (comment) archives that. It means that for the common case, when an app only ever deals with a single connection, you just write

navigator.presentation.receiver.waitForConnection().then(
  (connection) => doStuff(connection));

And for pages which supports multiple connections you'd do something like:

var myConnections = [];
function updateConnections() {
  navigator.presentation.receiver.connections.then(
    (connections) => {
      myConnections = connections;
      updateUI();
    }
}
navigator.presentation.receiver.onconnectionavailable = updateConnections;
updateConnections();

Yes, someone might try to support the single-connection usecase by writing

navigator.presentation.receiver.connections.then(
  (connections) => doStuff(connections[0]));

But there's really very little reason to do so since the correct code both looks more correct and is just as simple.

We could even cover this case too by saying that the .connections promise shouldn't resolve for the first time until the connection which caused the page to get started has been properly created. That should be easy to do implementation-wise.

@avayvod
Copy link
Contributor

avayvod commented Oct 8, 2015

I'm not sure I understand the proposed behavior of waitForNextConnection. What qualifies as "next". > I.e. if a connection is established, and 5 seconds later the page for the first time calls
waitForNextConnection, will that wait for the second connection?
Does each call to waitForNextConnection "consume" a connection from the queue? And so the
scenario above is guaranteed to always return the first connection.

I believe that's the intent, yes.

What happens if you have two parts of the page which both needs access to the connection.
Does that mean that the second caller of waitForNextConnection will wait for a second connection which is never coming?

Yes. Do you think that could be a common problem? Why wouldn't the page just pass the first connection it receives to its other parts?

I believe that losing the single connection would be a problem and most of the presentation pages would like to allow users to reconnect, so having waitForConnection() return the new connection sounds useful (the page would call it again when the first connection changes to terminated state, for example).

I think we should think about what we're trying to archive with this API. My goals have been:

Make it possible to get a list of connections and get notified whenever that list changes.

I think the multiple connections case is far less common so I'm not sure why we should optimize the first iteration of the spec for that.

If possible, create a simpler syntax sugar API for the common case of only having a single connection. (Have we added the API to make multiple connections opt-in yet?)

No, we haven't added such an API, so even if the page only cares about one connection, it can potentially get more connections but ignore them. The UA could then terminate the ignored connection after a timeout, for example. It might be harder to do though if we introduce the sequence<PresentationConnection> to the API in any form.

Reduce the risk of races.

I think having just one method accomplishes that better than three overlapping things. Having an event and two promises for one and many connections seems to be more likely to introduce races (e.g. event is fired for a new connection, the page calls getConnections() but the connection is lost in the meanwhile so the page doesn't get any connection when the promise is resolved).

WDYT?
I also think that the minimalism approach to this spec is also a good argument in favor of just one method: we could always add better syntax for multiple connections support later.

@sicking
Copy link

sicking commented Oct 8, 2015

What happens if you have two parts of the page which both needs access to the connection.
Does that mean that the second caller of waitForNextConnection will wait for a second connection which is never coming?/

Yes. Do you think that could be a common problem? Why wouldn't the page just pass the first connection it receives to its other parts?

I don't know how common it will be to have multiple parts of a page which all need access to the list of connections.

What I do know is that pages are often vastly more complex than we give them credit for. So I would not be surprised if it will happen more often than we think. For example I would expect people to write frameworks or libraries that take care of a bunch of the connection management. But that those frameworks/libraries won't cover all the cases that a page cares about, and so the page will also use the "raw" API we're defining here.

I also know that keeping state known to the browser hidden from pages almost always ends up with developers eventually wanting access to that state. Similarly, defining "smart" APIs which try to hide details from developers more often than not just makes things harder for developers.

So having an API which, after you've called waitForConnection() once, makes it impossible to get at that connection object again, sounds like something that will be a pain for developers. It creates hidden state which the page author can't get to.

I believe that losing the single connection would be a problem and most of the presentation pages would like to allow users to reconnect, so having waitForConnection() return the new connection sounds useful (the page would call it again when the first connection changes to terminated state, for example).

I thought reconnecting means that you're still using the same PresentationConnection object? So for reconnecting we wouldn't fire onconnectionavailable nor resolve any "queuing" waitForConnection()?

Make it possible to get a list of connections and get notified whenever that list changes.

I think the multiple connections case is far less common so I'm not sure why we should optimize the first iteration of the spec for that.

I agree that we should definitely not optimize for multiple connections. But I do think that we should support it. If we should optimize for anything, it should be for supporting a single connection.

This is actually why I don't like the current waitForConnection() proposal. It seems decent at handling multiple connections, but it seems far from the API that we'd do if we only cared about supporting a single connection.

@avayvod
Copy link
Contributor

avayvod commented Oct 9, 2015

I thought reconnecting means that you're still using the same PresentationConnection object? So for
reconnecting we wouldn't fire onconnectionavailable nor resolve any "queuing" waitForConnection()?

I meant a case when the user launches a presentation (e.g. a video) on the screen and then the controlling device loses Wi-Fi or runs out of power - the presentation should continue playing or maybe pause and wait for the user to connect from the rebooted or even another device. In this case the first PresentationConnection would definitely go to the terminated state and the page would have to wait for another one. So using only one PresentationConnection at a time doesn't mean using the one same connection during the lifetime of the presentation.

For example I would expect people to write frameworks or libraries that take care of a bunch of the
connection management. But that those frameworks/libraries won't cover all the cases that a page
cares about, and so the page will also use the "raw" API we're defining here.

I would expect the libraries to give access to the existing connection so the page doesn't have to use the API for that esp. if the spec will say the same connection can never be obtained again via the API. I'd say that compared to the above problem of reconnecting, the problem of libraries doing the wrong thing is less important.

This is actually why I don't like the current waitForConnection() proposal. It seems decent at handling
multiple connections, but it seems far from the API that we'd do if we only cared about supporting a
single connection.

I don't think it's that far: the only difference is how it would behave if called twice - return the same connection or wait for the other one. Maybe we should spec that it will return the same connection until it's terminated and then it will return the next one that's not? Then we might have two methods to also support multiple connections:

interface PresentationReceiver {
  // Returns he first PresentationConnection in the queue that has non-terminated state. Will return the      
  // same connection until it becomes terminated, then will move on to the next one.
  Promise<PresentationConnection> getActiveConnection();

  // Returns the next connection in the connections queue; never returns the same connection.
  Promise<PresentationConnection> getNextConnection();

I also know that keeping state known to the browser hidden from pages almost always ends up with
developers eventually wanting access to that state. Similarly, defining "smart" APIs which try to hide
details from developers more often than not just makes things harder for developers.

I definitely agree but it depends on the algorithms defined in the spec, too, doesn't it? If the behavior of "smart" methods is well defined, it's no longer magic, it's just convenient.

@mounirlamouri
Copy link
Member Author

It seems that it's very easy to shot yourself in the foot in various ways with waitForNextConnection(). What about implementing connections as a synchronous property with an event notifying the page that the property has been updated?

Jonas previously argued that websites might try to get the first connection at page load. I'm not sure how much we should care about that given that it would obviously be an invalid assumption to think that there is a connection available at page load.

We could also propose a library simplifying the API for very basic use cases like a one connection with the receiver stopping the presentation when the connection is closed. As Anton pointed, we can't expect that to be the most common use case but we can help people trying to do exactly that by providing them the right framework in user space.

At that point, I feel that we will not be able to find an API that will ideally match all possibles UC so I think we should fin an API that, used correctly, will work for all these UC even if it might require more work for some of them.

@sicking
Copy link

sicking commented Oct 13, 2015

I meant a case when the user launches a presentation (e.g. a video) on the screen and then the controlling device loses Wi-Fi or runs out of power - the presentation should continue playing or maybe pause and wait for the user to connect from the rebooted or even another device. In this case the first PresentationConnection would definitely go to the terminated state and the page would have to wait for another one. So using only one PresentationConnection at a time doesn't mean using the one same connection during the lifetime of the presentation.

This is a good point.

Though for the case when the connection quickly transitions to terminated state, I think we could easily make a waitForNextConnection() function stop returning that connection and instead wait for another one.

But I agree with your conclusion that even in the "single controller" use case, we'll need to worry about having the API juggle multiple connections.

So I think what we're looking for is an API which is focused on letting the page enumerate the set of controllers and be notified when this changes. Here's what I propose:

interface PresentationReceiver {
  readonly attribute Promise<PresentationConnectionList> connections;
};

interface PresentationConnectionList : EventTarget {
  readonly attribute sequence<PresentationConnection> connections;
  attribute EventHandler onconnectionavailable;
};

This is essentially what @mounirlamouri is suggesting. It only adds one async step which would allow the implementation to easily wait with returning the list for the first time until it has populated it with the initial connection.

Or we could make it even simpler and merge PresentationReceiver and Presentation since the former only contains a single property:

partial interface Presentation {
  readonly attribute Promise<PresentationConnectionList> connections;
};

interface PresentationConnectionList : EventTarget {
  readonly attribute sequence<PresentationConnection> connections;
  attribute EventHandler onconnectionavailable;
};

@mounirlamouri
Copy link
Member Author

Your PresentationConnectionListproposal sgtm. I would prefer to keep PresentationReceiver so it is easy for a web page to feature detect whether it is being run in a receiving mode by doing navigator.presentation.receiver != null.

Mark and Anton, WDYT?

@avayvod
Copy link
Contributor

avayvod commented Oct 13, 2015

I see two scenarios here:

  • the UA doesn't support the receiver API at all
  • the UA doesn't allow the page to become a presentation (e.g. user dismissed a permission or something)
    Does it make a difference then if there's a property that can be undefined or a promise that will be rejected?

If yes, I'm happy with having navigator.presentation.receiver.connections as Mounir suggests.
If not, I'm happy with what Jonas suggests.

So either we have:

navigator.presentation.receiver.connections.then(function(connectionsList) {
  for (c : connectionsList.connections) c.send("Ready to rock!");
};

or

navigator.presentation.receiver.then(function(connectionsList) {
  for (c : connectionsList.connections) c.send("Ready to rock!");
};

@avayvod
Copy link
Contributor

avayvod commented Oct 13, 2015

Mounir, in the latter case, I believe we could do the feature detection you want and merge PresentationConnectionsList into Presentation, right?
I think that's the way to go then.

@mounirlamouri
Copy link
Member Author

I'm not entirely sure what you meant but I think that navigator.presentation.connections would be terribly confusing (unclear that these are the receiving connections). I don't really mind if PresentationConnectionList has a connections property or value.

@sicking
Copy link

sicking commented Oct 14, 2015

FWIW, I don't think that navigator.presentation.connections.then(...) is that confusing. It's basically short for navigator.presentation.incomingConnections.then(...), and generally we get far more complaints that we use too long API names, than that we use names too short to be understandable.

@mounirlamouri
Copy link
Member Author

In addition of the confusion, navigator.presentation.receiver is more future proof. If we add anything related to the receiving side of the API, we can add it there. I think it is reasonable to imagine that we might add stuff to the API when the protocol will be standardized because some "hand-wavy" considerations might be clearer.

I don't understand why a long name would be a problem, it's not like there was any downside in doing var receiver = navigator.presentation.receiver; in JS.

@sicking
Copy link

sicking commented Oct 14, 2015

My experience is that authors strongly prefer short names. It's one of the main feedbacks that we got on IndexedDB for example. But that's definitely also in part because names are easy to bikeshed.

So I definitely don't care strongly. If navigator.presentation.receiver.connections.then(...) is the preferred syntax then I'm quite fine with that.

@mounirlamouri
Copy link
Member Author

Sorry for reviving this thread but while I was thinking about the implementation, I realised that we might need to have a better definition of whan the |connections| are expected to be. Should we keep disconnected/terminated connections in there? or should the list only reflects currently available connections?

I would prefer the latter but in that case, we might want to rename the |connectionavailable| event to something like |change| because there might be also disconnections. Arguably, we can have an event for new connections and one for disconnections. I wouldn't mind either.

WDYT?

@sicking
Copy link

sicking commented Oct 21, 2015

I definitely think that we should remove "terminated" connections.

"disconnected" connections can transition back to "connected", right? If the other side calls reconnect()?

If so, it might make sense to keep them in connections. But I don't have a strong opinion either way. Though obviously if we remove "disconnected" connections from connections, we'll have to fire connectionavailable when they are added back. I would probably have a weak preference for that given that I think that many developers are not going to do the work to support reconnecting connections. But that's definitely a weak preference.

The reason I thought that we had a connectionavailable event rather than a change event is that we already fire statechange events on connections when they drop out of the connection list. I'm fine either way. Either way it's probably worth exposing the newly added/removed connection on the event, so that pages don't have to do a diff between two arrays to find what the new connection is.

@avayvod
Copy link
Contributor

avayvod commented Oct 21, 2015

I agree with the following:

  • if the connection goes to terminated state, it's removed from connections
  • the page can learn about the removal by listening to connection's onstatechange and checking against the terminated state
  • disconnected connections should stay in connections as the page will receive onstatechange with the state set to connected so onsessionavailable would be redundant

@mounirlamouri
Copy link
Member Author

Ok. Sounds good. Will write the spec changes.

@mounirlamouri mounirlamouri self-assigned this Nov 19, 2015
mounirlamouri added a commit to mounirlamouri/presentation-api that referenced this issue Nov 19, 2015
mounirlamouri added a commit to mounirlamouri/presentation-api that referenced this issue Nov 19, 2015
mounirlamouri added a commit to mounirlamouri/presentation-api that referenced this issue Dec 2, 2015
mfoltzgoogle added a commit that referenced this issue Dec 2, 2015
Update PresentationReceiver spec to match the results from the discussions in #201
@mounirlamouri
Copy link
Member Author

I think this is now fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants