Closed Bug 888915 Opened 11 years ago Closed 6 years ago

Convert SeaMonkey Downloads Manager to Downloads.jsm

Categories

(SeaMonkey :: Download & File Handling, defect)

defect
Not set
blocker

Tracking

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

RESOLVED FIXED
Tracking Status
seamonkey2.52 --- wontfix
seamonkey2.60 --- fixed
seamonkey2.53 --- fixed
seamonkey2.57esr --- fixed

People

(Reporter: philip.chee, Assigned: frg)

References

(Blocks 7 open bugs, )

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 files, 17 obsolete files)

70.49 KB, image/jpeg
Details
73.04 KB, image/jpeg
Details
67.10 KB, image/jpeg
Details
76.45 KB, patch
iannbugzilla
: 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
iannbugzilla
: review+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
14.17 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
48.47 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
70.05 KB, patch
Details | Diff | Splinter Review
62.30 KB, patch
iannbugzilla
: review+
iannbugzilla
: 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
iannbugzilla
: review+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
11.67 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
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.
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
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
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.
Summary: Convert SeaMonkey to Downloads.jsm → Convert SeaMonkey Downloads Manager to Downloads.jsm
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)
Thanks for the heads up!
Flags: needinfo?(philip.chee)
Attached 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.)
Attached 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 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.
Attached 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 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.
Attached 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 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-
(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
Attached 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 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 on attachment 8648439 [details] [diff] [review]
Updated for review comments

New patch on the way?
Attachment #8648439 - Flags: feedback?(iann_bugzilla) → feedback-
Will fixing this bug include fixing bug #918188?
Attached 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.
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+
Attached 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.
Attached 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.
And as soon as the download is finished it's gray again
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.
Depends on: 1253585
Having never received an answer to my comment #24, I again ask:  
Will bug #918188 also be resolved by fixing this bug?
Blocks: 1266965
>> Having never received an answer to my comment #24, I again ask:  
>> Will bug #918188 also be resolved by fixing this bug?

No.
Attached patch 888915-WIP.patch (obsolete) — Splinter Review
Up to date patch for current 2.47a1 c-c.
Only bit rot fixes.
Just as a note in downloadmanager.js types.contains() should change to types.includes() as the former is deprecated.
Blocks: 1224326
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
Bug 851471 actually.
Thanks for the heads up. We have a WIP patch ready and will track changes to Bug 851471.
Depends on: 851471
See Also: → 1365882
Severity: normal → blocker
Attached 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)
Attached 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 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: nobody → frgrahl
Status: NEW → ASSIGNED
User Story: (updated)
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)
Comment on attachment 8872028 [details] [diff] [review]
888915-downloadmanagerintial.patch

LGTM r=me
Attachment #8872028 - Flags: review?(iann_bugzilla) → review+
https://hg.mozilla.org/comm-central/rev/f80e43c05f8892809d13e8a0fa2d5aa839657d4a

Leaving the bug open till I have time to open followup bugs.
See Also: → 1370981
Blocks: 1373679
Depends on: 1408630
User Story: (updated)
Blocks: 1430374
Depends on: 1444217
Blocks: 1464499
part 1 Move downloads from common to components. 2.53 version
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
Forgot to qrefresh
Attachment #8980750 - Attachment is obsolete: true
Part 3. Fix the taskbar progress. 2.53 version.
Minor tweak. Appconstants not needed.
Attachment #8980783 - Attachment is obsolete: true
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?
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?
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?
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.
User Story: (updated)
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 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 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+
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
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
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
Part 5 mostly a placeholder right now for further nsIDownloadManager removals outside the actual download manager.
Part 4 2.53
Attachment #8987383 - Attachment is obsolete: true
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?
User Story: (updated)
User Story: (updated)
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.
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?
Attachment #8987384 - Attachment is obsolete: true
Attachment #8989050 - Attachment is obsolete: true
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?
Part 5 for 2.53
Attachment #8989082 - Attachment is obsolete: true
part 6 for 2.53
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?
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?
Blocks: 1473394
Blocks: 1473395
Blocks: 1473396
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 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 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+
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
Blocks: 1474945
Blocks: 1474946
Attachment #8989830 - Flags: feedback?(rsx11m.pub)
Attachment #8989834 - Flags: feedback?(rsx11m.pub)
Blocks: 1514585
Blocks: 1514604
See Also: → 1514729
Blocks: 1562537
Comment on attachment 8980963 [details] [diff] [review]
888915-part3-DownloadsTaskbar-253.patch

```
>-        this.showDownloadManager(aDownload);

>+        Cc["@mozilla.org/suite/suiteglue;1"]
>+          .getService(Ci.nsISuiteGlue)
>+          .showDownloadManager(download);
```

Because I'd cheated and used `this.` it didn't matter that the interface didn't declare the extra parameter, but this change therefore caused bug 1514585. Sorry about that.
Regressions: 1612725
Regressions: 1622830
Blocks: 1629499
You need to log in before you can comment on or make changes to this bug.