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

Adds steps for initiating presentation from default presentation request. #369

Merged
merged 5 commits into from Nov 23, 2016

Conversation

mfoltzgoogle
Copy link
Contributor

This PR addresses the issues noted in Issue #359 ("setting presentation.defaultRequest will have no effect" note is ambiguous). Specifically:

  1. It clarifies that initiation of a presentation from the default request must come from the browser "chrome", not from any action of the page.
  2. It lists specific steps for initiating a presentation from start() that check preconditions, allow the user to select a display, and return a Promise.
  3. It lists specific steps for initiating a presentation from a default request that checks preconditions.
  4. It clarifies that user agents that do not support initiation from a default request should ignore any value set for navigator.presentation.defaultRequest.

@mfoltzgoogle
Copy link
Contributor Author

Copy link
Member

@tidoust tidoust left a 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
Copy link
Member

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"?

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.

Input
</dt>
<dd>
<var>W</var>, the document that the user intends to present
Copy link
Member

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?

Copy link
Contributor Author

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.
Copy link
Member

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

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.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

s/the the/the/

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.

@mfoltzgoogle
Copy link
Contributor Author

Did you have any comments @schien? Otherwise I plan to merge tomorrow Pacific time.

Copy link
Contributor

@schien schien left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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"?

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.

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.

None yet

3 participants