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

Processing steps for id now removes the fragment #1122

Merged
merged 13 commits into from
Jun 6, 2024
Merged

Processing steps for id now removes the fragment #1122

Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
09ee6fd
(unrelated) more formal language to set scope's query and fragment to…
mgiuca May 6, 2024
d96b247
id processing: Set fragment to null in the processor.
mgiuca May 6, 2024
2bbb99b
Do not exclude fragments when comparing the manifest id with the docu…
mgiuca May 6, 2024
6814169
Properly link to exclude fragments.
mgiuca May 6, 2024
04a8f52
Added note about why we still exclude fragments when comparing to the…
mgiuca May 6, 2024
70792c0
Rewrote the exclude fragments note, and made exclude fragments optional.
mgiuca May 6, 2024
2540edf
Fix id link
mgiuca May 10, 2024
03f8c65
Fix id link
mgiuca May 10, 2024
11ff7cb
Tidy.
mgiuca May 22, 2024
1282936
Respond to review (non-normative phrasing).
mgiuca May 22, 2024
9ac3a95
Rewrite OPTIONAL.
mgiuca May 22, 2024
ee7537c
Merge branch 'main' into id-no-fragment
marcoscaceres Jun 6, 2024
86c8d94
Apply suggestions from code review
marcoscaceres Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 43 additions & 8 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,8 +626,7 @@ <h3>
</li>
<li>If |scope| is failure, return.
</li>
<li>From |scope|, remove the [=url/query=] and [=url/fragment=]
components.
<li>Set |scope|'s [=URL/query=] and [=URL/fragment=] to null.
</li>
<li>If |manifest|["start_url"] is not [=URL/within scope=] of
|scope|, return.
Expand Down Expand Up @@ -859,13 +858,26 @@ <h3>
application, it SHOULD treat that manifest as a description of a
distinct application, even if it is served from the same URL as that
of another application. When the user agent sees a manifest where
|manifest|["id"] is [=url/equal=] with [=URL serializer/exclude
fragment|exclude fragment true=] to the [=identity=] of an
already-installed application, it SHOULD be used as a signal that
|manifest|["id"] is [=url/equal=] (OPTIONAL: with
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove OPTIONAL... it's a bit confusing. Let's just assume a single model here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The purpose of OPTIONAL here is that some UAs may decide not to do it if they are sure the fragments were already stripped. (e.g. if they always strip fragments before writing them into the database, and there never was a time when they didn't do that, or they ran a migration at some point stripping fragments from the database, then they don't need to exclude fragments here).

Not doing so could actually significantly help those clients, for example, they could then compare the hashes of the app IDs rather than having to perform a URL equals operation (which can't be done using a SQL query). This doesn't only affect major browsers, but also any website that indexes apps -- we'd like it to be optional for them so they can simply assume the fragment-removed app IDs are primary keys in their database and do simple lookups. If we made this mandatory, then every entity that indexes apps would need to have no canonical representation of an app ID, and always do the URL equals excluding fragments comparison.

So I did go back and forth on this a bit, but settled on making it OPTIONAL by way of saying "hey, if you've been storing app IDs with their fragments, you probably should exclude fragments when you compare them, but you don't have to do that if you've treated the data properly per the algorithm, and there are significant simplifications to the data model if you don't do this."

Copy link
Member

Choose a reason for hiding this comment

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

Right, yeah. that seems ok. I wonder if it could be better phrased though the "(OPTIONAL: ...)" looks a bit odd to me. Trying to think of a better way to phrase it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marcoscaceres input needed: I rewrote it as "(with exclude fragments OPTIONALLY set to true)". Is this sufficient to capture RFC 2119's keyword "OPTIONAL"? (Note that Respec doesn't italicize the word "OPTIONALLY" nor does this adverbial form appear in RFC 2119)

Otherwise if it has to exactly use those words, I think the best phrasing is the somewhat verbose: "(the user agent MAY set exclude fragments to true)".

Copy link
Member

@marcoscaceres marcoscaceres May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm 🤔 let’s go with MAY as I just know people will complain otherwise.

I’ll try to see if I can think of some other phrasing in the future.

Really sorry about all the churn on this!

It should be good to merge otherwise (and as a bonus 🍨, this helped fix two bugs in WebKit, so massive thank you!!!)

[=URL/equals/exclude fragments=] set to true) to the [=identity=] of
an already-installed application, it SHOULD be used as a signal that
this manifest is a replacement for the already-installed
application's manifest, and not a distinct application, even if it is
served from a different URL than the one seen previously.
</p>
<p class="note" title="Exclude fragments for good measure">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p class="note" title="Exclude fragments for good measure">
<p class="note" title="Exclude fragments for comparison">

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rephrased as "is best practice". The "good measure" was supposed to convey the fact that you don't really need to do it, but if you can then just do it anyway to sanitize bad data.

I hope this also conveys that user agents don't have to do this and it can be beneficial not to, because it affords optimizations like hashing.

Since the [=process the id member|processing algorithm=] removes the
[=URL/fragment=] from the <code>[=id=]</code> member, it is not
mgiuca marked this conversation as resolved.
Show resolved Hide resolved
strictly necessary to [=URL/equals/exclude fragments=] when checking
for a matching application. However, since old versions of this spec
(and, possibly, old user agents) did not remove the [=URL/fragment=]
from the [=URL=] at parse time, and relied only on
[=URL/equals/exclude fragments|excluding fragments=] during
comparisons, there may be historical app data with [=URL/fragments=]
in the <code>[=id=]</code>. Therefore, it is best practice for user
agents to [=URL/equals/exclude fragments=] even when comparing two
[=URLs=] that should not have any fragments.
</p>
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved
<p class="note">
The [=identity=] can be used by a service that collects lists of web
applications to uniquely identify applications.
Expand Down Expand Up @@ -896,6 +908,8 @@ <h3>
<li>If |id| is not [=same origin=] as |manifest|["start_url"],
return.
</li>
<li>Set |id|'s [=URL/fragment=] to null.
</li>
<li>Set |manifest|["id"] to |id|.
</li>
</ol>
Expand Down Expand Up @@ -935,7 +949,7 @@ <h3>
"https://example.com/my-app/#here"
</td>
<td>
"https://example.com/my-app/#here"
"https://example.com/my-app/"
</td>
</tr>
<tr>
Expand Down Expand Up @@ -971,6 +985,28 @@ <h3>
"https://example.com/foo"
</td>
</tr>
<tr>
<td>
"foo?x=y"
</td>
<td>
"https://example.com/my-app/start"
</td>
<td>
"https://example.com/foo?x=y"
</td>
</tr>
<tr>
<td>
"foo#heading"
</td>
<td>
"https://example.com/my-app/start"
</td>
<td>
"https://example.com/foo"
</td>
</tr>
<tr>
<td>
"./foo"
Expand Down Expand Up @@ -1298,8 +1334,7 @@ <h3>
</li>
<li>If the [=document=]'s [=document|processed manifest=] is not
null, and [=document=]'s [=document|processed manifest=]'s id is
not [=URL/equal=] with [=URL serializer/exclude fragment|exclude
fragment true=] to |manifest|["id"], return.
not [=URL/equal=] to |manifest|["id"], return.
</li>
<li>[=Process the `scope` member=] passing |json|, |manifest|, and
|manifest URL|.
Expand Down
Loading