Last Comment Bug 825849 - Add a RemoveAllDownloads API to nsIDownloadHistory
: Add a RemoveAllDownloads API to nsIDownloadHistory
Status: RESOLVED FIXED
: addon-compat
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: Marco Bonardo [::mak]
:
Mentors:
Depends on: 922727
Blocks: 826702 822572
  Show dependency treegraph
 
Reported: 2013-01-02 02:24 PST by Marco Bonardo [::mak]
Modified: 2014-04-08 20:23 PDT (History)
3 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
early wip (13.07 KB, patch)
2013-01-02 08:21 PST, Marco Bonardo [::mak]
no flags Details | Diff | Review
patch v1.0 (34.61 KB, patch)
2013-01-03 15:33 PST, Marco Bonardo [::mak]
no flags Details | Diff | Review
patch v1.1 (46.72 KB, patch)
2013-01-04 07:08 PST, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Review
patch v1.2 (47.53 KB, patch)
2013-01-04 11:04 PST, Marco Bonardo [::mak]
gavin.sharp: superreview+
Details | Diff | Review
patch v1.3 (49.03 KB, patch)
2013-01-04 15:15 PST, Marco Bonardo [::mak]
no flags Details | Diff | Review

Description Marco Bonardo [::mak] 2013-01-02 02:24:51 PST
We need an API that we can use from javascript to remove all downloads history.
Comment 1 Marco Bonardo [::mak] 2013-01-02 06:00:28 PST
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
Comment 2 Marco Bonardo [::mak] 2013-01-02 06:11:14 PST
and fwiw I already started
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-01-02 06:14:19 PST
Yours. Review is going to be very easy ;)
Comment 4 Marco Bonardo [::mak] 2013-01-02 06:28:28 PST
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).
Comment 5 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-01-02 07:07:54 PST
I agree on both points.
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-01-02 07:10:01 PST
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).
Comment 7 Marco Bonardo [::mak] 2013-01-02 08:21:29 PST
Created attachment 697033 [details] [diff] [review]
early wip
Comment 8 Marco Bonardo [::mak] 2013-01-03 02:33:21 PST
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.
Comment 9 Marco Bonardo [::mak] 2013-01-03 02:34:54 PST
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.
Comment 10 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-01-03 03:38:00 PST
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.
Comment 11 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-01-03 03:39:32 PST
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.
Comment 12 Marco Bonardo [::mak] 2013-01-03 15:33:09 PST
Created attachment 697697 [details] [diff] [review]
patch v1.0

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.
Comment 13 Marco Bonardo [::mak] 2013-01-04 07:08:56 PST
Created attachment 697904 [details] [diff] [review]
patch v1.1

Cleaned up and ready for review
Comment 14 Marco Bonardo [::mak] 2013-01-04 07:26:02 PST
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.
Comment 15 Marco Bonardo [::mak] 2013-01-04 07:26:50 PST
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 16 Marco Bonardo [::mak] 2013-01-04 07:29:13 PST
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.
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-01-04 08:05:31 PST
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 (...).
Comment 18 Marco Bonardo [::mak] 2013-01-04 11:04:51 PST
Created attachment 698001 [details] [diff] [review]
patch v1.2

Addressed review comments
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-01-04 13:43:27 PST
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.
Comment 20 Marco Bonardo [::mak] 2013-01-04 15:15:44 PST
Created attachment 698112 [details] [diff] [review]
patch v1.3

addressed SR, fixed warning
Comment 21 Marco Bonardo [::mak] 2013-01-04 17:26:59 PST
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)
Comment 23 Marco Bonardo [::mak] 2013-01-05 01:26:13 PST
the changes should be backwards compatible for add-ons, though if someone implements nsIDownloadHistory (unlikely) they will need to add the new method.
Comment 24 Phil Ringnalda (:philor) 2013-01-05 16:26:13 PST
https://hg.mozilla.org/mozilla-central/rev/9b891394bb82

Note You need to log in before you can comment on or make changes to this bug.