Re: [screen-wake-lock] editorial: Tidy up uses of "in parallel". (#299)

Thanks for the review, @annevk. I've pushed some new commits addressing some of the points you've brought up.

> Some thoughts:
> 
> * obtain permission shouldn't run in parallel, it should be invoked from in parallel, but it doesn't make sense for it itself to then also go in parallel again (in parallel steps cannot return things)

I was hoping to get rid of the algorithm altogether in #298, but in the meantime I've dropped the "in parallel" bit in this PR.

> * A slightly clearer style is to say "run these steps in parallel" and then have a subsequent step return the promise.

Done.

> * You really want to use "queue a global task" and always pass it the global and task source to use explicitly. That's the way forward.

Is there any particular reason for using "global" here? I recently defined a task source in #301 and was using the regular "queue a task" language with it.

> Questions:
> 
> * I'm not sure about hidden documents. There are some issues about this against HTML as well. (Checking the `hidden` attribute is definitely wrong though. You don't want to invoke IDL members in your algorithms as a general rule.)

Hmm, I wonder if the algorithms in "handling document loss of {full activity, visibility}" could set some flag that could be checked in the parallel steps of `WakeLock.request()`. Some sort of "_abort when_ some-flag is set // _if aborted_ reject promise".

> * Yeah, it seems "acquire a wake lock" could be run twice if you invoke the method twice.

Would it be OK (from an idiomatic perspective) to remove the check altogether and sort of under-specify the behavior by leaving it up to each implementation to determine whether to ask the platform for a lock?

> * The order of event/promise in `release()` seems fine. It might be clearer to run "release a wake lock" when the slot is false and then have a single return step at the end.

Thanks, done.
  
>   * If you don't want `release()` to fire an event directly (though not sure if it matters much), you could make it queue a task to run "release a wake lock" and resolve the promise with undefined. That way other callers of "release a wake lock" would still dispatch the event in whatever task they find themselves in.

I'm trying to determine if there's some general guidance here. If it's basically "fire events away and only queue a task to fire them if necessary", then I'm fine with the changes being done here.

-- 
GitHub Notification of comment by rakuco
Please view or discuss this issue at https://github.com/w3c/screen-wake-lock/pull/299#issuecomment-777514937 using your GitHub account


-- 
Sent via github-notify-ml as configured in https://github.com/w3c/github-notify-ml-config

Received on Thursday, 11 February 2021 14:48:18 UTC