Skip to content
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

Editorial: correct method control flow #1152

Merged
merged 1 commit into from
May 29, 2017

Conversation

jugglinmike
Copy link
Contributor

The previous control flow for the ServiceWorkerRegistration#update
algorithm did not return a value under certain error conditions. Update
the algorithm steps to consistently return the Promise defined to track
the status of the operation.

The previous control flow for the `ServiceWorkerRegistration#update`
algorithm did not return a value under certain error conditions. Update
the algorithm steps to consistently return the Promise defined to track
the status of the operation.
@ylafon
Copy link
Member

ylafon commented May 27, 2017

Marked as non-substantive for IPR from ash-nazg.

@jungkees jungkees self-requested a review May 29, 2017 02:02
Copy link
Collaborator

@jungkees jungkees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for spotting. update() should have returned a promise and done things in parallel as addressed by this PR.

@jungkees jungkees merged commit 474b57f into w3c:master May 29, 2017
jungkees added a commit that referenced this pull request May 29, 2017
Reviewing the algorithm again, update() method isn't supposed to run in
parallel until the update job is scheduled by Run Job algorithm.
Necessary changes for the original PR will be addressed separately.

This reverts commit 474b57f.
@jungkees
Copy link
Collaborator

@jugglinmike, sorry I didn't recall my intention on the steps of this method reviewing back after a long time and reverted the patch.

The update() method isn't actually supposed to go in parallel until the update job that it creates is scheduled by Run Job algorithm later. The issue that you posed in this PR should still be addressed. I'll do that by filing an issue and moving the guarding steps to appropriate places - maybe to the Update algorithm. (Need to check.)

jungkees added a commit that referenced this pull request May 30, 2017
This patch fixes the control flow of registration.update() method such
that the algorithm returns a promise rejected with an exception on error
conditions. Before this patch, the steps rejected a non-returned promise
and aborted the steps right away.

Issue raised through #1152.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants