Implement browsingData.removeCookies WebExtension API method on android

RESOLVED FIXED in Firefox 56

Status

enhancement
P5
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: shatur, Assigned: shatur, Mentored)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [triaged])

Attachments

(1 attachment)

Assignee

Description

2 years 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

2 years ago
Blocks: 1362118

Updated

2 years ago
Priority: -- → P5
Whiteboard: [triaged]
Assignee

Updated

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

Comment 2

2 years 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 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/33154264e3d9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Nice! Thank you so much, Tushar!
Assignee

Comment 14

2 years 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

2 years ago
Attachment #8878210 - Flags: review?(mixedpuppy)

Comment 15

2 years 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: → 1387957

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.