/presentation-api

New issue

Step 11 of getAvailability should not run "in parallel" #381

Closed
tidoust opened this Issue Dec 1, 2016 · 5 comments

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@tidoust @mfoltzgoogle
@tidoust
Contributor

tidoust commented Dec 1, 2016 • edited

I failed to notice that when I reviewed the update, but I think "in parallel" is wrong in step 11 of the getAvailability algorithm which instructs the user agent to "Run the algorithm to monitor the list of available presentation displays in parallel".

In practice, if the algorithm is run in parallel, that means the first time an app calls getAvailability, it will always end up with a PresentationAvailability object whose value is false (set by step 9.1 since the list of available presentation displays is empty to start with). Is that the intent? I would rather expect the value to convey the outcome of running the monitoring algorithm.
@mfoltzgoogle
Contributor

mfoltzgoogle commented Dec 1, 2016

Agreed.
@mfoltzgoogle
Contributor

mfoltzgoogle commented Dec 1, 2016

Note that it looks like "in parallel" was added for Issue #335 filed by @schien, to resolve the Promise early.

The issue seems to be:

  • If monitoring has not run before for the URLs in the request, we should wait until it completes to return a value.
  • If monitoring has run before then we can resolve immediately with a known value, and run it again to refresh the value.

Practically speaking, I expect that developers will need to add an onchange handler to get correct availability information, so the page will always get the correct value eventually. I guess it's a question of whether they care that the initial value may be based on incomplete information.

@schien et al what do you think?
@tidoust
Contributor

tidoust commented Dec 1, 2016

If monitoring has not run before for the URLs in the request, we should wait until it completes to return a value.

"In parallel" gives the exact opposite result: it makes the monitoring run in the background and thus the promise gets resolved immediately (before monitoring is complete). Actually, we would not even need a Promise given that all other steps can run synchronously.

If monitoring has run before then we can resolve immediately with a known value, and run it again to refresh the value.

