908240 - Legacy downloads not executed by nsIHelperAppLauncher should be added to history

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The nsIHelperAppLauncher implementation adds the download to the history:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1927

In other cases, we should do this in DownloadLegacySaver, as soon as we have
the referrer, like nsDownloadManager.cpp does:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2891

This affects, for example, pages saved using "Save Page As".

Note that the old MIME info check may be replaced with a check to see if
aCancelable is an instance of nsIHelperAppLauncher.

Comment 1

[:Paolo Amadini]

Attachment: The patch

In this patch, I stored _cancelable and checked it later because this is also
useful when handling cancellation in bug 899102.

Comment 2

[Neil Deakin]

Comment on attachment 798604 [details] [diff] [review]
The patch

>+   * @param aAlreadyAddedToHistory
>+   *        Indicates that the nsIExternalHelperAppService component already
>+   *        added the download to the browsing history, unless it was started
>+   *        from a private browsing window.  When this parameter is false, the
>+   *        download is added to the browsing history here, if it is public.

If it is however a private browsing window, what does the argument indicate?


>-      this._deferDownload.promise.then(function (aDownload) {
>-        aDownload.saver.onTransferStarted(aRequest);
>+      this._deferDownload.promise.then(download => {
>+        download.saver.onTransferStarted(
>+                         aRequest,
>+                         this._cancelable instanceof Ci.nsIHelperAppLauncher);

So, when this a private download, what does 'this._cancelable instanceof Ci.nsIHelperAppLauncher' result in?


>   /**
>+   * Reference to the component that is executing the download, that allows
>+   * cancellation.
>+   */

The sentence is a bit odd here with this comma in it.

 
>-  PlacesUtils.history.removeAllPages();
>+  yield promiseClearHistory();
> 
>+  // Wait for the removal notifications, that may still be pending.

Remove the comma.

Comment 3

[:Paolo Amadini]

Attachment: Updated comments

(In reply to Neil Deakin from comment #2)
> >-      this._deferDownload.promise.then(function (aDownload) {
> >-        aDownload.saver.onTransferStarted(aRequest);
> >+      this._deferDownload.promise.then(download => {
> >+        download.saver.onTransferStarted(
> >+                         aRequest,
> >+                         this._cancelable instanceof Ci.nsIHelperAppLauncher);
> 
> So, when this a private download, what does 'this._cancelable instanceof
> Ci.nsIHelperAppLauncher' result in?

This still evaluates to true, so the download is not added to history. I've clarified the comment in the called method.

Comment 4

[:Paolo Amadini]

Attachment: Fix expiration test

This patch fixes a DownloadList test that failed because the new visits added
for current downloads couldn't be expired because they weren't older than 7 days.

Comment 5

[Neil Deakin]

Paolo, is this patch ready for review?

Comment 6

[:Paolo Amadini]

(In reply to Neil Deakin from comment #5)
> Paolo, is this patch ready for review?

I considered this ready for check-in after more manual testing, but if you want to take a look to the unit test changes feel free to do so (I've already taken out the dump statement).

Comment 7

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/493dd25e60c2

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/493dd25e60c2

Comment 9

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

Backed out in https://hg.mozilla.org/integration/fx-team/rev/94d10f159d3a for causing XPCShell failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=27601606&tree=Fx-Team

Comment 10

[:Paolo Amadini]

(In reply to Wes Kocher (:KWierso) from comment #9)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/94d10f159d3a
> for causing XPCShell failures like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27601606&tree=Fx-Team

This is most probably a bug in the test, due to a rounding issue when converting microseconds to milliseconds. I added a one second margin to be on the safe side, and re-landed directly:

https://hg.mozilla.org/integration/fx-team/rev/86fb0f0330c8

Comment 11

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #10)
> I added a one second margin to be on the safe side, and re-landed directly:
> https://hg.mozilla.org/integration/fx-team/rev/86fb0f0330c8

I intended to add that, but lost the change in the integration branch. New commit:

https://hg.mozilla.org/integration/fx-team/rev/27d9c8816436

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86fb0f0330c8
https://hg.mozilla.org/mozilla-central/rev/27d9c8816436