695178 - Download Manager

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Erin Lancaster [:elan]]

Discussing using system. Can still share necko stack?

Comment 1

[Gian-Carlo Pascutto [:gcp]]

What's the issues with using/not using the Necko stack? (Why do we care?)

Comment 2

[Gian-Carlo Pascutto [:gcp]]

http://developer.android.com/reference/android/app/DownloadManager.Request.html

addRequestHeader(String header, String value)
Add an HTTP header to be included with the download request.

We should be able to pass forward any cookies for sites that require authentication.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

As discussed on IRC, there's still an issue with apps that "unexpectedly" offer us a download, and we have to take over from Necko there. The problem is that this will cause us to re-request the file, which might not work for some services that only allow you to download once (i.e. many online file sharing sites).

Perhaps we can *try* to start the second download in the manager, and if that succeeds, only then stop the download in Necko.

Comment 5

[Armen [:armenzg]]

Is this bug for the lack of a way to load up a download manager? (the development terminology makes it hard for me).

Thanks!

Comment 6

[Gian-Carlo Pascutto [:gcp]]

This bug is to have Fennec use the Android system service (and user interface) for long running downloads.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Matt - still looking for a way to plug into the system DB for downloads

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

We need to do some initial research to see whether this is even possible, before beta cutoff

Comment 13

[Matt Brubeck (:mbrubeck)]

For comparison, here's some of the difference in code in the stock browser between the new public API (DownloadManager, available in Android 2.3 and later) and the previous implementation using private APIs (android.provider.Downloads):
https://github.com/android/platform_packages_apps_browser/commit/c6076533b489c17493640c14935324a30f582903

