Convert SeaMonkey Downloads Manager to Downloads.jsm

RESOLVED FIXED

Status

defect
--
blocker
RESOLVED FIXED
6 years ago
6 months ago

People

(Reporter: philip.chee, Assigned: frg)

Tracking

(Blocks 9 bugs)

SeaMonkey Tracking Flags

(seamonkey2.52 wontfix, seamonkey2.53 affected, seamonkey2.57esr fixed, seamonkey2.60 fixed)

Details

()

User Story

On the Pro side it no longer crashes the browser after resume :)

MIA/non working/buggy:

- Import downloads.sqlite not implemented.
- Options for Smafebrowsing and unblocking missing (new functionality)
- "Search Downloads" not working.
- Does not autoresume downloads after restart.
- nsIChannel error thrown after program close and resume download. Needs resume twice.
- Local and mail attachment file saves do not always show file size.
- Progressmeter removed in: Bug 1430374 "Remove support for treecol/treecell[type=progressmeter]". 2.57+.

Fixed:
- "Clear List" not working. (fixed part 4)
- "Open Containing Folder" not working during transfer. (fixed part 4)
- Download Progress only working in Download Manager itself. (fixed)

- Retry/Resume error:  (fixed part 4)

Timestamp: 5/27/2017, 12:57:49 PM
Error: A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

- (fixed part 4) no longer see it.
Date: Sat May 27 2017 12:57:23 GMT+0200 (W. Europe Standard Time)
Full Message: DownloadError: [Exception... "It was attempted to resume the request, but the entity has changed in the meantime"  nsresult: "0x804b0020 (NS_ERROR_ENTITY_CHANGED)"  location: "JS frame :: resource://gre/modules/DownloadCore.jsm :: this.DownloadError :: line 1524"  data: no]
Full Stack: this.DownloadError@resource://gre/modules/DownloadCore.jsm:1559:16
onSaveComplete@resource://gre/modules/DownloadCore.jsm:1955:42

Attachments

(16 attachments, 17 obsolete attachments)

70.49 KB, image/jpeg
Details
73.04 KB, image/jpeg
Details
67.10 KB, image/jpeg
Details
76.45 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
16.69 KB, patch
Details | Diff | Splinter Review
15.22 KB, patch
Details | Diff | Splinter Review
48.88 KB, patch
Details | Diff | Splinter Review
16.79 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
14.17 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
48.47 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
70.05 KB, patch
Details | Diff | Splinter Review
62.30 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
29.89 KB, patch
Details | Diff | Splinter Review
11.73 KB, patch
Details | Diff | Splinter Review
29.18 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
11.67 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
From Bug 851471 - Decommission nsIDownloadManager:

> When the new JavaScript API for downloads is enabled in its final version and
> host applications have started using it in place of nsIDownloadManager,
> automated tests for the latter on mozilla-central should be decommissioned.
> 
> This can be achieved gradually by first hiding compilation of the entire
> "toolkit/components/downloads" module behind a build-time setting, then
> removing it from the tree when all consumers have switched over.
Reporter

Updated

6 years ago
Summary: Move comm-central to the new JavaScript API for downloads when nsIDownloadManager is decommissioned → Move SeaMonkey to the new JavaScript API for downloads when nsIDownloadManager is decommissioned

Comment 1

6 years ago
I'm reversing the dependency because of how we're tracking the work remaining
before we can disable nsIDownloadManager in all the default builds, though
technically we're not blocking on the SeaMonkey migration.

There are several uses of nsIDownloadManager in the "suite" folder, though most
of them are in user interface tests:

http://mxr.mozilla.org/comm-central/search?string=nsIDownloadManager&find=suite

Speaking of tests, Downloads.jsm has a much simpler interface that, differently
from the old nsIDownloadManager, has also a very thorough test suite, so many
tests checking details of download behavior probably don't need to be ported.
Blocks: 851471
Depends on: 847863
No longer depends on: jsdownloads, 851471
Summary: Move SeaMonkey to the new JavaScript API for downloads when nsIDownloadManager is decommissioned → Convert SeaMonkey to Downloads.jsm

Comment 2

6 years ago
Also, we're about to enable the use of Downloads.jsm in Desktop Nightly, and
later we will clean up the code (now it is complicated because it supports
both back-ends). You'll be able to use that as an example. If you need more
features from the API, feel free to file new bugs blocking bug 825588.
Reporter

Updated

6 years ago
Summary: Convert SeaMonkey to Downloads.jsm → Convert SeaMonkey Downloads Manager to Downloads.jsm

Comment 3

4 years ago
So, this bug is about converting to Downloads.jsm, for which there are several front-end implementations around, including Thunderbird's new and pretty self-contained "about:downloads" and the Downloads window of Webapp Runtime.

