1363001 - Implement browsingData.removeDownloads WebExtension API method on android

Status: RESOLVED FIXED

(WebExtensions :: Android, enhancement, P5)

Description

[Tushar Saini (:shatur)]

This method clears the browser's download history. The doc can be found at [1].

[1]. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeDownloads

Comment 1

[Tushar Saini (:shatur)]

Attachment: Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

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

Comment 2

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review154774

::: mobile/android/modules/Sanitizer.jsm:253
(Diff revision 1)
>          };
>          FormHistory.count({}, countDone);
>        }
>      },
>  
> +    downloadHistory: {

I've added a new method, which only deals with downloads history. The existing method, ie, downloadFiles, not only deletes download-history but also remove downloaded files, but it is not required for our case.

There are two options to resolve this, either I can introduce a flag in existing method, so as to enable/disable deletion of files, or write a new method seprately, which I've done for now. Or do you've anything else in mind?

Note: For now maybe its not optimized, I am just verifying its behaviour.

Comment 3

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

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

Comment 4

[:Grisha Kruglov]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review157322

Not familiar with the code, will rely on Matt's review.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_downloads.html:109
(Diff revision 2)
> +
> +    // Clear downloads with old since value.
> +    await setupDownloads();
> +    extension.sendMessage(method, {since: REFERENCE_DATE - 100000});
> +    await extension.awaitMessage("downloadsRemoved");
> +   	await checkDownloads(false, false);

tab characters

::: mobile/android/modules/Sanitizer.jsm:36
(Diff revision 2)
>  
>  this.EXPORTED_SYMBOLS = ["Sanitizer"];
>  
>  function Sanitizer() {}
>  Sanitizer.prototype = {
> -  clearItem: function (aItemName)
> +  clearItem: function (aItemName, startTime)

Same comment as I've left on the removeHistory PR: this introduces an inconsistent, potentially misleading API for the Sanitizer, as only a subset of `clear` methods actually support the `startTime` filtering.

::: mobile/android/modules/Sanitizer.jsm:264
(Diff revision 2)
> +        let downloads = yield list.getAll();
> +        var finalizePromises = [];
> +
> +        for (let download of downloads) {
> +          // Delete history items visisted after startTime
> +          if(download.startTime.getTime() >= startTime){

Inconsistent styling

::: mobile/android/modules/Sanitizer.jsm:265
(Diff revision 2)
> +        var finalizePromises = [];
> +
> +        for (let download of downloads) {
> +          // Delete history items visisted after startTime
> +          if(download.startTime.getTime() >= startTime){
> +            if (download.stopped && (!download.hasPartialData || download.error)) {

So you're not removing partial, inactive downloads? Is that documented in the WE API docs?

Comment 5

[Matthew Wein [:mattw]]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads 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 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review161272

My review doesn't include the changes to `Sanitizer.jsm`, but that's being covered by another reviewer. Nice work on the WebExtension pieces.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

Be sure Grisha's comments are addressed, otherwise a 3rd r? is not necessary.

Comment 8

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

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

Comment 9

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review162722

::: mobile/android/modules/Sanitizer.jsm:54
(Diff revision 3)
> +
> +  clearItem: function (aItemName, startTime)
> +  {
> +    // Only a subset of items support deletion with startTime.
> +    if (typeof startTime != "undefined") {
> +      switch (aItemName) {

Here, we can add other items as well, which will support deletion with since like history. Does this looks good?

Comment 10

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review157322

> So you're not removing partial, inactive downloads? Is that documented in the WE API docs?

Our chrome counterpart deletes both stopped as well as completed downloads and does not remove paused downloads from download history.

Comment 11

[:Grisha Kruglov]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review168960

::: mobile/android/modules/Sanitizer.jsm:53
(Diff revision 3)
> +  },
> +
> +  clearItem: function (aItemName, startTime)
> +  {
> +    // Only a subset of items support deletion with startTime.
> +    if (typeof startTime != "undefined") {

Ideally you're looking to enforce correct consumption of this API, as opposed to just doing something in case of incorrect uses - over time that might lead to subtle, hard to trace bugs; this is javascript, after all.

This module has an implicit contract with the outside world, so we're trying to ensure that the outside world follows it, and if not - fail the request, letting calling code know why (incorrect arguments, etc).

And so I imagine something roughly along these lines will be a better fit:

switch(aItemName) {
 case requestType:
   ...if arguments are not correct ...
   reject("Invalid arguments")
   ...else...
   clear(args)
}

::: mobile/android/modules/Sanitizer.jsm:55
(Diff revision 3)
> +  clearItem: function (aItemName, startTime)
> +  {
> +    // Only a subset of items support deletion with startTime.
> +    if (typeof startTime != "undefined") {
> +      switch (aItemName) {
> +        case 'downloadHistory':

This is removeDownloads bug, I'm guessing copy/paste error?

::: mobile/android/modules/Sanitizer.jsm:269
(Diff revision 3)
>          };
>          FormHistory.count({}, countDone);
>        }
>      },
>  
> +    downloadHistory: {

I think you copy/pasted things from a different bug without fully updating the code?

::: mobile/android/modules/Sanitizer.jsm:275
(Diff revision 3)
> +      clear: Task.async(function* (startTime) {
> +        let refObj = {};
> +        TelemetryStopwatch.start("FX_SANITIZE_DOWNLOADS", refObj);
> +
> +        // Initialize startTime to 0, if it is undefined
> +        if (typeof startTime == "undefined")

Don't skip on {} for any new code.

Comment 12

[Jan Henning [:JanH]]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review168960

> I think you copy/pasted things from a different bug without fully updating the code?

Repeating what I mentioned on IRC, it would be nice to avoid duplicating most of the code of downloadFiles right below. Maybe this code should be split into a helper function that's internal to Sanitizer.jsm (see [here](https://dxr.mozilla.org/mozilla-central/rev/ef9a0f01e4f68214f0ff8f4631783b8a0e075a82/toolkit/modules/sessionstore/SessionHistory.jsm#28) for an example on how to do that) and then let downloadHistory and downloadFiles just call that function with the appropriate parameters?

Comment 13

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

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

Comment 14

[Tushar Saini (:shatur)]

Hey JanH!

(In reply to Jan Henning [:JanH] from comment #12)
> Repeating what I mentioned on IRC, it would be nice to avoid duplicating
> most of the code of downloadFiles right below. Maybe this code should be
> split into a helper function that's internal to Sanitizer.jsm (see
> [here](https://dxr.mozilla.org/mozilla-central/rev/
> ef9a0f01e4f68214f0ff8f4631783b8a0e075a82/toolkit/modules/sessionstore/
> SessionHistory.jsm#28) for an example on how to do that) and then let
> downloadHistory and downloadFiles just call that function with the
> appropriate parameters?

While working on it, I think splitting the code will not do any good for us because that way we will still need to pass flags to clearItem [to distinguish between web-extension remove call and our normal call to remove downloadFiles]. Also, it  will cause the same problems Grisha addressed in comment 4. 

But undoubtedly, we do not want to duplicate the same code. So, what I've done is, I am passing a flag to downloadFile.clear() call from our filter method, ie, clearItem. I hope this solves our problem. If you've any suggestions, I am all ears :)

Thanks!!

Comment 15

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review174154

::: mobile/android/modules/Sanitizer.jsm:49
(Diff revision 4)
>      } else if (canClear) {
> -      return item.clear();
> +      return item.clear(startTime, deleteFiles);
> +    }
> +  },
> +
> +  clearItem: function(aItemName, startTime) {

I think we can handle all our flags here. This will let us filter our requests as well as reject all those calls which we do not handle. That way we will not be needed to split the code. Will it be suffice for our needs or do you've any opinion in mind?

Comment 16

[Tushar Saini (:shatur)]

Hey Jan!

I talked to Grisha last week and he told me that, he will be redirecting review to you(I hope I have flagged right Jan ;) ) So, I have flagged you for review. Please have a look.

I have also addressed the issue you raised in comment 12.

Thanks!!

Comment 17

[Jan Henning [:JanH]]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review176438

::: mobile/android/modules/Sanitizer.jsm:36
(Diff revision 4)
>  
>  this.EXPORTED_SYMBOLS = ["Sanitizer"];
>  
>  function Sanitizer() {}
>  Sanitizer.prototype = {
> -  clearItem: function(aItemName) {
> +  _clear: function(aItemName, startTime, deleteFiles) {

I'm a bit torn about extending the `clearItem` API like that, too, but as it's only one parameter and a few more data sources will support the `startTime` parameter if you implement the full gamut of `browsingData` extension support, I guess it's okay.

For the internal API however, including every potentially needed parameter here definitively doesn't feel quite right. Just use a generic `options` object and pass it through here.

You can then use something like `clear: Task.async(function* ({ startTime = 0, deleteFiles = true } = {})` and call it with `this._clear("downloadFiles", { startTime, deleteFiles: false })`.

::: mobile/android/modules/Sanitizer.jsm:49
(Diff revision 4)
>      } else if (canClear) {
> -      return item.clear();
> +      return item.clear(startTime, deleteFiles);
> +    }
> +  },
> +
> +  clearItem: function(aItemName, startTime) {

I think as the function that's going to be externally called, `clearItem` should remain at the top.

::: mobile/android/modules/Sanitizer.jsm:261
(Diff revision 4)
>          let refObj = {};
>          TelemetryStopwatch.start("FX_SANITIZE_DOWNLOADS", refObj);
>  
> +        // Initialize startTime to 0(i.e, everything), if it is undefined
> +        if (typeof startTime == "undefined")
> +          startTime = 0;

Missing braces (and shouldn't providing a default value in the parameter definition be enough?).

::: mobile/android/modules/Sanitizer.jsm:276
(Diff revision 4)
>            // Remove downloads that have been canceled, even if the cancellation
>            // operation hasn't completed yet so we don't check "stopped" here.
>            // Failed downloads with partial data are also removed.
>            if (download.stopped && (!download.hasPartialData || download.error)) {
> +            // For our web-extension consumer, where we delete with startTime
> +            if (download.startTime.getTime() >= startTime) {

Don't restrict this check to just the list entry removal, just chain this to the conditions right above.

Even though at the moment the two callers of this function will want to either
- delete everything (all entries and both entries and the files themselves)
- or else delete just a temporally limited subset and leave the files themselve alone

that's not guaranteed for all times - maybe at some point somebody wants to delete all recent downloads *including* files.

::: mobile/android/modules/Sanitizer.jsm:288
(Diff revision 4)
> -            // processing the other downloads in the list.
> +              // processing the other downloads in the list.
> -            finalizePromises.push(download.finalize(true).then(() => null, Cu.reportError));
> +              finalizePromises.push(download.finalize(true).then(() => null, Cu.reportError));
> +            }
>  
> -            // Delete the downloaded files themselves.
> +            // Delete the downloaded files themselves, if deleteFiles flag is true
> +            if (deleteFiles) {

The documentation on MDN notwithstanding
- clearing downloaded files when removing a history entry is the current standard behaviour for Firefox on Android. Deviating from this isn't ideal for behaving differently than if the user was clearing the download, but of course on the other hand possibly worse for add-on compatibility.
- As long as bug 1280184 isn't fixed, the files will be deleted anyway on Android M and newer.
- I've opened bug 1392768, which might change our behaviour here anyway and make the decision whether to delete files or not user-configurable (in which case it could arguably be said that add-ons should respect that choice). Or maybe file deletion will be gone for good, or everything'll stay the same, who knows.

Comment 18

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150094/diff/4-5/

Comment 19

[Jan Henning [:JanH]]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review178060

::: mobile/android/modules/Sanitizer.jsm:269
(Diff revision 5)
>          // downloaded files as well.
>          for (let download of downloads) {
>            // Remove downloads that have been canceled, even if the cancellation
>            // operation hasn't completed yet so we don't check "stopped" here.
>            // Failed downloads with partial data are also removed.
> -          if (download.stopped && (!download.hasPartialData || download.error)) {
> +          // A check for our web-extension consumer, where we delete with startTime

Not sure if this comment helps more than it confuses if you look at it outside of the context of this patch.

If you read this block without the benefit of the diff view highlighting, this makes it look like the whole condition chain was possibly included only for the benefit of extensions.

Maybe rather "The startTime check is provided for add-ons that may want to delete only recent downloads." or something like that?

::: mobile/android/modules/Sanitizer.jsm:270
(Diff revision 5)
>          for (let download of downloads) {
>            // Remove downloads that have been canceled, even if the cancellation
>            // operation hasn't completed yet so we don't check "stopped" here.
>            // Failed downloads with partial data are also removed.
> -          if (download.stopped && (!download.hasPartialData || download.error)) {
> +          // A check for our web-extension consumer, where we delete with startTime
> +          if (download.stopped && (!download.hasPartialData || download.error) && download.startTime.getTime() >= startTime) {

Style nit: This line is getting a bit long, please put the date check on a separate line.

::: mobile/android/modules/Sanitizer.jsm:281
(Diff revision 5)
>              // don't need to wait for the procedure to be complete before
>              // processing the other downloads in the list.
>              finalizePromises.push(download.finalize(true).then(() => null, Cu.reportError));
> +          }
>  
> -            // Delete the downloaded files themselves.
> +          // Delete the downloaded files themselves, if deleteFiles flag is true

Maybe just move the original comment into the if block - the "if deleteFiles flag is true" bit should be obvious from the code itself in this case.

::: mobile/android/modules/Sanitizer.jsm:282
(Diff revision 5)
>              // processing the other downloads in the list.
>              finalizePromises.push(download.finalize(true).then(() => null, Cu.reportError));
> +          }
>  
> -            // Delete the downloaded files themselves.
> +          // Delete the downloaded files themselves, if deleteFiles flag is true
> +          if (deleteFiles) {

That was't what I intendend.

What I mean is - we should only attempt deleting a file if the whole `download.stopped && ...` is satisfied - including the new `startTime` check, so please move this into the if block above.

Comment 20

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150094/diff/5-6/

Comment 21

[Jan Henning [:JanH]]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review178318

Comment 22

[Tushar Saini (:shatur)]

Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150094/diff/6-7/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d381f5c5e703
Implement browsingData.removeDownloads WebExtension API method on android. r=bsilverberg,JanH

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d381f5c5e703

Comment 25

[CosminB]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe verify-“ flag.

Thanks!