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

Clean up Client API to not expose reserved clients #1259

Merged
merged 1 commit into from
Jan 13, 2018
Merged

Conversation

jungkees
Copy link
Collaborator

@jungkees jungkees commented Jan 12, 2018

To reflect the decision to not expose reserved clients
(#1216) and to spec
.resultingClientId and .targetClientId in Nightly only, this:

  • Removes

    • Client.reserved and its associated reserved state (V1, Nightly)
    • ClientQueryOptions.includeReserved (V1, Nightly)
    • FetchEvent.reservedClientId (V1)
    • FetchEvent.targetClientId (V1)
    • FetchEventInit.reservedClientId (V1)
    • FetchEventInit.targetClientId (V1)
  • Changes

    • FetchEvent.reservedClientId to FetchEvent.resultingClientId (Nightly)
    • FetchEventInit.reservedClientId to FetchEventInit.resultingClientId
      (Nightly)
    • Handle Fetch to set FetchEvent.clientId for a navigation request to
      the empty string (V1)
  • Corrects

    • matchedClients with clientObjects in Clients.matchAll() (V1, Nightly)

Related issue: #1245.

This also cleans up sort condition steps in Clients.matchAll() that
fixes #1080 (V1, Nightly)

(Changes for the Clients interface's methods will be addressed as
separate PRs.)


Preview | Diff

To reflect the decision to not expose reserved clients
(#1216) and to spec
.resultingClientId and .targetClientId in Nightly only, this:

* Removes
 - Client.reserved and its associated reserved state (V1, Nightly)
 - ClientQueryOptions.includeReserved (V1, Nightly)
 - FetchEvent.reservedClientId (V1)
 - FetchEvent.targetClientId (V1)
 - FetchEventInit.reservedClientId (V1)
 - FetchEventInit.targetClientId (V1)

* Changes
 - FetchEvent.reservedClientId to FetchEvent.resultingClientId (Nightly)
 - FetchEventInit.reservedClientId to FetchEventInit.resultingClientId
   (Nightly)
 - Handle Fetch to set FetchEvent.clientId for a navigation request to
   the empty string (V1)

* Corrects
 - matchedClients with clientObjects in Clients.matchAll() (V1, Nightly)

Related issue: #1245.

This also cleans up sort condition steps in Clients.matchAll() that
fixes #1080 (V1, Nightly)

(Changes for the Clients interface's methods will be addressed as
separate PRs.)
@jungkees
Copy link
Collaborator Author

clients.get(resultingClientId) should resolve or reject when the resulting client becomes execution ready or thrown away. I'll spec that behavior in a separate PR only for Nightly. For V1, I'll clean up the behaviors of the Clients interface's methods in yet another PR.

@jakearchibald
Copy link
Contributor

LGTM. As you say, the only missing bit is making sure clients.get() waits for execution-ready

@wanderview
Copy link
Member

LGTM.

jungkees added a commit to jungkees/web-platform-tests that referenced this pull request Jan 13, 2018
As per the decision in w3c/ServiceWorker#1216.

Related spec PR: w3c/ServiceWorker#1259.
@jungkees
Copy link
Collaborator Author

Related PR for WPT: web-platform-tests/wpt#9023.

@jungkees jungkees merged commit 07e9487 into master Jan 13, 2018
@jungkees jungkees deleted the client-cleanup branch January 13, 2018 06:50
@mfalken
Copy link
Member

mfalken commented Jan 15, 2018

LGTM too.

mfalken added a commit to mfalken/ServiceWorker that referenced this pull request Jun 7, 2018
jungkees pushed a commit that referenced this pull request Jun 13, 2018
To reflect the decision that we don't expose reserved clients (#1216),
this change introduces the client's discarded flag, and uses that flag and the
execution ready flag to resolve the promise when the client reaches one of those
states instead of capturing and exposing the reserved client.

Follow-up to 07e9487 (#1259).
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.

clarify Clients.matchAll() focus order for nested frames
4 participants