Closed
Bug 888915
Opened 11 years ago
Closed 6 years ago
Convert SeaMonkey Downloads Manager to Downloads.jsm
Categories
(SeaMonkey :: Download & File Handling, defect)
SeaMonkey
Download & File Handling
Tracking
(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)
(deleted),
image/jpeg
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
iannbugzilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
iannbugzilla
:
review+
iannbugzilla
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
iannbugzilla
:
review+
iannbugzilla
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
iannbugzilla
:
review+
iannbugzilla
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
iannbugzilla
:
review+
iannbugzilla
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
iannbugzilla
:
review+
iannbugzilla
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
(deleted),
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.
Reporter | ||
Updated•11 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•11 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
Summary: Move SeaMonkey to the new JavaScript API for downloads when nsIDownloadManager is decommissioned → Convert SeaMonkey to Downloads.jsm
Comment 2•11 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•11 years ago
|
Summary: Convert SeaMonkey to Downloads.jsm → Convert SeaMonkey Downloads Manager to Downloads.jsm
Comment 3•10 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)
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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 7•10 years ago
|
||
Comment on attachment 8604377 [details] [diff] [review]
WIP
also, with recent pull (to fix bug 777770), this patch
seems to be bitrotted.
Comment 8•10 years ago
|
||
(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.)
Comment 9•10 years ago
|
||
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•9 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-
Comment 11•9 years ago
|
||
(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.
Comment 12•9 years ago
|
||
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•9 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-
Comment 14•9 years ago
|
||
(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.
Comment 15•9 years ago
|
||
(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.
Comment 16•9 years ago
|
||
Attachment #8648425 -
Flags: feedback?(philip.chee)
Attachment #8648425 -
Flags: feedback?(iann_bugzilla)
Attachment #8648425 -
Flags: feedback?(ewong)
Comment 17•9 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•9 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
Comment 19•9 years ago
|
||
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•9 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)
Comment 21•9 years ago
|
||
(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 22•9 years ago
|
||
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•9 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•9 years ago
|
||
Will fixing this bug include fixing bug #918188?
Comment 25•9 years ago
|
||
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)
Comment 26•9 years ago
|
||
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 27•9 years ago
|
||
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 28•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
>> 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•9 years ago
|
||
And as soon as the download is finished it's gray again
Comment 32•9 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.
Comment 33•9 years ago
|
||
Having never received an answer to my comment #24, I again ask:
Will bug #918188 also be resolved by fixing this bug?
Assignee | ||
Comment 34•9 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•8 years ago
|
||
Up to date patch for current 2.47a1 c-c.
Only bit rot fixes.
Comment 36•8 years ago
|
||
Just as a note in downloadmanager.js types.contains() should change to types.includes() as the former is deprecated.
Comment 37•8 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•8 years ago
|
||
Bug 851471 actually.
Assignee | ||
Comment 39•8 years ago
|
||
Thanks for the heads up. We have a WIP patch ready and will track changes to Bug 851471.
Depends on: 851471
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Blocks: 2.52BulkMalfunctions
Assignee | ||
Updated•8 years ago
|
Severity: normal → blocker
Assignee | ||
Comment 40•8 years ago
|
||
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•8 years ago
|
||
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•8 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•7 years ago
|
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
status-seamonkey2.52:
--- → affected
User Story: (updated)
Assignee | ||
Comment 43•7 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)
Comment 45•7 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•7 years ago
|
||
https://hg.mozilla.org/comm-central/rev/f80e43c05f8892809d13e8a0fa2d5aa839657d4a
Leaving the bug open till I have time to open followup bugs.
Keywords: leave-open
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Blocks: 2.53BulkMalfunctions
Assignee | ||
Updated•7 years ago
|
Blocks: 2.55BulkMalfunctions
Assignee | ||
Updated•7 years ago
|
Blocks: 2.56BulkMalfunctions
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Assignee | ||
Updated•7 years ago
|
Blocks: 2.57BulkMalfunctions
Assignee | ||
Comment 49•6 years ago
|
||
part 1 Move downloads from common to components. 2.53 version
Assignee | ||
Comment 50•6 years ago
|
||
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•6 years ago
|
||
Forgot to qrefresh
Attachment #8980750 -
Attachment is obsolete: true
Assignee | ||
Comment 52•6 years ago
|
||
Part 3. Fix the taskbar progress. 2.53 version.
Assignee | ||
Comment 53•6 years ago
|
||
Minor tweak. Appconstants not needed.
Attachment #8980783 -
Attachment is obsolete: true
Assignee | ||
Comment 54•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
User Story: (updated)
Comment 58•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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 63•6 years ago
|
||
Assignee | ||
Comment 64•6 years ago
|
||
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•6 years ago
|
||
Part 5 mostly a placeholder right now for further nsIDownloadManager removals outside the actual download manager.
Assignee | ||
Comment 67•6 years ago
|
||
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•6 years ago
|
User Story: (updated)
Assignee | ||
Updated•6 years ago
|
User Story: (updated)
Assignee | ||
Comment 68•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Attachment #8987384 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8989050 -
Attachment is obsolete: true
Assignee | ||
Comment 71•6 years ago
|
||
Assignee | ||
Comment 72•6 years ago
|
||
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•6 years ago
|
||
Part 5 for 2.53
Attachment #8989082 -
Attachment is obsolete: true
Assignee | ||
Comment 74•6 years ago
|
||
part 6 for 2.53
Assignee | ||
Comment 75•6 years ago
|
||
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•6 years ago
|
||
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?
Comment 79•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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 | ||
Comment 83•6 years ago
|
||
https://hg.mozilla.org/releases/comm-esr60/rev/98cd4033c1828b38ee4623679cb459e75fde2729
https://hg.mozilla.org/releases/comm-esr60/rev/b17374a480edf7e97b212725c68dd5bf0f05f831
https://hg.mozilla.org/releases/comm-esr60/rev/e3a120e32a4e6a23ec5a6758c286c80cd2062c6d
2.52 is not fully fixed so opted for wontfix. Stay tuned for follow-up patches.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-seamonkey2.53:
--- → affected
status-seamonkey2.57esr:
--- → fixed
status-seamonkey2.60:
--- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Assignee | ||
Updated•6 years ago
|
Attachment #8989830 -
Flags: feedback?(rsx11m.pub)
Assignee | ||
Updated•6 years ago
|
Attachment #8989834 -
Flags: feedback?(rsx11m.pub)
Assignee | ||
Updated•6 years ago
|
Comment 86•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•