But I think the easiest way not to be affected by the removal of nsIDownloadManager would be to move all of /mozilla/toolkit/mozapps/downloads and most of /mozilla/toolkit/components/downloads to subfolders of /suite/common/downloads.

I just wanted to give a heads up that the first step will be removing all the obsolete tests from those Toolkit folders, so if you're interested in keeping them and you're not planning on doing the migration soon, you'll need to recover them from an older revision later.
Flags: needinfo?(philip.chee)
Reporter

Comment 4

4 years ago
Thanks for the heads up!
Flags: needinfo?(philip.chee)
Posted patch WIP (obsolete) — Splinter Review
There are a lot of rough edges still. Things I know that definitely don't work:
Download Manager/Progress Dialog doesn't open automatically
Filtering isn't implemented
Some functions don't work in the progress dialog
Attachment #8604377 - Flags: feedback?(philip.chee)
Attachment #8604377 - Flags: feedback?(iann_bugzilla)
Attachment #8604377 - Flags: feedback?(ewong)
Comment on attachment 8604377 [details] [diff] [review]
WIP

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

::: suite/common/src/nsSuiteGlue.js
@@ +54,5 @@
>  // Devtools Preferences
>  const DEBUGGER_REMOTE_ENABLED = "devtools.debugger.remote-enabled";
>  const DEBUGGER_REMOTE_PORT = "devtools.debugger.remote-port";
>  const DEBUGGER_FORCE_LOCAL = "devtools.debugger.force-local";
> +const DEBUGGER_WIFI_VISIBLE = "devtools.remote.wifi.visible";

Out of curiosity, (and a minor nit) is this from a different patch?

