Closed Bug 696911 Opened 9 years ago Closed 8 years ago

Hide download notification if the download opens on completion

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 24
Tracking Status
fennec - ---

People

(Reporter: Margaret, Assigned: fedepaol)

References

Details

(Whiteboard: [mentor=margaret][lang=js])

Attachments

(2 files, 4 obsolete files)

No description provided.
margaret, can you look at this?
Assignee: nobody → margaret.leibovic
Priority: -- → P3
tracking-fennec: --- → 11+
tracking-fennec: 11+ → ?
Assignee: margaret.leibovic → nobody
Brian or Margaret - can you mentor this bug?
tracking-fennec: ? → -
I remember looking into this and not immediately being sure how to fix it, but that was a very long time ago, so hopefully I'm smarter now.

We probably need to make a change somewhere around here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/downloads.js#63

Also, this code was written before we had a download manager, so maybe we can do a better job combining it into the download manager. Like, do we still need that "Cancel download?" prompt?
Whiteboard: [mentor=margaret][lang=js]
Hi,

I am currently in a Software engineering class and would like to solve this issue for my project in class.  Is this bug available to be worked on?
(In reply to Kyle from comment #4)
> Hi,
> 
> I am currently in a Software engineering class and would like to solve this
> issue for my project in class.  Is this bug available to be worked on?

I think so. You might also want to join #mobile on IRC
Assignee: nobody → kjspence
Status: NEW → ASSIGNED
Yes, this bug is available to work on. Thanks, Mavericks!

Kyle, have you got a Fennec build environment set up yet? If not, you'll want to check out this wiki page:
https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec

I'm going to be going on vacation tomorrow, so I won't be around to help for over a week, but if you join #mobile on irc.mozilla.org, you should definitely be able to find help, especially during working hours in PST.
Yes,  I set up the environment and did a test build last night.  Is there any other tips I should know?
Hello, I'm Kyle's partner and just thought I should introduce myself.
Kyle and Aaron, I'm back from vacation now, so I can help out. Have you made any progress on this bug? Let me know if there are any issues you're running into!
Hi Margaret. I hope your vacation went well.  I have the build environment set up but have been having some trouble finding the bug in the code.  We have replicated the bug on an android device.  I have looked in the file mentioned in comment 3 as well as the "http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp" file.  I know that I need to look for a section that is setting the download notification image to show up but I cannot find it.  I would love to get a patch in sometime today or tomorrow.  Are you available to chat on IRC? I will be on #mobile with nick bug8.
Now a Download Finished notification does not show at all.  Could not find where the open on finish choice is being used.
Attachment #708016 - Flags: review+
Attachment #708016 - Flags: feedback+
Comment on attachment 708016 [details] [diff] [review]
Removed "Download Finished" notification from appearing since downloads are opening on completion WIP

When you upload a patch for review, you want to set review? with a reviewer specified.

Giving a review+ means that you reviewed the patch :)
Attachment #708016 - Flags: review?(margaret.leibovic)
Attachment #708016 - Flags: review+
Attachment #708016 - Flags: feedback+
Comment on attachment 708016 [details] [diff] [review]
Removed "Download Finished" notification from appearing since downloads are opening on completion WIP

Actually, switching to feedback? since you said this is a WIP.

