1362998 - Implement browsingData.removeCookies WebExtension API method on android

Status: RESOLVED FIXED

(WebExtensions :: Android, enhancement, P5)

Description

[Tushar Saini (:shatur)]

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

Comment 1

[Tushar Saini (:shatur)]

Attachment: Bug 1362998 - Implement browsingData.removeCookies WebExtension API method on android.

Review commit: https://reviewboard.mozilla.org/r/149562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149562/

Comment 2

[:Grisha Kruglov]

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.

Comment 3

[Matthew Wein [:mattw]]

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.

Comment 4

[Shane Caraveo (: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.

Comment 5

[Bob Silverberg [:bsilverberg]]

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.

Comment 6

[Tushar Saini (:shatur)]

(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 7

[Shane Caraveo (:mixedpuppy)]

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)

Comment 8

[Tushar Saini (:shatur)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149562/diff/1-2/

Comment 9

[Bob Silverberg [:bsilverberg]]

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.

Comment 10

[Tushar Saini (:shatur)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149562/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33154264e3d9
Implement browsingData.removeCookies WebExtension API method on android. r=bsilverberg

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33154264e3d9

Comment 13

[Caitlin Neiman [:caitmuenster]]

Nice! Thank you so much, Tushar!

Comment 14

[Tushar Saini (:shatur)]

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!!

Comment 15

[Shane Caraveo (: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.

Comment 16

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeCookies#Browser_compatibility