816318 - Use System download manager on GB+

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: WIP Path

I was curious how hard using the system download manager would be. Turns out, its pretty easy. Also, turns out the system download manager kinda sucks.

Pros:
* Runs in a service so download keep going if Fennec dies
* Users are used to looking for things here
* Native DM is better about handling metered connections than us
* Automagically get some upgrades (for instance fancy notifications) when new android versions are released

Cons:
* Have to wait for new Android versions to get upgrades
* No way to pause downloads (at least on my devices)
* Tapping the native notifications doesn't provide any useful feedback
* Native UI is awful about errors states. For instance, if there isn't enough space available the download just tries and fails over and over and over.
* We'll still have to maintain some of our own code in order to do things like clear private data correctly. The interaction with private browsing is a bit unclear as well. Maybe we just fall back to our old download manager when PB is enabled for this tab?

Overall, I'm not sure this is worth pursuing, but I wanted to put the patch up for anyone interested. WIP works for everything but private mode/clear private data.

Comment 1

[Thomas Stache]

Pro: the Android Download Manager manages to complete a download over a network interruption (e.g. switch from Wifi to 3G), while Firefox simply shows an error.

Comment 3

[Aaron Train [:aaronmt]]

Do want.

Comment 5

[Wesley Johnston (:wesj)]

Josh is going to look at this. We may need some UX feedback. Do we keep our DM in cases where we use Android's?

Comment 6

[Josh Dover [:jdover]]

Attachment: Use android download manager

This patch uses Android's download manager when 1) the user is not in a private tab and 2) the device is 2.3+. Otherwise, we default to Fennec's built in download manager. Downloads made using the system manager DO NOT show up in about:downloads

Comment 7

[Josh Dover [:jdover]]

Attachment: Open system download manager from menu

Comment 8

[Christian Ascheberg]

(In reply to Josh Dover from comment #6)
> Created attachment 8371110 [details] [diff] [review]
> Use android download manager
> 
> This patch uses Android's download manager when 1) the user is not in a
> private tab [...]

You should consider to use the native download manager for private downloads as well, see bug 927418 and comments. The main point is that files downloaded in private browsing mode are supposed to be kept on the device permanently, just as regular downloads are.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8371110 [details] [diff] [review]
Use android download manager

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

::: mobile/android/base/DownloadHelper.java
@@ +31,5 @@
> +
> +    private DownloadHelper(Context context) {
> +        mContext = context;
> +        GeckoAppShell.getEventDispatcher().registerEventListener("Download:Start", this);
> +        GeckoAppShell.getEventDispatcher().registerEventListener("Download:ShouldUseNative", this);

Lets not use a message like this.  I don't want javascript to care...

@@ +59,5 @@
> +    }
> +
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        Log.d(LOGTAG, "recieved message: " + message.toString());

Remove this logging

@@ +84,5 @@
> +                DownloadManager.Request.VISIBILITY_VISIBLE_NOTIFY_COMPLETED);
> +
> +            DownloadManager manager =
> +              (DownloadManager) mContext.getSystemService(Context.DOWNLOAD_SERVICE);
> +            Log.d(LOGTAG, "Starting download: " + request.toString());

And this.

