701797 - save as pdf does not provide status notification

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lawrence Mandel [:lmandel] (use needinfo)]

When I click Save as PDF a new download item appears in my notifications bar. Typically, download notifications contain a status bar that indicates progress. The Save as PDF download does not have a status bar. This notification also does not disappear until I remove it manually.

Comment 1

[:Margaret Leibovic]

(In reply to Lawrence Mandel [:lmandel] from comment #0)
> When I click Save as PDF a new download item appears in my notifications
> bar. Typically, download notifications contain a status bar that indicates
> progress. The Save as PDF download does not have a status bar.

Since PDFs are small, the download probably completes before you get the change to see the progress indicator.

> This notification also does not disappear until I remove it manually.

This is bug 696911.

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Margaret Leibovic [:margaret] from comment #1)
> > This notification also does not disappear until I remove it manually.
> 
> This is bug 696911.

To be clear, in my case the PDF did not open on completion and the downloading notification was still listed on my phone.

Comment 3

[Armen [:armenzg]]

(In reply to Lawrence Mandel [:lmandel] from comment #2)
> (In reply to Margaret Leibovic [:margaret] from comment #1)
> > > This notification also does not disappear until I remove it manually.
> > 
> > This is bug 696911.
> 
> To be clear, in my case the PDF did not open on completion and the
> downloading notification was still listed on my phone.

This is very easy to reproduce.
In other words, if I was to file this bug it would be "save as PDF is 99% broken". Being 1% that it has told me that it has downloaded without being able to view it/open it.

Comment 4

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

Brian - What's the deal here? Once the download completes, we should open the pDF, or at least open when you tap the system notification.

Comment 5

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Brian - What's the deal here? Once the download completes, we should open
> the pDF, or at least open when you tap the system notification.

It seems like there is a problem with opening PDFs from the download completion notification. I found that if I tap on a notification for a PDF download from the web, it also does not open, but tapping on a notification for an APK download does the right thing.

Comment 6

[:Margaret Leibovic]

Looking into this more, I found that when the save as PDF code got ported over from XUL fennec, the "Browser:SaveAs:Return" part was left out, which handled notifying observers that the download was done (http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser-ui.js#1060).

Brian, if you don't mind, I'm just going to take this bug, since I think I know how to fix it.

Comment 8

[:Margaret Leibovic]

Attachment: patch (use download manager API in saveAsPDF)

Instead of executing our own queries against the downloads database, we should just be using the download manager API. With this change, the download progress notifications work as though this were a normal download (a progress bar now actually shows up in the notification!).

Comment 9

[:Margaret Leibovic]

Attachment: patch to fix progress notifications for printing

In order to use the download manager to track printing status, we need to fire STATE_IS_NETWORK notifications, since that's what the download manager expects. I'm not sure if this fix is hacky or not, but it doesn't look like it will cause any trouble.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 576094 [details]
patch to fix progress notifications for printing

Review of attachment 576094 [details]:
-----------------------------------------------------------------

::: layout/printing/nsPrintData.cpp
@@ +138,1 @@
>      DoOnProgressChange(0, 0, true, nsIWebProgressListener::STATE_START|nsIWebProgressListener::STATE_IS_DOCUMENT);

Untested, but could these changes just use another bitwise-or on the current |DoOnProgressChange| call? For example:

using namespace nsIWebProgressListener;
DoOnProgressChange(0, 0, true, STATE_START|STATE_IS_DOCUMENT|STATE_IS_NETWORK);

Comment 11

[Cristian Nicolae (:xti)]

When a webpage is saved as PDF and only the Downloading notification was displayed, the PDF file is created in (local)/download, but its size is 0 Kb. If the app is closed and reopened and if a any webpage is opened and is saved as PDF then the first saved PDF file will change its size to the double of the normal size. Also the first PDF file could not be opened. 
Because of this bug, if two or more webpages are saved as PDF, Fennec will download 2 ore more files with the same content, but different names (PDF file names are saved accordingly to the opened webpages).

Comment 12

[Wesley Johnston (:wesj)]

Heh. I did this same thing for XUL Fennec once (bug 646262), but distracted myself with rewriting the Download Manager API and never finished it.

Comment 13

[:Margaret Leibovic]

(In reply to Jared Wein [:jwein and :jaws] from comment #10)
> Comment on attachment 576094 [details] [review]
> patch to fix progress notifications for printing
> 
> Review of attachment 576094 [details] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/printing/nsPrintData.cpp
> @@ +138,1 @@
> >      DoOnProgressChange(0, 0, true, nsIWebProgressListener::STATE_START|nsIWebProgressListener::STATE_IS_DOCUMENT);
> 
> Untested, but could these changes just use another bitwise-or on the current
> |DoOnProgressChange| call? For example:
> 
> using namespace nsIWebProgressListener;
> DoOnProgressChange(0, 0, true,
> STATE_START|STATE_IS_DOCUMENT|STATE_IS_NETWORK);

I haven't tested this either, but I think that this doesn't really make given the semantics of these flags, and I couldn't find anywhere else where we do something like this. I realize what we're doing here is already kind of a hack, but at least we can make it a semantically correct hack :)

Comment 14

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #13)

> I haven't tested this either, but I think that this doesn't really make
> given the semantics of these flags...

s/make/make sense/

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Margaret Leibovic [:margaret] from comment #13)
> (In reply to Jared Wein [:jwein and :jaws] from comment #10)
> > Comment on attachment 576094 [details] [review] [diff] [details] [review]
> > patch to fix progress notifications for printing
> > 
> > Review of attachment 576094 [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/printing/nsPrintData.cpp
> > @@ +138,1 @@
> > >      DoOnProgressChange(0, 0, true, nsIWebProgressListener::STATE_START|nsIWebProgressListener::STATE_IS_DOCUMENT);
> > 
> > Untested, but could these changes just use another bitwise-or on the current
> > |DoOnProgressChange| call? For example:
> > 
> > using namespace nsIWebProgressListener;
> > DoOnProgressChange(0, 0, true,
> > STATE_START|STATE_IS_DOCUMENT|STATE_IS_NETWORK);
> 
> I haven't tested this either, but I think that this doesn't really make
> given the semantics of these flags, and I couldn't find anywhere else where
> we do something like this. I realize what we're doing here is already kind
> of a hack, but at least we can make it a semantically correct hack :)

Maintaining semantic correctness would probably entail that we don't state the progress has changed from 100 to 100 as well (same for 0 to 0).

(In reply to Cristian Nicolae (:xti) from comment #11)
> When a webpage is saved as PDF and only the Downloading notification was
> displayed, the PDF file is created in (local)/download, but its size is 0
> Kb. If the app is closed and reopened and if a any webpage is opened and is
> saved as PDF then the first saved PDF file will change its size to the
> double of the normal size. Also the first PDF file could not be opened. 
> Because of this bug, if two or more webpages are saved as PDF, Fennec will
> download 2 ore more files with the same content, but different names (PDF
> file names are saved accordingly to the opened webpages).

Cristian: Did you see this behavior with or without applying these patches?

Comment 16

[:Margaret Leibovic]

I landed the first patch because it definitely improves save as PDF download notifications, so I didn't want to let it sit:

https://hg.mozilla.org/projects/birch/rev/ff5faa59c065

I'll keep this bug open until the second patch gets reviewed/lands. Without that patch, the download completed notification still won't appear, so that is a known issue.

Comment 17

[:Margaret Leibovic]

Comment on attachment 576094 [details]
patch to fix progress notifications for printing

I moved this patch to a follow-up bug (bug 704691).

Comment 18

[:Margaret Leibovic]

(Marking this fixed, since the rest of the work can be done in the follow-up.)

Comment 19

[Andreea Pod]

When I save a file as PDF the only notification I get is the one with downloading the file. When opening the system notification there is a status bar for the PDF file but after the download is completed the notification disappears. Is this ok so far? And the rest will be done in the follow-up bug so I can mark this bug as verified?

Mozilla /5.0 (Android;Linux armv7l;rv:11.0a1) Gecko/20111124 Firefox/11.0a1 Fennec/11.0a1
Device: LG Optimus 2X (Android 2.2)

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Brian - What's the deal here? Once the download completes, we should open
> the pDF, or at least open when you tap the system notification.

To follow-up on this comment (that I had previously missed), I don't expect or want the PDF to open automatically in this situation. I'm have already viewed the content in the browser.

Comment 21

[Armen [:armenzg]]

I'm having several inconsistencies (20111125 build).

I have been able to hit these problems.

Problem A - only hit it once
* I click on "Save as PDF" and nothing is notified
* I check /sdcard/Downloads and nothing was there.
* I tried several times with several sites. It seems that closing the browser or having downloaded a normal file could have fixed it.

Problem B - I hit it all the time
* I click on "Save as PDF" and I see "Downloading ...pdf"
* When it finishes the "Download...pdf"  completely dissapears
** Normal downloads they stay there until I actually clear it
* When it finishes there is no message saying "...pdf has finished downloading"
** Normal downloads they have that

Not sure if bug 704691 is the follow up of problem B or not.

FTR I have not lost the the ability to download files.

Comment 22

[:Margaret Leibovic]

(In reply to Andreea Pod from comment #19)
> When I save a file as PDF the only notification I get is the one with
> downloading the file. When opening the system notification there is a status
> bar for the PDF file but after the download is completed the notification
> disappears. Is this ok so far? And the rest will be done in the follow-up
> bug so I can mark this bug as verified?

Yes, this will be fixed by bug 704691.

(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #21)

> Problem A - only hit it once
> * I click on "Save as PDF" and nothing is notified
> * I check /sdcard/Downloads and nothing was there.
> * I tried several times with several sites. It seems that closing the
> browser or having downloaded a normal file could have fixed it.

You should file a new bug if you can reproduce this.

> Problem B - I hit it all the time
> * I click on "Save as PDF" and I see "Downloading ...pdf"
> * When it finishes the "Download...pdf"  completely dissapears
> ** Normal downloads they stay there until I actually clear it
> * When it finishes there is no message saying "...pdf has finished
> downloading"
> ** Normal downloads they have that
> 
> Not sure if bug 704691 is the follow up of problem B or not.

Yes, it will fix that problem. See comment 16.

Comment 23

[Andreea Pod]

Marking this as verified fixed, build: Mozilla /5.0 (Android;Linux armv7l;rv:11.0a1) Gecko/20111127 Firefox/11.0a1 Fennec/11.0a1
Device: LG Optimus 2X (Android 2.2)