-
Notifications
You must be signed in to change notification settings - Fork 40
Adds steps for initiating presentation from default presentation request. #369
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
Conversation
Rawgit URL for review: https://rawgit.com/w3c/presentation-api/issue-359-default-request/index.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.
See minor nits inline. This looks to me otherwise!
<p> | ||
Support for the initiation of a <a>presentation connection</a> from | ||
the browser is OPTIONAL. | ||
Support initating presentation using the <a>default presentation |
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.
s/initating/initiating
Also, shouldn't that be "Support for initiating"?
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.
Input | ||
</dt> | ||
<dd> | ||
<var>W</var>, the document that the user intends to present |
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 may be reading that incorrectly but, for me, the document that the user intends to present is the document that will be created on the receiving side when the presentation gets started. Use "the document on which the user expresses an intent to start presentation", instead?
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.
Yes, that's what I meant to say :) Updated text per your suggestion.
</h4> | ||
<p> | ||
When the <code><dfn for="PresentationRequest">start</dfn></code> | ||
method is called, the <a>user agent</a> MUST run the following | ||
steps to <dfn>start a presentation</dfn>: | ||
steps to <dfn>select a presentation display</dfn>, and then | ||
<a>start a presentation connection</a> with the selected display. |
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.
Although I don't see anyone could mis-interpret this and run the second algorithm twice, I would drop the end of the sentence.
In practice, the user agent will not run "select a presentation display", and then "start a presentation connection". It will rather run "select a presentation display" which includes a call to "start a presentation connection".
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.
@@ -1208,6 +1306,8 @@ | |||
The event must not bubble, must not be cancelable, and has no | |||
default action. | |||
</li> | |||
<li>Let <var>U</var> be the the user agent connected to D. |
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.
s/the the/the/
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.
Did you have any comments @schien? Otherwise I plan to merge tomorrow Pacific time. |
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.
overall lgtm.
@@ -1170,9 +1150,124 @@ | |||
all remaining steps. | |||
</li> | |||
<li>Otherwise, the user <em>granted permission</em> to use a |
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.
Not sure why we use past tense here, "user grants permission" should be enough. Same thing in "user denied permission" few lines before.
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.
Both done.
@@ -1195,8 +1293,8 @@ | |||
</li> | |||
<li>Add <var>S</var> to the <a>set of controlled presentations</a>. | |||
</li> | |||
<li> | |||
<a>Resolve</a> <var>P</var> with <var>S</var>. | |||
<li>If <var>P</var> was provided, <a>resolve</a> <var>P</var> with |
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.
Use "If P is provided"?
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.
This PR addresses the issues noted in Issue #359 ("setting presentation.defaultRequest will have no effect" note is ambiguous). Specifically:
start()
that check preconditions, allow the user to select a display, and return a Promise.navigator.presentation.defaultRequest.