Closed Bug 825849 Opened 11 years ago Closed 11 years ago

Add a RemoveAllDownloads API to nsIDownloadHistory

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat)

Attachments

(1 file, 4 obsolete files)

We need an API that we can use from javascript to remove all downloads history.
Assignee: mak77 → mano
Are you going to work on this? Since you still have to finish the other patches I was planning to do this in the next hour
and fwiw I already started
Flags: needinfo?(mano)
Yours. Review is going to be very easy ;)
Assignee: mano → mak77
Flags: needinfo?(mano)
Btw, after looking around, we don't have any other use for a generic remove-by-transition API, indeed I think I'm rather going to add a RemoveAllDownloads API to nsIDownloadHistory, that so far contains only AddDownload (ideally it should also have a RemoveDownload that forwards to RemovePage... but let's avoid scope creep).
I agree on both points.
Btw, the only reason we need a special api here is because we don't want to remove regular visits to pages which were also downloaded. Other than that there's *no* performance gain over just using removePages (because we need to pick the places_ids either way).
Attached patch early wip (obsolete) — Splinter Review
We may have some fixes to do to the result observers, cause removals will either cause onDeleteURI or onDeleteVisits (when the page has other visits), and unfortunately the latter doesn't say anything useful to a transition view (the notification just tells "some visits to this uri have been removed").
The view may just trust items will be removed and clear itself after firing the removal API call, though then it will start getting a bunch of useless notifications. Being a complex view it's also likely it may try to refresh at each removal. To be verified.
on the other side we may even add a special observer notification fired by the removeAllDownloads call, and ignore any remove notification until we get that. Or a callback function.
Things to consider:
1) We do want to add a RemoveDownload API sooner than later. This will fix this minor bug that removes all visits to a URL when the corresponding download is removed. It's an edge case, sure, but it shows why this API makes sense. You're then going to face the same dilemma regarding notifications. Thus I think it makes sense to add/extend a/the onDeleteVist/s notification to report the transition type.
2) I don't recall how we do this elsewhere... We should make sure that for the "clear" case the downloads view is only notified once (invalidateContainer). However, other views, which may also happen to list downloads (e.g. the History menu) should be notified normally.
3) It would be nice to avoid special "download" notification in the Places API. For places, a download is just another transition type.
By the way, if we cannot get something done for 20, I'm just going to use removePages and hack the view to ignore notifications.
Summary: Add a new API to remove visits by transition → Add a RemoveAllDownloads API to nsIDownloadHistory
Attached patch patch v1.0 (obsolete) — Splinter Review
Still needs cleanup, but it's working afaict.
Unfortunately it's a bit more complicated then I wanted, but it paves the way to future additions. Most of the complication comes from the need to distinguish onDeleteVisits from onDeleteURI (we should likely come with a better solution in future) and to be able to handle onDeleteVisits for a given transition in the view.
The batching works similarly to the existing visits removals, after 5 single changes we fallback to a full refresh.
This should make "easy" to implement a RemoveDownloads API that removes all downloads visits for a given URI (just add the uri to the RemoveFilter and start the runnable).
So far I've hooked it up to a contextual menu Clear List options, mostly to be able to test it on the battle field.
Attachment #697033 - Attachment is obsolete: true
Attached patch patch v1.1 (obsolete) — Splinter Review
Cleaned up and ready for review
Attachment #697697 - Attachment is obsolete: true
Comment on attachment 697904 [details] [diff] [review]
patch v1.1

Review of attachment 697904 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +2377,5 @@
>  
>  NS_IMETHODIMP
>  nsDownloadManager::OnDeleteVisits(nsIURI *aURI, PRTime aVisitTime,
>                                    const nsACString& aGUID,
> +                                  uint16_t aReasonm, uint32_t aTransitionType)

typo "aReasonm" fixed locally.
Shorlander said on IRC "I think Clear Downloads would work as well as Clear Download History. I don't think History is technically wrong in PBM since it is a history it is just temporary.".  I'm going for the simple Clear Downloads label.
Comment on attachment 697904 [details] [diff] [review]
patch v1.1

I'll file a separate bug to add RemoveDownloadsfor(aURI), that's less important than RemoveAllDownloads but should definitely be fixed.
Attachment #697904 - Flags: superreview?(gavin.sharp)
Attachment #697904 - Flags: review?(mano)
Blocks: 826702
Comment on attachment 697904 [details] [diff] [review]
patch v1.1

Review of attachment 697904 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice work, thanks a ton!

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +456,5 @@
>                  (!this._dataItem && this._file))
>        case "downloadsCmd_openReferrer":
>          return this._dataItem && !!this._dataItem.referrer;
> +      case "downloadsCmd_clearDownloads":
> +        return true;

The command should be disabled when the list is empty, IMO.

