Implement browsingData.removeCookies WebExtension API method on android

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Android
P5
normal
RESOLVED FIXED
4 months ago
7 days ago

People

(Reporter: shatur, Assigned: shatur, Mentored)

Tracking

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

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

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [triaged])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
This method clears browser's cookies. The doc can be found at [1].

[1]. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeCookies
(Assignee)

Updated

4 months ago
Blocks: 1362118

Updated

4 months ago
Priority: -- → P5
Whiteboard: [triaged]
(Assignee)

Updated

2 months ago
Assignee: nobody → tushar.saini1285
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment hidden (mozreview-request)

Comment 2

2 months ago
mozreview-review
Comment on attachment 8878210 [details]
Bug 1362998 - Implement browsingData.removeCookies WebExtension API method on android.

https://reviewboard.mozilla.org/r/149562/#review157312

Not familiar with the code, although changes look sane to me - although I'd investigate if there's a batch-delete exposed by the cookie manager that might be a safer way to do this.
Attachment #8878210 - Flags: review?(gkruglov)

Comment 3

2 months ago
Comment on attachment 8878210 [details]
Bug 1362998 - Implement browsingData.removeCookies WebExtension API method on android.

Moving review to Shane because I have a lot on my plate right now - sorry for not doing this earlier.
Attachment #8878210 - Flags: review?(mwein) → review?(mixedpuppy)
Comment on attachment 8878210 [details]
Bug 1362998 - Implement browsingData.removeCookies WebExtension API method on android.

Bob implemented the browser side, so I'd like him to review what is happening here to ensure compatibility as well as to look at whether code can be shared between the two.
Attachment #8878210 - Flags: review?(bob.silverberg)

Comment 5

a month ago
mozreview-review
Comment on attachment 8878210 [details]
Bug 1362998 - Implement browsingData.removeCookies WebExtension API method on android.

https://reviewboard.mozilla.org/r/149562/#review161302

::: mobile/android/components/extensions/ext-browsingData.js:12
(Diff revision 1)
>  XPCOMUtils.defineLazyModuleGetter(this, "Services",
>                                    "resource://gre/modules/Services.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "SharedPreferences",
>                                    "resource://gre/modules/SharedPreferences.jsm");
>  
> +let clearCookies = async function(options) {

Regarding Shane's comment in the bug about shared code, that does seem like a reasonable thing to do. Maybe we could extract this code to a module in /toolkit and then use it from there in ext-browsingData.js in both /browser and /mobile?

::: mobile/android/components/extensions/ext-browsingData.js:31
(Diff revision 1)
> +    // Iterate through the cookies and delete any created after our cutoff.
> +    let cookiesEnum = cookieMgr.enumerator;
> +    while (cookiesEnum.hasMoreElements()) {
> +      let cookie = cookiesEnum.getNext().QueryInterface(Ci.nsICookie2);
> +
> +      if (cookie.creationTime >= since) {

When I compare this to the code that's in browser/components/extensions/ext-browsingData.js, I notice that it's slightly different. The file in /browser uses `if (cookie.creationTime >= PlacesUtils.toPRTime(options.since)) {`. Why the change here?

::: mobile/android/components/extensions/test/mochitest/chrome.ini:10
(Diff revision 1)
>  tags = webextensions
>  
>  [test_ext_browserAction_getTitle_setTitle.html]
>  [test_ext_browserAction_onClicked.html]
> +[test_ext_browsingData_cookies.html]
> +[test_ext_browsingData_settings.html]

This file is not included in this patch.
Attachment #8878210 - Flags: review?(bob.silverberg) → review-
(Assignee)

Comment 6

a month ago
(In reply to Bob Silverberg [:bsilverberg] from comment #5)
> Regarding Shane's comment in the bug about shared code, that does seem like
> a reasonable thing to do. Maybe we could extract this code to a module in
> /toolkit and then use it from there in ext-browsingData.js in both /browser
> and /mobile?

About this, I was thinking once we implement all of our methods, then it will be really easy to figure out what code can be shared and refactor it.
 
> When I compare this to the code that's in
> browser/components/extensions/ext-browsingData.js, I notice that it's
> slightly different. The file in /browser uses `if (cookie.creationTime >=
> PlacesUtils.toPRTime(options.since)) {`. Why the change here?

I tried using PlaceUtils but get to know that, at present we do not ship PlaceUtils on fennec. Once we do that, we can share this code! 

Thanks!!
Comment on attachment 8878210 [details]
Bug 1362998 - Implement browsingData.removeCookies WebExtension API method on android.

r?me when things are further along (I'll also remove r?me on related bugs)
Attachment #8878210 - Flags: review?(mixedpuppy)
Comment hidden (mozreview-request)

Comment 9

a month ago
mozreview-review
Comment on attachment 8878210 [details]
Bug 1362998 - Implement browsingData.removeCookies WebExtension API method on android.

https://reviewboard.mozilla.org/r/149562/#review164712

It looks like you have addressed my issues, thanks Tushar. Please mark them as either fixed or dropped in the review so we can keep track of what's been addressed.
Attachment #8878210 - Flags: review?(bob.silverberg) → review+
Comment hidden (mozreview-request)

Comment 11

a month ago
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/33154264e3d9
Implement browsingData.removeCookies WebExtension API method on android. r=bsilverberg

Comment 12

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/33154264e3d9
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Nice! Thank you so much, Tushar!
(Assignee)

Comment 14

28 days ago
Hey Shane!

Sorry to not flag you before, can you have a look and tell us if its good. And if not I think we can back this out and make necessary changes. Thanks!!
(Assignee)

Updated

28 days ago
Attachment #8878210 - Flags: review?(mixedpuppy)

Comment 15

28 days ago
mozreview-review
Comment on attachment 8878210 [details]
Bug 1362998 - Implement browsingData.removeCookies WebExtension API method on android.

https://reviewboard.mozilla.org/r/149562/#review167394

::: mobile/android/components/extensions/ext-browsingData.js:14
(Diff revision 3)
>  XPCOMUtils.defineLazyModuleGetter(this, "SharedPreferences",
>                                    "resource://gre/modules/SharedPreferences.jsm");
>  
> +let clearCookies = async function(options) {
> +  if (options.originTypes &&
> +      (options.originTypes.protectedWeb || options.originTypes.extension)) {

I would rather have seen these removed from the schema, but not worth backout.
Attachment #8878210 - Flags: review?(mixedpuppy) → review+
See Also: → bug 1387957
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.