Closed Bug 695178 Opened 8 years ago Closed 8 years ago

Download Manager

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+
fennec 11+ ---

People

(Reporter: elan, Assigned: bnicholson)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: late-l10n, Whiteboard: [strings][has patch][QA+])

Attachments

(9 files, 5 obsolete files)

Discussing using system. Can still share necko stack?
Priority: P1 → P3
What's the issues with using/not using the Necko stack? (Why do we care?)
Assignee: nobody → gpascutto
No longer blocks: 695461
Depends on: 695461
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.
Duplicate of this bug: 697419
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.
Is this bug for the lack of a way to load up a download manager? (the development terminology makes it hard for me).

Thanks!
This bug is to have Fennec use the Android system service (and user interface) for long running downloads.
Duplicate of this bug: 705694
Matt - still looking for a way to plug into the system DB for downloads
Assignee: gpascutto → mbrubeck
Priority: P3 → P2
Duplicate of this bug: 704840
tracking-fennec: --- → 11+
Depends on: download-manager
Duplicate of this bug: 728696
Duplicate of this bug: 725151
We need to do some initial research to see whether this is even possible, before beta cutoff
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.
blocking-fennec1.0: --- → +
Status: NEW → ASSIGNED
Attached patch about:downloads (obsolete) — Splinter Review
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.
Attached image Screenshot
Adding the UX team
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.
Assignee: mbrubeck → wjohnston
Whiteboard: [strings]
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)
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?
Keywords: late-l10n
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 :)
Attached image 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 :)
Includes file size, source, and date downloaded. Happy to have this broken out as a separate bug if necessary.
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
Re-nom'ng to consider as a beta instead of final blocker, per comment 17
blocking-fennec1.0: + → ?
blocking-fennec1.0: ? → beta+
Priority: P2 → P1
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #606379 - Attachment is obsolete: true
Attachment #610344 - Flags: review?(mark.finkle)
Attached image Delete prompt
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
Attachment #610344 - Flags: review?(mark.finkle) → review-
Attached image 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 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?
Attached patch Patch v2 (obsolete) — Splinter Review
> * 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.
Attachment #610344 - Attachment is obsolete: true
Whiteboard: [strings] → [strings][has patch]
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.
Attachment #610957 - Attachment is patch: true
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
Attachment #610957 - Flags: review-
Wes is working hard on the content providers, so I am handing the finish-up work for this patch to Brian.
Assignee: wjohnston → bnicholson
Attached patch patch v3 (obsolete) — Splinter Review
added dynamic updates and download states
Attachment #610957 - Attachment is obsolete: true
Attachment #611609 - Flags: review?(mark.finkle)
Attached patch patch v4 (obsolete) — Splinter Review
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().
Attachment #611609 - Attachment is obsolete: true
Attachment #611609 - Flags: review?(mark.finkle)
Attachment #611651 - Flags: review?(wjohnston)
Attachment #611651 - Flags: review?(mark.finkle)
Attached patch patch v5Splinter Review
Removed status indicator and only show completed downloads as discussed in IRC.
Attachment #611651 - Attachment is obsolete: true
Attachment #611651 - Flags: review?(wjohnston)
Attachment #611651 - Flags: review?(mark.finkle)
Attachment #611662 - Flags: review?(wjohnston)
Attachment #611662 - Flags: review?(mark.finkle)
Blocks: 741655
Blocks: 741654
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?)
Attachment #611662 - Flags: review?(wjohnston) → review+
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).
Attachment #611662 - Flags: review?(mark.finkle) → review+
Will downloads in Firefox on ICS also show up in the system one? I think they should.
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)
(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.
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.
(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.
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.
Flags: in-litmus?(fennec)
Whiteboard: [strings][has patch] → [strings][has patch][QA+]
https://hg.mozilla.org/mozilla-central/rev/48cb615fbcb5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Blocks: 744025
Depends on: 747351
Depends on: 747388
Depends on: 747323
Download manager is available.
Verified fixed on: Firefox 15.0a1 (2012-05-22)
Device: LG Optimus 2X (Android 2.2.2)
Status: RESOLVED → VERIFIED
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
Flags: in-litmus?(fennec) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.