search

w3c / web-locks

Cross-tab resource coordination API
https://w3c.github.io/web-locks/
Other
124 stars 16 forks source link

Early return when a request is not grantable #83

Closed saschanaz closed 3 years ago

saschanaz commented 3 years ago

Fixes #82

Preview | Diff

saschanaz commented 3 years ago

Doing this makes the "request is not the first item" check in the term "grantable" redundant, since:

  1. The queue processing algorithm only checks the grantability of the first item, so it's always the first.
  2. ifAvailable: true checks grantability before enqueuing, so the queue must be empty for it to be grantable

So, I think we can remove the check and instead add a queue emptiness check for ifAvailable. What do you think, @inexorabletash ?

inexorabletash commented 3 years ago

Aside: I noticed "To get the lock request queue from lock request queue map queueMap from resource name name, run these steps:" should read: "To get the lock request queue from lock request queue map queueMap for resource name name, run these steps:". We should fix that too, either this PR or elsewhere.

inexorabletash commented 3 years ago

Doing this makes the "request is not the first item" check in the term "grantable" redundant, since:

  1. The queue processing algorithm only checks the grantability of the first item, so it's always the first.
  2. ifAvailable: true checks grantability before enqueuing, so the queue must be empty for it to be grantable

So, I think we can remove the check and instead add a queue emptiness check for ifAvailable. What do you think, @inexorabletash ?

To confirm, the ifAvailable check would say e.g. "If ifAvailable is true, and either queueMap is not empty or request is not grantable, then ..."

Then... SGTM I think. I'd add a "Note: ..." after the definition of grantable to indicate that this does not consider the state of the request queue.

@pwnall can you also think through this?

saschanaz commented 3 years ago

either queueMap is not empty

(Should be queue instead of queueMap)

inexorabletash commented 3 years ago

either queueMap is not empty

(Should be queue instead of queueMap)

Yes, thanks!

If I haven't said so before: thank you very much for jumping in. As you can probably tell, I haven't been spending much time thinking about this spec recently, so having you look at it in detail is greatly appreciated. If you think I'm missing something (like this) please do call it out. And if I'm terse in my comments it's just because I'm distracted by other shiny things. You're doing awesome, please keep it up!

(On that note, there's an outstanding PR on adopting Storage terminology that it'd be great to get your eyes/feedback on.)

inexorabletash commented 3 years ago

Thank you! Apologies for the delay.

saschanaz commented 3 years ago

Thanks!

BTW this didn't implement https://github.com/WICG/web-locks/pull/83#issuecomment-946250361, should I open another PR or do you have any objection?

inexorabletash commented 3 years ago

Let's do another PR