@@ +1018,5 @@
> +
> +      // Expose this listener via wifi discovery, if enabled.
> +      if (Services.prefs.getBoolPref(DEBUGGER_WIFI_VISIBLE) &&
> +          !Services.prefs.getBoolPref(DEBUGGER_FORCE_LOCAL)) {
> +        listener.discoverable = true;

Not sure about this as well.
Comment on attachment 8604377 [details] [diff] [review]
WIP

also, with recent pull (to fix bug 777770),  this patch
seems to be bitrotted.
(In reply to Edmund Wong from comment #6)
> ::: suite/common/src/nsSuiteGlue.js
> Out of curiosity, is this from a different patch?
Yes, it's from bug 1131997. Sorry about that. (It will go away when I update my tree.)
Posted patch WIP (obsolete) — Splinter Review
Updated for bitrot.
Attachment #8604377 - Attachment is obsolete: true
Attachment #8604377 - Flags: feedback?(philip.chee)
Attachment #8604377 - Flags: feedback?(iann_bugzilla)
Attachment #8604377 - Flags: feedback?(ewong)
Attachment #8609832 - Flags: feedback?(philip.chee)
Attachment #8609832 - Flags: feedback?(iann_bugzilla)
Attachment #8609832 - Flags: feedback?(ewong)

Comment 10

4 years ago
Comment on attachment 8609832 [details] [diff] [review]
WIP

When trying to open Download Manager from Tools menu get following message in error console:
Timestamp: 29/06/15 22:48:28
Error: NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]
Source File: chrome://communicator/content/tasksOverlay.js
Line: 30
Attachment #8609832 - Flags: feedback?(iann_bugzilla) → feedback-
(In reply to Ian Neal from comment #10)
> When trying to open Download Manager from Tools menu get following message
> in error console:
> Timestamp: 29/06/15 22:48:28
> Error: NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code:
> 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]
> Source File: chrome://communicator/content/tasksOverlay.js
> Line: 30

Sounds like the patch bitrotted again and didn't apply properly.
Posted patch Updated for bitrot (obsolete) — Splinter Review
Attachment #8609832 - Attachment is obsolete: true
Attachment #8609832 - Flags: feedback?(philip.chee)
Attachment #8609832 - Flags: feedback?(ewong)
Attachment #8642108 - Flags: feedback?(philip.chee)
Attachment #8642108 - Flags: feedback?(iann_bugzilla)
Attachment #8642108 - Flags: feedback?(ewong)

Comment 13

4 years ago
Comment on attachment 8642108 [details] [diff] [review]
Updated for bitrot

From testing to download to a location:
* "Clear List" button seems to always be disabled (right click, "Remove From List" does work on individual items though).
* When you cancel a download you get the following in the error console:
Timestamp: 15/08/15 15:59:13
Error: NS_BASE_STREAM_CLOSED: Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIStreamListener.onDataAvailable]
Source File: resource://gre/modules/DownloadCore.jsm
Line: 2049
* When you right click and "Open containing folder", you get the following two messages in the error console:
Timestamp: 15/08/15 16:01:14
Error: TypeError: file is undefined
Source File: chrome://communicator/content/downloads/downloadmanager.js
Line: 211
Timestamp: 15/08/15 16:01:14
Error: An error occurred executing the cmd_show command: [Exception... "[JavaScript Error: "file is undefined" {file: "chrome://communicator/content/downloads/downloadmanager.js" line: 211}]'[JavaScript Error: "file is undefined" {file: "chrome://communicator/content/downloads/downloadmanager.js" line: 211}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 96"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 99
* When you right click and "Open", you get the following two messages in the error console:
Timestamp: 15/08/15 16:02:52
Error: TypeError: file is undefined
Source File: chrome://communicator/content/downloads/downloadmanager.js
Line: 152
Timestamp: 15/08/15 16:02:52
Error: An error occurred executing the cmd_open command: [Exception... "[JavaScript Error: "file is undefined" {file: "chrome://communicator/content/downloads/downloadmanager.js" line: 152}]'[JavaScript Error: "file is undefined" {file: "chrome://communicator/content/downloads/downloadmanager.js" line: 152}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 96"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 99

Testing to open immediate does work.
Attachment #8642108 - Flags: feedback?(iann_bugzilla) → feedback-
(In reply to Ian Neal from comment #13)
> From testing to download to a location:
> * "Clear List" button seems to always be disabled (right click, "Remove From
> List" does work on individual items though).
Yeah, there's no obvious equivalent to .cleanUp() in Downloads.jsm, so I haven't written this yet.

> * When you cancel a download you get the following in the error console:
> Timestamp: 15/08/15 15:59:13
> Error: NS_BASE_STREAM_CLOSED: Component returned failure code: 0x80470002
> (NS_BASE_STREAM_CLOSED) [nsIStreamListener.onDataAvailable]
> Source File: resource://gre/modules/DownloadCore.jsm
> Line: 2049
Core(!) bug, I guess.

> Error: TypeError: file is undefined
Oops. Will fix.
(In reply to comment #14)
> there's no obvious equivalent to .cleanUp() in Downloads.jsm

That's because it got moved to DownloadList.jsm... it's called removeFinished()... will fix.
Posted patch Updated for review comments (obsolete) — Splinter Review
Attachment #8648425 - Flags: feedback?(philip.chee)
Attachment #8648425 - Flags: feedback?(iann_bugzilla)
Attachment #8648425 - Flags: feedback?(ewong)

Comment 17

4 years ago
Comment on attachment 8648425 [details] [diff] [review]
Updated for review comments

>+++ b/suite/common/downloads/downloadmanager.js
>@@ -651,22 +620,20 @@ var gDownloadDNDObserver = {
>   onDragStart: function (aEvent)
>   {
>     if (!gDownloadTreeView ||
>         !gDownloadTreeView.selection ||
>         !gDownloadTreeView.selection.count)
>       return;
> 
>     var selItemData = gDownloadTreeView.getRowData(gDownloadTree.currentIndex);
>-    var file = GetFileFromString(selItemData.file);
>-
>-    if (!file.exists())
>+    if (!selItemData.target.exists)
>       return;
The Firefox equivalent talks about having to do this "synchronously because this is a DOM event", so still uses file.exists()
> 
>-    var url = Services.io.newFileURI(file).spec;
>+    var url = Services.io.newFileURI(new nsLocalFile(selItemData.target.path)).spec;
You still need a file for a little later...
>     var dt = aEvent.dataTransfer;
>     dt.mozSetDataAt("application/x-moz-file", file, 0);
...just here
>     dt.setData("text/uri-list", url + "\r\n");
>     dt.setData("text/plain", url + "\n");
>     dt.effectAllowed = "copyMove";
>     if (gDownloadTreeView.selection.count == 1)
>       dt.setDragImage(gDownloadStatus, 16, 16);
>   },

Is it worth making use of FileUtils.jsm and NetUtil.jsm?
Attachment #8648425 - Flags: feedback?(iann_bugzilla) → feedback-

Comment 18

4 years ago
(In reply to neil@parkwaycc.co.uk from comment #14)
> (In reply to Ian Neal from comment #13)
> > * When you cancel a download you get the following in the error console:
> > Timestamp: 15/08/15 15:59:13
> > Error: NS_BASE_STREAM_CLOSED: Component returned failure code: 0x80470002
> > (NS_BASE_STREAM_CLOSED) [nsIStreamListener.onDataAvailable]
> > Source File: resource://gre/modules/DownloadCore.jsm
> > Line: 2049
> Core(!) bug, I guess.

Tested it that it happens on Firefox too and logged as Bug 1195053
Posted patch Updated for review comments (obsolete) — Splinter Review
Well, that simplifies those changes, anyway. Also this gives me the chance to obsolete the old patch which I forgot to do before ;-)
Attachment #8642108 - Attachment is obsolete: true
Attachment #8648425 - Attachment is obsolete: true
Attachment #8642108 - Flags: feedback?(philip.chee)
Attachment #8642108 - Flags: feedback?(ewong)
Attachment #8648425 - Flags: feedback?(philip.chee)
Attachment #8648425 - Flags: feedback?(ewong)
Attachment #8648439 - Flags: feedback?(philip.chee)
Attachment #8648439 - Flags: feedback?(iann_bugzilla)
Attachment #8648439 - Flags: feedback?(ewong)

Comment 20

4 years ago
Comment on attachment 8648439 [details] [diff] [review]
Updated for review comments

>+++ b/suite/common/downloads/downloadmanager.js
>       case "cmd_clearList":
>-        return gDownloadTreeView.rowCount && gDownloadManager.canCleanUp;
>+        // Since active downloads always sort before removable downloads,
>+        // we only need to check that the last download has stopped.
>+        return gDownloadTreeView.rowCount &&
>+               gDownloadTreeView.getRowData(gDownloadTreeView.rowCount - 1).stopped;
Do we really want clear list to clear paused downloads?
Flags: needinfo?(neil)
(In reply to Ian Neal from comment #20)
> (From update of attachment 8648439 [details] [diff] [review])
> >       case "cmd_clearList":
> >-        return gDownloadTreeView.rowCount && gDownloadManager.canCleanUp;
> >+        // Since active downloads always sort before removable downloads,
> >+        // we only need to check that the last download has stopped.
> >+        return gDownloadTreeView.rowCount &&
> >+               gDownloadTreeView.getRowData(gDownloadTreeView.rowCount - 1).stopped;
> Do we really want clear list to clear paused downloads?
Yeah, this should use isActive. (I actually had it as isActive earlier and then changed it because I forgot that I'm emulating it. D'oh!)
Flags: needinfo?(neil)
Comment on attachment 8648439 [details] [diff] [review]
Updated for review comments

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

::: suite/common/downloads/downloadmanager.js
@@ +7,4 @@
>  
>  const nsIDownloadManager = Components.interfaces.nsIDownloadManager;
> +const nsLocalFile = Components.Constructor("@mozilla.org/file/local;1",
> +                    Components.interfaces.nsILocalFile, "initWithPath");

Just a nit (not that it would affect the 
functionality of the code).

I think it would be a little less confusing if
this was indented flushed with the "@mozilla...".
And since that'd result in a longer line,
maybe have "initWithPath" on its own line.

Comment 23

4 years ago
Comment on attachment 8648439 [details] [diff] [review]
Updated for review comments

New patch on the way?
Attachment #8648439 - Flags: feedback?(iann_bugzilla) → feedback-

Comment 24

4 years ago
Will fixing this bug include fixing bug #918188?
Posted patch Updated for review comments (obsolete) — Splinter Review
Attachment #8648439 - Attachment is obsolete: true
Attachment #8648439 - Flags: feedback?(philip.chee)
Attachment #8648439 - Flags: feedback?(ewong)
Attachment #8666538 - Flags: feedback?(philip.chee)
Attachment #8666538 - Flags: feedback?(iann_bugzilla)
Attachment #8666538 - Flags: feedback?(ewong)
Just managed to get some time to build on my Win8.1 and here are a few items:

1) Download's faster  :)
2) Resume doesn't work.  (http://ftp.nara.wide.ad.jp/pub/Linux/slackware/slackware-14.1-iso/)

I get this error:

Timestamp: 10/8/2015 12:25:45 PM
Error: A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Thu Oct 08 2015 12:25:40 GMT+0800 (China Standard Time)
Full Message: DownloadError: [Exception... "This request is not resumable, but it was tried to resume it, or to request resume-specific data"  nsresult: "0x804b0019 (NS_ERROR_NOT_RESUMABLE)"  location: "JS frame :: resource://gre/modules/DownloadCore.jsm :: this.DownloadError :: line 1498"  data: no]
Full Stack: this.DownloadError@resource://gre/modules/DownloadCore.jsm:1530:16
DCS_execute/task_DCS_execute/backgroundFileSaver.observer.onSaveComplete@resource://gre/modules/DownloadCore.jsm:1900:42

Source Code:
0

I just tried this on 2.33, and apparently it doesn't work with that ftp site,
although clicking on resume creates a new download entry in the download manager
list.
Comment on attachment 8666538 [details] [diff] [review]
Updated for review comments

Apparently that link isn't resumable.  Will find a place that can resume
it.

Updated

4 years ago
Attachment #8666538 - Flags: feedback?(iann_bugzilla) → feedback+
Comment on attachment 8666538 [details] [diff] [review]
Updated for review comments

Looks good.  I'm finding it is faster for some reason.  Maybe it's
just the server or something.
Attachment #8666538 - Flags: feedback?(ewong) → feedback+
Assignee

Comment 29

4 years ago
Posted image Clipboard.jpg
had some time yesterday evening and did put the patch in 2.41 

User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0 SeaMonkey/2.41

if this isn't supposed to work in this version disregard the commets.

1.) Clear most of the time grayed out

see screenshot

2.) Tools->Download Manager and Control-J not working

Timestamp: 1/20/2016 8:55:08 AM
Error: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDownloadManager.addPrivacyAwareListener]
Source File: resource://gre/modules/DownloadTaskbarProgress.jsm
Line: 139

Timestamp: 1/20/2016 8:55:19 AM
Error: TypeError: Components.classes['@mozilla.org/suite/suiteglue;1'].getService(...).showDownloadManager is not a function
Source File: chrome://communicator/content/tasksOverlay.js
Line: 30

3.) Pausing a Download works but entry in log:

Timestamp: 1/20/2016 10:14:59 AM
Error: NS_BASE_STREAM_CLOSED: Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIStreamListener.onDataAvailable]
Source File: resource://gre/modules/DownloadCore.jsm
Line: 2049

4.) Error when you try to clear a failed download while you still download the file a second time:

