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

Updates to PR #277 that addresses issues #254, #269, #273 #278

Merged
merged 4 commits into from
Apr 21, 2016

Conversation

tidoust
Copy link
Member

@tidoust tidoust commented Apr 15, 2016

This is pull request for the pull request #277...

I have the following comments on PR #277:

  1. The definition of the set of availability objects still asserts that availabilityUrl is the parameter passed to getAvailability(). That's an old remain of some past version where getAvailability() indeed took a parameter.
  2. Getting the connectionList attribute creates a new PresentationConnectionList instance for each call, whereas I would assume we would rather return the same instance each time.
  3. Getting the connectionList attribute always returns a promise that is immediately resolved. I believe the reason why we want a Promise in the first place is to ensure that the app gets a connectionList with at least one presentation connection in it (the one that gave birth to the receiving browsing context), so we should wait for that to happen, or we should not return a Promise.
  4. There should no longer be any need to "start monitoring incoming presentation connections" when getting the connectionList attribute, since that is now done in the "starting a presentation" steps

This PR updates the PR #277 to address each of these points:

  1. I dropped "parameter" and used "presentation request URL" instead
  2. I added the fact that a PresentationReceiver has an associated PresentationConnectionList object, and that getting the connectionList attribute resolves the returned promise with that object.
  3. Getting the connectionList attribute now only returns a resolved promise if the set of presentation controllers is non empty. The promise is resolved when the first PresentationConnection object is added to the list otherwise (note that the connectionavailable event is not fired when that happens, which seems to be the right thing to do).
  4. I dropped the step that instructed the user agent to start monitoring incoming presentation connections.

For 2., it could be better to find a real dfn name for the PresentationConnectionList object associated with the PresentationReceiver object, but I could not really come up with a good name (we already have "set of presentation controllers", I tried "incoming connection reporter" but that looked awkwards).

…c#269)

That used to be the case, but getAvailability now automatically uses the
presentation request URL.
Getting the connectionList attribute now returns the same
PresentationConnectionList instance, and only resolves the promise when the
set of presentation controllers is non empty (as things stood, the use of a
Promise was useless).

The commit also drops the step that instructed the receiving user agent to start
monitoring incomping presentation connections if not already started. This is
done in the "starting a presentation" algorithm, no need to repeat it here.
@@ -2282,6 +2283,10 @@
<a>receiving user agent</a>.
</p>
<p>
A <a>PresentationReceiver</a> object is associated with a
<a>PresentationConnectionList</a> object when it is created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second "it" is a little ambiguous. Suggested rephrasing:

When the PresentationReceiver object is created, a PresentationConnectionList object is also created and associated with that PresentationReceiver object.

@markafoltz
Copy link
Contributor

Thanks @tidoust I agree with your improvements.

One more detail: in the case where there is already at least one PresentationConnection in the list, and connectionList is retrieved twice (without a change in the set of controllers), should we return two Promises? Or the second time should we return the (settled) Promise created by first call?

For reference Issue #201 is where the current PresentationReceiver API was hashed out, mostly following the proposal by @sicking.

@tidoust
Copy link
Member Author

tidoust commented Apr 19, 2016

I updated the PR based on your feedback.

I confess I do not know whether two consecutive retrieval of connectionList should return the same settled Promise. When the promise is unsettled, it makes sense to reuse it. When the promise is settled, I thought user agents might want to garbage collect that object whenever possible.

Trying to find a similar example elsewhere, I bumped into the Battery Status API spec, which seems to reuse the same promise:
http://www.w3.org/TR/battery-status/#the-navigator-interface

I would suggest to ask the editors... @anssiko and @mounirlamouri, should getting the connectionList property always return the same promise? (and why?)

@@ -2418,14 +2423,22 @@
</li>
<li>Add <var>S</var> to the <a>set of presentation controllers</a>.
</li>
<li>If there is an unsettled <a>Promise</a> <var>P</var> from a
previous call to get the <a for=
"PresentationReceiver">connectionList</a> attribute for the same
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the update above, this could be simply "If there is an unsettled Promise P associated with the PresentationReceiver object, ..."

@markafoltz
Copy link
Contributor

Could there be any difference in API behavior with one choice or the other? For example what does it mean to run the following:

PresentationReceiver.connectionList.then(function(list) { console.log("first"); })
PresentationReceiver.connectionList.then(function(list) { console.log("second"); })

Do we expect to see "first", "second" or just "second"?

@tidoust
Copy link
Member Author

tidoust commented Apr 20, 2016

In both cases, you would see both "first" and "second" (a second call to then on the same Promise does not replace the first). The order is guaranteed if connectionList always returns the same Promise. There may be edge cases if connectionList only returns the same Promise as long as it is unsettled, although I think things will just work as expected in the end.

That said, having to think about edge cases typically shows that the design could be improved. Also, come to think about it, developers probably expect an attribute to always return the same object in any case. Last but not least, as pointed out by @schien in #281, the IDL currently requires the attribute to always return the same Promise (due to the presence of [SameObject]).

I'll update the PR.

This update should address last Pull Request comments, as well as w3c#281.

Note I ended up introducing two new idioms to talk about the Promise and
PresentationConnectionList singletons, as I believe it improves the readability
of algorithms, and makes the list of objects that user agents need to maintain
explicit.

However, as usual, finding names for things is hard. I came up with
"presentation controllers monitor" and "presentation controllers promise".
It may be possible to find better names. For instance, instead of "monitor", we
could use "manager", "holder", "wrapper", "container".
@tidoust
Copy link
Member Author

tidoust commented Apr 20, 2016

I updated the pull request as planned. This should also fix #281 "PresentationReceiver.connectionList doesn't return the same object all the time".

As explained in the commit message, I introduced two new idioms: "presentation controllers monitor" (the PresentationConnectionList object) and "presentation controllers promise" (the Promise to get that object) to make it easy to reference these concepts in the spec. It may be possible to find better names.

(FWIW, I'll be out of office for about a week starting tomorrow)

<li>If the <a>set of presentation controllers</a> contains at least
one <a>presentation connection</a>, then run the following steps:
<ol>
<li>Let the <a>presentation controllers monitor</a> be a new <a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to be explicit that the connections property of the new PresentationConnectionList should be populated with the contents of the presentation controllers monitor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that getting the connections property of the new PresentationConnectionList de facto returns "the non-terminated set of presentation connections in the set of presentation controllers":
https://w3c.github.io/presentation-api/#dom-presentationconnectionlist-connections

That quoted sentence could perhaps be moved here, as the section that describes the PresentationConnectionList interface should probably remain generic and not assume that there is going to be only one PresentationConnectionList instance per browsing context.

@markafoltz
Copy link
Contributor

@tidoust, overall looks good. I traced through the various possibilities and concluded the following:

  1. There always at most one presentation controllers promise per receiving browsing context
  2. There is always at most one PresentationConnectionList per context that will be used to resolve the promise in Multi-display support #1
  3. That singleton list gets a 'connected' event as additional connections are made.

Since you'll be out I'll merge in your changes and tweak a few things to make those assumptions clearer, and add a step to populate the PresentationConnectionList.

Thanks again!

@markafoltz markafoltz merged commit 34c8b84 into w3c:issues-254-269-273 Apr 21, 2016
@tidoust tidoust deleted the issues-254-269-273 branch May 4, 2016 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants