Closed Bug 561765 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
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.
Attachment #441499 - Flags: review?(mark.finkle)
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?
Attachment #441499 - Flags: review?(mark.finkle) → review+
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
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.
Assignee: nobody → 21
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(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.
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Depends on: 562043
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11742 updated to regression test this bug.
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.