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.

Sign up for GitHub

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

Jump to bottom

Should the fragment identifier be stripped from the scriptURL? #1249

Closed
cdumez opened this issue Dec 15, 2017 · 8 comments
Closed

Should the fragment identifier be stripped from the scriptURL? #1249

cdumez opened this issue Dec 15, 2017 · 8 comments

Comments

@cdumez
Copy link

cdumez commented Dec 15, 2017

Should the fragment identifier be stripped from the scriptURL? I cannot find any indication that we should in the specification. However, Firefox and Chrome seem to do so and it is tested via:

  • service-workers/service-worker/serviceworkerobject-scripturl.https.html
@cdumez
Copy link
Author

cdumez commented Dec 15, 2017

cc @beidson

@bsittler
Copy link

See #742

@cdumez
Copy link
Author

cdumez commented Dec 16, 2017

#742 was closed in 2015 and it seems Firefox and Chrome still have not aligned with the specification.
Also, there is disagreement between web-platform-tests and the specification.

@cdumez
Copy link
Author

cdumez commented Dec 16, 2017

cc @jungkees

@jungkees
Copy link
Collaborator

Good catch! The spec and the test don't agree now. I'm happy to change the spec if the existing implementations agree on not storing fragments. If so, I presume it wouldn't break things much, right? Also, that behavior aligns to what we do for storing scope urls. @aliams, could you confirm if it'd be okay for Edge too?

/cc @jakearchibald @wanderview @mattto

@jungkees jungkees mentioned this issue Dec 16, 2017
Closed
hubot pushed a commit to WebKit/WebKit-http that referenced this issue Dec 17, 2017
Strip fragment identifier from ServiceWorker's scriptURL
45152af 
https://bugs.webkit.org/show_bug.cgi?id=180887

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Rebaseline WPT test now that one more check is passing.

* web-platform-tests/service-workers/service-worker/serviceworkerobject-scripturl.https-expected.txt:

Source/WebCore:

Strip fragment identifier from ServiceWorker's scriptURL to match Firefox and Chrome.
This behavior does not appear to be specified so I filed:
- w3c/ServiceWorker#1249

No new tests, rebaselined existing test.

* workers/service/server/SWServerWorker.cpp:
(WebCore::m_script):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@226014 268f45cc-cd09-0410-ab3c-d52691b4dbfc
jungkees added a commit that referenced this issue Dec 19, 2017
@jungkees
Remove fragment identifiers from script url and scope url
c3455b0 
The spec and the implementations did not agree on the behavior: the spec
didn't remove fragment identifiers and had ServiceWorker.scriptURL and
ServiceWorkerRegistration.scope return urls without stripping them.
Chrome, Firefox, and Edge don't return fragments.

This changes the spec to remove the fragment indentifiers from the
script url and scope url given to register() method before storing them,
in order to match the implementations.

Fixes #1249.
@jungkees
Copy link
Collaborator

I tested with Edge and confirmed it doesn't return fragments either.

@jungkees jungkees mentioned this issue Dec 19, 2017
Merged
@docluv
Copy link

docluv commented Dec 20, 2017

Are the hash fragments not stored as part of the request URL?

And would the fragment even make it to the service worker since that part of the URL would not be sent to the server? They are really to drive client-side behavior and not network/server behavior.
If a fragment was used it is retained in the client-side to drive the desired behavior, but would not initiate a network request, assuming the base url did not change.

@jungkees
Copy link
Collaborator

For service workers' script urls and scope urls, we intend to not store the fragments in the internal spec structs by this change. Please note that we don't remove the fragments of the request URLs in the use of the Cache API though.

jungkees added a commit that referenced this issue Dec 22, 2017
@jungkees
Remove fragment identifiers from script url and scope url (#1251)
58fd467 
The spec and the implementations did not agree on the behavior: the spec
didn't remove fragment identifiers and had ServiceWorker.scriptURL and
ServiceWorkerRegistration.scope return urls without stripping them.
Chrome, Firefox, and Edge don't return fragments.

This changes the spec to remove the fragment indentifiers from the
script url and scope url given to register() and getRegistration()
before storing them, in order to match the implementations.

Fixes #1249.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bsittler @docluv @cdumez @jungkees