Skip to content

Issue #387: Monitoring algo takes pending request to start into account #392

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 21, 2016
Merged

Issue #387: Monitoring algo takes pending request to start into account #392

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97ee0f6
Issue #387: Monitoring algo now takes pending request to start into a…
tidoust Dec 14, 2016
e14c2ec
Issue #387: Use availabilitySet in the monitoring algorithm
tidoust Dec 16, 2016
4491260
Issue #387: drop "exposed to JS" and allow monitoring aggregation
tidoust Dec 20, 2016
db2e701
Issue #387: Update notes with @mfoltzgoogle suggestions
tidoust Dec 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 71 additions & 57 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,10 +1117,10 @@ <h4>
abort these steps.
</li>
<li>If there is already an unsettled <a>Promise</a> from a previous
call to <code>start</code> on any <code>PresentationRequest</code>
in the same <a>controlling browsing context</a>, return a new
<a>Promise</a> rejected with an <a>OperationError</a> exception and
abort all remaining steps.
call to <a for="PresentationRequest">start</a> on any
<a>PresentationRequest</a> in the same <a>controlling browsing
context</a>, return a new <a>Promise</a> rejected with an
<a>OperationError</a> exception and abort all remaining steps.
</li>
<li>Let <var>P</var> be a new <a>Promise</a>.
</li>
Expand Down Expand Up @@ -1542,6 +1542,11 @@ <h3>
any <a>available presentation display</a> for at least one of the
<a>presentation request URLs</a> of the request.
</p>
<p>
The <a>presentation display availability</a> for a presentation
request is eligible for garbage collection when no ECMASCript code
can observe the <code>PresentationAvailability</code> object.
</p>
<p>
If the <a>controlling user agent</a> can <a>monitor the list of
available presentation displays</a> in the background (without a
Expand All @@ -1552,8 +1557,9 @@ <h3>
</p>
<p>
The <dfn for="PresentationAvailability">value</dfn> attribute MUST
return the last value it was set to. The value is updated by the
<a>monitor the list of available presentation displays</a> algorithm.
return the last value it was set to. The value is initialized and
updated by the <a>monitor the list of available presentation
displays</a> algorithm.
</p>
<p>
The <dfn for="PresentationAvailability">onchange</dfn> attribute is
Expand All @@ -1564,15 +1570,6 @@ <h3>
<h4>
The set of presentation availability objects
</h4>
<p>
Each <code>PresentationRequest</code> has a <a>presentation
availability promise</a> and a <a>presentation display
availability</a>, which are initially set to <code>null</code>. The
<dfn>presentation availability promise</dfn> for a
<code>PresentationRequest</code> is a <code>Promise</code> object
that <a data-lt="resolve">resolves</a> to the <a>presentation
display availability</a> for the request.
</p>
<p>
The <a>user agent</a> MUST keep track of the <dfn>set of
presentation availability objects</dfn> created by the
Expand Down Expand Up @@ -1677,19 +1674,15 @@ <h4>
</li>
</ol>
</li>
<li>If the <a>presentation availability promise</a> for
<code>presentationRequest</code> is not <code>null</code>, return
the <code>presentationRequest</code> object's <a>presentation
availability promise</a> and abort these steps.
<li>If there is an unsettled <a>Promise</a> from a previous call to
Copy link
Contributor

Choose a reason for hiding this comment

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

If the returned Promise is settled, this will repeat all the steps below, generating a new Availability object, which is what we were trying to avoid. Drop "unsettled" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the returned Promise is settled, it will generate a new Promise, but will return the same PresentationAvailability object if it still exists "If the presentation display availability for presentationRequest is not null, then Resolve P with the request's presentation display availability" (step 6).

This works as I would like it to work but I am unclear how we would like it to. As noted in:

I think the intent with getAvailability is to return the same Promise as long as it is unsettled. To be confirmed with @schien (also raised in #388 (comment)). If the goal is rather to return the same Promise as long as the code holds a reference to a Promise previously returned by that function, the suggestion below does not work.
#390 (comment)

If I drop "unsettled", the same Promise will be returned as long as it has not been garbage collected. Is it the intent?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I drop "unsettled", the same Promise will be returned as long as it has not been garbage collected. Is it the intent?

We (Chrome) are implementing the behavior right now (to align with the spec before this PR).

My concern was with the creation of duplicate Availability objects, which is addressed by re-reading the spec.

I don't feel strongly about the lifetime of the Promises generated, although a live but resolved Promise must keep the Availability object alive (otherwise it would be unable to pass it to a resolver function), so developers will need to be careful to not hold references to resolved Promises.

I'm also okay with generating a new Promise each time, but I think @schein raised the original concern with resolution order, and at TPAC we agreed to follow the Battery API model of returning the same Promise to avoid this issue [1]. So I think the language you've proposed is okay for this PR; I can re-open the issue of Promise re-use if I'm not happy with the final result.

For comparison, the Web Bluetooth API generates a new Promise each time watchAdvertisements() is called [2].

[1] https://www.w3.org/2016/09/22-webscreens-minutes#ins11
[2] https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothdevice-watchadvertisements

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll have to check our implementation here to see how to match the spec. We just landed a patch to make the Promise an internal slot of the Availability object so the same one is always returned.

https://codereview.chromium.org/2572473003

Copy link
Member Author

Choose a reason for hiding this comment

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

It may indeed be overly restrictive to impose the use of a new Promise as soon as the previous one is settled. Dropping "unsettled" would give you the freedom that you need. I did not do it but, FWIW, I'm fine with it!

<a for="PresentationRequest">getAvailability</a> on
<code>presentationRequest</code>, return that <a>Promise</a> and
abort these steps.
</li>
<li>Otherwise, set the <code>presentationRequest</code> object's
<a>presentation availability promise</a> to a newly created <code>
Promise</code> object and set <var>P</var> to this
<code>Promise</code>.
<li>Otherwise, let <var>P</var> be a new <a>Promise</a>.
</li>
<li>Return the <code>presentationRequest</code> object's
<a>presentation availability promise</a>, but continue running
these steps <a>in parallel</a>.
<li>Return <var>P</var>, but continue running these steps <a>in
parallel</a>.
</li>
<li>If the user agent is unable to continuously <a>monitor the list
of available presentation displays</a> in the background, but can
Expand Down Expand Up @@ -1717,34 +1710,20 @@ <h4>
</li>
<li>Set the <a>presentation display availability</a> for
<var>presentationRequest</var> to a newly created
<code>PresentationAvailability</code> object, and let <var>A</var>
be that object.
</li>
<li>Set the <code>value</code> property of <var>A</var> as follows:
<ol>
<li>
<code>false</code> if the <a>list of available presentation
displays</a> is empty.
</li>
<li>
<code>true</code> if there is at least one <a>available
presentation display</a> for some member of
<var>presentationUrls</var>. That is, there is an entry
<var>(presentationUrl, display)</var> in the <a>list of
available presentation displays</a> for some
<var>presentationUrl</var> in <var>presentationUrls</var>.
</li>
<li>
<code>false</code> otherwise.
</li>
</ol>
<a>PresentationAvailability</a> object, and let <var>A</var> be
that object.
</li>
<li>Create a tuple <em>(<var>A</var>,
<var>presentationUrls</var>)</em> and add it to the <a>set of
presentation availability objects</a>.
</li>
<li>Run the algorithm to <a>monitor the list of available
presentation displays</a> <a>in parallel</a>.
presentation displays</a>.
<div class="note">
The monitoring algorithm must be run at least one more time
after the previous step to pick up the tuple that was added to
the <a>set of presentation availability objects</a>.
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

The algorithm must be run at least one more time after the previous step to pick up the tuple that was added to the set of availability objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

</li>
<li>
<a>Resolve</a> <var>P</var> with <var>A</var>.
Expand All @@ -1761,11 +1740,28 @@ <h4>
"PresentationRequest" data-lt="start">select a presentation
display</a>, the <a>user agent</a> MUST <dfn>monitor the list of
available presentation displays</dfn> by running the following
steps.
steps:
</p>
<ol link-for="PresentationAvailability">
<li>Set the <a>list of available presentation displays</a> to the
empty list.
<li>Let <var>availabilitySet</var> be a shallow copy of the <a>set
Copy link
Contributor

Choose a reason for hiding this comment

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

availabilitySet seems to be unused below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, that was an oversight.

of presentation availability objects</a>.
</li>
<li>If there is a pending request to <a for="PresentationRequest"
data-lt="start">select a presentation display</a> for a
<a>PresentationRequest</a> and if the <a>PresentationRequest</a>'s
<a>presentation display availability</a> is <code>null</code>, then
run the following substeps:
<ol>
<li>Let <var>A</var> be a newly created
Copy link
Contributor

Choose a reason for hiding this comment

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

Another approach, which might be better or worse, is to define a separate set of "pending presentation start requests" and have the monitoring algorithm consider those in addition to the "set of availability objects." It seems that re-using "set of availability objects" for both getAvailability() and start() adds complexity to the monitoring algorithm.

Maybe something worth trying in a branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need a set though? There can be only one "pending presentation start request" per controlling browsing context (enforced by step 4 in the "select a presentation display" algorithm).

Or does that mean that I'm incorrectly restricting the monitoring algorithm and the "set of presentation availability objects" to the controlling browsing context?

No matter what, I agree we seem to have two algorithms into one: one "monitoring" algorithm that is meant to track availability changes of existing objects on a periodic basis, and one algorithm that we want to apply to a specific availability object (a temporary one when start is called, one from the set when getAvailability is called). It may be clearer to split things into two algorithms, although they obviously share the "Retrieve presentation displays" step.

(By the way, shouldn't the above mentioned step 4 be extended to reject the promise if there is an unsettled promise in nested contexts or in ancestor contexts as well?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a set though? There can be only one "pending presentation start request" per controlling browsing context (enforced by step 4 in the "select a presentation display" algorithm).

Yes there will be one per browsing context, but the monitoring steps run (in principle) by aggregating across browsing contexts, so there might be simultaneous requests from different contexts. Or are we assuming that all state mentioned in the spec is associated with a specific browsing context?

(By the way, shouldn't the above mentioned step 4 be extended to reject the promise if there is an unsettled promise in nested contexts or in ancestor contexts as well?)

Good point and I likely agree, although it adds implementation complexity. File as a separate issue and I'll think about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or are we assuming that all state mentioned in the spec is associated with a specific browsing context?

That would be my assumption unless stated otherwise (for instance, the definition of the "set of controlled presentations" is explicit that the set spans browsing contexts).

Aggregating the monitoring steps across browsing contexts is a valid optimization, but do we need to specify it? Does that change any visible behavior?

We could complete the note next to the algorithm to mention that user agents may want to handle monitoring across browsing contexts.

(By the way, shouldn't the above mentioned step 4 be extended to reject the promise if there is an unsettled promise in nested contexts or in ancestor contexts as well?)
Good point and I likely agree, although it adds implementation complexity. File as a separate issue and I'll think about it.

Done in #394

Copy link
Contributor

Choose a reason for hiding this comment

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

Aggregating the monitoring steps across browsing contexts is a valid optimization, but do we need to specify it? Does that change any visible behavior?

It does not change visible behavior but it does change whether we need to aggregate multiple pending requests to start() in the spec. Another way to put it: the spec should not be written to prohibit running the monitoring algorithm once to handle multiple pending start() requests.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not change visible behavior but it does change whether we need to aggregate multiple pending requests to start() in the spec. Another way to put it: the spec should not be written to prohibit running the monitoring algorithm once to handle multiple pending start() requests.

I completed the editorial note that follows the algorithm to say that user agents may aggregate monitoring activities across browsing contexts. This implies aggregating multiple pending requests to start(), which our algo does not specify but I think that should be left up to the user agent.

Said differently, we could turn the algorithm into an algorithm that runs across contexts, but then we would have no way to test whether an implementation follows that algorithm since there would be no observable difference compared to the algorithm we have right now. So I would suggest to keep it simple (single context only) and let user agents optimize as they wish.

<a>PresentationAvailability</a> object.
</li>
<li>Create a tuple <em>(<var>A</var>,
<var>presentationUrls</var>)</em> where
<var>presentationUrls</var> is the <a>PresentationRequest</a>'s
<a>presentation request URLs</a> and add it to
<var>availabilitySet</var>.
</li>
</ol>
</li>
<li>Let <var>newDisplays</var> be an empty list.
</li>
Expand All @@ -1776,9 +1772,12 @@ <h4>
<li>Retrieve <a>presentation displays</a> (using an implementation
specific mechanism) and set <var>newDisplays</var> to this list.
</li>
<li>Set the <a>list of available presentation displays</a> to the
empty list.
</li>
<li>For each member <em>(<var>A</var>,
<var>availabilityUrls</var>)</em> of the <a>set of presentation
availability objects</a>, run the following steps:
<var>availabilityUrls</var>)</em> of <var>availabilitySet</var>,
run the following steps:
<ol>
<li>Set <var>previousAvailability</var> to the value of
<var>A</var>'s <a>value</a> property.
Expand All @@ -1805,6 +1804,10 @@ <h4>
</li>
</ol>
</li>
<li>If <var>A</var>'s <a>value</a> property has not yet been
initialized, then set <var>A</var>'s <a>value</a> property to
<var>newAvailability</var> and skip the following step.
</li>
<li>If <var>previousAvailability</var> is not equal to
<var>newAvailability</var>, then <a>queue a task</a> to run the
following steps:
Expand All @@ -1821,15 +1824,26 @@ <h4>
</ol>
</li>
</ol>
<div class="note">
The <a>controlling user agent</a> may choose how often to
<a>monitor the list of available presentation displays</a>,
including grouping requests from <a for=
"PresentationRequest">start</a> and <a for=
"PresentationRequest">getAvailability</a>, and aggregating them
across <a data-lt="browsing context">browsing contexts</a>.
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be simpler to say:

The controlling user agent may choose how often to monitor the list of available presentation displays, including grouping requests from start() and getAvailability(), and aggregating them across browsing contexts.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is clear and it reads better. I replaced the whole note with the suggested text.

<p>
When a <a>PresentationAvailability</a> object is no longer alive
(i.e., is eligible for garbage collection), the <a>user agent</a>
SHOULD run the following steps:
When a <a>presentation display availability</a> object is eligible
for garbage collection, the <a>user agent</a> SHOULD run the
following steps:
</p>
<ol>
<li>Let <var>A</var> be the newly deceased
<a>PresentationAvailability</a> object
</li>
<li>Find and remove any entry <em>(<var>A</var>,
<var>availabilityUrl</var>)</em> in the <a>set of presentation
availability objects</a> for the newly deceased <var>A</var>.
availability objects</a>.
</li>
<li>If the <a>set of presentation availability objects</a> is now
empty and there is no pending request to <a for=
Expand Down