Closed Bug 1362998 Opened 7 years ago Closed 7 years ago

Implement browsingData.removeCookies WebExtension API method on android

Categories

(WebExtensions :: Android, enhancement, P5)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: shatur, Assigned: shatur, Mentored)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [triaged])

Attachments

(1 file)

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
Blocks: 1362118
Priority: -- → P5
Whiteboard: [triaged]
Assignee: nobody → tushar.saini1285
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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 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-
(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 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+
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/33154264e3d9
Implement browsingData.removeCookies WebExtension API method on android. r=bsilverberg
https://hg.mozilla.org/mozilla-central/rev/33154264e3d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Nice! Thank you so much, Tushar!
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!!
Attachment #8878210 - Flags: review?(mixedpuppy)
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.