Timestamp: 1/20/2016 8:57:26 AM
Error: A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Wed Jan 20 2016 08:57:25 GMT+0100 (W. Europe Standard Time)
Full Message: Win error 32 during operation remove on file D:\Newstuff\klcp_update_1186_20160118.exe.part (The process cannot access the file because it is being used by another process.
)
Full Stack: JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: PendingErrors.register :: line 191
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.completePromise :: line 715
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 937
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 813
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.scheduleWalkerLoop/< :: line 747
Source File: (unknown module)
Line: 0
Source Code:
0

5.) Only the part file is allocated. If you start a second download for the same file you will also not be prompted. Previously the target file without the .part ending was also created with 0 bytes.
Assignee

Comment 30

4 years ago
Posted image Clear Button enabled
>> 1.) Clear most of the time grayed out

Funny. when you clear tle list one by one and a download is still active the clear button is enabled

In the case the 0 byte exe was also allocated.
Assignee

Comment 31

4 years ago
And as soon as the download is finished it's gray again

Comment 32

3 years ago
The first thing I'll remove from mozilla-central are the tests for nsIDownloadManager, that are currently disabled on all trees that define MOZ_JSDOWNLOADS. I'm doing this now since we're converting tests for e10s, and removing dead test code will help. I also think the Toolkit implementation of nsIDownloadManagerUI is not actually used by SeaMonkey, so it could also go away, though the interface definition will remain.

