Skip to content

Add a storage key comparison algorithm #134

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

Merged
merged 2 commits into from
Feb 11, 2022

Conversation

recvfrom
Copy link
Contributor

@recvfrom recvfrom commented Feb 10, 2022

This PR adds and exports an algorithm for comparing two storage keys. This will be useful for non-storage APIs that want to partition state using storage keys.

Also, adding my name to the 'Acknowledgments' section since I forgot to do this with my previous PR. :D

Fixes #133

I don't think tests or implementation bugs are needed for this change since this PR doesn't change any existing functionality.

  • At least two implementers are interested (and none opposed):
    • Chrome
    • Firefox
  • Tests are written and can be reviewed and commented upon at:
    • Not required
  • Implementation bugs are filed:
    • Chrome: Not required
    • Firefox: Not required
    • Safari: Not required

Preview | Diff

This PR adds and exports an algorithm for comparing two
storage keys. This will be useful for non-storage APIs
that want to partition state using storage keys.

Also, adding my name to the 'Acknowledgments' section
since I forgot to do this with my previous commit. :D
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks, looks good modulo nits.

storage.bs Outdated
@@ -229,6 +229,17 @@ settings object</a> <var>environment</var>, run these steps:
<li><p>Return <var>key</var>.
</ol>

<p>To determine whether a <a>storage key</a> <var>A</var>
<dfn export for="storage key" id=concept-storage-key-equals lt=equal>equals</dfn> <var>B</var>, run
Copy link
Member

Choose a reason for hiding this comment

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

Let's repeat "a storage key" in front of B for maximum clarity.

Copy link
Member

Choose a reason for hiding this comment

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

Also no need to include an ID here. For new things we let Bikeshed generate one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modeled it off of this: https://github.com/whatwg/url/blob/main/url.bs#L1144-L1146

There they didn't repeat the type for B, but I agree that it's helpful for maximum clarity. Also, manually adding the id lets us prepend concept to it, which seems like something that's done in a lot of places (without the id specified, bikeshed makes it storage-key-equals). I'm happy to drop it, though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, concept- is a legacy pre-Bikeshed thing.

Also, always fun to have your own code come back to bite you. I can fix that one.

Copy link
Member

Choose a reason for hiding this comment

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

storage.bs Outdated
these steps:

<ol>
<li><p>If <var>A</var>'s <a for="storage key">origin</a> is not <a lt="same origin">same-origin</a>
Copy link
Member

Choose a reason for hiding this comment

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

"same origin" sans hyphen is what we typically use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modeled this after https://github.com/whatwg/url/blob/main/url.bs#L2730-L2731, but it looks like "same origin with" is indeed much more commonly used in places like https://html.spec.whatwg.org/multipage/origin.html. Will change!

@recvfrom
Copy link
Contributor Author

Thanks for the review @annevk, all changes should now be addressed

@annevk annevk merged commit bea19b7 into whatwg:main Feb 11, 2022
@annevk
Copy link
Member

annevk commented Feb 11, 2022

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add an algorithm for comparing storage keys
2 participants