Legacy downloads not executed by nsIHelperAppLauncher should be added to history

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 1 bug)

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

6 years ago
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.
Assignee

Updated

6 years ago
Blocks: 910731
Assignee

Comment 1

6 years ago
Posted patch The patch (obsolete) — Splinter Review
In this patch, I stored _cancelable and checked it later because this is also
useful when handling cancellation in bug 899102.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #798604 - Flags: review?(enndeakin)

Comment 2

6 years ago
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.
Attachment #798604 - Flags: review?(enndeakin) → review+
Assignee

Comment 3

6 years ago
Posted patch Updated comments (obsolete) — Splinter Review
(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.
Attachment #798604 - Attachment is obsolete: true
Assignee

Comment 4

6 years ago
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.
Attachment #800170 - Attachment is obsolete: true

Comment 5

6 years ago
Paolo, is this patch ready for review?
Blocks: 908246
Blocks: 908244
Assignee

Comment 6

6 years ago
(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).
https://hg.mozilla.org/mozilla-central/rev/493dd25e60c2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 10

6 years ago
(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
Assignee

Comment 11

6 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/86fb0f0330c8
https://hg.mozilla.org/mozilla-central/rev/27d9c8816436
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.