@@ +110,5 @@
> +    private void sendShouldUseNative() {
> +        try {
> +            JSONObject response = new JSONObject();
> +            response.put("useNative", Build.VERSION.SDK_INT >= Build.VERSION_CODES.GINGERBREAD);
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Download:Native", response.toString()));

I'm going to throw out an idea here that mfinkle may love or hate. We could accomplish most of what you're doing by implementing our own nsIDownloadManager for Fennec:

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIDownloadManager?redirectlocale=en-US&redirectslug=nsIDownloadManager

We should be able to flag Gecko to only register/use it on Android 2.3+ by setting a flag in the chrome.manifest: https://developer.mozilla.org/en-US/docs/Chrome_Registration#osversion
A lot of the DownloadManager has been deprecated awhile ago. Also, a lot of these are just for UI, which we wouldn't really have, so we'd really only need:

nsIDownload addDownload(aDownloadType, aSource, aTarget, aDisplayName, aMIMEInfo, aStartTime, aTempFile, aCancelable, aIsPrivate);
// Do we have enough info in addDownload to actually get the headers we need...
// Cancel and remove are the same thing to Android...
void cancelDownload(in unsigned long aID);
void removeDownload(in unsigned long aID);
defaultDownloadsDirectory
userDownloadsDirectory

// ---------------------------------Things we can't implement AFAICT
// void pauseDownload(in unsigned long aID);
// void removeDownloadsByTimeframe(in long long aBeginTime, in long long aEndTime);
// void cleanUp();
// void cleanUpPrivate();
// void resumeDownload(in unsigned long aID);
// void retryDownload(in unsigned long aID);
// readonly attribute nsISimpleEnumerator activePrivateDownloads;
// void addPrivacyAwareListener(in nsIDownloadProgressListener aListener);



// --------------- Things that are likely unnecessary (and hard to implement?)
// void addListener(in nsIDownloadProgressListener aListener);
// nsIDownload getDownload(in unsigned long aID);
// void getDownloadByGUID(in ACString aGUID, in nsIDownloadManagerResult aCallback);
// void removeListener(in nsIDownloadProgressListener aListener);
// activeDownloadCount
// activePrivateDownloadCount;
// activeDownloads
// canCleanUpPrivate;
// canCleanUp
// DBConnection
// privateDBConnection;

::: mobile/android/chrome/content/downloads.js
@@ +279,5 @@
>      Downloads.updateNotification(aDownload, new DownloadProgressNotifOptions(aDownload, [PAUSE_BUTTON, CANCEL_BUTTON]));
>    },
>  
> +  onStateChange: function(aWebProgress, aRequest, aState, aStatus, aDownload) {
> +    if (!aDownload.isPrivate && Downloads.useNativeDownloadManager) {

Looking at the Android docs I saw:
https://developer.android.com/reference/android/app/DownloadManager.Request.html#setVisibleInDownloadsUi%28boolean%29

Maybe we can use the Native system but just not make these visible?

Comment 10

[Wesley Johnston (:wesj)]

Also cc'ing ibarlow to see what he'd like to do with private downloads here.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 8371111 [details] [diff] [review]
Open system download manager from menu

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

Once the private downloads stuff is figured out, this looks good :)

::: mobile/android/base/BrowserApp.java
@@ +2253,5 @@
> +            if (Tabs.getInstance().getSelectedTab().isPrivate() ||
> +                    Build.VERSION.SDK_INT >= Build.VERSION_CODES.GINGERBREAD) {
> +                Tabs.getInstance().loadUrlInTab(AboutPages.DOWNLOADS);
> +            } else {
> +                startActivity(GeckoAppShell.getIntentForActionString("android.intent.action.VIEW_DOWNLOADS"));

Use DownloadManager.ACTION_VIEW_DOWNLOADS instead of the string here.

Comment 12

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

(In reply to Wesley Johnston (:wesj) from comment #9)

> I'm going to throw out an idea here that mfinkle may love or hate. We could
> accomplish most of what you're doing by implementing our own
> nsIDownloadManager for Fennec:
> 
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIDownloadManager?redirectlocale=en-US&redirectslug=nsIDownloadManager
> 
> We should be able to flag Gecko to only register/use it on Android 2.3+ by
> setting a flag in the chrome.manifest:
> https://developer.mozilla.org/en-US/docs/Chrome_Registration#osversion
> A lot of the DownloadManager has been deprecated awhile ago. Also, a lot of
> these are just for UI, which we wouldn't really have, so we'd really only
> need:
> 
> nsIDownload addDownload(aDownloadType, aSource, aTarget, aDisplayName,
> aMIMEInfo, aStartTime, aTempFile, aCancelable, aIsPrivate);
> // Do we have enough info in addDownload to actually get the headers we
> need...
> // Cancel and remove are the same thing to Android...
> void cancelDownload(in unsigned long aID);
> void removeDownload(in unsigned long aID);
> defaultDownloadsDirectory
> userDownloadsDirectory
> 
> // ---------------------------------Things we can't implement AFAICT
> // void pauseDownload(in unsigned long aID);
> // void removeDownloadsByTimeframe(in long long aBeginTime, in long long
> aEndTime);
> // void cleanUp();
> // void cleanUpPrivate();
> // void resumeDownload(in unsigned long aID);
> // void retryDownload(in unsigned long aID);
> // readonly attribute nsISimpleEnumerator activePrivateDownloads;
> // void addPrivacyAwareListener(in nsIDownloadProgressListener aListener);
> 
> 
> 
> // --------------- Things that are likely unnecessary (and hard to
> implement?)
> // void addListener(in nsIDownloadProgressListener aListener);
> // nsIDownload getDownload(in unsigned long aID);
> // void getDownloadByGUID(in ACString aGUID, in nsIDownloadManagerResult
> aCallback);
> // void removeListener(in nsIDownloadProgressListener aListener);
> // activeDownloadCount
> // activePrivateDownloadCount;
> // activeDownloads
> // canCleanUpPrivate;
> // canCleanUp
> // DBConnection
> // privateDBConnection;

I think this could end badly. There is too much that would not be implemented. That means it's not a good fit. If Gecko had a small simple interface for basic downloads, and a large richer interface for fancy stuff, I would be happier about implementing the simple interface.

Comment 13

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

(In reply to Christian Ascheberg from comment #8)
> (In reply to Josh Dover from comment #6)
> > Created attachment 8371110 [details] [diff] [review]
> > Use android download manager
> > 
> > This patch uses Android's download manager when 1) the user is not in a
> > private tab [...]
> 
> You should consider to use the native download manager for private downloads
> as well, see bug 927418 and comments. The main point is that files
> downloaded in private browsing mode are supposed to be kept on the device
> permanently, just as regular downloads are.

I agree with this. We use the Gecko downloader for Private Downloads now, IIRC. No reason to not use the system downloader.

Comment 14

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

(In reply to Josh Dover from comment #6)
> Created attachment 8371110 [details] [diff] [review]
> Use android download manager
> 
> This patch uses Android's download manager when 1) the user is not in a
> private tab and 2) the device is 2.3+. Otherwise, we default to Fennec's
> built in download manager. Downloads made using the system manager DO NOT
> show up in about:downloads

I get disappointed that we still need our own downloader. Two separate systems that very greatly in capabilities in the product feels crappy. Not much you can do about it I guess....

Unless we make a simple Java based activity for 2.2 and 2.3 - Something that is comparable to what the system downloader does. Maybe even lifted from the 2.3 source?

Comment 15

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

(In reply to Mark Finkle (:mfinkle) from comment #14)

> Unless we make a simple Java based activity for 2.2 and 2.3 - Something that
> is comparable to what the system downloader does. Maybe even lifted from the
> 2.3 source?

This also gives us downloads in the background, even if Fennec is killed. The Gecko-based downloader can't do that.

Comment 16

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #10)
> Also cc'ing ibarlow to see what he'd like to do with private downloads here.

I think we should treat them like normal downloads and put them all in the Android download manager. I do not want to keep a fennec download manager around just for the purpose of catching private files -- seems more confusing than valuable. On about:privatebrowsing, we even call out that "files you download will be kept".

Comment 17

[Josh Dover [:jdover]]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> (In reply to Josh Dover from comment #6)
> > Created attachment 8371110 [details] [diff] [review]
> > Use android download manager
> > 
> > This patch uses Android's download manager when 1) the user is not in a
> > private tab and 2) the device is 2.3+. Otherwise, we default to Fennec's
> > built in download manager. Downloads made using the system manager DO NOT
> > show up in about:downloads
> 
> I get disappointed that we still need our own downloader. Two separate
> systems that very greatly in capabilities in the product feels ****. Not
> much you can do about it I guess....
> 
> Unless we make a simple Java based activity for 2.2 and 2.3 - Something that
> is comparable to what the system downloader does. Maybe even lifted from the
> 2.3 source?

Putting together and maintaining a Java Download activity for an API version that I imagine will lose support in the near future (it accounts for ~ 1% of Android users) seems a bit overkill to me, and it also doesn't change the fact that we have two download mechanisms.

Comment 18

[Wesley Johnston (:wesj)]

We looked at Froyo users and it will be a bit before we can deprecate it entirely. I think to make this decision we need tolook into using a Java UI on Froyo a bit more. i.e. There are some classes in Froyo that deal with this stuff (for instance, a Download form the stock browser goes through:

http://androidxref.com/2.2.3/xref/packages/apps/Browser/src/com/android/browser/BrowserActivity.java#2845 to http://androidxref.com/2.2.3/xref/packages/providers/DownloadProvider/src/com/android/providers/downloads/DownloadProvider.java#363 to
androidxref.com/2.2.3/xref/packages/providers/DownloadProvider/src/com/android/providers/downloads/DownloadService.java

i.e. adding something to that database forces it to start downloading. Can we access that database and add our own downloads on Froyo? What does the stock browser use for UI and can we use it? Can you find a Froyo device and try to look into this a bit josh?

Comment 19

[Josh Dover [:jdover]]

After some investigation it looks like we *may* be able to use the DownloadProvider in Froyo that Wes linked to in Comment 18. Unfortunately, I do not have a Froyo device to test this on right now, and Fennec is crashing (even without my changes) on emulator on my computer.

Even if we can get this hack to work, I am nervous about a couple of things with this approach. First, I think we will inevitably have device specific bugs when hacking into the OS like this which will bring more work to support older devices when we have an implementation that already works. Second, Wes mentioned to me that Tor users will probably want to keep their download activity within Fennec, meaning we will probably be keeping around our old download code anyways (hidden behind a pref).

Comment 20

[Josh Dover [:jdover]]

After discussion in mobile-firefox-dev, the team prefers to keep our old code for 2.2 devices. Just need to make the fix in Comment 16 to treat private downloads the same as normal downloads.

Comment 21

[Christian Ascheberg]

Do you migrate existing finished downloads to show up in the Android download manager?

Comment 22

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

I saw some chatter on IRC to add a preference, in about:config, to allow people to switch between the System or Gecko download managers. This is not the direction we want to go. The Gecko DM is not long for this world. When we drop Froyo support, we will drop the Gecko DM.

I would only entertain options to use Gecko to do the actual download, but still register the download file with the System DM once the download was completed. However, even that is out of scope for this bug.

Newman convinced me that having Gecko pass the download action to Java was the best approach. Java can decide to send to the System DM or to pass back to Gecko (for Froyo). Essentially, if you ask "How would GeckoView allow embedding applications download files" it has to fall out this way.

Ideally, this would be exposed as a onDownload(url, ...) type of API in GeckoView. In order to make that happen, Gecko needs to tell Java a download is ready to happen. Java can tell Gecko "thanks, i'll handle it from here" or "thanks, but you take this one" or "abort the whole thing".

Comment 23

[Wesley Johnston (:wesj)]

The words "Gecko Download Manager" are causing confusion here. They can refer to our download notification UI. about:downloads. The use of Downloads.jsm. etc.

But we're trying to keep things extremely simple here so that when we don't have to write ANY new code to support Froyo. i.e. If the pref is set, we use the exact same code path we're using right now. If its not set, we use the new code. We're doing that because its the safest and simplest way to guarantee were not regressing things on older platforms.

Writing a ladder that required us to send every requested download (including all its headers) to Java and then back (where we'd kick it off again, hopefully with some tricks to avoid a loop) would significantly increase the risk of regressions from these patches.

If we do keep something around, I'd be fine ripping it down to a bare minimum. I don't care if we have an about:downloads in the tree, or even notifications when downloads are ongoing. i.e we can rip out ALL of the Fennec download code once we're done supporting Froyo. A pref (if we kept it) would just keep us from cancelling and sending the download to Java. i.e. Gecko would download things silently. Users who want a UI in addition can write an add-on.

I think I would say that having a pref that made us download and then pushed things to the system download manager doesn't make a ton of sense, since the people who would flip such a pref are likely the same people who don't want Android to know what they're downloading. But I also agree that's out of scope here.

Comment 24

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

Let's avoid a pref. Since we need to do this at runtime, a pref doesn't help too much. Where would the pref be checked?

We can already do an API version check from C++. Does this check need to happen in C++?

Comment 25

[Wesley Johnston (:wesj)]

Marking for sec and privacy review. This will potentially expose our downloads to any app that has access to the system download manager feature. It also exposes us to any security bugs in the system download manager.

Comment 26

[Josh Dover [:jdover]]

Attachment: 01 - Use Android's DownloadManager on supported devices

Comment 27

[Josh Dover [:jdover]]

Attachment: 02 - Open the system download manager when tapping download menu

Comment 28

[Wesley Johnston (:wesj)]

Comment on attachment 8390689 [details] [diff] [review]
01 - Use Android's DownloadManager on supported devices

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

Some ideas for how I think this should look. If Ally is around tomorrow, can you ask her what you need to do for privacy/sec review here as well (I feel like that's changed recently and I may not have done it right here).

::: mobile/android/base/DownloadHelper.java
@@ +21,5 @@
> +/*
> + * Recieves requests from Gecko via the 'Download:Start' to start downloads with Android's
> + * DownloadManager. Requests should contain the source URI and all request headers.
> + */
> +public final class DownloadHelper implements GeckoEventListener {

Since we're running this way, I think maybe we should implement this in GeckoView. i.e. in GeckoView.java we can add a setDownloadListener(DownloadListener listener) method, just like WebView. Then GeckoView will listen for these events. This class can implement DownloadListener instead. Unfortunately, we still need a way to tell Gecko what to do, so our Listener interface might have to return a boolean to indicate whether or not we should cancel the download.

@@ +57,5 @@
> +    }
> +
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        Log.d(LOGTAG, "recieved message: " + message.toString());

Remove this, or hide it behind a bool/flag somehow.

@@ +64,5 @@
> +                useGeckoDownload(message, false);
> +                startDownloadWithSystemManager(message);
> +            } else {
> +                useGeckoDownload(message, true);
> +            }

So if we flip to messaging.jsm, this will be something like:

JSONObject response = new JSONObject();
if (shouldUseNative()) {
  startDownloadWithSystemManager(message);
  response.put("cancel", true);
} else {
  response.put("cancel", false);
}
EventDispatcher.sendResponse(message, response);

@@ +85,5 @@
> +                DownloadManager.Request.VISIBILITY_VISIBLE_NOTIFY_COMPLETED);
> +
> +            DownloadManager manager =
> +              (DownloadManager) mContext.getSystemService(Context.DOWNLOAD_SERVICE);
> +            Log.d(LOGTAG, "Starting download: " + request.toString());

And remove/pref this.

::: mobile/android/chrome/content/downloads.js
@@ +219,5 @@
>    this.buttons = aButtons;
>  }
>  
> +var DownloadHandler = {
> +  PROCESSING: "PROCESSING",

I'd just make this an int. Also, a brief comment explaining these guys would be nice.

@@ +250,5 @@
> +    };
> +
> +    let msg = {type: "Download:Start"};
> +    msg.id = aDownload.id;
> +    msg.uri = aDownload.source.spec;

I'd just use one object here. i.e.:

let msg = {
  type: ...
  id: ...
  uri: ...
}

@@ +257,5 @@
> +    let visitor = new requestHeaderVisitor();
> +    aRequest.visitRequestHeaders(visitor);
> +    msg.headers = visitor.getHeaders();
> +
> +    Services.androidBridge.handleGeckoMessage(JSON.stringify(msg));

We should be able to use Messaging.jsm to make this a little nicer. i.e. import it in here and then you can just use:

sendMesssageToJava(msg, function(data) {
    // parse data.
});

@@ +266,5 @@
> +      let message = JSON.parse(aData);
> +      this._setStatus(message.id, message.useGecko);
> +
> +      let download = this._getDownload(message.id);
> +      if (message.useGecko) {

I'd just make this parameter "cancel" or something and avoid talking about Gecko.

@@ +285,5 @@
> +        array = this._actionQueue[aDownload.id];
> +      }
> +
> +      array.push(callback);
> +      return;

Remove this.

@@ +287,5 @@
> +
> +      array.push(callback);
> +      return;
> +    } else if (this._shouldDownload(aDownload)) {
> +      this._processActions(aDownload);

How do we get here exactly. i.e. Things are in the queue, but we're not processing this download anymore?

@@ +303,5 @@
> +    }
> +  },
> +
> +  _setStatus: function(id, useGecko) {
> +    this._downloads[id] = useGecko;

I keep thinking there must be a way to have one list here... Maybe a list of objects... I'm not sure that's any better, but I think having two is a bit confusing.

@@ +311,5 @@
> +    return this._downloads[aDownload.id];
> +  },
> +
> +  _isProcessingDownload: function(aDownload) {
> +    return this._downloads[aDownload.id] == this.PROCESSING;

===

@@ +324,5 @@
> +      aDownload.cancel();
> +      aDownload.remove();
> +    } else if (aDownload.percentComplete == 100) {
> +      // Small downloads may complete before we were able to cancel it.
> +      aDownload.remove();

:( Do we need to delete the file here.... Maybe we should pause the downloads and just restart them when we hear back from Java. I wonder if that would fix some of these issues for us (i.e. the notification problems...)

Comment 29

[Wesley Johnston (:wesj)]

Comment on attachment 8390691 [details] [diff] [review]
02 - Open the system download manager when tapping download menu

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

::: mobile/android/base/BrowserApp.java
@@ +2336,5 @@
>          if (itemId == R.id.downloads) {
> +            if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.GINGERBREAD) {
> +                Tabs.getInstance().loadUrlInTab(AboutPages.DOWNLOADS);
> +            } else {
> +                startActivity(GeckoAppShell.getIntentForActionString("android.intent.action.VIEW_DOWNLOADS"));

All this does is return new Intent(aAction); (with a null check for the string). Lets just do that here.

Comment 30

[Josh Dover [:jdover]]

Attachment: 01 - Use Android's DownloadManager on supported devices

(In reply to Wesley Johnston (:wesj) from comment #28)
> ::: mobile/android/base/DownloadHelper.java
> @@ +21,5 @@
> > +/*
> > + * Recieves requests from Gecko via the 'Download:Start' to start downloads with Android's
> > + * DownloadManager. Requests should contain the source URI and all request headers.
> > + */
> > +public final class DownloadHelper implements GeckoEventListener {
> 
> Since we're running this way, I think maybe we should implement this in
> GeckoView. i.e. in GeckoView.java we can add a
> setDownloadListener(DownloadListener listener) method, just like WebView.
> Then GeckoView will listen for these events. This class can implement
> DownloadListener instead. Unfortunately, we still need a way to tell Gecko
> what to do, so our Listener interface might have to return a boolean to
> indicate whether or not we should cancel the download.

Could you explain this more? Why would GeckoView have to be the one to reply to the message? Couldn't GeckoView send the JSONObject to DownloadListener and it could reply to it from there?
 
> @@ +287,5 @@
> > +
> > +      array.push(callback);
> > +      return;
> > +    } else if (this._shouldDownload(aDownload)) {
> > +      this._processActions(aDownload);
> 
> How do we get here exactly. i.e. Things are in the queue, but we're not
> processing this download anymore?

This is after the Java has told us not to cancel the download, all of the notification updates are still going to be sent through queueAction, so (on the first call) we should process the actions in the queue, and then call the callback. After the first call, the queue will be empty and _processActions will return without doing anything.

> 
> @@ +324,5 @@
> > +      aDownload.cancel();
> > +      aDownload.remove();
> > +    } else if (aDownload.percentComplete == 100) {
> > +      // Small downloads may complete before we were able to cancel it.
> > +      aDownload.remove();
> 
> :( Do we need to delete the file here.... Maybe we should pause the
> downloads and just restart them when we hear back from Java. I wonder if
> that would fix some of these issues for us (i.e. the notification
> problems...)

Added the deleting of the file. And the documentation of resume(), pause(), cancel() and remove() don't specify how those should affect downloads in different states and I was getting errors (NS_ERROR_NOT_EXPECTED) when trying to do something like cancel a download that paused previously. This seemed to be the only way to do it, also this does cut down on download times for files because we do not delay starting the download. However, it does mean that for downloads that end up using Android's DM, we will have a small amount of data downloaded by Gecko before the DL is started in Android, only to be deleted later. But I imagine this to be no more than a few kilobytes as this message passing is fairly quick. 

Also, I refactored things a bit though so there is no longer a check on if the percentComplete field.

Comment 31

[Josh Dover [:jdover]]

Attachment: 02 - Open the system download manager when tapping download menu

Comment 32

[Josh Dover [:jdover]]

The issues I mentioned in Comment #30 could probably simplified if we were using Downloads.jsm

Comment 33

[Wesley Johnston (:wesj)]

(In reply to Josh Dover [:jdover] from comment #30)
> Could you explain this more? Why would GeckoView have to be the one to reply
> to the message? Couldn't GeckoView send the JSONObject to DownloadListener
> and it could reply to it from there?

Hmm... In the end, I just want the GeckoView API to look right. That could mean GeckoView gets the message and forwards it, or you could have a class list this process the JSON into an object(s) that we pass to the DownloadListener registered in GeckoView.

We already do some similar things here for Prompts:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoView.java#182

but all that processing is in GeckoView. Also, looking at the WebView DownloadListener, it doesn't look like it passes enough information for us:

onDownloadStart(String url, String userAgent, String contentDisposition, String mimetype, long contentLength)

i.e. no headers, so we may have to vary in that method signature a bit.

Comment 34

[Josh Dover [:jdover]]

Attachment: 01 - Use Android's DownloadManager on supported devices

Comment 35

[Josh Dover [:jdover]]

Attachment: 02 - Open the system download manager when tapping download menu

Comment 36

[Wesley Johnston (:wesj)]

Comment on attachment 8394428 [details] [diff] [review]
01 - Use Android's DownloadManager on supported
 devices

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

This looks really good. Just one question about when we're calling registerDownload in JS. I'm just marking as - to make sure you see and respond.

::: mobile/android/base/DownloadHelper.java
@@ +61,5 @@
> +    public boolean onDownloadStart(String url, Map<String, String> headers) {
> +        if (shouldUseNative()) {
> +            startDownloadWithSystemManager(url, headers);
> +            return true;
> +        } else {

Remove this else and just return false.

@@ +72,5 @@
> +        // Do nothing, about:downloads will display the downloads
> +    }
> +
> +    // Once we use an intialized GeckoView in fennec, we can remove this and allow GeckoView to
> +    // handle the messages and use the DownloadListener interface.

We already use a GeckoView. Why can't we initialize this there?

@@ +83,5 @@
> +                    JSONUtils.parseObjectToMap(message.optJSONObject("headers"));
> +
> +                final boolean cancelGeckoDownload = onDownloadStart(url, headers);
> +                final JSONObject response = new JSONObject();
> +                response.put("id", message.getInt("id"));

Do you really need an ID here?

@@ +85,5 @@
> +                final boolean cancelGeckoDownload = onDownloadStart(url, headers);
> +                final JSONObject response = new JSONObject();
> +                response.put("id", message.getInt("id"));
> +                response.put("cancel", cancelGeckoDownload);
> +                EventDispatcher.sendResponse(message, response);

We haven't been doing it, but it would be bad if we never sent a response to a message actually. Maybe this should go in a finally block...

::: mobile/android/base/util/JSONUtils.java
@@ +57,5 @@
>              throw new IllegalArgumentException(name + "=" + uuidString, e);
>          }
>      }
> +
> +    public static Map<String, String> parseObjectToMap(final JSONObject object) throws JSONException {

Hmm... This only works for String->String maps, but I think that's pretty useful (just a deceptive name :) ). We could build this out someday to at least take a Class argument and let you build String -> Class maps....

::: mobile/android/chrome/content/downloads.js
@@ +220,5 @@
>    this.progress = aDownload.percentComplete;
>    this.buttons = aButtons;
>  }
>  
> +var DownloadHandler = {

A brief description of what this class does seems like it would be helpful.

@@ +224,5 @@
> +var DownloadHandler = {
> +  _downloads: {},
> +
> +  registerDownload: function(aRequest, aDownload) {
> +    if (this._shouldCleanupDownload(aDownload)) {

This is the only bit that has me confused. _shouldCleanup basically checks if this was canceled. If it was, we delete it from the list. That would seem to mean we're calling this too often, but if we're doing that, deleting it scares me a bit? I see that we call this in every stateChange call, can we make that more specific?

@@ +225,5 @@
> +  _downloads: {},
> +
> +  registerDownload: function(aRequest, aDownload) {
> +    if (this._shouldCleanupDownload(aDownload)) {
> +      return delete this._downloads[aDownload.id];

delete apparently does return something, but I don't think you want to return it here.

@@ +226,5 @@
> +
> +  registerDownload: function(aRequest, aDownload) {
> +    if (this._shouldCleanupDownload(aDownload)) {
> +      return delete this._downloads[aDownload.id];
> +    } if (this._isProcessingDownload(aDownload) || !aRequest || !aDownload) {

This maybe should log something. Not the download name, but some sort of error message. Getting here without a Request or a Download would mean something went very wrong. In fact, maybe it should fall through and throw?

@@ +231,5 @@
> +      return;
> +    }
> +
> +    this._downloads[aDownload.id] = {
> +      cancel: undefined,

Just don't put this if its undefined...

@@ +262,5 @@
> +    };
> +
> +    sendMessageToJava(msg, aData => {
> +      let message = JSON.parse(aData);
> +      this._setCancel(message.id, message.cancel);

Here aDownload.id should be useable and correct.

@@ +358,4 @@
>    },
>  
> +  onStateChange: function(aWebProgress, aRequest, aState, aStatus, aDownload) {
> +    DownloadHandler.registerDownload(aRequest, aDownload);

Should this be moved into the DOWNLOAD_QUEUED bit below?

Comment 37

[Josh Dover [:jdover]]

Attachment: 01 - Use Android's DownloadManager on supported devices

(In reply to Wesley Johnston (:wesj) from comment #36)
> @@ +72,5 @@
> > +        // Do nothing, about:downloads will display the downloads
> > +    }
> > +
> > +    // Once we use an intialized GeckoView in fennec, we can remove this and allow GeckoView to
> > +    // handle the messages and use the DownloadListener interface.
> 
> We already use a GeckoView. Why can't we initialize this there?

It looks like there is more work to move some of the initialization that GeckoView does
out of Fennec itself:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoView.java#59

> 
> Hmm... This only works for String->String maps, but I think that's pretty
> useful (just a deceptive name :) ). We could build this out someday to at
> least take a Class argument and let you build String -> Class maps....

I tried to think of simple way to do with this (I have a limited knowledge of generics)
without using any reflection and didn't see any obvious way. The challenge is that
many of the types you may want to cast to are primitives that don't have Class, so you
would probably have to use the Java equivalents (ie Integer, Double, etc.).

> 
> @@ +224,5 @@
> > +var DownloadHandler = {
> > +  _downloads: {},
> > +
> > +  registerDownload: function(aRequest, aDownload) {
> > +    if (this._shouldCleanupDownload(aDownload)) {
> 
> This is the only bit that has me confused. _shouldCleanup basically checks
> if this was canceled. If it was, we delete it from the list. That would seem
> to mean we're calling this too often, but if we're doing that, deleting it
> scares me a bit? I see that we call this in every stateChange call, can we
> make that more specific?

Greatly simplified by calling only in DOWNLOAD_QUEUED

> 
> This maybe should log something. Not the download name, but some sort of
> error message. Getting here without a Request or a Download would mean
> something went very wrong. In fact, maybe it should fall through and throw?
> 

I was able to remove this by calling registerDownload only in DOWNLOAD_QUEUED.

Comment 38

[Wesley Johnston (:wesj)]

Comment on attachment 8398180 [details] [diff] [review]
01 - Use Android's DownloadManager on supported devices

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

Great. I think while we wait for sec reviews, we could try landing this as a Nightly only feature. We could update shouldUseNative to return false if !AppConstants.RELEASE_BUILD. You mind writing a separate patch?

Comment 39

[Broton]

What's the status of this bug.

Comment 40

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

(In reply to Broton from comment #39)
> What's the status of this bug.

This bug is on hold until we drop support for older versions of Android that would require us to support two different code paths. We are also still considering that the Gecko downloader gives us more features.

That said, I think there is a separate bug about adding the downloaded file into the System Downloader, after it has been completely downloaded by Gecko. That would allow people to see Firefox downloads in the System Downloader too.

Comment 41

[Christian Ascheberg]

(In reply to Mark Finkle (:mfinkle) from comment #40)
> That said, I think there is a separate bug about adding the downloaded file
> into the System Downloader, after it has been completely downloaded by
> Gecko. That would allow people to see Firefox downloads in the System
> Downloader too.

That is bug 927464, currently WONTFIX because of this bug, see bug 927464 comment 4.

Comment 42

[Wesley Johnston (:wesj)]

Attachment: Patch

This just adds downloads to the system database for us. There's no additional data leakage (i.e. just the file name and mimetype) than we have from our normal work to notify the media scanner when we download something (in fact, I moved that behind a pref now, and its private browsing aware now).

It does require a permission bump (to not show the notification). We could potentially remove that bump and just NOT show our own downloadComplete notifications, but its nice to have some consistency....

Comment 43

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

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

Can we split this patch into:
1. Stuff that requires a permission bump
2. Stuff that does not

Cause it seems like we could land some of the addToRecentDocs changes without the bump, right?

Comment 44

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/e34214539e10
https://hg.mozilla.org/integration/fx-team/rev/71978c3737e0

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e34214539e10
https://hg.mozilla.org/mozilla-central/rev/71978c3737e0

Comment 46

[Aaron Train [:aaronmt]]

*klaxon*

This landed with a permission change and no Nightly only check, assuming we want to continue to 'group' up permission changes and keep it on Nightly for the time being.

Comment 47

[Christian Ascheberg]

I would like to get more information on this bug. As this patch just adds completed downloads to the Android download manager (was also filed as bug 927464), are there still further plans to remove Firefox's own download manager and completely use Androids?

Also, downloaded files should not be treated differently during private browsing, see bug 927418.

Comment 48

[Wesley Johnston (:wesj)]

I think there's pretty steep resistance to using Android's networking stack. We could make the Tools->Downloads menu item open the android download manager instead of our own (on v11+ devices).

I don't have strong preferences about the private browsing bit. Just wanted to be safe (and taking a bit of a queue from the windows code). These are still "exposed" to any app with permissions to read the downloads dir though. We could just turn off the media scan bit if in private mode. Or we could do nothing.

Feel free to file bugs on both things if they are what you want? I'm trying to get this on the roadmap and we can track them against it.

Comment 49

[Ed Morley [:emorley]]

A description of why the permission request added here is required, is missing from:
https://support.mozilla.org/en-US/kb/how-firefox-android-use-permissions-it-requests

Please could you add it / let the SUMO team know what to put? :-)

Comment 51

[Wesley Johnston (:wesj)]

Pinging SUMO people so they can update this at the right time. Thanks :)

Comment 52

[Richard Newman [:rnewman]]

relnote-firefox, Wes?

Comment 53

[Kevin Brosnan [Ex-Mozilla]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Suggested wording]: Use Android Download manager to keep track of downloaded files
[Links (documentation, blog post, etc)]: this bug

Comment 54

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with Kevin's wording.

Comment 55

[Cristina Madaras, QA [:CristinaM]]

I'm testing this with a Nexus 4 (Android 4.4.4) and the downloaded files are displayed in both, Firefox Download Manager and Android's Download Manager. 
On Samsung Galaxy R (Android 2.3.4) the downloaded files are displayed only on Firefox Download Manager. Is this the expected result?

Comment 56

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

(In reply to Cristina Madaras, QA [:CristinaM] from comment #55)
> I'm testing this with a Nexus 4 (Android 4.4.4) and the downloaded files are
> displayed in both, Firefox Download Manager and Android's Download Manager. 
> On Samsung Galaxy R (Android 2.3.4) the downloaded files are displayed only
> on Firefox Download Manager. Is this the expected result?

Yes. The System Download Manager feature is supported on Android 3.1 and higher.

Comment 57

[Joni Chan]

Updated article here: https://support.mozilla.org/en-US/kb/how-firefox-android-use-permissions-it-requests/revision/84765. 

Waiting for technical check from Roland.

Comment 58

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

i think joni's article is 99% complete but i'd like to know the exact permission.

:wesj which permission is it exactly for the download manager?
I read
https://support.google.com/googleplay/answer/6014972?p=app_permissions&rd=1%29

but i couldn't figure out which permission was changed!

For the sake of completeness i'd like to know because i bet there are one or two power users out there who want to know too :-) !

Comment 59

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

The actual permission is: DOWNLOAD_WITHOUT_NOTIFICATION

I found some info here:
http://androidpermissions.com/permission/android.permission.DOWNLOAD_WITHOUT_NOTIFICATION

Comment 60

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

(In reply to Mark Finkle (:mfinkle) from comment #59)
> The actual permission is: DOWNLOAD_WITHOUT_NOTIFICATION
> 
> I found some info here:
> http://androidpermissions.com/permission/android.permission.
> DOWNLOAD_WITHOUT_NOTIFICATION

awesome thanks finkle, un-NeedInfo'ing wesj

Comment 61

[m3m3bvg]

(In reply to Wesley Johnston (:wesj) from comment #0)
> Created attachment 686326 [details] [diff] [review]
> WIP Path
> 
> I was curious how hard using the system download manager would be. Turns
> out, its pretty easy. Also, turns out the system download manager kinda
> sucks.
> 
> Pros:
> * Runs in a service so download keep going if Fennec dies
> * Users are used to looking for things here
> * Native DM is better about handling metered connections than us
> * Automagically get some upgrades (for instance fancy notifications) when
> new android versions are released
> 
> Cons:
> * Have to wait for new Android versions to get upgrades
> * No way to pause downloads (at least on my devices)
> * Tapping the native notifications doesn't provide any useful feedback
> * Native UI is awful about errors states. For instance, if there isn't
> enough space available the download just tries and fails over and over and
> over.
> * We'll still have to maintain some of our own code in order to do things
> like clear private data correctly. The interaction with private browsing is
> a bit unclear as well. Maybe we just fall back to our old download manager
> when PB is enabled for this tab?
> 
> Overall, I'm not sure this is worth pursuing, but I wanted to put the patch
> up for anyone interested. WIP works for everything but private mode/clear
> private data.



Pro:Firefox does not open every time I install an apk that I downloaded via Firefox.

Comment 62

[Jan Henning [:JanH]]

@m3m3bvg: Could that be bug 1075476 you're hitting?