Extend browsingData to support removing cookies by host

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: General
RESOLVED FIXED
2 months ago
13 days ago

People

(Reporter: Thomas Wisniewski, Assigned: Thomas Wisniewski)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla56
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 months ago
This was split off from bug 1340511, in the interest of getting support for cookies implemented separately from localStorage and indexedDB.
(Assignee)

Comment 1

2 months ago
Created attachment 8882027 [details] [diff] [review]
1376991-extend_browsingData_to_restrict_removing_cookies_to_given_hostnames.diff

I had some time today, so here's a patch containing just the cookie-related bits from my patch in the original bug, cleaned up and now with test cases.
Assignee: nobody → wisniewskit
Status: NEW → ASSIGNED
Attachment #8882027 - Flags: review?(mixedpuppy)
(Assignee)

Comment 2

2 months ago
Created attachment 8882206 [details] [diff] [review]
1376991-extend_browsingData_to_restrict_removing_cookies_to_given_hostnames.diff

Sorry for the churn, but looking at the patch again today with fresh eyes I see that the logic when both "hostnames" and "since" are provided was not doing what one would probably expect (that is, to clear just those cookies "since" the time for the listed "hostnames"). Here's a new version of the patch which does that (including a related test).
Attachment #8882027 - Attachment is obsolete: true
Attachment #8882027 - Flags: review?(mixedpuppy)
Attachment #8882206 - Flags: review?(mixedpuppy)
Any chance you can push to reviewboard?
Flags: needinfo?(wisniewskit)
(Assignee)

Comment 4

2 months ago
Comment on attachment 8882206 [details] [diff] [review]
1376991-extend_browsingData_to_restrict_removing_cookies_to_given_hostnames.diff

Sure! It's at https://reviewboard.mozilla.org/r/153540
Attachment #8882206 - Attachment is obsolete: true
Flags: needinfo?(wisniewskit)
Attachment #8882206 - Flags: review?(mixedpuppy)
Comment hidden (mozreview-request)

Comment 6

2 months ago
mozreview-review
Comment on attachment 8882414 [details]
Bug 1376991 - Extend browsingData to restrict removing cookies to a give list of hostnames;

https://reviewboard.mozilla.org/r/153540/#review158710
Attachment #8882414 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 7

2 months ago
Thanks for the review. I'm doing a try-run before requesting check-in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb0e95d91b4fe293b33c452f6c0ccec6f5517aaa
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 months ago
I pushed 2 small tweaks to make it pass try. A new run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8dfdbc4bb7f18f329f83e8f8854f4cd0bff99f4

Comment 10

2 months ago
mozreview-review
Comment on attachment 8882540 [details]
Fixes for try run failures

https://reviewboard.mozilla.org/r/153656/#review159628

this is fine, just roll it into the original patch
Attachment #8882540 - Flags: review+
(Assignee)

Comment 11

2 months ago
mozreview-review
Comment on attachment 8882414 [details]
Bug 1376991 - Extend browsingData to restrict removing cookies to a give list of hostnames;

https://reviewboard.mozilla.org/r/153540/#review159658

Carrying over r+.
Attachment #8882414 - Flags: review+
(Assignee)

Comment 12

2 months ago
mozreview-review-reply
Comment on attachment 8882414 [details]
Bug 1376991 - Extend browsingData to restrict removing cookies to a give list of hostnames;

https://reviewboard.mozilla.org/r/153540/#review158710

I just made a couple of minor tweaks to account for straightforward failures on the try-run (an eslint fix and clearing the cookies before tests). A new try-run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8dfdbc4bb7f18f329f83e8f8854f4cd0bff99f4
(Assignee)

Updated

2 months ago
Attachment #8882540 - Attachment is obsolete: true
(Assignee)

Comment 13

2 months ago
Alright, I've folded my follow-up patch into the original after getting the go-ahead from :mixedpuppy on IRC. Carrying over r+ and requesting checkin for the final patch.
Keywords: checkin-needed
MozReview is still showing your fix-up commit (and Autoland wants to push it as a result). Please try to clean that up.
Keywords: checkin-needed
(Assignee)

Comment 15

2 months ago
>MozReview is still showing your fix-up commit (and Autoland wants to push it as a result). Please try to clean that up.

Sorry to bug you about it, but how would I do that? I've twice "finished the review" and "discarded" it, but I don't see any "drop this patch" type of option in MozReview, so I'm stuck (and no one seems to be on #mozreview right now). Is there someone else I should ping to find out?
Flags: needinfo?(ryanvm)
Maybe Mark does.
Flags: needinfo?(ryanvm) → needinfo?(mcote)

Comment 17

2 months ago
Hi Thomas, you need to rebase your commits to merge them together (it doesn't look like the first commit includes the fixes in the second commit).  There are a few ways to do this; probably the easiest is "hg histedit <parent>" where <parent> is the commit you based your first patch on ("hg log --graph" should show this clearly).  Then you can change the second commit from "pick" to "fold", and it will be combined into the first one.  Then push it up for review again (MozReview will understand that the second commit has been dropped and will remove the patch accordingly).
Flags: needinfo?(mcote)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 months ago
Thanks, mcote! It looks like that did the trick. Re-requesting checkin.
Keywords: checkin-needed

Comment 20

2 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5394bfb80035
Extend browsingData to restrict removing cookies to a give list of hostnames; r=mixedpuppy
Keywords: checkin-needed

Comment 21

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5394bfb80035
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.