-
Notifications
You must be signed in to change notification settings - Fork 40
Step 11 of getAvailability should not run "in parallel" #381
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
Comments
Agreed. |
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:
Practically speaking, I expect that developers will need to add an @schien et al what do you think? |
"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.
A second call to
I note that could affect tests as well, and @tomoyukilabs raised the question of the "initial value" in: I would personally expect that initial value to convey "complete" information, meaning the "same" result as if I had called This is what the test on |
Correct, but we'll still need a Promise to be able to notify the caller when availability information is known.
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 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." |
…o 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 w3c#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 w3c#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 w3c#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.
…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
Should be addressed by PR #387. |
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 aPresentationAvailability
object whosevalue
isfalse
(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 thevalue
to convey the outcome of running the monitoring algorithm.The text was updated successfully, but these errors were encountered: