Closed Bug 1363001 Opened 3 years ago Closed 2 years ago

Implement browsingData.removeDownloads WebExtension API method on android

Categories

(WebExtensions :: Android, enhancement, P5)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: shatur, Assigned: shatur, Mentored, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [triaged])

Attachments

(1 file)

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
Blocks: 1362118
Priority: -- → P5
Whiteboard: [triaged]
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.
Assignee: nobody → tushar.saini1285
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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?
Attachment #8878829 - Flags: review?(gkruglov)
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.
Attachment #8878829 - Flags: review?(mwein) → review?(mixedpuppy)
Attachment #8878829 - Flags: review?(bob.silverberg)
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.
Attachment #8878829 - Flags: review?(bob.silverberg) → review+
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.
Attachment #8878829 - Flags: review?(mixedpuppy)
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 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 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.
Attachment #8878829 - Flags: review?(gkruglov)
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?
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 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?
Attachment #8878829 - Flags: review?(gkruglov) → review?(jh+bugzilla)
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 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.
Attachment #8878829 - Flags: review?(jh+bugzilla)
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.
Attachment #8878829 - Flags: review?(jh+bugzilla)
Comment on attachment 8878829 [details]
Bug 1363001 - Implement browsingData.removeDownloads WebExtension API method on android.

https://reviewboard.mozilla.org/r/150094/#review178318
Attachment #8878829 - Flags: review?(jh+bugzilla) → review+
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d381f5c5e703
Implement browsingData.removeDownloads WebExtension API method on android. r=bsilverberg,JanH
https://hg.mozilla.org/mozilla-central/rev/d381f5c5e703
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1395078
Depends on: 1397818
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!
Flags: needinfo?(tushar.saini1285)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.