746976 - Tapping on the download notification does not open the Download Manager

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Nightly/14.0a1 (2012-04-19)
Device: HTC Desire
OS: Android 2.2.2

Steps to reproduce:
1. Go to 1.usa.gov/deeXKM
2. Wait for the download to finish.
3. Slide down the notification bar.
4. Tap on the download notification.

Expected results:
The Download Manager is opened.

Actual results:
Nothing happens and the notification is lost.

Comment 1

[Adrian Tamas (:AdrianT)]

Attachment: logs

Comment 2

[Aaron Train [:aaronmt]]

Using your link, I download a PDF. It sounds like your Android install does not have a document viewer, or any other program associated with PDF. I get a 'Complete action using' prompt to open in Document Viewer. What happens if you try a different file with a different extension, such as MID (Music Player): http://goo.gl/bl0Uv ?

Comment 3

[Adrian Tamas (:AdrianT)]

This issue seems to be caused by Bug 746985. I think this can be duped after it. Tapping on the mid file download notification opens the Download Manager.

Comment 4

[Adrian Tamas (:AdrianT)]

(In reply to adrian tamas from comment #3)
> This issue seems to be caused by Bug 746985. I think this can be duped after
> it. Tapping on the mid file download notification opens the Download Manager.

Please disregard my last comment. It seems that Fennec is not recognizing mid as a supported file type which results in the Download Manager being opened - Bug 747323 was logged for the issue. 
Using goo.gl/hf4Y9 - a ppt file - the file is opened directly in OpenOffice. The issue is not seen using the Firefox XUL 12 Beta 6 build where for any file, supported or unsupported, when tapping the notification the Download Manager is opened.

Comment 5

[Wesley Johnston (:wesj)]

So to clarify, if there's no app registered to handle this file type, we should show the (Fennec) download manager.

Comment 6

[Cristian Nicolae (:xti)]

This issue is still reproducing on the latest Nightly build. Also it occurs on the latest Beta 15.b04 too.

--
Device: Galaxy Nexus
OS: Android 4.1.1

Comment 8

[Federico Paolinelli]

Hi there, is it ok to work on this bug?

Comment 9

[Wesley Johnston (:wesj)]

Actually, looking at this, I think this is entirely a JS bug. We'll need to modify this function:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/downloads.js#38

to check if the file can be opened or not. That calls file.launch() from javascript with something that's an nsIFile object. I think we'd rather pass the download to this method instead (see slightly below where we call openDownload). The download object is defined here:

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIDownload

and has an (optional) nsIMimeInfo property:

http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsIMIMEInfo.idl#127

which has a possibleLocalHandlers property that gives an array of nsILocalHandlerApp. If that list is zero length, I think we should just open the download manager (calling BrowserApp.addTab("about:downloads") should do the trick). if you're curious BrowserApp is a big object defined here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#200

====
If you want to get a bit more advanced, we should probably pass the download id to the download manager (I'd try opening "about:downloads?id=" + download.id), and we'll then have to modify the download manager to select the download and scroll it into view:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutDownloads.js

But that's probably a good follow up bug as well.

Comment 10

[Federico Paolinelli]

(In reply to Wesley Johnston (:wesj) from comment #9)
> Actually, looking at this, I think this is entirely a JS bug. We'll need to
> modify this function:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> downloads.js#38
> 
> to check if the file can be opened or not. That calls file.launch() from
> javascript with something that's an nsIFile object. I think we'd rather pass
> the download to this method instead (see slightly below where we call
> openDownload). The download object is defined here:
> 
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIDownload
> 
> and has an (optional) nsIMimeInfo property:
> 
> http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsIMIMEInfo.
> idl#127
> 
> which has a possibleLocalHandlers property that gives an array of
> nsILocalHandlerApp. If that list is zero length, I think we should just open
> the download manager (calling BrowserApp.addTab("about:downloads") should do
> the trick). if you're curious BrowserApp is a big object defined here:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#200
> 
> ====


I'am treading carefully on this stuff, but after spending (quite a lot of) time digging into the code, it seems to me that the implementation of mimeInfo for android does not implements getPossibleLocalHandlers:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/android/nsMIMEInfoAndroid.cpp#346

There are a couple of methods that end into geckoApp checking the package manager for possible applications, but no one is exported to the idl.

On the other hand, one could simply check if the f.launch() from downloads.js fails and open the downloads tab from there. 

Something like 

    try {
      f.launch();
    } catch (ex) {
      // open the tab
    }

The exception is thrown from GeckoAppShell if no activity able to process the file is found:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#972

Which results into a return NS_ERROR_FAILURE from the Launch() implementation which (I hope) results into the exception thrown in the js call. 

I do agree this may not be the only reason that leads to a failure in opening the file but still, opening the download tab is better than doing nothing even in this case.

Comment 11

[Wesley Johnston (:wesj)]

Sounds like a good plan! (this is the second time I've suggested getPossibleLocalHandlers and forgot that it isn't implemented).

Comment 12

[Federico Paolinelli]

I managed to have the downloads tab open, and to pass the id of the tapped download to the Downloads class (http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutDownloads.js#119)

Now, how do I perform the scrolling? I saw some methods of BrowserApp that end up calling _zoomToElement, but I don't know it this is the right direction.

Comment 13

[Wesley Johnston (:wesj)]

The simplest way to do it is probably to just get the row for the download (maybe we should remove the '_' and make Downloads._getElementForDownload(guid) "public") and the call element.scrollIntoView(). scrollIntoView puts things at the top or bottom of the screen and doesn't animate the transition, but here that's probably fine.

_zoomToElement will center the row (if possible) and animate the transition, if you think either of those are required.

Comment 14

[Federico Paolinelli]

Attachment: First submission of the patch

This is my first attempt. I also implemented what you suggested about opening to the right row.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 750134 [details] [diff] [review]
First submission of the patch

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

Looks pretty good to me. Just a few nits really.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +152,5 @@
>      let normalEntries = this.getDownloads({ isPrivate: false });
> +    
> +    let self = this;
> +    this._stepAddEntries(normalEntries, this._normalList, 1, function() {
> +      self._scrollToSelectedDownload();

You could remove this function entirely this and just pass in this._scrollToSelectedDownload.bind(this) instead.

@@ +387,5 @@
>  
> +  _stepAddEntries: function dv__stepAddEntries(aEntries, aList, aNumItems, aCallback) {
> +    
> +    if (aEntries.length == 0){
> +      if (aCallback !== undefined){

I'd just do if (aCallback) here. Also, we don't use { on single line if statements in javascript (that's only for true for our mobile javascript, the rest of the platform may have different rules... yeah, not great we know).

@@ +400,5 @@
>  
>      // Add another item to the list if we should; otherwise, let the UI update
>      // and continue later
>      if (aNumItems > 1) {
> +      this._stepAddEntries(aEntries, aList, aNumItems - 1, aEntriesAdded);

I think aEntriesAdded should be aCallback?

@@ +582,5 @@
> +    let id = query.substring(3);
> +    if (!id) {
> +      return;
> +    }    
> +    downloadElement = this._getElementForDownload(id);

check to make sure we found something i.e. if (downloadElement)

::: mobile/android/chrome/content/downloads.js
@@ +34,5 @@
>      Services.obs.addObserver(this, "xpcom-shutdown", true);
>      Services.obs.addObserver(this, "last-pb-context-exited", true);
>    },
>  
> +  openDownload: function dl_openDownload(aFileURI, aDownloadId) {

Might was well name this aGuid

@@ +39,4 @@
>      let f = this._getLocalFile(aFileURI);
>      try {
>        f.launch();
> +    } catch (ex) { 

Lets add a comment here explaining what we're doing. Something like "If launching the file failed, show it in the download manager."

Comment 16

[Federico Paolinelli]

Attachment: Second attempt

This is the patch with your suggestions. Hope I did not miss anything.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 751796 [details] [diff] [review]
Second attempt

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

Nice! and Thanks!

::: mobile/android/chrome/content/aboutDownloads.js
@@ +149,5 @@
>      this._stepAddEntries(privateEntries, this._privateList, privateEntries.length);
>  
>      // Add non-private downloads
>      let normalEntries = this.getDownloads({ isPrivate: false });
> +    

nit: Whitespace, here and below.

::: mobile/android/chrome/content/downloads.js
@@ +66,5 @@
>        observe: function (aSubject, aTopic, aData) {
>          if (aTopic == "alertclickcallback") {
>            if (aDownload.state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED) {
>              // Only open the downloaded file if the download is complete
> +            self.openDownload(aDownload.target.spec, aDownload.guid);

Looking at this now, we should probably just pass the download in, and then pull it apart in openDownload. That's trivial enough I'm fine doing it when we land this.

Comment 18

[Federico Paolinelli]

Attachment: Third version

I removed the whitespaces and passed aDownload to openDownload as suggested.

In case everything (hopefully) looks fine may I add the checkin needed keyword?

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 752837 [details] [diff] [review]
Third version

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

Great! Yeah, I'll add checkin-needed

Comment 20

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7eacb1aea9a4

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7eacb1aea9a4