Closed
Bug 901360
Opened 11 years ago
Closed 9 years ago
Convert to Downloads.jsm in Firefox for Android
Categories
(Firefox for Android Graveyard :: Download Manager, defect)
Tracking
(firefox31 wontfix, firefox32 wontfix, firefox34 wontfix, firefox35 wontfix, firefox36 wontfix, firefox37 fixed, relnote-firefox 37+, fennec+)
People
(Reporter: wesj, Assigned: Margaret)
References
Details
(Keywords: addon-compat, dev-doc-needed, perf)
Attachments
(5 files, 18 obsolete files)
60.91 KB,
patch
|
Details | Diff | Splinter Review | |
7.80 KB,
patch
|
Details | Diff | Splinter Review | |
1.60 KB,
patch
|
Details | Diff | Splinter Review | |
14.05 KB,
patch
|
Details | Diff | Splinter Review | |
2.74 KB,
patch
|
Details | Diff | Splinter Review |
Desktop is moving to use Downloads.jsm for in progress downloads (and storing completed downloads in places.sqlite). While the old download manager code isn't going away, it's probably going to move into a state of less repair. We should look into using the new code and finding alternatives for storing previous download info. Maybe we can look into using the system download manager for storing info again...
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•11 years ago
|
tracking-fennec: ? → +
Updated•11 years ago
|
Summary: Investigate moving download management to Downloads.jsm → Investigate moving download management to Downloads.jsm in Firefox for Android
Comment 2•11 years ago
|
||
Most of the required changes are in "mobile/android/chrome/content/downloads.js", that is already a pretty self-contained and short module (less that 300 lines). I think most of the conversion involves replacing the addPrivacyAwareListener call with the registration of a Downloads view. There is some working example code at: https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Downloads.jsm This code should probably keep a Set or a Map to detect when an interesting "state change" in a download occurs, for example when it transitions to a state for which a notification should be shown: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#756 Download objects have slightly different properties and methods than nsIDownload. The "Conversion from nsIDownloadManager" documentation section contains some useful notes and pointers on the differences.
Comment 3•11 years ago
|
||
This will be required for some things like the new malware protection. See bug 895476.
Comment 5•10 years ago
|
||
WIP Refactor download notifications, using Downloads.jsm and Notifications.jsm
Comment 6•10 years ago
|
||
WIP Refactors aboutDownloads.js to use Downloads.jsm instead of nsIDownloadManager.
Comment 7•10 years ago
|
||
I've been working on this as a way to get more familiar with the Fennec code-base. I've attached my current patches, and it has been lightly tested via emulator. Any feedback would be greatly appreciated. Brief overview: * downloads.js becomes DownloadNotifications,jsm; uses Downloads.jsm and Notifications.jsm * aboutDownloads.js is refactored to use Downloads.jsm * browser.js (and related) is modified to load Download.jsm's implementation of nsITransfer.
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8375360 [details] [diff] [review] Refactor notifications to use Downloads.jsm Thanks for your patches! It looks like bnicholson has a lot of experience with the download manager, so let's ask him for feedback :)
Attachment #8375360 -
Flags: feedback?(bnicholson)
Assignee | ||
Updated•10 years ago
|
Attachment #8375361 -
Flags: feedback?(bnicholson)
Comment 9•10 years ago
|
||
Comment on attachment 8375360 [details] [diff] [review] Refactor notifications to use Downloads.jsm Review of attachment 8375360 [details] [diff] [review]: ----------------------------------------------------------------- This looks great so far! I've outlined several questions/comments below. Also, our test framework does not have much coverage for downloads (if any at all), so please make sure you test as much as you can as you work on this refactor. Just a few things worth testing: * Do the obvious actions (start/pause/cancel) work? * Does a paused download correctly resume? * If Fennec is swiped from the recent apps list during a download, does Fennec stay alive? * If a download is cancelled, are all traces of it removed from the filesystem? * If a download is in progress or paused, I shouldn't be able to remove the notification. * If a download is cancelled or finished, I should be able to remove the notification. Note that these scenarios may not all even be working correctly in the existing code (again, we don't have a lot of test coverage). But, at minimum, we want to make sure we don't regress existing behavior. Flagging feedback for wesj as well since he's done some considerable work on downloads/notifications. ::: mobile/android/modules/DownloadNotifications.jsm @@ +16,5 @@ > +XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm"); > + > +XPCOMUtils.defineLazyGetter(this, "gStrings", > + () => Services.strings.createBundle("chrome://browser/locale/browser.properties")); > +Object.defineProperty(this, "gNativeWindow", Why not use XPCOMUtils.defineLazyGetter here like you did for gStrings? @@ +22,5 @@ > + > +var DownloadNotifications = { > + init: function () { > + if (!this._viewAdded) { > + this.notifications = new Map(); Please extract this.notifications outside the class so we don't expose this map with the exported DownloadNotifications. @@ +84,5 @@ > + this._fileName = OS.Path.basename(aDownload.target.path); > + > + this.id = null; > + > + this._updateFromDownload(); I'd remove this since show() already calls it. @@ +138,5 @@ > + aOptions.progress = this.download.progress; > + aOptions.persistent = true; > + }, > + > + show: function () { Currently, `show` is fairly ambiguous; calling show() can actually result in a hide(). So this method should really be split into two separate methods: show and update... @@ +143,5 @@ > + this._updateFromDownload(); > + > + if (this._show) { > + if (!this.id) { > + this.id = Notifications.create(this.options); ...and this can be moved into the new `show`, without `this.id check` since `this.id` will always be false upon creation... @@ +145,5 @@ > + if (this._show) { > + if (!this.id) { > + this.id = Notifications.create(this.options); > + } else { > + Notifications.update(this.id, this.options); ...and this can be moved into the new `update`. @@ +168,5 @@ > + } > + }, > + > + pauseDownload: function () { > + this.download.cancel().then(null, Cu.reportError); Is this is a typo? I wouldn't expect pauseDownload() to cancel the download. @@ +172,5 @@ > + this.download.cancel().then(null, Cu.reportError); > + }, > + > + resumeDownload: function () { > + this.download.start().then(null, Cu.reportError); This also looks wrong; currently, we're using download.resume() -- not start -- to resume downloads (see http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/downloads.js#116). @@ +179,5 @@ > + cancelDownload: function () { > + this.hide(); > + > + this.download.cancel().then(null, Cu.reportError); > + this.download.removePartialData().then(null, Cu.reportError); Have you confirmed that removePartialData() works correctly on Android? I notice that our existing downloads.js does not call this method. @@ +207,5 @@ > +}; > + > +function DownloadNotificationButton(aButtonId, aIconUrl) { > + this.buttonId = aButtonId; > + this.title = gStrings.GetStringFromName("alertDownloads" + aButtonId.replace(/^./, c => c.toUpperCase())); Doing this replace here is kind of hacky and fragile. Let's just have a third argument, aStringName, where you explicitly pass in alertDownloadsPause/alertDownloadsResume/alertDownloadsCancel below. @@ +212,5 @@ > + this.icon = aIconUrl; > +} > + > +DownloadNotificationButton.prototype = { > + onClicked: function (aId, aCookie) { s/aCookie/aDownload/ @@ +219,5 @@ > + Cu.reportError("No DownloadNotification for button"); > + return; > + } > + > + let fn = notification[this.buttonId + "Download"]; This also feels like a hack. Instead, I'd recommend using Object.create() to make a PauseButton, ResumeButton, and CancelButton that inherit from the same prototype. Then you can create some generic method (e.g., `handleClick(notification)`), and each subclass can implement this by calling the appropriate method on DownloadNotification. @@ +224,5 @@ > + fn.call(notification, aId, aCookie); > + } > +}; > + > +const gButtons = { Nit: Move this above DownloadNotifications declaration
Attachment #8375360 -
Flags: feedback?(wjohnston)
Attachment #8375360 -
Flags: feedback?(bnicholson)
Attachment #8375360 -
Flags: feedback+
Comment 10•10 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #9) > Comment on attachment 8375360 [details] [diff] [review] > Refactor notifications to use Downloads.jsm > > Review of attachment 8375360 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks great so far! I've outlined several questions/comments below. > > Also, our test framework does not have much coverage for downloads (if any > at all), so please make sure you test as much as you can as you work on this > refactor. Just a few things worth testing: > * Do the obvious actions (start/pause/cancel) work? Yes. > * Does a paused download correctly resume? Yes, for the most part. There is a bug with resuming a download from a server that implements Last-Modified/E-Tag headers but not Range requests (e.g. Python's SimpleHTTPServer), but it located inside Downloads.jsm, not this patch. > * If Fennec is swiped from the recent apps list during a download, does > Fennec stay alive? No. Fennec would need to use Android's DownloadManager API to be able to download while not active. If these patches land, my suggestion would be to implement a new DownloadSaver type uses that API. > * If a download is cancelled, are all traces of it removed from the > filesystem? Yes. > * If a download is in progress or paused, I shouldn't be able to remove the > notification. Works. > * If a download is cancelled or finished, I should be able to remove the > notification. Works. > > Note that these scenarios may not all even be working correctly in the > existing code (again, we don't have a lot of test coverage). But, at > minimum, we want to make sure we don't regress existing behavior. > > Flagging feedback for wesj as well since he's done some considerable work on > downloads/notifications. > > ::: mobile/android/modules/DownloadNotifications.jsm > @@ +16,5 @@ > > +XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm"); > > + > > +XPCOMUtils.defineLazyGetter(this, "gStrings", > > + () => Services.strings.createBundle("chrome://browser/locale/browser.properties")); > > +Object.defineProperty(this, "gNativeWindow", > > Why not use XPCOMUtils.defineLazyGetter here like you did for gStrings? Because defineLazyGetter calls the lambda function once, and caches the result. AFAICT, it's possible for Services.wm.getMostRecentWindow() to return a different object every time it's called. To be honest thought, the NativeWindow service doesn't need to live in the browser window global, since it doesn't actually use or need the window object (AFAICT). I thought about refactoring it into separate modules in the spirit of Notifications.jsm and then importing Toasts.jsm, but decided that might be too much change. > > @@ +22,5 @@ > > + > > +var DownloadNotifications = { > > + init: function () { > > + if (!this._viewAdded) { > > + this.notifications = new Map(); > > Please extract this.notifications outside the class so we don't expose this > map with the exported DownloadNotifications. Done. > @@ +84,5 @@ > > + this._fileName = OS.Path.basename(aDownload.target.path); > > + > > + this.id = null; > > + > > + this._updateFromDownload(); > > I'd remove this since show() already calls it. Done. > > @@ +138,5 @@ > > + aOptions.progress = this.download.progress; > > + aOptions.persistent = true; > > + }, > > + > > + show: function () { > > Currently, `show` is fairly ambiguous; calling show() can actually result in > a hide(). So this method should really be split into two separate methods: > show and update... I've gone back and forth over this. I originally implemented it as you had suggested. The problem I ran into is that when a download is added, it can be in state where no notification should be shown. Later on, it can transition to a state requiring a notification. If the function is split up, this moves the logic for checking if the notification itself needs to be created up a level, to DownloadNotification.onDownload{Added,Changed}. I readily admit the name 'show' is confusing, but I lack a better idea of what to call it. > > @@ +143,5 @@ > > + this._updateFromDownload(); > > + > > + if (this._show) { > > + if (!this.id) { > > + this.id = Notifications.create(this.options); > > ...and this can be moved into the new `show`, without `this.id check` since > `this.id` will always be false upon creation... See above. > > @@ +145,5 @@ > > + if (this._show) { > > + if (!this.id) { > > + this.id = Notifications.create(this.options); > > + } else { > > + Notifications.update(this.id, this.options); > > ...and this can be moved into the new `update`. Also see above. > > @@ +168,5 @@ > > + } > > + }, > > + > > + pauseDownload: function () { > > + this.download.cancel().then(null, Cu.reportError); > > Is this is a typo? I wouldn't expect pauseDownload() to cancel the download. It is confusing, but that's how Downloads.jsm works. It doesn't have a concept of pause. If a download is cancelled and 'tryToKeepPartialData' is true, the .part file is kept. If the download is later restarted, it will start off from where the .part file ends. See https://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#1039 for desktop's usage of Downloads.jsm, which is where I cribbed this from. Personally, I think Downloads.jsm might be better off exposing a single state property for downloads, like the one synthesized by DownloadsCommon.jsm (https://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#856), instead of forcing callers to check multiple properties and calculate the state themselves. > > @@ +172,5 @@ > > + this.download.cancel().then(null, Cu.reportError); > > + }, > > + > > + resumeDownload: function () { > > + this.download.start().then(null, Cu.reportError); > > This also looks wrong; currently, we're using download.resume() -- not start > -- to resume downloads (see > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ > downloads.js#116). See the previous comment. Also, 'download' in that link is a nsIDownload object, not a Download.jsm Download object. > > @@ +179,5 @@ > > + cancelDownload: function () { > > + this.hide(); > > + > > + this.download.cancel().then(null, Cu.reportError); > > + this.download.removePartialData().then(null, Cu.reportError); > > Have you confirmed that removePartialData() works correctly on Android? I > notice that our existing downloads.js does not call this method. It does. > > @@ +207,5 @@ > > +}; > > + > > +function DownloadNotificationButton(aButtonId, aIconUrl) { > > + this.buttonId = aButtonId; > > + this.title = gStrings.GetStringFromName("alertDownloads" + aButtonId.replace(/^./, c => c.toUpperCase())); > > Doing this replace here is kind of hacky and fragile. Let's just have a > third argument, aStringName, where you explicitly pass in > alertDownloadsPause/alertDownloadsResume/alertDownloadsCancel below. Done. > > @@ +212,5 @@ > > + this.icon = aIconUrl; > > +} > > + > > +DownloadNotificationButton.prototype = { > > + onClicked: function (aId, aCookie) { > > s/aCookie/aDownload/ Done. > > @@ +219,5 @@ > > + Cu.reportError("No DownloadNotification for button"); > > + return; > > + } > > + > > + let fn = notification[this.buttonId + "Download"]; > > This also feels like a hack. Instead, I'd recommend using Object.create() to > make a PauseButton, ResumeButton, and CancelButton that inherit from the > same prototype. Then you can create some generic method (e.g., > `handleClick(notification)`), and each subclass can implement this by > calling the appropriate method on DownloadNotification. What if I added a 4th parameter to the DownloadNotificationButton, a function that takes a DownloadNotification? DownloadNotificationButton.onClicked() would then look-up the notification and then call the function provided with it. e.g: new DownloadNotificationButton("pause", "drawable://pause", "alertDownloadsPause", aNotification => aNotification.pauseDownload()); > > @@ +224,5 @@ > > + fn.call(notification, aId, aCookie); > > + } > > +}; > > + > > +const gButtons = { > > Nit: Move this above DownloadNotifications declaration Done. I've updated my patch, but I'm not sure of protocol for attaching it. Do I attach it and mark the current one as obsolete?
Comment 11•10 years ago
|
||
(In reply to James Gilbertson from comment #10) > > * If Fennec is swiped from the recent apps list during a download, does > > Fennec stay alive? > > No. Fennec would need to use Android's DownloadManager API to be able to > download while not active. If these patches land, my suggestion would be to > implement a new DownloadSaver type uses that API. You verified this already by testing? I ask because our current workaround for this is to use Android foreground notifications with a Service in the same process as our Activity, which effectively prevents the Activity from getting killed in the background (or being swiped from apps). That's partly what the ongoing property is used for, as you can see here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/NotificationHandler.java#79 So unless the existing code is broken, Fennec (and therefore downloads) *should* stay alive in these situations. I wouldn't expect this patch to break that workaround since it still passes the ongoing property for in-progress downloads, but worth testing nonetheless. BTW, there's currently work underway for using the Android download manager in bug 816318. > I've gone back and forth over this. I originally implemented it as you had > suggested. The problem I ran into is that when a download is added, it can > be in state where no notification should be shown. What state is that? Stopped? > I readily admit the name 'show' is confusing, but I lack a better idea of > what to call it. How about showOrUpdate? It's an obscure method, so it deserves a suitably obscure name :) > > @@ +168,5 @@ > > > + } > > > + }, > > > + > > > + pauseDownload: function () { > > > + this.download.cancel().then(null, Cu.reportError); > > > > Is this is a typo? I wouldn't expect pauseDownload() to cancel the download. > > It is confusing, but that's how Downloads.jsm works. It doesn't have a > concept of pause. If a download is cancelled and 'tryToKeepPartialData' is > true, the .part file is kept. If the download is later restarted, it will > start off from where the .part file ends. Okay, I was thinking the whole time that the download you were acting on here was an nsIDownload, when really it's a Download object from DownloadCore.jsm. Yeesh. I'm clearly pretty ignorant about this new downloads API, so we may want to find an additional reviewer here (unless wesj is familiar with Downloads.jsm). > What if I added a 4th parameter to the DownloadNotificationButton, a > function that takes a DownloadNotification? > DownloadNotificationButton.onClicked() would then look-up the notification > and then call the function provided with it. > > e.g: > new DownloadNotificationButton("pause", "drawable://pause", > "alertDownloadsPause", aNotification => aNotification.pauseDownload()); Sure, that works for me. > I've updated my patch, but I'm not sure of protocol for attaching it. Do I > attach it and mark the current one as obsolete? Yes, exactly. Please also flag the new patch for feedback from me and wesj. Also, sorry for the slow review; my review queue has been unusually stacked for the past few days. I'll try to take a look at your second patch tomorrow.
Comment 12•10 years ago
|
||
Comment on attachment 8375361 [details] [diff] [review] Refactor about:downloads to use Downloads.jsm Review of attachment 8375361 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me; thanks for all the effort. There aren't any glaring changes or improvements that I see that would need to be made, so I think this will be landable with the remaining TODOs fixed. Flagging wesj here too just in case he has any comments.
Attachment #8375361 -
Flags: feedback?(wjohnston)
Attachment #8375361 -
Flags: feedback?(bnicholson)
Attachment #8375361 -
Flags: feedback+
Comment 13•10 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #11) > (In reply to James Gilbertson from comment #10) > > > * If Fennec is swiped from the recent apps list during a download, does > > > Fennec stay alive? > > > > No. Fennec would need to use Android's DownloadManager API to be able to > > download while not active. If these patches land, my suggestion would be to > > implement a new DownloadSaver type uses that API. > > You verified this already by testing? I ask because our current workaround > for this is to use Android foreground notifications with a Service in the > same process as our Activity, which effectively prevents the Activity from > getting killed in the background (or being swiped from apps). That's partly > what the ongoing property is used for, as you can see here: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ > NotificationHandler.java#79 Via emulator. Might be different on actual devices, but I'm not sure. > > So unless the existing code is broken, Fennec (and therefore downloads) > *should* stay alive in these situations. I wouldn't expect this patch to > break that workaround since it still passes the ongoing property for > in-progress downloads, but worth testing nonetheless. > > BTW, there's currently work underway for using the Android download manager > in bug 816318. I know :) My original inspiration for working on this was that I wanted to see how hard it would be to integrate Android's Download Manager. > > > I've gone back and forth over this. I originally implemented it as you had > > suggested. The problem I ran into is that when a download is added, it can > > be in state where no notification should be shown. > > What state is that? Stopped? Any state that isn't downloading, paused, or finished. One case would be if the download is failed for some reason. If the user restarts it via about:downloads, it'll need to display a notification. > > > I readily admit the name 'show' is confusing, but I lack a better idea of > > what to call it. > > How about showOrUpdate? It's an obscure method, so it deserves a suitably > obscure name :) Done. > > What if I added a 4th parameter to the DownloadNotificationButton, a > > function that takes a DownloadNotification? > > DownloadNotificationButton.onClicked() would then look-up the notification > > and then call the function provided with it. > > > > e.g: > > new DownloadNotificationButton("pause", "drawable://pause", > > "alertDownloadsPause", aNotification => aNotification.pauseDownload()); > > Sure, that works for me. Done.
Comment 14•10 years ago
|
||
Attachment #8375360 -
Attachment is obsolete: true
Attachment #8375360 -
Flags: feedback?(wjohnston)
Attachment #8379280 -
Flags: feedback?(wjohnston)
Attachment #8379280 -
Flags: feedback?(bnicholson)
Comment 15•10 years ago
|
||
Rebased on top of changeset b89a9d7b4ca0
Attachment #8379282 -
Flags: feedback?(wjohnston)
Attachment #8379282 -
Flags: feedback?(bnicholson)
Updated•10 years ago
|
Attachment #8375361 -
Attachment is obsolete: true
Attachment #8375361 -
Flags: feedback?(wjohnston)
Updated•10 years ago
|
Attachment #8379282 -
Attachment is patch: true
Reporter | ||
Comment 16•10 years ago
|
||
Comment on attachment 8379280 [details] [diff] [review] Refactor notifications to use Downloads.jsm (v2) Overall this looks really really nice. I'm going to dig in a bit now, but paolo should really review some of it. Me/brian can look over the UI bits if he assures us the downloads.jsm stuff is correctly hooked up. One thing to note, Android doesn't have the places.db so AFAIK this data is going to be lost as soon as your browser session ends. We were going to move to the System download database to fix that, but since then we've modified that goal to just use the system download manager entirely. Thats not going to happen for another release though, and even after we may keep this around as a backup for people who want out of the System downloads stuff.
Attachment #8379280 -
Flags: feedback?(wjohnston) → feedback?(paolo.mozmail)
Comment 17•10 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #16) > One thing to note, Android doesn't have the places.db so AFAIK this data is > going to be lost as soon as your browser session ends. We were going to move > to the System download database to fix that, but since then we've modified > that goal to just use the system download manager entirely. Thats not going > to happen for another release though, and even after we may keep this around > as a backup for people who want out of the System downloads stuff. Downloads.jsm does have a history mechanism separate of places.db. By default, for every platform other then B2G, it only persists in-progress and paused downloads. For B2G though, it retains all recent downloads (http://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#288). Would it make sense to investigate using the B2G approach for Android?
Updated•10 years ago
|
Attachment #8379280 -
Flags: feedback?(bnicholson) → feedback+
Comment 18•10 years ago
|
||
Comment on attachment 8379282 [details] [diff] [review] Refactor about:downloads to use Downloads.jsm (v2) Review of attachment 8379282 [details] [diff] [review]: ----------------------------------------------------------------- These look good to me.
Attachment #8379282 -
Flags: feedback?(bnicholson) → feedback+
Comment 19•10 years ago
|
||
Comment on attachment 8379280 [details] [diff] [review] Refactor notifications to use Downloads.jsm (v2) Review of attachment 8379280 [details] [diff] [review]: ----------------------------------------------------------------- Thank you a lot for working on this conversion! It will improve performance a lot, and is a step forward towards decommissioning the old API. (In reply to James Gilbertson from comment #10) > Yes, for the most part. There is a bug with resuming a download from a > server that implements Last-Modified/E-Tag headers but not Range requests > (e.g. Python's SimpleHTTPServer), but it located inside Downloads.jsm, not > this patch. Can you file a bug for this? I don't recall having heard of this issue, that might be worth fixing in the future. > Personally, I think Downloads.jsm might be better off exposing a single > state property for downloads, like the one synthesized by > DownloadsCommon.jsm > (https://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/ > src/DownloadsCommon.jsm#856), instead of forcing callers to check multiple > properties and calculate the state themselves. For the record, we moved away from that model because different consumers needed different summarizations of the state properties, and also adding a new value to the set of valid states always required updating all consumers. We still use the old enumeration in Desktop just because we have a lot of legacy dependencies on it (including CSS styles!). ::: mobile/android/modules/DownloadNotifications.jsm @@ +66,5 @@ > + let notification = new DownloadNotification(aDownload); > + gNotifications.set(aDownload, notification); > + > + notification.showOrUpdate(); > + if (aDownload.currentBytes == 0) { All the notifications in Downloads.jsm are asynchronous, and there is no guarantee that the state of a Download object will transition from any given combination of properties. For example, a download that is already stopped, canceled, or in rare cases even finalized can be passed to onDownloadAdded. In general, this isn't an issue because the consumer would keep track of the state transitions that it specifically needs, and here it is correctly handled in the DownloadNotification object. This is to say that the purpose of the currentBytes check here is not clear to me, but the download object is not guaranteed to transition from this state (the download may have already started receiving data in the background before the add notification is received by the view). @@ +104,5 @@ > +DownloadNotification.prototype = { > + _updateFromDownload: function () { > + this._downloading = !this.download.stopped; > + this._paused = this.download.canceled && this.download.hasPartialData; > + this._finished = this.download.succeeded; nit: I'd call this "_succeeded" since it is mapped to the Downloads.jsm property of the same name, and is false when a download finished with an error. The rest of the patch looks good, I don't see any possible race condition that is not already handled internally by Downloads.jsm.
Attachment #8379280 -
Flags: feedback?(paolo.mozmail)
Attachment #8379280 -
Flags: feedback?(bnicholson)
Attachment #8379280 -
Flags: feedback+
Comment 20•10 years ago
|
||
(In reply to :Paolo Amadini from comment #19) > Comment on attachment 8379280 [details] [diff] [review] > Refactor notifications to use Downloads.jsm (v2) > > Review of attachment 8379280 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you a lot for working on this conversion! It will improve performance > a lot, and is a step forward towards decommissioning the old API. > > (In reply to James Gilbertson from comment #10) > > Yes, for the most part. There is a bug with resuming a download from a > > server that implements Last-Modified/E-Tag headers but not Range requests > > (e.g. Python's SimpleHTTPServer), but it located inside Downloads.jsm, not > > this patch. > > Can you file a bug for this? I don't recall having heard of this issue, that > might be worth fixing in the future. > I will. I remember I was going to see if I could create a test case reproducing it, but it slipped from my mind. > > Personally, I think Downloads.jsm might be better off exposing a single > > state property for downloads, like the one synthesized by > > DownloadsCommon.jsm > > (https://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/ > > src/DownloadsCommon.jsm#856), instead of forcing callers to check multiple > > properties and calculate the state themselves. > > For the record, we moved away from that model because different consumers > needed different summarizations of the state properties, and also adding a > new value to the set of valid states always required updating all consumers. > We still use the old enumeration in Desktop just because we have a lot of > legacy dependencies on it (including CSS styles!). That makes sense. Thanks for the explanation. I do wonder though if it might make sense to have an explicit "paused" property (maybe as a read-only getter), or some documentation on how pause should work. It took a bit for me to wrap my head around the concept of pause being "canceled with some partial data", since "cancel" and "pause" feel like very different concepts (at least to me). > > ::: mobile/android/modules/DownloadNotifications.jsm > @@ +66,5 @@ > > + let notification = new DownloadNotification(aDownload); > > + gNotifications.set(aDownload, notification); > > + > > + notification.showOrUpdate(); > > + if (aDownload.currentBytes == 0) { > > All the notifications in Downloads.jsm are asynchronous, and there is no > guarantee that the state of a Download object will transition from any given > combination of properties. For example, a download that is already stopped, > canceled, or in rare cases even finalized can be passed to onDownloadAdded. > In general, this isn't an issue because the consumer would keep track of the > state transitions that it specifically needs, and here it is correctly > handled in the DownloadNotification object. > > This is to say that the purpose of the currentBytes check here is not clear > to me, but the download object is not guaranteed to transition from this > state (the download may have already started receiving data in the > background before the add notification is received by the view). The intent of the check is to display a toast stating that a download has been started. It is hacky (and apparently broken :)), but I couldn't think of any other obvious way to implement it.
Comment 21•10 years ago
|
||
Comment on attachment 8379280 [details] [diff] [review] Refactor notifications to use Downloads.jsm (v2) Review of attachment 8379280 [details] [diff] [review]: ----------------------------------------------------------------- I'm assuming feedback flag got reverted unintentionally.
Attachment #8379280 -
Flags: feedback?(bnicholson) → feedback+
Reporter | ||
Comment 22•10 years ago
|
||
Comment on attachment 8379282 [details] [diff] [review] Refactor about:downloads to use Downloads.jsm (v2) Review of attachment 8379282 [details] [diff] [review]: ----------------------------------------------------------------- Just nitpicking mostly. This seems like good cleanup. We're on the verge of moving over to the native download manager, so this may not live long, but hopefully some of it can stay around as a fall back.... ::: mobile/android/chrome/content/aboutDownloads.js @@ +26,3 @@ > > + if (!this._eventListenerAdded) { > + // move the target download ref onto the click event during the capture phase Try to avoid comments that just repeat what the code says. @@ +105,5 @@ > + updateVisibility: function (aDownload) { > + if (this.isVisible(aDownload)) { > + this.element.removeAttribute("hidden"); > + } else { > + this.element.setAttribute("hidden", ""); Might as well set this to true. @@ +188,5 @@ > + this.listElement.removeChild(item.element); > + } > +}; > + > +var gDownloadLists = { let @@ +323,5 @@ > + // template properties below > + get domain() this._domain, > + get fileName() this._fileName, > + get id() this._id, > + get iconUrl() this._iconUrl, Use brackets here. @@ +333,3 @@ > } > }, > + get startDate() this._startDate, And spaces between multi-line methods.
Attachment #8379282 -
Flags: feedback?(wjohnston) → feedback+
Comment 23•10 years ago
|
||
Updated in response to feedback
Attachment #8379280 -
Attachment is obsolete: true
Comment 24•10 years ago
|
||
Attachment #8379282 -
Attachment is obsolete: true
Comment 25•10 years ago
|
||
How's the work proceeding here? Is there any help I could provide? Since patches like these tend to become outdated quickly, and we're working on removing the legacy Download Manager API from the mozilla-central tree, it would be great if we could move forward soon. This is the last blocker, as Firefox for Metro is not considered a hard blocker anymore. This should be coordinated with bug 816318, but the latter is not a substitute for this one, as they're about two different things, despite they touch the same code. Bug 816318 is still based on the legacy back-end API, that is unsupported and will miss features like the new malware protection based on Application Reputation.
Updated•10 years ago
|
Summary: Investigate moving download management to Downloads.jsm in Firefox for Android → Convert to Downloads.jsm in Firefox for Android
Comment 26•10 years ago
|
||
The v3 patches have taken into account all the feedback I've received so far. I've been a bit stretched for time since then (having a daughter can do that!). I'll try to re-base the patches and request review sometime this week.
Comment 27•10 years ago
|
||
(In reply to James Gilbertson from comment #26) > The v3 patches have taken into account all the feedback I've received so > far. I've been a bit stretched for time since then (having a daughter can do > that!). > > I'll try to re-base the patches and request review sometime this week. Thanks a lot!
Comment 28•10 years ago
|
||
Attachment #8384082 -
Attachment is obsolete: true
Attachment #8396884 -
Flags: review?(wjohnston)
Comment 29•10 years ago
|
||
Attachment #8384083 -
Attachment is obsolete: true
Attachment #8396885 -
Flags: review?(wjohnston)
Reporter | ||
Comment 30•10 years ago
|
||
Comment on attachment 8396884 [details] [diff] [review] Refactor notifications to use Downloads.jsm (v4) Review of attachment 8396884 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good to me. Its going to throw bug 816318 way off track, but we'll work that out there I think. Lets land this first. In fact, we should probably give this a week of test time first just so it has some basic testing on Nightly.... ::: mobile/android/modules/DownloadNotifications.jsm @@ +66,5 @@ > + let notification = new DownloadNotification(aDownload); > + gNotifications.set(aDownload, notification); > + > + notification.showOrUpdate(); > + if (aDownload.currentBytes == 0) { === @@ +104,5 @@ > +DownloadNotification.prototype = { > + _updateFromDownload: function () { > + this._downloading = !this.download.stopped; > + this._paused = this.download.canceled && this.download.hasPartialData; > + this._succeeded = this.download.succeeded; This is a little weird to have these states all split up. Can we just have a single "state" member? @@ +218,5 @@ > + this.showing = false; > + } > +}; > + > +function DownloadNotificationButton(aButtonId, aIconUrl, aTitleStringName, aOnClicked) { We're trying to move away from prefixing arguments with 'a'.
Attachment #8396884 -
Flags: review?(wjohnston) → review+
Comment 31•10 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #30) > This is a little weird to have these states all split up. Can we just have a > single "state" member? See the second paragraph in comment 19 for the reason why we found that using a single state variable for downloads in the legacy API introduced problems over time (while intuitively it seemed to make more sense at first).
Reporter | ||
Comment 32•10 years ago
|
||
Comment on attachment 8396885 [details] [diff] [review] Refactor about:downloads to use Downloads.jsm (v4) Review of attachment 8396885 [details] [diff] [review]: ----------------------------------------------------------------- This is big, but it looks pretty good to me. Just some nits. I'm also going to ask margaret for a feedback. If you have time to give this a once over, I'd appreciate it. ::: mobile/android/chrome/content/aboutDownloads.js @@ +77,5 @@ > ]; > }, > > + addContextMenuEventListener: function (aElement) { > + aElement.addEventListener("contextmenu", aEvent => this.onContextMenu(aEvent)); I don't really like the extra anonymous function here. I'd either just this.onContextMenu.bind(this) or just pass this in and rename onContextMenu to handleEvent. @@ +151,5 @@ > + // most recent downloads first > + return b.startTime - a.startTime; > + }; > + > + let at = this.listElement.firstChild; I don't really like the name "at", I have to read a few lines down to figure out what it is. @@ +190,5 @@ > + this.listElement.removeChild(item.element); > + } > +}; > + > +let gDownloadLists = { We don't usually use the 'g' notation in Fennec. Since this is pretty window local, I wouldn't add it. @@ +215,5 @@ > + Downloads.getList(Downloads.ALL) > + .then(list => { > + for (let download of finished) { > + list.remove(download).then(null, Cu.reportError); > + deleteDownload(download); Do you need to cleanup the real lists here? @@ +254,5 @@ > + }, > + > + _updateFromDownload: function () { > + this._state = {}; > + for (let name of kDownloadStatePropertyNames) { kDownloadStatePropertyNames.forEach() @@ +260,5 @@ > } > }, > > + get stateChanged() { > + for (let name of kDownloadStatePropertyNames) { return kDownloadStatePropertyNames.some(); @@ +304,4 @@ > }, > > + onClick: function (aEvent) { > + if (this.download.succeeded) { We have bugs about non-existent files not showing any sort of message. But then again, I'd prefer to not get wrapped up in fixing already existing bugs here. @@ +333,3 @@ > }, > > + get state() { Lets name this something like readableState or something to designate that its more than just the download's state. @@ +352,5 @@ > > + if (name) { > + return gStrings.GetStringFromName(name); > + } else { > + return ""; Don't bother with the dangling else if we don't need it.
Attachment #8396885 -
Flags: review?(wjohnston)
Attachment #8396885 -
Flags: review+
Attachment #8396885 -
Flags: feedback?(margaret.leibovic)
Comment 33•10 years ago
|
||
Attachment #8396884 -
Attachment is obsolete: true
Comment 34•10 years ago
|
||
Attachment #8396885 -
Attachment is obsolete: true
Attachment #8396885 -
Flags: feedback?(margaret.leibovic)
Updated•10 years ago
|
Attachment #8404463 -
Flags: feedback?(margaret.leibovic)
Comment 35•10 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #32) > @@ +215,5 @@ > > + Downloads.getList(Downloads.ALL) > > + .then(list => { > > + for (let download of finished) { > > + list.remove(download).then(null, Cu.reportError); > > + deleteDownload(download); > > Do you need to cleanup the real lists here? Removing downloads from the list only deletes partially downloaded files. Completed files need to be be manually deleted.
Assignee | ||
Comment 36•10 years ago
|
||
Comment on attachment 8404463 [details] [diff] [review] Refactor about:downloads to use Downloads.jsm (v5) Review of attachment 8404463 [details] [diff] [review]: ----------------------------------------------------------------- Great work! This is such a big improvement to our about:downloads code, thanks for all your help :) Disclaimer: I focused on the high-level structure of this patch, although I did read through all the changes in aboutDownloads.js, and I didn't spot any problems. ::: mobile/android/chrome/content/aboutDownloads.js @@ +40,2 @@ > > + this._eventListenerAdded = true; It looks like contextMenu.init is only called once, is there a reason we need this check? @@ +204,5 @@ > + removeFinished: function () { > + let finished = this.finished; > + if (finished.length == 0) { > + return; > + } It might be worth noting that we should never call removeFinished in this case, since the context menu item should be hidden if there are no finished downloads. @@ +319,5 @@ > > + get size() { > + if (this.download.hasProgress) { > + return DownloadUtils.convertByteUnits(this.download.totalBytes).join(""); > + } else { Nit: This else is unnecessary, since there's a return in the if statement. ::: mobile/android/chrome/content/browser.js @@ +24,5 @@ > Cu.import("resource://gre/modules/accessibility/AccessFu.jsm"); > #endif > > +XPCOMUtils.defineLazyModuleGetter(this, "DownloadNotifications", > + "resource://gre/modules/DownloadNotifications.jsm"); There doesn't seem to be much reason to use a lazy getter, since we always call DownloadNotifications.init during startup. ::: mobile/android/installer/package-manifest.in @@ +427,5 @@ > @BINPATH@/components/TestInterfaceJS.manifest > #endif > > +@BINPATH@/components/Downloads.manifest > +@BINPATH@/components/DownloadLegacy.js Why do we need to add this? I thought the point of this change was using Downloads.jsm instead of the old legacy download components.
Attachment #8404463 -
Flags: feedback?(margaret.leibovic) → feedback+
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → james.gilbertson
Comment 37•10 years ago
|
||
(In reply to :Margaret Leibovic from comment #36) > ::: mobile/android/installer/package-manifest.in > @@ +427,5 @@ > > @BINPATH@/components/TestInterfaceJS.manifest > > #endif > > > > +@BINPATH@/components/Downloads.manifest > > +@BINPATH@/components/DownloadLegacy.js > > Why do we need to add this? I thought the point of this change was using > Downloads.jsm instead of the old legacy download components. We need it in order to override the implementation of nsITransfer so that it uses Downloads.jsm instead of the legacy download components.
Comment 38•10 years ago
|
||
Attachment #8404462 -
Attachment is obsolete: true
Comment 39•10 years ago
|
||
Attachment #8404463 -
Attachment is obsolete: true
Updated•10 years ago
|
Keywords: checkin-needed
Comment 40•10 years ago
|
||
Comment on attachment 8407322 [details] [diff] [review] Refactor notifications to use Downloads.jsm (v6) Carrying over review+ from wesj.
Attachment #8407322 -
Flags: review+
Comment 41•10 years ago
|
||
Comment on attachment 8407324 [details] [diff] [review] Refactor about:downloads to use Downloads.jsm (v6) Carrying over review+ from wesj. Your patches won't be checked in unless they have an r+ from someone. If you already got an r+ on a previous version and are just addressing any comments, just put r+ on them yourself. Also, you might want to add the r=wesj in the patch description, and remove the [PATCH 1/2] part, replacing it by what that part of the patch actually does rather than the generic bug description. (We can read the description by following the Bug 901360 part)
Attachment #8407324 -
Flags: review+
Comment 42•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8487d4305be3 https://hg.mozilla.org/integration/fx-team/rev/b56da878caed
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 43•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8487d4305be3 https://hg.mozilla.org/mozilla-central/rev/b56da878caed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Comment 44•10 years ago
|
||
\o/
Reporter | ||
Comment 45•10 years ago
|
||
I backed this out of Aurora. We'll try to fix the regressions on nightly: https://hg.mozilla.org/integration/fx-team/rev/136592f9f5a4
Comment 46•10 years ago
|
||
fx-team != Aurora :confused: Anyway, merge of backout: https://hg.mozilla.org/mozilla-central/rev/136592f9f5a4
Status: RESOLVED → REOPENED
status-firefox31:
--- → fixed
status-firefox32:
--- → affected
Resolution: FIXED → ---
Target Milestone: Firefox 31 → ---
Reporter | ||
Comment 47•10 years ago
|
||
Argh. Sorry, my brain was confused there apparently. Will reland this on nightly and backout of Aurora.
Reporter | ||
Comment 48•10 years ago
|
||
backed out of aurora.
Reporter | ||
Comment 49•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/894a8aef0e46
Updated•10 years ago
|
Comment 50•10 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #47) > Argh. Sorry, my brain was confused there apparently. Will reland this on > nightly and backout of Aurora. Did you reland this in Nightly? Or are you still planning to do so?
Flags: needinfo?(wjohnston)
Updated•10 years ago
|
Flags: needinfo?(wjohnston)
Comment 51•10 years ago
|
||
Now that bug 911636 for the WebApp Runtime landed, and Android is the last core product using the old, slow nsIDownloadManager API, are you considering addressing the already filed regressions and re-landing this? (And this bug is a prerequisite for enabling downloaded malware protection on Android.)
Flags: needinfo?(wjohnston)
Reporter | ||
Comment 52•10 years ago
|
||
No one from our team is looking at this right now. I'd nom it for tracking 35 if we want to try and move its prority up (but we can't nom for 35 yet).
Flags: needinfo?(wjohnston)
Comment 53•10 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #52) > No one from our team is looking at this right now. I'd nom it for tracking > 35 if we want to try and move its prority up (but we can't nom for 35 yet). Done. To summarize why tracking is requested: - Prerequisite for enabling malware protection on Android - Required to remove old API from mozilla-central - Not secondarily, this is already done at 80%!
tracking-firefox35:
--- → ?
Assignee | ||
Updated•10 years ago
|
tracking-fennec: + → ?
Updated•10 years ago
|
Updated•10 years ago
|
tracking-fennec: ? → 35+
Reporter | ||
Comment 54•10 years ago
|
||
This is the previous two patches unbitrotted.
Attachment #8407322 -
Attachment is obsolete: true
Attachment #8407324 -
Attachment is obsolete: true
Attachment #8490397 -
Flags: review+
Reporter | ||
Comment 55•10 years ago
|
||
Fixing pdf downloads depends on bug 1063217. This is the Fennec part of it. I'll wait for it to land before landing this.
Attachment #8490437 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 56•10 years ago
|
||
This makes us persist download history forever, same as our old behavior.
Attachment #8490439 -
Flags: review?(paolo.mozmail)
Reporter | ||
Comment 57•10 years ago
|
||
This one gets a bit large. It fixed clicking on notifications, but has to move us over to the new notification api entirely. Those notifications needs a unique id for the downloads, but the new downloads don't really have one. I made one instead by just concating the uri, destionaion file, and start time. It also adds a flags parameter to some of our openTab methods to allow matching the tab url based on a prefix.
Attachment #8490443 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 58•10 years ago
|
||
Bug 1008143 is caused by the video not supported partial downloads. TBH, I'm not sure if it does or not, but our current saving technique using nsIWebProwserPersist doesn't mark it as having partial data. I fixed the notifications and about:downloads to not allow pausing if we don't have partial data.
Attachment #8490445 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 59•10 years ago
|
||
Every time we load now, we are notified about downloads, even if they're done. This makes us bail and not show notifications for finished or paused downloads during startup (we will show them for downloads we're resuming)
Attachment #8490446 -
Flags: review?(margaret.leibovic)
Comment 60•10 years ago
|
||
Comment on attachment 8490439 [details] [diff] [review] Path 3/? I'll take a look at the use of Downloads.jsm in the other patches too. For this one, I just wanted to have a confirmation that, because of how the Android UX is designed, there isn't a tendency to actually keep the downloads forever, but the user would easily clear the list. Otherwise, it's worth filing a follow-up bug to investigate the long-term performance effects of this. Maybe we could set a reasonable expiration, and then maybe have a system to include past downloads that are still on the file system in the view, even if the item has expired.
Reporter | ||
Comment 61•10 years ago
|
||
(In reply to :Paolo Amadini from comment #60) > Comment on attachment 8490439 [details] [diff] [review] > Path 3/? > > I'll take a look at the use of Downloads.jsm in the other patches too. For > this one, I just wanted to have a confirmation that, because of how the > Android UX is designed, there isn't a tendency to actually keep the > downloads forever, but the user would easily clear the list. > > Otherwise, it's worth filing a follow-up bug to investigate the long-term > performance effects of this. Maybe we could set a reasonable expiration, and > then maybe have a system to include past downloads that are still on the > file system in the view, even if the item has expired. Our current behavior is to keep them until you remove them from our DownloadManager (about:downloads). At that point we remove them from disk. We do that even for files that we automatically launch when finished (I need to check if that is broken here actually.... does this still use HelperAppDialog.js? We make some decisions about what to allow launching or downloading in there that are important for security.) That matches Android's native and expected behavior I think.
Comment 62•10 years ago
|
||
Comment on attachment 8490437 [details] [diff] [review] Patch 2/? - Fix pdf downloads Review of attachment 8490437 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +1195,5 @@ > + }); > + > + let list = yield Downloads.getList(download.source.isPrivate ? Downloads.PRIVATE : Downloads.PUBLIC) > + yield list.add(download); > + yield download.start(); You don't need to wait for the download to finish here, so you can move the start call before the addition to the list, and setting the startTime in advance is not required anymore.
Comment 63•10 years ago
|
||
Comment on attachment 8490439 [details] [diff] [review] Path 3/? Review of attachment 8490439 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +321,5 @@ > return (aDownload.startTime > maxTime) || > aDownload.hasPartialData || > !aDownload.stopped; > +#elif defined(MOZ_WIDGET_ANDROID) > + return true; I suggest refactoring the comments a bit, moving the B2G comment in the B2G section, and adding the information from comment 61 in the Android section.
Attachment #8490439 -
Flags: review?(paolo.mozmail) → review+
Comment 64•10 years ago
|
||
Comment on attachment 8490443 [details] [diff] [review] Patch 4/? Review of attachment 8490443 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/modules/DownloadNotifications.jsm @@ +126,5 @@ > + .then((downloads) => { > + var res = null; > + for (let download of downloads) { > + var cookie2 = getCookieFromDownload(download); > + if (cookie2 === cookie) { Maybe it could be simpler to use a Map or WeakMap (in the other direction) updated by list callbacks, like we do on Desktop, but I'm not actually sure. @@ +271,5 @@ > let title = strings.GetStringFromName("downloadCancelPromptTitle"); > let message = strings.GetStringFromName("downloadCancelPromptMessage"); > > if (Services.prompt.confirm(null, title, message)) { > + download.cancel().removePartialData().catch(Cu.reportError); There seems to be a typo, anyways, the calls to "cancel" and removePartialData are supposed to happen one right after the other, with no waiting. See bug 911636 comment 33. Given how much confusion this creates, I'm increasingly convinced that a better API design would have been a parameter on the "cancel" call, similarly to the "finalize" method, but this is what we have for now...
Comment 65•10 years ago
|
||
Comment on attachment 8490446 [details] [diff] [review] Patch 6/? Review of attachment 8490446 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/modules/DownloadNotifications.jsm @@ +90,5 @@ > > onDownloadAdded: function (download) { > + if (download.succeeded) { > + return; > + } The technique we use elsewhere is that the Promise returned when registering for list events isn't resolved until all the existing downloads have been added to the list. So, we set a variable when the registration promise is resolved, and don't show any completion notifications until it is set. This avoids race conditions in case a new, fast to complete download is added to the list.
Assignee | ||
Comment 66•10 years ago
|
||
Comment on attachment 8490437 [details] [diff] [review] Patch 2/? - Fix pdf downloads Review of attachment 8490437 [details] [diff] [review]: ----------------------------------------------------------------- Will this patch resolve bug 997723? Maybe we can split this off if we're ready to land the rest of this bug before bug 1063217 lands. Looks fine to me, assuming the APIs you're using work :) Please just double check the seemingly extraneous changes that ended up in here. ::: mobile/android/chrome/content/browser.js @@ -130,5 @@ > ["PermissionsHelper", ["Permissions:Get", "Permissions:Clear"], "chrome://browser/content/PermissionsHelper.js"], > ["FeedHandler", ["Feeds:Subscribe"], "chrome://browser/content/FeedHandler.js"], > ["Feedback", ["Feedback:Show"], "chrome://browser/content/Feedback.js"], > ["SelectionHandler", ["TextSelection:Get"], "chrome://browser/content/SelectionHandler.js"], > - ["Notifications", ["Notification:Event"], "chrome://browser/content/Notifications.jsm"], What is this change for? What will handle "Notification:Event" messages without this? @@ +410,5 @@ > window.addEventListener("MozShowFullScreenWarning", showFullScreenWarning, true); > > NativeWindow.init(); > LightWeightThemeWebInstaller.init(); > + DownloadNotifications.init(); This change should be part of the last patch, right? Just trying to make sure I'm not missing something here... @@ +1193,5 @@ > + saver: "pdf", > + startTime: Date.now(), > + }); > + > + let list = yield Downloads.getList(download.source.isPrivate ? Downloads.PRIVATE : Downloads.PUBLIC) If we're in private browsing mode, should we also be specifying that the download source is private in createDownload? https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm#createDownload%28%29 @@ -1203,5 @@ > - let ms = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService); > - let mimeInfo = ms.getFromTypeAndExtension("application/pdf", "pdf"); > - > - let webBrowserPrint = aBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor) > - .getInterface(Ci.nsIWebBrowserPrint); Heh, I remember writing this crap long ago. Good to see it go away!
Attachment #8490437 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 67•10 years ago
|
||
Comment on attachment 8490443 [details] [diff] [review] Patch 4/? Review of attachment 8490443 [details] [diff] [review]: ----------------------------------------------------------------- f+ for now because I think I made enough comments that I would like to see a new version of this before it lands. Also, I don't really understand Paolo's comment about the successive calls to cancel() and removePartialData(), but it sounds like there's something to address there. ::: mobile/android/chrome/content/browser.js @@ +1111,3 @@ > * @return the tab with the given URL, or null if no such tab exists > */ > + getTabWithURL: function getTabWithURL(aURL, aFlags) { Nit: I would call this aOptions not aFlags, since we usually use "flags" for binary flags. We have grown quite a collection of these little helper methods in browser.js, but I'm fine with us augmenting them as necessary. ::: mobile/android/modules/DownloadNotifications.jsm @@ +58,5 @@ > > let notifications = new Map(); > > var DownloadNotifications = { > + _notificationKey: "downloads", Should this be something more unique, like a uuid? It's an edge case, but an add-on could register a notification key that conflicts. Also, since it's only used in this file, maybe it would be better to just declare it as a const like NOTIFICATION_KEY. @@ +119,5 @@ > notification.hide(); > notifications.delete(download); > + }, > + > + _findDownloadForCookie: function(cookie) { Nit: Add some documentation explaining this returns a promise. @@ +123,5 @@ > + _findDownloadForCookie: function(cookie) { > + return Downloads.getList(Downloads.ALL) > + .then(list => list.getAll()) > + .then((downloads) => { > + var res = null; Unused variable. @@ +143,5 @@ > + showInAboutDownloads: function (download) { > + let hash = "#" + window.encodeURIComponent(download.target.path); > + > + // we can't use selectOrOpenTab, since it uses string equality to find a tab > + window.BrowserApp.selectOrOpenTab("about:downloads" + hash, { startsWith: true }); What will the value of this hash look like? What if the user has an "about:downloads#10" tab open and we're looking for "about:downloads#1"? Or do we just want to open any old about:downloads tab if it's open? In that case we wouldn't even need to add the hash here. At the very least, I think you should update this comment to better explain why we need to use this startsWith option. @@ +152,5 @@ > + if (download.succeeded) { > + // We don't call Download.launch(), because there's (currently) no way to > + // tell if the file was actually launched or not, and we want to show > + // about:downloads if the launch failed. > + let file = new FileUtils.File(download.target.path); Is FileUtils the right thing do use? I think we should probably be using OS.File instead, since it's newer and supports better async operations. @@ +167,5 @@ > + > + onButtonClick: function(button, cookie) { > + this._findDownloadForCookie(cookie).then((download) => { > + if (button === kButtons.PAUSE.buttonId) { > + Services.console.logStringMessage("Cancel"); Looks like you left in some debug logging here. @@ +172,5 @@ > + download.cancel().catch(Cu.reportError); > + } else if (button === kButtons.RESUME.buttonId) { > + download.start().catch(Cu.reportError); > + } else if (button === kButtons.CANCEL.buttonId) { > + download.cancel().removePartialData()..catch(Cu.reportError); Typo: 2 periods. @@ +180,3 @@ > }; > > +function getCookieFromDownload(download) { Nit: Add some documentation about what this cookie is used for. ::: mobile/android/modules/Notifications.jsm @@ +259,5 @@ > return this; > } > } > + > +Services.obs.addObserver(Notifications, "Notification:Event", false); Oh, this is where this went... any reason not to just keep this in browser.js like it was before?
Attachment #8490443 -
Flags: review?(margaret.leibovic) → feedback+
Assignee | ||
Comment 68•10 years ago
|
||
Comment on attachment 8490445 [details] [diff] [review] Patch 5/? Review of attachment 8490445 [details] [diff] [review]: ----------------------------------------------------------------- This sounds like a good way to work around the problem.
Attachment #8490445 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 69•10 years ago
|
||
Comment on attachment 8490446 [details] [diff] [review] Patch 6/? Review of attachment 8490446 [details] [diff] [review]: ----------------------------------------------------------------- I like Paolo's suggestion, let's see if we can do that.
Attachment #8490446 -
Flags: review?(margaret.leibovic) → review-
Assignee | ||
Updated•10 years ago
|
Assignee: james.gilbertson → wjohnston
Reporter | ||
Comment 70•10 years ago
|
||
Here's a build of these patches for QA: http://people.mozilla.org/~wjohnston/downloadsjsm.apk
Reporter | ||
Comment 71•10 years ago
|
||
(In reply to :Paolo Amadini from comment #65) > Comment on attachment 8490446 [details] [diff] [review] > Patch 6/? > > Review of attachment 8490446 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/modules/DownloadNotifications.jsm > @@ +90,5 @@ > > > > onDownloadAdded: function (download) { > > + if (download.succeeded) { > > + return; > > + } > > The technique we use elsewhere is that the Promise returned when registering > for list events isn't resolved until all the existing downloads have been > added to the list. So, we set a variable when the registration promise is > resolved, and don't show any completion notifications until it is set. > > This avoids race conditions in case a new, fast to complete download is > added to the list. I'm a little confused by this. During startup, we do restart downloads that were cancelled (or paused?) before. Won't this cause us to ignore them? Can you point me to the desktop code?
Comment 72•10 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #71) > > > onDownloadAdded: function (download) { > > > + if (download.succeeded) { > > > + return; > > > + } > > > > The technique we use elsewhere is that the Promise returned when registering > > for list events isn't resolved until all the existing downloads have been > > added to the list. So, we set a variable when the registration promise is > > resolved, and don't show any completion notifications until it is set. > > > > This avoids race conditions in case a new, fast to complete download is > > added to the list. > > I'm a little confused by this. During startup, we do restart downloads that > were cancelled (or paused?) before. Won't this cause us to ignore them? Sorry, it was me that confused the "download notification" with the "attention" indicator (green download icon) shown when a download completes. So, if I understand correctly, this actually adds the item to a system list, and this makes sense for automatically resumed downloads on startup. That said, after startup you may still want to add downloads that succeed rapidly, as there are no guarantees that the notification is sent before a download completes. Thus, the initial check to exit the function could be "download.succeeded && !this._viewAdded" or something similar. > Can you point me to the desktop code? It turns out we didn't end up implementing the check this way in Desktop due to the legacy architecture, though we intended to. We do use the addView promise checking in a couple other places: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#1203 http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadList.jsm#412
Comment 73•10 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #70) > Here's a build of these patches for QA: > http://people.mozilla.org/~wjohnston/downloadsjsm.apk The build looks fine, I've tested on the following devices, to cover most of the Android versions: *HTC Desire HD (Android 2.3.5) *Samsung Galaxy Nexus (Android 4.2.1) *LG Nexus (Android 4.4.4) I've tested: *Downloads functionality *Saving through context menu *Saving different file types *Notification flows, and notification consistency between android notifications and about:downloads *Pause, resume, cancel, retry, delete, delete all *Checked that pausing is not allowed if we don't have partial data *Save as PDF
Keywords: qawanted
Assignee | ||
Comment 74•10 years ago
|
||
wesj, how close are we to landing this? At this point maybe we should just wait to land it at the beginning of the Fx36 cycle.
Flags: needinfo?(wjohnston)
Assignee | ||
Comment 75•10 years ago
|
||
I'll take this over to finish it. wesj, can you upload any updated patches you might have hanging around?
Assignee: wjohnston → margaret.leibovic
Reporter | ||
Comment 76•10 years ago
|
||
I don't think I have any patches lying around for this. Bug 1070086 introduced a message to Java when downloads are removed that will need to be ported over to our onDownloadRemoved method in DownloadNotifications.jsm. https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm/DownloadList#addView%28%29 I was mostly hung up on some tests for the pdf download saver (but that bug is close to done too...): https://bugzilla.mozilla.org/show_bug.cgi?id=1063217
Flags: needinfo?(wjohnston)
Assignee | ||
Updated•10 years ago
|
tracking-fennec: 35+ → 36+
Assignee | ||
Comment 77•10 years ago
|
||
Comment on attachment 8490446 [details] [diff] [review] Patch 6/? There's enough going on in this bug. Let's move this patch (and the discussion about it) to a follow-up.
Attachment #8490446 -
Attachment is obsolete: true
Assignee | ||
Comment 78•10 years ago
|
||
I've un-bitrotted the patches here and addressed review comments. Unfortunately, it looks like we might need to land bug 1063217 before we can land the patches here. Either that, or we need a different approach for the save as PDF patch. In any case, I'm going to upload the updated patches in case anyone wants to take a look.
Attachment #8490397 -
Attachment is obsolete: true
Attachment #8490437 -
Attachment is obsolete: true
Attachment #8490439 -
Attachment is obsolete: true
Attachment #8490443 -
Attachment is obsolete: true
Attachment #8490445 -
Attachment is obsolete: true
Assignee | ||
Comment 79•10 years ago
|
||
Assignee | ||
Comment 80•10 years ago
|
||
Assignee | ||
Comment 81•10 years ago
|
||
Assignee | ||
Comment 82•10 years ago
|
||
Assignee | ||
Comment 83•10 years ago
|
||
wesj is back from PTO and said he would take this back. I'm happy to help review/test!
Assignee: margaret.leibovic → wjohnston
Reporter | ||
Comment 84•10 years ago
|
||
Based on what I can see in the PDF Saver bug, Windows throws up a dialog if you try and create a fake "printer" for this that fails in the test. Our gtk code seems to have a memory leak. I can fix both, but I expect they'll drag on for a bit. I wonder if for now (here) we'd just accept not putting these pdf files in our own download manager? (We could put them in Android's on HC+ if we want). Then we could land this now, and backout if we determine the PDF saver is a requirement. I don't know of any other way to put the pdf-download into the Downloads.jsm database. This will be a large, but fairly easy backout (i.e. the code is fairly self contained). I can even refactor it to be #ifdef DOWNLOADS_JSM_SUPPORT.
Reporter | ||
Comment 85•10 years ago
|
||
needinfo to mfinkle about not pdf not appearing in downloads, and paolo about any alternative ways to add downloads to the download database?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(mark.finkle)
Assignee | ||
Comment 86•10 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #84) > I wonder if for now (here) we'd just accept not putting these > pdf files in our own download manager? (We could put them in Android's on > HC+ if we want). What will the "Save as PDF" UX be like then? Would users have a clear way to get at the PDF they save? Right now the download notification seems like the only thing we give users. I would love for us to be able to land this bug without blocking on "Save as PDF", but I think we need to make sure we don't regress that feature. Or are you suggesting we just regress it on Nightly for now so that we can land the bulk of this code, but then block shipping on fixing that? I think I would be okay with that.
Reporter | ||
Comment 87•10 years ago
|
||
Yeah. I think we could block shipping on fixing the pdf saver bug (or finding a way to add these to the list). Since I think this is used for a read-it-later functionality by some users I think you're right, we probably can't ship with it not working. Since most of these changes are in a new file, I think it would be good/easy to ifdef this anyway. Will look into that.
Comment 88•10 years ago
|
||
I just commented in bug 1063217 before seeing the last few comments here - I think we're close to landing it, so there's probably no need for an alternate solution. If the memory leaks are only on Linux and can't be easily solved, we can file a separate bug for them and just disable the DownloadPDFSaver tests there.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(mark.finkle)
Assignee | ||
Comment 89•9 years ago
|
||
We're not tracking this for a particular release, but once we finish bug 1063217 I can help try to land this.
tracking-fennec: 36+ → +
Assignee | ||
Comment 90•9 years ago
|
||
I was going to push an updated patch series to try, but it's closed. However, it seems to be working fine locally. In the meantime if QA would like to test, I put a build here: http://people.mozilla.org/~mleibovic/tmp/downloads.apk
Assignee | ||
Comment 91•9 years ago
|
||
I'll own finishing this. If a try push is green, is there anything preventing us from landing these patches? https://tbpl.mozilla.org/?tree=Try&rev=4f7014598cc3
Assignee: wjohnston → margaret.leibovic
Flags: needinfo?(wjohnston)
Flags: needinfo?(paolo.mozmail)
Comment 92•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #91) > I'll own finishing this. If a try push is green, is there anything > preventing us from landing these patches? I'm not aware of any other issue preventing this from landing. I believe the dependent bug 997723, bug 997756, and bug 1008143 can be closed when this lands, as they are not issues with this patch series anymore, right?
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 93•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #91) > I'll own finishing this. If a try push is green, is there anything > preventing us from landing these patches? > > https://tbpl.mozilla.org/?tree=Try&rev=4f7014598cc3 Green try build! I'll land this when fx-team re-opens, and I can post to mobile-firefox-dev about it to look out for regressions. (In reply to :Paolo Amadini from comment #92) > (In reply to :Margaret Leibovic from comment #91) > > I'll own finishing this. If a try push is green, is there anything > > preventing us from landing these patches? > > I'm not aware of any other issue preventing this from landing. > > I believe the dependent bug 997723, bug 997756, and bug 1008143 can be > closed when this lands, as they are not issues with this patch series > anymore, right? Yes, these should all be fixed by the new patches. We can get QA to verify that and close them when this lands.
Assignee | ||
Comment 94•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cc2573030f5b https://hg.mozilla.org/integration/fx-team/rev/8f7bc3a0a8dc https://hg.mozilla.org/integration/fx-team/rev/80c64d4cb87b https://hg.mozilla.org/integration/fx-team/rev/1ab5f84fa165 https://hg.mozilla.org/integration/fx-team/rev/f03d296ee8dd
Flags: needinfo?(wjohnston)
Comment 95•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cc2573030f5b https://hg.mozilla.org/mozilla-central/rev/8f7bc3a0a8dc https://hg.mozilla.org/mozilla-central/rev/80c64d4cb87b https://hg.mozilla.org/mozilla-central/rev/1ab5f84fa165 https://hg.mozilla.org/mozilla-central/rev/f03d296ee8dd
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment 96•9 years ago
|
||
If this is safe to uplift, please nominate before Mon Dec 22 to get into 35.
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 97•9 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #96) > If this is safe to uplift, please nominate before Mon Dec 22 to get into 35. Oh, this must have been an old flag. We most definitely do not want to uplift this anywhere.
status-firefox34:
--- → wontfix
status-firefox35:
--- → wontfix
status-firefox36:
--- → wontfix
status-firefox37:
--- → fixed
tracking-firefox35:
+ → ---
Flags: needinfo?(margaret.leibovic)
Comment 98•9 years ago
|
||
Adding the addon-compat keyword since this has a significant impact on download-related add-ons, and dev-doc-needed since this should also be mentioned as news for developers in this release. This section of the documentation is particularly relevant: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm#Conversion_from_nsIDownloadManager
Keywords: addon-compat,
dev-doc-needed
Assignee | ||
Updated•9 years ago
|
Comment 100•9 years ago
|
||
relnote updated for Beta as "Improved download performance with new download manager back-end"
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•