(The stock browser does not use the addCompletedDownload method that we are interested in, so there's no direct equivalent in that code to what we would need to do.)

Here's a post about how to use the private android.provider.Downloads API from third-party code:
http://blog.lytsing.org/archives/135.html

It mentions that some of the columns changed between Android 2.1 and 2.2, so if we want to use the private API then we would need separate code paths for different versions.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: about:downloads

WIP of an about:downloads page written in HTML, made to mimick our current about:addons (somewhat, sharing styles turned out to be hard, so I just copied over the pieces I needed for now).

Wanted to get the bare minimum we need. This just shows downloads (all of them, even active ones. probably should not show them if the user can't do anything with them?) and has "Open" and "Delete" buttons. Also, its got super cool animation. I'll post a screenshot and we can see what UX thinks.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Adding the UX team

Comment 17

[Matt Brubeck (:mbrubeck)]

Giving this to Wes for now, but I can take it again if needed.  Since this adds strings and UI, should it block beta?  Can we still get this in despite string freeze?  (Any way we can use strings that are already in toolkit or elsewhere?)

By the way, the DTD file seems to be missing from the WIP patch.

Comment 18

[Ian Barlow (:ibarlow)]

Wes, this looks good! I have some minor visual tweaks I would suggest, but before that just wanted to ask if we are able to add

* A progress indicator
* Pause / Cancel controls during downloading
* Time / Date (less important)

Comment 19

[Wesley Johnston (:wesj)]

Time and Date are relatively "easy". Showing in-progress downloads is more work (I was planning to filter them out of the list for now), as well as adding "Retry" or "Redownload" for completed ones.

I want, for a first draft, the minimum functionality we need to be useful to users. Does UX feel showing in progress downloads is part of that?

Can we design something that we can build in steps, I can write the code with the idea in mind that we'll need this stuff eventually, but I'll do the other pieces in P5 follow up bugs? If they get done, yay! If not... we'll live?

Comment 20

[Ian Barlow (:ibarlow)]

Sure, I'm fine with that. Wes, is there a build with this somewhere I can try? Or, can you post a screen shot of both the downloads UI and the addons UI side by side?

The main thing here is to get the styling to match closely, and I want to make sure I'm commenting based on screenshots from the same device :)

Comment 21

[Ian Barlow (:ibarlow)]

Attachment: comments

Based on the comparison screens Wes sent me, here are some styling suggestions. I think we're quite close, we just want to tweak things to look more like the Addons UI.

I'll post an additional mockup after this that includes more file information, in case we have time to sneak some in :)

Comment 22

[Ian Barlow (:ibarlow)]

Attachment: Additional, nice-to-have details

Includes file size, source, and date downloaded. Happy to have this broken out as a separate bug if necessary.

Comment 23

[Wesley Johnston (:wesj)]

Because it might be useful to someone else sometime, I ported the AddonsManager and DownloadManager to work as normal webpages by stubbing out our platform API calls (and using some fake databases):

http://dl.dropbox.com/u/72157/aboutTest/AboutTest.html

Comment 24

[Johnathan Nightingale [:johnath]]

Re-nom'ng to consider as a beta instead of final blocker, per comment 17

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Patch

I think this is pretty good to go for now. Doesn't show in progress downloads, but has most of the other pieces we wanted. I am not doing a very good job sharing style's with the addons manager here. I can do that before we go forward, in which case, consider this a feedback request. Otherwise, I think its ready for review. In either case, I'll do that in a second patch.

I'll post some screenshots.

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Empty download manager

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Full download manager

Comment 28

[Wesley Johnston (:wesj)]

Attachment: Delete prompt

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 610344 [details] [diff] [review]
Patch

General:
* I don't like the buttons. It's hard to tell what row the buttons are associated with. Can we use a context menu instead? Long tap on the download item shows "Open" and "Delete". Also, tapping on a download item will open.
* Personally, I think we should remove the ICS-only download manager if we land this. We can add more features to this UI. We can't add features to the built-in download manager. I think the consistency will be good for users too.

>diff --git a/mobile/android/chrome/content/aboutDownloads.js b/mobile/android/chrome/content/aboutDownloads.js

Add the mini MPL 2 license

>+let Ci = Components.interfaces, Cc = Components.classes, Cu = Components.utils;
>+Cu.import("resource://gre/modules/Services.jsm");

Add a blank line between

>+let gStringBundle = Services.strings.createBundle("chrome://browser/locale/aboutDownloads.properties");

gStringBundle -> Strings

>+
>+function init() {
>+  Downloads.getDownloads();
>+}
>+
>+function uninit() {
>+}

Should these just be part of Downloads ?


>+                       "</li>";
>+let Downloads = {

Add a blank line

>+  _initStatement: function dv__initStatement() {
>+    if (this._stmt)
>+      this._stmt.finalize();
>+
>+    this._stmt = this._dlmgr.DBConnection.createStatement(
>+      "SELECT id, target, name, source, state, startTime, endTime, referrer, " +
>+             "currBytes, maxBytes, state IN (?1, ?2, ?3, ?4, ?5) isActive " +
>+      "FROM moz_downloads " +
>+      "WHERE NOT isActive " +
>+      "ORDER BY isActive DESC, endTime DESC, startTime DESC");
>+  },

>+  _stepDownloads: function dv__stepDownloads(aNumItems) {
>+    try {
>+        if (!this._stmt.executeStep()) {

indent is off (4 spaces but should be 2)

>+            // Show empty message if needed
>+            // this._ifEmptyShowMessage();
>+            console.log("Done with list!");

Should we finalize the stmt in here somewhere too?

>+        this._list.innerHTML += this._createItem(downloadTemplate, attrs);

Can we use a documentFragment of just cache the string in a variable, waiting until the end to update the list just one time?

>+    catch (e) {
>+      // Something went wrong when stepping or getting values, so clear and quit
>+      console.log("Error: " + e);
>+      this._stmt.reset();

Is reset enough here? or should we finalize too?

>+      this._stepDownloads(aNumItems - 1);
>+    }
>+    else {

} else {

>+      let delay = Math.min(this._list.itemCount * 10, 300);

itemCount? for a <ul>? I think this is left over from the XUL code. Maybe _list.children.length ?

>+  getDownloads: function() {
>+    this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);

Seems an odd place to init this when it's used in other functions. How about making it a lazy-inited global, "DownloadMgr" ? or maybe in the init() function if you make it part of Downloads.

>+    this._list = document.getElementById("downloads-list");
>+    this._list.addEventListener("click", function(event) {

Hmm, I guess this is kind of an "init" function

>+      while(target && target.nodeName != "li") {

while (

>+  get selectedItem() {
>+    return document.querySelector("li.selected");
>+  },
>+
>+  set selectedItem(val) {
>+    let oldItem = this.selectedItem;
>+    if (oldItem)
>+      oldItem.classList.remove("selected");
>+
>+    if (val) val.classList.add("selected");
>+  },

Using a context menu could remove the need to track "selected"

>+  removeDownload: function(aItem) {

>+    let res = Services.prompt.confirm(window, gStringBundle.GetStringFromName("downloadAction.promptRemove"), f.leafName);
>+    if(res) {

if (

>diff --git a/mobile/android/chrome/content/aboutDownloads.xhtml b/mobile/android/chrome/content/aboutDownloads.xhtml

>+<html xmlns="http://www.w3.org/1999/xhtml"
>+      xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

Do we need the XUL namespace?

>diff --git a/mobile/android/locales/en-US/chrome/aboutDownloads.dtd b/mobile/android/locales/en-US/chrome/aboutDownloads.dtd
>+<!ENTITY aboutDownloads.header2                    "Your Downloads">

Save bumping the entity to "2" until we need to :)

r-, but not bad and let's get Ian & Madhava to feedback too

Comment 30

[Ian Barlow (:ibarlow)]

Attachment: visual tweaks

Looks great Wes! Just some minor visual tweaks attached.

Also, I'm ok with Mark's suggestion about using a long press context menu instead of inline buttons. I didn't have much of an issue with figuring out which row the buttons went with, but I agree it's more efficient to just open the download when you tap on it.

I also agree with using this as our download manager in ICS, since this UI is already pretty solid, feels on brand, and we can keep adding the features we want without relying too heavily on Android.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 610344 [details] [diff] [review]
Patch

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

::: mobile/android/locales/en-US/chrome/aboutDownloads.dtd
@@ +1,1 @@
> +<!ENTITY aboutDownloads.title                      "Downloads Manager">

Can we go with "Downloads" for consistency with desktop?

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Patch v2

> * I don't like the buttons. It's hard to tell what row the buttons are
> associated with. Can we use a context menu instead? Long tap on the download
> item shows "Open" and "Delete". Also, tapping on a download item will open.
Done.

> * Personally, I think we should remove the ICS-only download manager if we
> land this. We can add more features to this UI. We can't add features to the
> built-in download manager. I think the consistency will be good for users
> too.
Done.

> gStringBundle -> Strings
gStrings I assumed based on your apps patch.

> Should these just be part of Downloads ?
Yes. done.

> Should we finalize the stmt in here somewhere too?
Added this, but I think we should be fine.

> Can we use a documentFragment of just cache the string in a variable,
> waiting until the end to update the list just one time?
Found a nice alternative in insertAdjacentHtml

> Is reset enough here? or should we finalize too?
Not needed according to https://developer.mozilla.org/en/Storage#Resetting_a_Statement

> itemCount? for a <ul>? I think this is left over from the XUL code. Maybe
> _list.children.length ?
Whoops. Yep. Thanks.

> Seems an odd place to init this when it's used in other functions. How about
> making it a lazy-inited global, "DownloadMgr" ?
Went with the lazy global here.

> Do we need the XUL namespace?
I had to cheat to get cropping in the center of strings here (without writing a special cropping library. Platform should support that. I'm going to file a bug.

Comment 33

[Wesley Johnston (:wesj)]

Whoops. I removed the prompt for deleting files, since it felt kinda redundant with the context menu, but I left the promptRemove string in there. Will remove on checkin.

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 610957 [details] [diff] [review]
Patch v2

This looks pretty good. Needs a few things:

Missed a Makefile change required to build:

diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in

 FENNEC_PP_XML_FILES = \
   res/layout/abouthome_content.xml \
   res/menu/gecko_menu.xml \
-  res/menu-v11/gecko_menu.xml \
   $(NULL)
 

>diff --git a/mobile/android/chrome/content/aboutDownloads.js b/mobile/android/chrome/content/aboutDownloads.js

This file needs to hook up some download notification observers like here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/downloads.js#60
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/downloads.js#140

So the list can be auto-updated when a new download completes. Right now I need to refresh the page manually.

>+let Downloads = {
>+
>+  init: function () {

nit: remove the blank line

>+  _stepDownloads: function dv__stepDownloads(aNumItems) {

>+      let attrs = {

>+        open: this.openString,
>+        remove: this.removeString,

I think we can remove these two strings

>diff --git a/mobile/android/chrome/content/aboutDownloads.xhtml b/mobile/android/chrome/content/aboutDownloads.xhtml

>+  <link rel="icon" type="image/png" sizes="32x32" href="chrome://branding/content/favicon32.png" />
>+  <link rel="icon" type="image/png" sizes="64x64" href="chrome://branding/content/favicon64.png" />

This doesn't seem to work for making the favicon appear. Let's just use the "favicon32.png" link and drop the "sizes" attribute. That's how the rest of the pages are setup and the favicon works there.

>diff --git a/mobile/android/locales/en-US/chrome/aboutDownloads.properties b/mobile/android/locales/en-US/chrome/aboutDownloads.properties

>+downloadAction.promptRemove=Delete File

This is not used anymore

r- for the dynamic list updates

Comment 35

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes is working hard on the content providers, so I am handing the finish-up work for this patch to Brian.

Comment 36

[Brian Nicholson (:bnicholson)]

Attachment: patch v3

added dynamic updates and download states

Comment 37

[Brian Nicholson (:bnicholson)]

Attachment: patch v4

A few fixes:
* Puts new downloads at the top of the list, not the bottom
* If starting a download already in the list, the download is updated, preventing duplicates
* I noticed that if I killed Fennec during a download and reopen Fennec, the download is first added to the list as a queued download with -1 bytes before the download starts. To fix this, I included the download size in the update.
* I initially thought that the "endTime" download attribute would be empty until the download finished, but it looks like the endTime immediately gets set to the time the download starts (?). Also, the endTime doesn't change when the download is finished - I don't know whether this is a bug or if I misunderstand the endTime field. Since nsIDownload doesn't have an endTime attribute, I just gave the current time using new Date().

Comment 38

[Brian Nicholson (:bnicholson)]

Attachment: patch v5

Removed status indicator and only show completed downloads as discussed in IRC.

Comment 39

[Wesley Johnston (:wesj)]

Comment on attachment 611662 [details] [diff] [review]
patch v5

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

Looks good to me.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +36,5 @@
> +let Downloads = {
> +  init: function () {
> +    this._list = document.getElementById("downloads-list");
> +    this._list.addEventListener("click", function (event) {
> +      event.preventDefault();

Probably not needed.

@@ +109,5 @@
> +      "WHERE NOT isActive " +
> +      "ORDER BY endTime DESC");
> +  },
> +
> +  _updateItem: function (aKey, aValues) {

Not used anymore. I'm not sure I love textConent = className either, but I don't really have a better solution when we eventually do this (Maybe just replace innerHTML entirely?)

Comment 40

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 611662 [details] [diff] [review]
patch v5

I don't have any additional comments. I agree with Wes'.

We should consider using a consistent approach in about:addons and about:downloads in the future. Some of that is design work (using context menus) and some is code (suing the same row generation approach).

Comment 41

[Madhava Enros [:madhava]]

Will downloads in Firefox on ICS also show up in the system one? I think they should.

Comment 42

[Wesley Johnston (:wesj)]

I'm getting errors when I try to open downloads. And the themeing is broken... (i.e. don't check this in yet)

04-04 12:03:02.747 27047 27056 W System.err: java.lang.NullPointerException
04-04 12:03:02.747 27047 27056 W System.err: 	at org.mozilla.gecko.GeckoActivity.checkIfGeckoActivity(GeckoActivity.java:60)
04-04 12:03:02.747 27047 27056 W System.err: 	at org.mozilla.gecko.GeckoActivity.startActivity(GeckoActivity.java:42)
04-04 12:03:02.747 27047 27056 W System.err: 	at org.mozilla.gecko.GeckoAppShell.openUriExternal(GeckoAppShell.java:952)
04-04 12:03:02.747 27047 27056 W System.err: 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
04-04 12:03:02.747 27047 27056 W System.err: 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
04-04 12:03:02.754 27047 27056 W System.err: 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:464)
04-04 12:03:02.754 27047 27056 W System.err: 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:106)

Comment 43

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Madhava Enros [:madhava] from comment #41)
> Will downloads in Firefox on ICS also show up in the system one? I think
> they should.

Yes, the downloads on ICS will still show up in the system manager. That is handled by the "Downloads:Done" message.

Comment 44

[Wesley Johnston (:wesj)]

Here's some screenshots madhava:

http://people.mozilla.com/~wjohnston/screenshots/dm1.png - Normal
http://people.mozilla.com/~wjohnston/screenshots/dm2.png - Item tapped
http://people.mozilla.com/~wjohnston/screenshots/dm3.png - Context menu

Brian, somethings up with the theming (sorry, should have caught it earlier). The rows are the wrong height it looks like.

Comment 45

[Walter K.]

(In reply to Wesley Johnston (:wesj) from comment #44)
> Here's some screenshots madhava:

Will there be a menu option to copy the complete URL of the downloaded item? Copy URL (a.k.a Copy Link) and Share would make download manager menu much more useful.

Comment 46

[Wesley Johnston (:wesj)]

We're just trying to get the basic shell done here. Feel free to file follow up bugs for additional things. I know brian filed a few for showing in-progress downloads as well as adding "Pause", "Resume", etc.

Comment 47

[Brian Nicholson (:bnicholson)]

Attachment: basic download manager with proper theme

Comment 48

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/48cb615fbcb5

Comment 49

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/48cb615fbcb5

Comment 50

[Andreea Pod]

Download manager is available.
Verified fixed on: Firefox 15.0a1 (2012-05-22)
Device: LG Optimus 2X (Android 2.2.2)

Comment 51

[Adrian Tamas (:AdrianT)]

The test suite covering the feature can be found here:

https://moztrap.mozilla.org/manage/cases/?filter-suite=67

The suite is added as part of the BFTs run