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

[tidoust]

This PR is meant to replace PR #390. It implements the changes discussed there.

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 Steps that re-use objects should specify the relevant settings object/Javascript Realm #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 Step 11 of getAvailability should not run "in parallel" #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 Steps that re-use objects should specify the relevant settings object/Javascript Realm #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.

[markafoltz]

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?

[tidoust]

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?

[markafoltz]

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

[markafoltz]

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

[tidoust]

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!

[markafoltz]

availabilitySet seems to be unused below?

[tidoust]

Oops, that was an oversight.

[markafoltz]

The comment about "exposed" doesn't seem necessary: if it's not accessible to script then there can be no event handler for 'change', and so the step below just becomes a no-op.

[tidoust]

I believe something like this is needed. Let me try to clarify why:

Given a fresh browser session (no previous call to the Presentation API and no previous run of the monitoring algorithm) and a PresentationRequest request, if there is an available presentation display for request, I'd like two things when I call getAvailability:

1. to get a PresentationAvailability object whose value property is true
2. not to receive a change event unless availability really changes

In other words, I would expect the following code not to report the error messages:

request.getAvailability().then(function (A) {
  if (A.value !== true) {
    console.error('The available presentation display was not found!');
  }
  A.onchange = function (evt) {
    // The first "change" event received should change the value to "false"
    if (A.value === true) {
      console.error('Change event received but value did not change!');
    }
    A.onchange = null;
  };
});

If we do not make the "exposed" distinction, the monitoring algorithm will always "queue a task" to change the value and fire a "change" event. That task will run after the getAvailability promise is resolved (promise resolution is a micro-task, and has priority over regular tasks), and thus A.value will be false. Also, since that task gets run after the getAvailability promise is resolved, the JS code will have had time to attach to the change event, and the task is no longer a no-op: the event handler will be able to catch that change event, which we do not want because we're just setting the initial value.

That task is useful for all other PresentationAvailability objects in the set, meaning those that exist due to previous calls to getAvailability.

I'm not crazy about "exposed to ECMAScript code". Another way to do this would be to track (A, availabilityUrls, initialized) tuples with initialized set to false initially. The algorithm would directly set value and set this flag to true if it is not set, and queue a task to change the value otherwise. I was trying to avoid introducing new variables. What do you think?

[markafoltz]

I would like to not rely on "exposed to ECMAscript" to define behavior since (as an implementer) I'm not entirely sure what that means.

Here are two thoughts:

• The first time the monitoring algorithm runs for A, the promise for A should be unsettled, correct? Could you fire the event only after the availability promise is resolved?

• Could step 8 of getAvailability initialize value to undefined, and then the event would fire only when the previous value was set?

[tidoust]

I would like to not rely on "exposed to ECMAscript" to define behavior since (as an implementer) I'm not entirely sure what that means.

OK, let's try to get rid of it.

The first time the monitoring algorithm runs for A, the promise for A should be unsettled, correct? Could you fire the event only after the availability promise is resolved?

That would work but I do not know how to introduce the availability promise at this step of the monitoring algorithm: "If there is an unsettled Promise from the call to getAvailability that created A" seems obscure. Or not?

Could step 8 of getAvailability initialize value to undefined, and then the event would fire only when the previous value was set?

Problem is the value property is a boolean which cannot be undefined. Or perhaps we can still say "If A's value has not been initialized yet" (and drop steps that initialize "value" elsewhere) and let this as an implementation exercise to track whether initialization occurred already?

[tidoust]

I pushed an update to the PR that replaces "exposed to ECMAScript" with an "initialized" notion, explained in an editorial note next to the step that uses it. I'd say that's enough, but if you feel something more formal is needed, we would need to introduce an "initialized" boolean flug for each tuple in the set.

[markafoltz]

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.

[tidoust]

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

[markafoltz]

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.

[tidoust]

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

[markafoltz]

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.

[tidoust]

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.

[markafoltz]

FYI, here is a rawgit URL to help with review.

https://rawgit.com/tidoust/presentation-api/issue-387/index.html#interface-presentationavailability

[markafoltz]

Overall looks good. I have some suggested phrasing on the NOTE-s you can incorporate at your pleasure.

[markafoltz]

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.

[tidoust]

done.

[markafoltz]

I think this is self-explanatory since an attribute can only be initialized once; drop the note?

[tidoust]

ok, dropped.

[markafoltz]

Maybe just say "eligible for garbage collection" to align with the earlier comment about ECMAScript observability.

[tidoust]

done.

[markafoltz]

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.

[tidoust]

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

[markafoltz]

LGTM. Thanks for working on this @tidoust.