A second call to getAvailability will always return the presentation availability promise set during the first call immediately (step 2). This aborts the rest of the algorithm. Which means step 11. will not run and the user agent is not required to run the monitoring algorithm again (it may still do so given that the spec says it may monitor on a continuous basis but that's not mandated in response to the second call to getAvailability).

Practically speaking, I expect that developers will need to add an onchange handler to get correct availability information, so the page will always get the correct value eventually. I guess it's a question of whether they care that the initial value may be based on incomplete information.

I note that could affect tests as well, and @tomoyukilabs raised the question of the "initial value" in:
w3c/web-platform-tests#4192 (comment)

I would personally expect that initial value to convey "complete" information, meaning the "same" result as if I had called start instead of getAvailability. In other words, in simple scenarios, I would expect developers to be able to rely on the resolution of the Promise without having to listen to the change event. What would the Promise convey otherwise?

This is what the test on PresentationAvailability assumes for now.

tidoust referenced this issue in w3c/web-platform-tests Dec 2, 2016

Merged

[presentation-api] Refine getAvailability() test #4257

@mfoltzgoogle
Contributor

mfoltzgoogle commented Dec 3, 2016

"In parallel" gives the exact opposite result: it makes the monitoring run in the background and thus the promise gets resolved immediately (before monitoring is complete). Actually, we would not even need a Promise given that all other steps can run synchronously.

Correct, but we'll still need a Promise to be able to notify the caller when availability information is known.

A second call to getAvailability will always return the presentation availability promise set during the first call immediately (step 2). This aborts the rest of the algorithm. Which means step 11. will not run and the user agent is not required to run the monitoring algorithm again

Correct, the set of non-empty Availability objects is what requires the user agent to monitor continuously, not additional invocations of getAvailability(). The existing Promise would be settled with the previous result. So we don't need to worry about the re-invocation case.

I would expect developers to be able to rely on the resolution of the Promise without having to listen to the change event. What would the Promise convey otherwise?

I agree, and that's what I was proposing in #381 (comment). And dropping "in parallel" achieves that result.

It's possible that a second availability object is created with the same set of URLs as an existing PresentationRequest. In that case, we don't need to wait for the monitor steps to complete and can get a result from an existing object. But I think that's an optimization that doesn't need to be spelled out in the spec.

Before submitting a PR, I wanted to give @schien a chance to weigh in because he filed an issue that added "in parallel."

@tidoust tidoust added a commit to tidoust/presentation-api that referenced this issue Dec 14, 2016

@tidoust tidoust Issue #387: Monitoring algo now takes pending request to start into a… 
…ccount

Main changes:

- Prose similar to that used in the Web Bluetooth spec used for the garbage
collection note for the "presentation display availability" to clarify the
intent. We may want to adjust this text in #391.
- Notion of "presentation availability promise" dropped. That promise is now
referenced in `getAvailability` as "an unsettled Promise from a previous call
to `getAvailability` on `presentationRequest`". This avoids having to be
explicit about garbage collection rules.
- Step that instructs the UA to run the monitoring algorithm no longer runs
"in parallel" (see #381)
- Note added next to that step to clarify that the monitoring algorithm needs
to run again even if it is still running
- The monitoring algorithm now starts by making a shallow copy of the "set of
presentation availability objects" which gets completed with the right tuple if
there is a pending call to `start` (note there can be at most one such pending
call per controlling browsing context)
- Steps that update the `value` property adjusted to set the value directly for
`PresentationAvailability` objects that have not yet been exposed to ECMAScript
object. This is triggered by the following (new) problem: the `getAvailibility`
promise gets resolved with a `PresentationAvailability` object as soon as the
monitoring algorithm is done running, but the monitoring algorithm "queued a
task" to update the `value` property of `PresentationAvailability` objects. The
`value` property of the returned `PresentationAvailability` object would always
have been the initial value (`false` in most cases), even if the monitoring
algorithm had found an available display. Also, we probably do not want to fire
`change` events for properties that JS code has not yet been given any chance
to read. Here as well, we may want to adjust this text in #391.
- The initial `value` of newly created `PresentationAvailability` objects is
now always `false`. There should be no need to set it to `true` given that the
monitoring algorithm refreshes that value right after that (and given the
previous fix).
- I added a note next to the monitoring algorithm to clarify that a user agent
may interrupt and re-run the algorithm to group requests, which seems like a
possible optimization.
 97ee0f6

tidoust referenced this issue Dec 14, 2016

Merged

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

@mfoltzgoogle mfoltzgoogle added a commit that referenced this issue Dec 21, 2016

@tidoust @mfoltzgoogle tidoust + mfoltzgoogle Issue #387: Monitoring algo takes pending request to start into accou… 
…nt (#392)

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

Main changes:

- Prose similar to that used in the Web Bluetooth spec used for the garbage
collection note for the "presentation display availability" to clarify the
intent. We may want to adjust this text in #391.
- Notion of "presentation availability promise" dropped. That promise is now
referenced in `getAvailability` as "an unsettled Promise from a previous call
to `getAvailability` on `presentationRequest`". This avoids having to be
explicit about garbage collection rules.
- Step that instructs the UA to run the monitoring algorithm no longer runs
"in parallel" (see #381)
- Note added next to that step to clarify that the monitoring algorithm needs
to run again even if it is still running
- The monitoring algorithm now starts by making a shallow copy of the "set of
presentation availability objects" which gets completed with the right tuple if
there is a pending call to `start` (note there can be at most one such pending
call per controlling browsing context)
- Steps that update the `value` property adjusted to set the value directly for
`PresentationAvailability` objects that have not yet been exposed to ECMAScript
object. This is triggered by the following (new) problem: the `getAvailibility`
promise gets resolved with a `PresentationAvailability` object as soon as the
monitoring algorithm is done running, but the monitoring algorithm "queued a
task" to update the `value` property of `PresentationAvailability` objects. The
`value` property of the returned `PresentationAvailability` object would always
have been the initial value (`false` in most cases), even if the monitoring
algorithm had found an available display. Also, we probably do not want to fire
`change` events for properties that JS code has not yet been given any chance
to read. Here as well, we may want to adjust this text in #391.
- The initial `value` of newly created `PresentationAvailability` objects is
now always `false`. There should be no need to set it to `true` given that the
monitoring algorithm refreshes that value right after that (and given the
previous fix).
- I added a note next to the monitoring algorithm to clarify that a user agent
may interrupt and re-run the algorithm to group requests, which seems like a
possible optimization.

* Issue #387: Use availabilitySet in the monitoring algorithm

The variable is obviously useless if it is not referenced. This should have
been part of previous commit.

* Issue #387: drop "exposed to JS" and allow monitoring aggregation

This commit drops the fuzzy "exposed to ECMAScript" wording in the monitoring
algorithm, and replaces it with a more common "initialized" concept. The notion
is clarified in an editorial note next to the step that uses it.

The editorial note that follows the monitoring algorithm was also complete to
mention that user agents may aggregate monitoring activities across browsing
contexts.

* Issue #387: Update notes with @mfoltzgoogle suggestions
 0c800c5
@mfoltzgoogle
Contributor

mfoltzgoogle commented Dec 21, 2016

Should be addressed by PR #387.

mfoltzgoogle closed this Dec 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment