887655 - No download complete or "Open with" dialog displayed after using "Save as PDF"

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Nightly 25.0a1 2013-06-27/ Aurora 24.0a2 2013-06-27
Acer Iconia Tab A500 (Android 3.2)/ Samsung Galaxy Tab 2 (Android 4.1.1)

Steps to reproduce:
1) Load any website. For e.g. news.google.com
2) Open the Custom menu and choose "Save as PDF"

Expected results:
When the download is complete either the pdf file is opened or a download complete notification is displayed

Actual results:
There is no download complete notification - the download notification just disappears - and there is no prompt to choose with which app to open the file(On the Acer A500 I have 3 apps that can handle pdfs)

Comment 1

[Aaron Train [:aaronmt]]

Please find a regression range.

Comment 2

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

Brian - Didn't we change this on purpose? If we download a file that can be opened, we do not show the "download complete" ui. Right?

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Brian - Didn't we change this on purpose? If we download a file that can be
> opened, we do not show the "download complete" ui. Right?

Right; if I understand correctly, bug 696911 removes the complete notification if and only if the file is automatically opened. But from comment 0, it sounds like the file is not opened, so it's a bug if the notification disappears.

I'm confused about these dependencies. This sounds exactly like bug 880661, which blocks this one, but bug 880661 was very recently marked as verified. Adrian/Teodora, was bug 880661 not actually fixed? Or is this something different?

Also CC'ing Margaret and Federico since they've been following these bugs more closely.

Comment 4

[Brian Nicholson (:bnicholson)]

Maybe the difference is this bug is about "Save as PDF"? I'm not sure why they would the behavior would be any different.

Comment 5

[:Margaret Leibovic]

At this point, I feel like we should consider just backing out and WONTFIXing bug 696911 (and bug 880661). We never got any complaints about notifications hanging around too long, but we're certainly causing problems by removing them when we shouldn't.

Federico has done a great job working on these and investigating them, but I don't think we should let past effort dictate our current decisions.

Comment 6

[Teodora Vermesan (:TeoVermesan)]

There are two different bugs. Bug 880661 was fixed (I tryed to download an image,a ftp build and saved a video and the download complete notification appears). But when trying to save a page as pdf, no download complete notification appears. I will try to find a regression range.

Comment 7

[Teodora Vermesan (:TeoVermesan)]

The regression window is the same as in Bug 880661 (comment 3)
good build: 30.05.2013 
bad build:  31.05.2013 
-pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f66d956d188e&tochange=3c6f2394995d 

(caused via Bug 696911)

Comment 8

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

Ian - Comment 5 is an option (backing out the original patches). Are those valuable enough to keep, or do we backout?

Comment 9

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Ian - Comment 5 is an option (backing out the original patches). Are those
> valuable enough to keep, or do we backout?

Talked about this in IRC, and yes I am fine with it

Comment 10

[Ian Barlow (:ibarlow)]

Fine with backing out, that is.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Revert hiding the download notification on completion

Fixes the bug by backing out bug 696911 and bug 880661.

Comment 12

[:Margaret Leibovic]

Comment on attachment 776799 [details] [diff] [review]
Revert hiding the download notification on completion

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

I guess this means we're basically just WONTFIXing those other bugs. I kinda feel like we should actually be doing the backouts in those bugs and marking them as WONTFIX, but I suppose it doesn't really matter as long as we have the dependencies set correctly.

::: mobile/android/chrome/content/downloads.js
@@ +32,5 @@
>      // Monitor downloads and display alerts
>      this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
>      this._progressAlert = new AlertDownloadProgressListener();
>      this._dlmgr.addPrivacyAwareListener(this._progressAlert);
> +    Services.obs.addObserver(this, "xpcom-shutdown", true);

Since you're not actually doing a regular backout and reopening the other bugs, you might as well leave this out, since it's not needed anymore (or land a separate patch that removes it again).

Comment 13

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ce733487fc

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 776799 [details] [diff] [review]
Revert hiding the download notification on completion

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 696911
User impact if declined: Save as PDF is broken (file downloads, but user can't easily open it)
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): almost none; just a backout of regressing bug
String or IDL/UUID changes made by this patch: none

Comment 15

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/d4ce733487fc

Comment 16

[Aaron Train [:aaronmt]]

Adrian, can you verify this?

Comment 17

[bhavana bajaj [:bajaj]]

Comment on attachment 776799 [details] [diff] [review]
Revert hiding the download notification on completion

low risk backout, looks good to land

Comment 18

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/73c4cb1518bb

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: 
Device: Samsung Galaxy R
OS: Android 2.3.4

Comment 20

[Teodora Vermesan (:TeoVermesan)]

The download complete notification appears (07/21 Nightly build), but by tapping it you cannot open the file. I filled out a new bug 896350.

Comment 21

[Aaron Train [:aaronmt]]

Please remember to resolve this as verified.

Comment 22

[Teodora Vermesan (:TeoVermesan)]

I wanted to resolve this bug as verified only after verifying the status of firefox24.

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 24.0a2(2013-07-24)
Device: Samsung Galaxy Nexus
OS: Android 4.1.1