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 PresentationRequest to take multiple URLs #316

Merged
merged 8 commits into from
Jun 27, 2016

Conversation

markafoltz
Copy link
Contributor

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:

  • Updating example code
  • Updating interfaces, including adding a url property to PresentationConnection so the caller knows what is being presented
  • Updating the set of availability objects to have a list of URLs per object
  • Updating the list (set?) of available presentation displays to keep track of which displays are compatible with which URLs so the start algorithm can choose the appropriate URL for the chosen display
  • Various other algorithm and wording updates

@mounirlamouri @tidoust @sicking @schien Please take a look.

@@ -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"];
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@tidoust
Copy link
Member

tidoust commented Jun 14, 2016

See a couple of comments inline. The only substantive comment is that I would have expected the PresentationRequest constructor that only takes one URL to remain in the spec.

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)]
Copy link
Member

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?

Copy link
Member

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!

Copy link
Member

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.

Copy link
Contributor Author

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.

@anssiko
Copy link
Member

anssiko commented Jun 27, 2016

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 [Constructor] with a union type (DOMString[] or DOMString) as its argument over the use of multiple [Constructor] extended attributes [Constructor(DOMString url), Constructor(DOMString[] urls)].

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

@anssiko anssiko merged commit b6abdde into gh-pages Jun 27, 2016
@markafoltz markafoltz deleted the issue-311-multiple-urls branch September 27, 2016 21:36
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.

4 participants