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)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox31 wontfix, firefox32 wontfix, firefox34 wontfix, firefox35 wontfix, firefox36 wontfix, firefox37 fixed, relnote-firefox 37+, fennec+)

RESOLVED FIXED
Firefox 37
Tracking Status
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...
Blocks: 851471
Depends on: 847863
Keywords: perf
tracking-fennec: --- → ?
tracking-fennec: ? → +
Summary: Investigate moving download management to Downloads.jsm → Investigate moving download management to Downloads.jsm in Firefox for Android
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.
This will be required for some things like the new malware protection. See bug 895476.
Blocks: 936041
Blocks: 949544
No longer blocks: 949544
Depends on: 816318
WIP

Refactor download notifications, using Downloads.jsm and Notifications.jsm
WIP

Refactors aboutDownloads.js to use Downloads.jsm instead of nsIDownloadManager.
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.
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)
Attachment #8375361 - Flags: feedback?(bnicholson)
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+
(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?
(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 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+
(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.
Attachment #8375360 - Attachment is obsolete: true
Attachment #8375360 - Flags: feedback?(wjohnston)
Attachment #8379280 - Flags: feedback?(wjohnston)
Attachment #8379280 - Flags: feedback?(bnicholson)
Rebased on top of changeset b89a9d7b4ca0
Attachment #8379282 - Flags: feedback?(wjohnston)
Attachment #8379282 - Flags: feedback?(bnicholson)
Attachment #8375361 - Attachment is obsolete: true
Attachment #8375361 - Flags: feedback?(wjohnston)
Attachment #8379282 - Attachment is patch: true
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)
(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?
Attachment #8379280 - Flags: feedback?(bnicholson) → feedback+
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 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+
(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 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+
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+
Updated in response to feedback
Attachment #8379280 - Attachment is obsolete: true
Attachment #8379282 - Attachment is obsolete: true
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.
Summary: Investigate moving download management to Downloads.jsm in Firefox for Android → Convert to Downloads.jsm in Firefox for Android
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.
(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!
Attachment #8384082 - Attachment is obsolete: true
Attachment #8396884 - Flags: review?(wjohnston)
Attachment #8384083 - Attachment is obsolete: true
Attachment #8396885 - Flags: review?(wjohnston)
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+
(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).
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)
Attachment #8396884 - Attachment is obsolete: true
Attachment #8396885 - Attachment is obsolete: true
Attachment #8396885 - Flags: feedback?(margaret.leibovic)
Attachment #8404463 - Flags: feedback?(margaret.leibovic)
(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.
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: nobody → james.gilbertson
(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.
Attachment #8404462 - Attachment is obsolete: true
Attachment #8404463 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 8407322 [details] [diff] [review]
Refactor notifications to use Downloads.jsm (v6)

Carrying over review+ from wesj.
Attachment #8407322 - Flags: review+
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+
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
Depends on: 997723
Depends on: 997756
Depends on: 1000552
Depends on: 1005871
Depends on: 1005893
Depends on: 1008143
I backed this out of Aurora. We'll try to fix the regressions on nightly:

https://hg.mozilla.org/integration/fx-team/rev/136592f9f5a4
fx-team != Aurora :confused:

Anyway, merge of backout:
https://hg.mozilla.org/mozilla-central/rev/136592f9f5a4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 31 → ---
Argh. Sorry, my brain was confused there apparently. Will reland this on nightly and backout of Aurora.
backed out of aurora.
(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)
Flags: needinfo?(wjohnston)
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)
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)
(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-fennec: + → ?
tracking-fennec: ? → 35+
Depends on: 1063217
Attached patch Patch 1/2 (obsolete) — Splinter Review
This is the previous two patches unbitrotted.
Attachment #8407322 - Attachment is obsolete: true
Attachment #8407324 - Attachment is obsolete: true
Attachment #8490397 - Flags: review+
Attached patch Patch 2/? - Fix pdf downloads (obsolete) — Splinter Review
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)
Attached patch Path 3/? (obsolete) — Splinter Review
This makes us persist download history forever, same as our old behavior.
Attachment #8490439 - Flags: review?(paolo.mozmail)
Attached patch Patch 4/? (obsolete) — Splinter Review
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)
Attached patch Patch 5/? (obsolete) — Splinter Review
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)
Attached patch Patch 6/? (obsolete) — Splinter Review
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 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.
(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 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 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 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 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.
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+
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+
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+
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: james.gilbertson → wjohnston
Here's a build of these patches for QA:
http://people.mozilla.org/~wjohnston/downloadsjsm.apk
(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?
Keywords: qawanted
(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
(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
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)
I'll take this over to finish it. wesj, can you upload any updated patches you might have hanging around?
Assignee: wjohnston → margaret.leibovic
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)
tracking-fennec: 35+ → 36+
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
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
wesj is back from PTO and said he would take this back.

I'm happy to help review/test!
Assignee: margaret.leibovic → wjohnston
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.
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)
(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.
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.
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)
Depends on: 1095506
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+ → +
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
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)
(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)
Depends on: 1111659
(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.
Depends on: 1113092
Depends on: 1113110
If this is safe to uplift, please nominate before Mon Dec 22 to get into 35.
Flags: needinfo?(margaret.leibovic)
(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.
Flags: needinfo?(margaret.leibovic)
Depends on: 1113844
Blocks: 1114506
Blocks: 1114586
Blocks: 1114593
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
Blocks: 1114617
Blocks: 1114637
No longer blocks: 1114506
Depends on: 1114506
Depends on: 1116280
relnoted as "New download manager back-end"
Depends on: 1125043
Depends on: 1130834
relnote updated for Beta as "Improved download performance with new download manager back-end"
Depends on: 1141550
Depends on: 1134553
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.