Closed
Bug 696911
Opened 13 years ago
Closed 12 years ago
Hide download notification if the download opens on completion
Categories
(Firefox for Android Graveyard :: General, defect, P3)
Firefox for Android Graveyard
General
Tracking
(fennec-)
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)
4.62 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•13 years ago
|
||
margaret, can you look at this?
Assignee: nobody → margaret.leibovic
Priority: -- → P3
Updated•13 years ago
|
tracking-fennec: --- → 11+
Updated•12 years ago
|
tracking-fennec: 11+ → ?
Reporter | ||
Updated•12 years ago
|
Assignee: margaret.leibovic → nobody
Reporter | ||
Comment 3•12 years ago
|
||
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?
Comment 5•12 years ago
|
||
(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
Reporter | ||
Comment 6•12 years ago
|
||
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?
Comment 8•12 years ago
|
||
Hello, I'm Kyle's partner and just thought I should introduce myself.
Reporter | ||
Comment 9•12 years ago
|
||
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!
Comment 10•12 years ago
|
||
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.
Comment 11•12 years ago
|
||
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+
Reporter | ||
Comment 12•12 years ago
|
||
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+
Reporter | ||
Comment 13•12 years ago
|
||
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)
Reporter | ||
Comment 14•12 years ago
|
||
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-
Reporter | ||
Comment 15•12 years ago
|
||
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
Comment 16•12 years ago
|
||
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)
Reporter | ||
Comment 17•12 years ago
|
||
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-
Comment 18•12 years ago
|
||
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.
Comment 19•12 years ago
|
||
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.
Reporter | ||
Comment 20•12 years ago
|
||
(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.
Comment 21•12 years ago
|
||
Hi Kyle, are you still working on this bug? Is there anything you might need I could help with? Thanks!
Flags: needinfo?(kjspence)
Reporter | ||
Comment 22•12 years ago
|
||
Kyle told me that he's not working on this anymore.
Assignee: kjspence → nobody
Flags: needinfo?(kjspence)
Assignee | ||
Comment 23•12 years ago
|
||
Can I grab this bug and see if I can make it work?
Reporter | ||
Comment 24•12 years ago
|
||
(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!
Assignee | ||
Comment 25•12 years ago
|
||
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)
Reporter | ||
Updated•12 years ago
|
Attachment #712057 -
Attachment is obsolete: true
Reporter | ||
Comment 26•12 years ago
|
||
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+
Assignee | ||
Comment 27•12 years ago
|
||
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)
Reporter | ||
Comment 28•12 years ago
|
||
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+
Reporter | ||
Comment 29•12 years ago
|
||
(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 :)
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → fedepaol
Assignee | ||
Comment 30•12 years ago
|
||
Attachment #753975 -
Attachment is obsolete: true
Attachment #755003 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 31•12 years ago
|
||
Comment on attachment 755003 [details] [diff] [review]
Third attempt
This looks great!
Attachment #755003 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 32•12 years ago
|
||
Removed unused observer found while fixing this bug.
Attachment #755015 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 33•12 years ago
|
||
Comment on attachment 755015 [details] [diff] [review]
Cleaned up unused observer from Downloads
Thanks, this looks good.
Attachment #755015 -
Flags: review?(margaret.leibovic) → review+
Comment 34•12 years ago
|
||
Keywords: checkin-needed
Comment 35•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Comment 36•12 years ago
|
||
This caused a regression, please see bug 880661.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•