-
Notifications
You must be signed in to change notification settings - Fork 40
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 PresentationRequest to take multiple URLs #316
Conversation
@@ -511,14 +511,15 @@ | |||
// The Present button is visible if at least one presentation display is available | |||
var presentBtn = document.getElementById("presentBtn"); | |||
// It is also possible to use relative presentation URL e.g. "presentation.html" | |||
var presUrl = "http://example.com/presentation.html"; | |||
var presUrls = ["http://example.com/presentation.html", | |||
"http://another.com/alternate.html"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples should all use example.com
, example.net
, example.org
or subdomains:
http://www.w3.org/2001/06/manual/#Examples
I suggest to switch to: http://example.net/alternate.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
See a couple of comments inline. The only substantive comment is that I would have expected the Changes to algorithms look all good to me otherwise! |
@@ -928,7 +929,8 @@ | |||
Interface <a><code>PresentationRequest</code></a> | |||
</h3> | |||
<pre class="idl"> | |||
[Constructor(DOMString url)] | |||
[Constructor(DOMString url), | |||
Constructor(DOMString[] urls)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to allow (DOMString[] or DOMString) for simplicity and backward compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for me. That seems to improve readability as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest we keep the @mfoltzgoogle's initial form with two [Constructor]
s. That allows us to address both the singular url
as well as its plural form urls
in the algorithm that follows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed reply, was on vacation last week. The dual constructor form should be backwards compatible, although do I assume an implementation will be able to overload a single-argument constructor in this way? For example code generated from the .idl would need to examine the argument type to dispatch to an appropriate function.
I'm not opposed to using a union type instead if there's a preference.
Thanks @mfoltzgoogle for crafting this substantial PR. All - this PR has been open for 2 weeks, so my expectation is everyone has had time to review it. The comments received have been addressed by the editor sans @mounirlamouri's #316 (comment) that suggested using a single @mounirlamouri could you clarify the compatibility issue? Meanwhile, I'll merge merge this PR, with a condition that we may switch to the union type if the compatibility impact is deemed significant. |
This PR addresses Issue #311 (Add presentation request URL fallback mechanism). It modifies the PresentationRequest constructor to take an array of URLs each of which will be considered in order when starting or connecting to a presentation.
This necessitated several changes, including:
url
property to PresentationConnection so the caller knows what is being presented@mounirlamouri @tidoust @sicking @schien Please take a look.