@@ +504,5 @@
>          openURL(this._dataItem.referrer);
>          break;
>        }
> +      case "downloadsCmd_clearDownloads": {
> +        if (PrivateBrowsingUtils.isWindowPrivate(window)) {

Please ensure that we don't need to pass window.top, as discussed over IRC.

@@ +510,5 @@
> +        } else {
> +          Services.downloads.cleanUp();
> +          Cc["@mozilla.org/browser/download-history;1"]
> +            .getService(Ci.nsIDownloadHistory)
> +            .removeAllDownloads();

To avoid future bugs, let's clear history if the view is "attached" to a result (it isn't in pb mode).

::: toolkit/components/places/History.cpp
@@ +1479,5 @@
> +    }
> +
> +    // Wrap all notifications in a batch, so the view can handle changes in a
> +    // more performant way, by initiating a refresh after a limited number of
> +    // single changes.

We need to follow up on improving the performance of invalidateContainer in the downloads view (or maybe I'll just do so in the "shutdown" bug). At this point it's just as slow as calling nodeRemoved multiple times.

@@ +2511,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    NS_ERROR("Cannot add downloads to history from content process!");

cannot remove.

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +2873,5 @@
> +    // All visits for aTransitionType have been removed, if the query is
> +    // filtering on such transition type, this is equivalent to an onDeleteURI
> +    // notification.
> +    if ((mQueries[0]->Transitions()).Contains(aTransitionType)) {
> +      nsresult rv = OnDeleteURI(aURI, aGUID, aReason);

I wonder if it's worth renaming OnDeleteURI then (FromView?). No biggie either way.

::: toolkit/components/places/tests/unit/test_download_history.js
@@ +50,5 @@
> +/**
> + * Waits for the first onDeleteVisits notification to be received.
> + *
> + * @param aCallback
> + *        This function is called with the same arguments of onVisit.

Callback function to be called with the (...).
Attachment #697904 - Flags: review?(mano) → review+
Attached patch patch v1.2 (obsolete) — Splinter Review
Addressed review comments
Attachment #697904 - Attachment is obsolete: true
Attachment #697904 - Flags: superreview?(gavin.sharp)
Attachment #698001 - Flags: superreview?(gavin.sharp)
Comment on attachment 698001 [details] [diff] [review]
patch v1.2

>diff --git a/docshell/base/nsIDownloadHistory.idl b/docshell/base/nsIDownloadHistory.idl

>+   * @note This method is not guaranteed to be synchronous, since that depends
>+   *       on the current implementation.  It's suggested to use the
>+   *       implementation notifications to check for completion.

I would rephrase this as:

This addition is not guaranteed to be synchronous, since it delegates the actual addition to the underlying history implementation. If you need to observe the completion of the addition, use the underlying history implementation's notifications system (e.g. nsINavHistoryObserver for toolkit's implementation of this interface).

(with similar language s/addition/removal/ for the removalAllDownloads comment)

>+static PLDHashOperator NotifyVisitRemoval(PlaceHashKey* aEntry,
>+                                          void* aHistory)

>+  uint32_t transition = visits[0].transitionType < UINT32_MAX ?
>+                          visits[0].transitionType : 0;
>+  history->NotifyOnPageExpired(uri, visits[0].visitTime, removingPage,
>+                               visits[0].guid,
>+                               nsINavHistoryObserver::REASON_DELETED,
>+                               transition);

The constraint from the interface (transitionType must be passed only if all visits of that type are being removed) propagates all the way here, and it's confusing how this stuff works, so some comments would be nice. Something like: "FindRemovableVisits only sets the transition type on the VisitData objects it collects if the visits were filtered by transition type. RemoveVisitsFilter currently only supports filtering by transition type, and so FindRemovableVisits will either find all visits, or all visits of a given type. Therefore, if transitionType is set on this visit, we pass the transition type to NotifyOnPageExpired which in turns passes it to OnDeleteVisits to indicate that all visits of a given type were removed."

>diff --git a/toolkit/components/places/nsINavHistoryService.idl b/toolkit/components/places/nsINavHistoryService.idl

>+   * @param aTransitionType
>+   *        If it's a valid TRANSITION_* value, all visits of the specified type
>+   *        have been removed.
>    */
>   void onDeleteVisits(in nsIURI aURI,

This seems like a very odd way to notify about what needs notifying, but I don't have any better suggestions offhand, so I guess it's fine.
Attachment #698001 - Flags: superreview?(gavin.sharp) → superreview+
Attached patch patch v1.3Splinter Review
addressed SR, fixed warning
Attachment #698001 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=7297f96afa46 (The bc failure in browser_bug329212.js is not mine, I took an unlucky changeset out of inbound)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b891394bb82
Flags: in-testsuite+
Keywords: addon-compat
Target Milestone: --- → mozilla20
the changes should be backwards compatible for add-ons, though if someone implements nsIDownloadHistory (unlikely) they will need to add the new method.
https://hg.mozilla.org/mozilla-central/rev/9b891394bb82
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 922727
See Also: → 825318
You need to log in before you can comment on or make changes to this bug.