As far as I can tell, the above should not have any impact on SeaMonkey builds since it's only removing test coverage, and the tested functionality is going away as part of this bug. I still want to give a heads up that any unintentional fallout from the removal will have to be dealt with in comm-central.

Later I'll remove the nsIDownloadManager and nsIDownloadManagerUI interfaces. The constants you're using from nsIDownloadManager, like nsIDownloadManager.DOWNLOAD_DOWNLOADING, should be redefined elsewhere. The MOZ_JSDOWNLOADS build time conditionals will be unnecessary at this point and will go away too. This will definitely break SeaMonkey builds if this bug is not fixed. I don't have a timeline because the removal takes time and we'll plan for it when it's more convenient based on other mozilla-central maintenance work we have to do.

Updated

3 years ago
Depends on: 1253585

Comment 33

3 years ago
Having never received an answer to my comment #24, I again ask:  
Will bug #918188 also be resolved by fixing this bug?
Reporter

Updated

3 years ago
Blocks: 1266965
Assignee

Comment 34

3 years ago
>> Having never received an answer to my comment #24, I again ask:  
>> Will bug #918188 also be resolved by fixing this bug?

No.
Assignee

Comment 35

3 years ago
Posted patch 888915-WIP.patch (obsolete) — Splinter Review
Up to date patch for current 2.47a1 c-c.
Only bit rot fixes.

Comment 36

3 years ago
Just as a note in downloadmanager.js types.contains() should change to types.includes() as the former is deprecated.

Updated

3 years ago
Blocks: 1224326

Comment 37

2 years ago
After about four years since we switched to the new Downloads API, I'm going to remove the old code in bug 888915.
No longer blocks: 851471

Comment 38

2 years ago
Bug 851471 actually.
Assignee

Comment 39

2 years ago
Thanks for the heads up. We have a WIP patch ready and will track changes to Bug 851471.
Depends on: 851471
See Also: → 1365882
Assignee

Updated

2 years ago
Severity: normal → blocker
Assignee

Comment 40