(Things with review+ are ready to land in the tree, so for things that aren't ready to land, asking for feedback instead is the thing to do.)
Attachment #708016 - Flags: review?(margaret.leibovic) → feedback?(margaret.leibovic)
Comment on attachment 708016 [details] [diff] [review]
Removed "Download Finished" notification from appearing since downloads are opening on completion WIP

Kyle and I talked about this on IRC, and we agreed this isn't the correct approach because we still want the notification to show when the download doesn't automatically open.

What we need to do is figure out if the download was opened, and decide not to show the notification if that's the case. I wonder if there's a notification we can listen for that lets us know that the completed download was opened. Or if there's a way to see if the given file type would open automatically or not.
Attachment #708016 - Flags: feedback?(margaret.leibovic) → feedback-
Brian and I did some investigation, and I think we know how to fix this. To tell if the download is going to be automatically opened, we just need to check aDownload.MIMEInfo.hasDefaultHandler.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIDownload.idl#88
http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsIMIMEInfo.idl#59

In the case of downloads that aren't automatically opened (like saving an image from the context menu), this is false.

The one other thing you need to worry about is that we don't automatically open the download if Fennec is in the background, even if this is true. You can observe "application-background" and "application-foreground" notifications to keep track of this. For an example, see:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7033
This has been tested on a Nexus 7 and is confirmed as working.  The download only opens if Fennec is in the foreground, but the Download notifications still work correctly whether Fennec is in the foreground or background.
Attachment #708016 - Attachment is obsolete: true
Attachment #712057 - Flags: review?(margaret.leibovic)
Comment on attachment 712057 [details] [diff] [review]
"Download Finished" notification now only shows if the download does not open on completion

Thanks for the patch!

So in comment 16, you're saying that we'll still be showing the notification even if the download completes while the app is in the background? I would have thought hasDefaultHandler would still be true even if the app was in the background, preventing us from showing the notification when we should. I think we should definitely make sure to test edge cases around this.

I'd like to get an answer to this question, but this is still an r- because of the comment below.

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

>-        if (state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED) {
>+        if (!aDownload.MIMEInfo.hasDefaultHandler) {

I think we still need to be checking if the download is finished. This should probably be:

>+        if (state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED &&
>+           !aDownload.MIMEInfo.hasDefaultHandler) {
Attachment #712057 - Flags: review?(margaret.leibovic) → review-
I thought it might not be necessary because it is checked in the switch statement at: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/downloads.js#144.  If you still want it checked that way I can change it back.
The notification is always correct if Fennec is in the foreground or the background.  So the bug is fixed.  However I noticed that the download itself will not open until Fennec is brought back into the foreground.
(In reply to Kyle from comment #18)
> I thought it might not be necessary because it is checked in the switch
> statement at:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> downloads.js#144.  If you still want it checked that way I can change it
> back.

Yes, but that switch statement also includes all these other cases:

      case Ci.nsIDownloadManager.DOWNLOAD_FAILED:
      case Ci.nsIDownloadManager.DOWNLOAD_CANCELED:
      case Ci.nsIDownloadManager.DOWNLOAD_BLOCKED_PARENTAL:
      case Ci.nsIDownloadManager.DOWNLOAD_DIRTY:

That's why the check was in there specifically for the DOWNLOAD_FINISHED case.

(In reply to Kyle from comment #19)
> The notification is always correct if Fennec is in the foreground or the
> background.  So the bug is fixed.  However I noticed that the download
> itself will not open until Fennec is brought back into the foreground.

Yes, this is the problem I was saying we wanted to check out for. If the download isn't automatically opening, we want to make sure we still show the notification. So, if Fennec is in the background, we'll still want the notification to show up for users.
Hi Kyle, are you still working on this bug? Is there anything you might need I could help with? Thanks!
Flags: needinfo?(kjspence)
Kyle told me that he's not working on this anymore.
Assignee: kjspence → nobody
Flags: needinfo?(kjspence)
Can I grab this bug and see if I can make it work?
(In reply to Federico Paolinelli from comment #23)
> Can I grab this bug and see if I can make it work?

Definitely! Do you have a build environment set up? Feel free to join #mobile on irc.mozilla.org to ask questions!
Attached patch First submission of the patch (obsolete) — Splinter Review
This is the first attempt. I've made some tests and it looks fine. The notification is removed only if fennec is in foreground. Hope I did not miss anything..
Attachment #753513 - Flags: review?(margaret.leibovic)
Attachment #712057 - Attachment is obsolete: true
Comment on attachment 753513 [details] [diff] [review]
First submission of the patch

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

This is looking good! I have a few style comments, and a concern about whether all of the logic here is necessary, so I'm holding off on giving an r+ on this version.

::: mobile/android/chrome/content/downloads.js
@@ +28,5 @@
>    init: function dl_init() {
>      if (this._initialized)
>        return;
>      this._initialized = true;
> +    this.isForeground = true;

I think you can just initialize isForeground to true when you declare it up above. This Downloads object is created during startup, and the app should be in the foreground during that time.

@@ +109,4 @@
>    // observer for last-pb-context-exited
>    observe: function dl_observe(aSubject, aTopic, aData) {
> +    switch (aTopic) {
> +      case "last-pb-context-exited" :

You should put braces around the body of this case statement, since you're declaring local variables within it. This doesn't break anything now, but may cause unexpected bugs if we add more observers later.

@@ +122,5 @@
> +          }
> +        }
> +        break;
> +      case "application-foreground" :
> +          this.isForeground = true;

Nit: This line is indented too far.

@@ +125,5 @@
> +      case "application-foreground" :
> +          this.isForeground = true;
> +        break;
> +      case "application-background" :
> +          this.isForeground = false;

Same here.

@@ +131,4 @@
>      }
> +
> +
> +    

Nit: Don't add these extra whitespace lines.

@@ +175,5 @@
>    },
>  
> +  _handleFinishedDownload : function(aDownload) {
> +    if (aDownload.MIMEInfo.hasDefaultHandler && Downloads.isForeground) {
> +      Downloads.removeAlert(aDownload);

Do we actually need to remove the progress alert? I think it removes itself when the progress is done, so we actually just need to make sure we don't show the completed notification (basically, do nothing) in this case.

And if we don't need to call removeAlert, this logic could probably all just go in onDownloadStateChange, instead of creating a separate _handleFinishedDownload method.
Attachment #753513 - Flags: review?(margaret.leibovic) → feedback+
Attached patch Second submission (obsolete) — Splinter Review
I implemented all the changes suggested. 

You were right, the notification is automatically closed whenever the download finishes for any reason.

However, delegating the display of the notification to Downloads with showAlert BUT closing that notification directly from AlertDownloadProgressListener feels a bit asymmetrical.
Attachment #753513 - Attachment is obsolete: true
Attachment #753975 - Flags: review?(margaret.leibovic)
Comment on attachment 753975 [details] [diff] [review]
Second submission

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

This looks good, thanks for updating. I just have a few small nits about the logic and comment structure.

::: mobile/android/chrome/content/downloads.js
@@ +193,5 @@
>            }
>          }
>  
> +        // we want to show the download finished notification only if it is not automatically opened.
> +        // i.e. no default application or fennec is in background

Nit: Please use proper capitalization and sentence structure in comments.

@@ +194,5 @@
>          }
>  
> +        // we want to show the download finished notification only if it is not automatically opened.
> +        // i.e. no default application or fennec is in background
> +        if (state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED && 

Nit: Trailing whitespace (I really wish we had a linter that would just fix issues like this)

@@ +195,5 @@
>  
> +        // we want to show the download finished notification only if it is not automatically opened.
> +        // i.e. no default application or fennec is in background
> +        if (state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED && 
> +            (!aDownload.MIMEInfo.hasDefaultHandler || !Downloads.isForeground)) {

It took me a moment to verify that this logic is correct... I think it would be clearer if it was switched around to:

if (state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED &&
    !(aDownload.MIMEInfo.hasDefaultHandler && Downloads.isForeground)) {

And then update the comment to say that a download is only automatically opened if it has a default handler *and* the application is in the foreground (at least I find it easier to think about this way).
Attachment #753975 - Flags: review?(margaret.leibovic) → review+
(In reply to Federico Paolinelli from comment #27)

> However, delegating the display of the notification to Downloads with
> showAlert BUT closing that notification directly from
> AlertDownloadProgressListener feels a bit asymmetrical.

If you find ways to improve the logic of this code, you can file separate bugs and post patches. A lot of this downloads code was written in a rush, so it can most likely be improved :)
Assignee: nobody → fedepaol
Attached patch Third attemptSplinter Review
Attachment #753975 - Attachment is obsolete: true
Attachment #755003 - Flags: review?(margaret.leibovic)
Comment on attachment 755003 [details] [diff] [review]
Third attempt

This looks great!
Attachment #755003 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
Removed unused observer found while fixing this bug.
Attachment #755015 - Flags: review?(margaret.leibovic)
Comment on attachment 755015 [details] [diff] [review]
Cleaned up unused observer from Downloads

Thanks, this looks good.
Attachment #755015 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/684cbf5daa80
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Depends on: 880661
This caused a regression, please see bug 880661.
Depends on: 887655
You need to log in before you can comment on or make changes to this bug.