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

Proposal: add cookies.removeAll() method #690

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

aselya
Copy link
Contributor

@aselya aselya commented Sep 18, 2024

Proposal introducing a new API cookies.removeAll()

Copy link
Member

@oliverdunk oliverdunk left a comment

Choose a reason for hiding this comment

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

This generally looks good, thanks! I left a couple of comments.

proposals/Proposal: add cookies.removeAll() method.md Outdated Show resolved Hide resolved
### Behavior

The API will remove all cookies that match the `details` object parameter with the exception of the cases outlined in the implementation details.
If the extension does not have [host permissions](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#host_permissions) for this URL, the API call will fail.
Copy link
Member

Choose a reason for hiding this comment

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

If we allow calling this API without a url (see below) we should add a note here about the behavior if you only have some host permissions. Presumably, we would just remove the cookies for the sites you have access to.

@aselya aselya changed the title Create Proposal: add cookies.removeAll() method.md Proposal: add cookies.removeAll() method.md Sep 19, 2024
proposals/Proposal: add cookies.removeAll() method.md Outdated Show resolved Hide resolved
proposals/Proposal: add cookies.removeAll() method.md Outdated Show resolved Hide resolved

### Objective

This new API addresses two workflows for developers:
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the examples you provided is how extensions would have to achieve the functionality these days. Can you rephrase to emphasize that? To someone not deeply aware of the details, it can be read as part of the proposal for something net, not of existing behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the language and I would appreciate feedback on if my rephrasing addresses the issue you brought up.

proposals/Proposal: add cookies.removeAll() method.md Outdated Show resolved Hide resolved
@aselya aselya changed the title Proposal: add cookies.removeAll() method.md Proposal: add cookies.removeAll() method Sep 25, 2024
@aselya
Copy link
Contributor Author

aselya commented Sep 25, 2024

Wanted to add some context to why I'm proposing this.

When working on updating the implementation of the cookies API to include the hasCrossSiteAncestor value in the partitionKey, the unit testing I added as part of the change, revealed that my new implementation introduced a bug that caused multiple cookies to be removed with cookies.remove(). I caught the bug and fixed it before it was ever merged into chromium.

Despite the behavior being a bug, I found the ability to delete multiple cookies with the same call to be quite useful and thought it made sense to add it to the cookies api.

aselya and others added 7 commits October 1, 2024 10:51
Updating parameters to explicitly reference cookies.remove

Co-authored-by: Rob Wu <[email protected]>
Add more related issues
Update the motivation language to reflect the related issues.
Add link to bug in the motivation
Update the objective to be more explicit about the pain point for developers.
Update language to make `{}` and error
proposals/Proposal: add cookies.removeAll() method.md Outdated Show resolved Hide resolved

```
cookies.removeAll({
topLevelSite: “https://example.com”
Copy link
Member

Choose a reason for hiding this comment

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

Why is topLevelSite a separate key? I would expect it to be a member of partitionKey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having topLevelSite be a separate (optional) parameter addresses the second motivation "Removal of cookies associated with a topLevelSite".

This could be replaced by having the developer pass in a partitionKey that contains the toplevelSite and no value for hasCrossSiteAncestor, such aspartitionKey:{topLevelSite: "https/example.com"}. But I wanted to provide an easier option for developers who may not be as familiar with the intricacies of partitioned cookies.

aselya added 2 commits October 8, 2024 08:13
Update case: `partitionKey : {}` to match `cookies.getAll`
Update requirements of details object.
Added more detailed description of host permissions impact on API call
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.

4 participants