2 years ago
Posted patch 888915-WIP.patch (obsolete) — Splinter Review
Unbitrotted patch. Should I ask for review and just work from there in followup patches? It still has the problems I mentioned but it would be a start.
Attachment #8666538 - Attachment is obsolete: true
Attachment #8763994 - Attachment is obsolete: true
Attachment #8666538 - Flags: feedback?(philip.chee)
Attachment #8869084 - Flags: feedback?(iann_bugzilla)
Assignee

Comment 41

2 years ago
Posted patch 888915-WIP2.patch (obsolete) — Splinter Review
downloadtaskbarprogess.jsm needs a total rewrite but otherwise the browser incl. sessionrestore now actually works again. Left the old parts in for reference and as a starting point.
Attachment #8869084 - Attachment is obsolete: true
Attachment #8869084 - Flags: feedback?(iann_bugzilla)
Attachment #8869410 - Flags: feedback?(iann_bugzilla)

Comment 42

2 years ago
Comment on attachment 8869410 [details] [diff] [review]
888915-WIP2.patch

>+++ b/suite/common/downloads/downloadmanager.js
> 
>   doCommand: function(aCommand) {
>@@ -532,97 +513,86 @@ var dlTreeController = {
>         for (let row = start.value; row <= end.value; row++)
>           selItemData.push(gDownloadTreeView.getRowData(row));
>       }
>     }
> 
>     switch (aCommand) {
>       case "cmd_play":
>         for (let dldata of selItemData) {
>+          if (!dldata.stopped)
>+            dldata.cancel();
>+          else if (!dldata.succeeded)
>+            dldata.start();
>         }
>         break;
>       case "cmd_pause":
>         for (let dldata of selItemData)
>+          dldata.cancel();
>         break;
>       case "cmd_resume":
>         for (let dldata of selItemData)
>+          dldata.start();
>         break;
>       case "cmd_retry":
>         for (let dldata of selItemData)
>+          dldata.start();
>         break;
You could merge cmd_resume and cmd_retry here as they are the same code.

>+++ b/suite/common/downloads/progressDialog.js

>   doCommand: function(aCommand) {
>     switch (aCommand) {
>       case "cmd_pause":
>+        gDownload.cancel();
>         break;
>       case "cmd_resume":
>+        gDownload.start();
>         break;
>       case "cmd_retry":
>+        gDownload.start();
>         break;
You could merge cmd_resume and cmd_retry here as they are the same code.

f+
Attachment #8869410 - Flags: feedback?(iann_bugzilla) → feedback+
Assignee

Updated

2 years ago
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
User Story: (updated)
Assignee

Comment 43

2 years ago
As stated that is the version I would like to start with. It is just Neils patch cleaned up and fixed in a few places.

Despite the long list of problems it is functional.
Attachment #8869410 - Attachment is obsolete: true
Attachment #8872028 - Flags: review?(iann_bugzilla)
Assignee

Updated

2 years ago
Duplicate of this bug: 1368691

Comment 45

2 years ago
Comment on attachment 8872028 [details] [diff] [review]
888915-downloadmanagerintial.patch

LGTM r=me
Attachment #8872028 - Flags: review?(iann_bugzilla) → review+
Assignee

Comment 46

2 years ago
https://hg.mozilla.org/comm-central/rev/f80e43c05f8892809d13e8a0fa2d5aa839657d4a

Leaving the bug open till I have time to open followup bugs.

Updated

2 years ago
See Also: → 1370981
Assignee

Updated

2 years ago
Blocks: 1373679
Assignee

Updated

2 years ago
Duplicate of this bug: 1381332
Assignee

Updated

2 years ago
Depends on: 1408630
Assignee

Updated

2 years ago
User Story: (updated)

Updated

2 years ago
Blocks: 1430374
Assignee

Updated

Last year
Duplicate of this bug: 1440232
Assignee

Updated

Last year
Depends on: 1444217
Assignee

Updated

Last year
Blocks: 1464499
Assignee

Comment 49

Last year
part 1 Move downloads from common to components. 2.53 version
Assignee

Comment 50

Last year
Part 2 remove nsISuiteDownloadManagerUI. It is still used in some tests but totally broken after nsDownloadManagerUI removal. Remaing test cases need to be addressed in Bug 1464499.
2.53 version
Assignee

Comment 51

Last year
Forgot to qrefresh
Attachment #8980750 - Attachment is obsolete: true
Assignee

Comment 52

Last year
Part 3. Fix the taskbar progress. 2.53 version.
Assignee

Comment 53

Last year
Minor tweak. Appconstants not needed.
Attachment #8980783 - Attachment is obsolete: true
Assignee

Comment 54

Last year
Move downloads manager to components. 2.57 version. If it gets r+ and needs rebasing for comm-central I will do it during check-in.
Attachment #8980965 - Flags: review?(iann_bugzilla)
Attachment #8980965 - Flags: approval-comm-esr60?
Assignee

Comment 55

Last year
Remove obsolete nsISuiteDownloadManagerUI. 2.57 version. If it gets r+ and needs rebasing for comm-central I will do it during check-in.
Attachment #8980966 - Flags: review?(iann_bugzilla)
Attachment #8980966 - Flags: approval-comm-esr60?
Assignee

Comment 56

Last year
Fix taskbar progress. 2.57 version. If it gets r+ and needs rebasing for comm-central I will do it during check-in.

Stay tuned for part 4. The download manager is still broken in 2.57 but taskbar progress works with the patch. 

After everything is in place I will need to revisit private windows behaviour.
Attachment #8980967 - Flags: review?(iann_bugzilla)
Attachment #8980967 - Flags: approval-comm-esr60?
Assignee

Comment 57

Last year
Comment on attachment 8980967 [details] [diff] [review]
888915-part3-DownloadsTaskbar-257.patch

Btw. DownloadsCommon has some additional code in it not yet needed. The last patch for it will clean out all unused code.
Assignee

Updated

Last year
User Story: (updated)

Comment 58

Last year
Comment on attachment 8980965 [details] [diff] [review]
888915-part1-downloads2-257.patch

LGTM r/a=me
Attachment #8980965 - Flags: review?(iann_bugzilla)
Attachment #8980965 - Flags: review+
Attachment #8980965 - Flags: approval-comm-esr60?
Attachment #8980965 - Flags: approval-comm-esr60+

Comment 59

Last year
Comment on attachment 8980966 [details] [diff] [review]
888915-part2-nsISuiteDownloadManagerUI-257.patch

LGTM r/a=me
Attachment #8980966 - Flags: review?(iann_bugzilla)
Attachment #8980966 - Flags: review+
Attachment #8980966 - Flags: approval-comm-esr60?
Attachment #8980966 - Flags: approval-comm-esr60+

Comment 60

Last year
Comment on attachment 8980967 [details] [diff] [review]
888915-part3-DownloadsTaskbar-257.patch

LGTM r/a=me
and part 4?
Attachment #8980967 - Flags: review?(iann_bugzilla)
Attachment #8980967 - Flags: review+
Attachment #8980967 - Flags: approval-comm-esr60?
Attachment #8980967 - Flags: approval-comm-esr60+
Assignee

Comment 61

Last year
WIP patch for the 2.53 download manager window. Dates are formatted again and should be ok. Clear works.

DownloadsCommon is a mess and some things still broken.

Progressmeter also needs to be replaced to make this work in 2.57

Comment 62

Last year
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/336cb543d0d0
Part 1. Align Download Manager file structure with other components. r=IanN
https://hg.mozilla.org/comm-central/rev/72c25a32b210
Part 2. Remove obsolete nsISuiteDownloadManagerUI. r=IanN
https://hg.mozilla.org/comm-central/rev/7a0e82f04b58
Part 3. Fix the downloads taskbar progress. r=IanN
Assignee

Comment 64

Last year
It is still not where I want it but working.

search still broken. This probably needs a followup patch. I did hide the search field.

The progressmeter is also not replaced yet for 2.57.

End time and bytes transfered is erratic but this is a limitation of the current api. For email attachments the toolkit history apis choke here and return nothing. Exception is caught and logged.
Attachment #8983178 - Attachment is obsolete: true
Assignee

Comment 65

Last year
Part 5 mostly a placeholder right now for further nsIDownloadManager removals outside the actual download manager.
Assignee

Comment 66

Last year
Part 4 2.53
Attachment #8987383 - Attachment is obsolete: true
Assignee

Comment 67

Last year
Part 4 2.57 If it gets r+ and does not ally on c-c I will rebase.

Fixed:

- "Clear List" not working.
- "Open Containing Folder" not working during transfer.
- Date, time and status display.
- retry/resume seems to work fine. No more chrashes.

Not Fixed: 
- progressmeter for 2.57. Saved for another day. 
- unblocking blocked downloads. skeleton in downloadscommon.jsm. Stay tuned for another patch later.
- Some attachment (e.g. imap) and local file saves do not display size information and can't be opened. They are correctly saved. This seems to be a problem with the downloads api. The downloads.json is missing size and target.path entries for these. Fix me later or never.
- Failed transfers need to be restarted twice after program program close and reopen. Suspect another problem with the downlaods api. Might be fixed in 2.57.
- "Search Downloads" not working. Only did hide the searchbox right now. This needs another patch.
Attachment #8989051 - Flags: review?(iann_bugzilla)
Attachment #8989051 - Flags: approval-comm-esr60?
Assignee

Updated

Last year
User Story: (updated)
Assignee

Updated

Last year
User Story: (updated)
Assignee

Comment 68

Last year
Noticed that the otherDownloads3 string is not used. I will removed it either in a new version or during checkin if part 4 is ok as is.
Assignee

Comment 69

Last year
Comment on attachment 8989051 [details] [diff] [review]
888915-part4-downloads2-257.patch

Introduced a snafu in the transfer calculation.
Attachment #8989051 - Flags: review?(iann_bugzilla)
Attachment #8989051 - Flags: approval-comm-esr60?
Assignee

Comment 70

Last year
Attachment #8987384 - Attachment is obsolete: true
Assignee

Updated

Last year
Attachment #8989050 - Attachment is obsolete: true
Assignee

Comment 72

Last year
This one works. Sorry for the bug noise.
Attachment #8989051 - Attachment is obsolete: true
Attachment #8989083 - Flags: review?(iann_bugzilla)
Attachment #8989083 - Flags: approval-comm-esr60?
Assignee

Comment 73

Last year
Part 5 for 2.53
Attachment #8989082 - Attachment is obsolete: true
Assignee

Comment 74

Last year
part 6 for 2.53
Assignee

Comment 75

Last year
Removes all remaining nsIDownloadManager uses and fixes some minor problems. Preferences needed to move to async code. Tested and working.

The pageinfo.js save code is now much different from Fx. It works of course, but I am unsure if it is worth just to be able to display a different initial directory.
Attachment #8989830 - Flags: review?(iann_bugzilla)
Attachment #8989830 - Flags: feedback?(rsx11m.pub)
Attachment #8989830 - Flags: approval-comm-esr60?
Assignee

Comment 76

Last year
Removes no longer supported preferences. These could probably be reimplemented in our code but the day has only 24h so I pass.

I will file followup bugs for the remaining problems mainly the progress meter and  the new unblocking. The migration should be done by rolling back some bugs in a future 2.57 release branch.
Attachment #8989834 - Flags: review?(iann_bugzilla)
Attachment #8989834 - Flags: feedback?(rsx11m.pub)
Attachment #8989834 - Flags: approval-comm-esr60?
Assignee

Updated

Last year
Blocks: 1473394
Assignee

Updated

Last year
Blocks: 1473395
Assignee

Updated

Last year
Blocks: 1473396
Assignee

Updated

Last year
Duplicate of this bug: 1444217
Assignee

Updated

Last year
Duplicate of this bug: 1408630

Comment 79

Last year
Comment on attachment 8989083 [details] [diff] [review]
888915-part4-downloads2-257.patch

r/a=me
Attachment #8989083 - Flags: review?(iann_bugzilla)
Attachment #8989083 - Flags: review+
Attachment #8989083 - Flags: approval-comm-esr60?
Attachment #8989083 - Flags: approval-comm-esr60+

Comment 80

Last year
Comment on attachment 8989830 [details] [diff] [review]
888915-part5-downloads2-257.patch

r/a=me maybe spin off a bug to look at the differences you mentioned and decide if we unfork.
Attachment #8989830 - Flags: review?(iann_bugzilla)
Attachment #8989830 - Flags: review+
Attachment #8989830 - Flags: approval-comm-esr60?
Attachment #8989830 - Flags: approval-comm-esr60+

Comment 81

Last year
Comment on attachment 8989834 [details] [diff] [review]
888915-part6-downloads2-257.patch

r/a=me maybe spin off a bug to look at which, if any, prefs we reimplement.
Attachment #8989834 - Flags: review?(iann_bugzilla)
Attachment #8989834 - Flags: review+
Attachment #8989834 - Flags: approval-comm-esr60?
Attachment #8989834 - Flags: approval-comm-esr60+

Comment 82

Last year
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/4d63eb6735df
Part 4. Fix remaining Download Manager problems. r=IanN
https://hg.mozilla.org/comm-central/rev/6a6f9c616b1c
Part 5. Remove remaining uses of nsIDownloadManager and fix preferences options. r=IanN
https://hg.mozilla.org/comm-central/rev/7c799865bb7e
Part 6. Remove obsolete download prefs. r=IanN
Assignee

Updated

Last year
Duplicate of this bug: 1370981
Assignee

Updated

Last year
Blocks: 1474945
Assignee

Updated

Last year
Blocks: 1474946
Assignee

Updated

10 months ago
Duplicate of this bug: 1373679
Assignee

Updated

8 months ago
Attachment #8989830 - Flags: feedback?(rsx11m.pub)
Assignee

Updated

8 months ago
Attachment #8989834 - Flags: feedback?(rsx11m.pub)
Assignee

Updated

6 months ago
Blocks: 1514585
Assignee

Updated

6 months ago
Blocks: 1514604
Assignee

Updated

6 months ago
See Also: → 1514729
You need to log in before you can comment on or make changes to this bug.