561765 - PDF create from the "Save Page As PDF" menu should appear into the downloads panel

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Steps to reproduce:
 * go to google.com
 * open the site menu and click on "Save As PDF"

Actual Results:
 * no feedback

Expected results:
 * Something that told me what's happening

The proposed patch did not modify code from downloads.js. It just add a new binding and code into the Save As Pdf core function to appear into the download panel and show some notifications.

It is low risk in the sense that this is not a core part of the UI and the area affected by the new code is relatively small.

Comment 1

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

Comment on attachment 441499 [details] [diff] [review]
Patch

>diff -r a4131553b838 chrome/content/bindings/downloads.xml

Why do we need the new binding? Is it just to be safe? Wouldn't it be used for any download too, not just Save As PDF?

>diff -r a4131553b838 chrome/content/browser-ui.js

>+      // stubs for the nsIWebProgressListener interfaces which we don't use.
>+      onLocationChange : function() { throw "Unexpected onLocationChange"; },
>+      onStatusChange   : function() { throw "Unexpected onStatusChange";   },
>+      onSecurityChange : function() { throw "Unexpected onSecurityChange"; }

Do we need to throw here?

Comment 2

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

OK. I see that the print engine code never uses onLocationChange, onStatusChange and onSecurityChange by design.

I still think it is overkill to throw in the stub impls though

Comment 3

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

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/8b6303bbea6f

We need an approval to push this to m-1.1

I think the risk is low, but I want Stuart to weigh in too.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #1)
> (From update of attachment 441499 [details] [diff] [review])
> >diff -r a4131553b838 chrome/content/bindings/downloads.xml
> 
> Why do we need the new binding? Is it just to be safe? Wouldn't it be used for
> any download too, not just Save As PDF?

Because there is no "Pause" for printing and because there I've not touch the downloads.js code, I've tried to avoid showing the standart Cancel/Pause buttons

> 
> >diff -r a4131553b838 chrome/content/browser-ui.js
> 
> >+      // stubs for the nsIWebProgressListener interfaces which we don't use.
> >+      onLocationChange : function() { throw "Unexpected onLocationChange"; },
> >+      onStatusChange   : function() { throw "Unexpected onStatusChange";   },
> >+      onSecurityChange : function() { throw "Unexpected onSecurityChange"; }
> 
> Do we need to throw here?

No real reason for that, I've just looked at code from Firefox and do it their way.

Comment 5

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.5pre) Gecko/20100427 Namoroka/3.6.5pre Fennec/1.1b2pre

and

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:1.9.3a5pre) Gecko/20100427 Namoroka/3.7a5pre Fennec/1.1b2pre


...but there's a big problem with it. Follow-up bug filed: https://bugzilla.mozilla.org/show_bug.cgi?id=562043

Comment 6

[Aakash Desai [:aakashd]]

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11742 updated to regression test this bug.