Closed Bug 570745 Opened 9 years ago Closed 9 years ago

[f10s] Save As PDF menu does nothing


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: vingtetun, Assigned: vingtetun)




(1 file, 2 obsolete files)

Attached patch wip v0.1 (obsolete) — Splinter Review
No description provided.
Attached patch Patch (obsolete) — Splinter Review
This patch includes the workaround for loading content.js you provided by Mark on IRC.
Assignee: nobody → 21
Attachment #449890 - Attachment is obsolete: true
Attachment #450006 - Flags: review?(mark.finkle)
Comment on attachment 450006 [details] [diff] [review]

Looks good, but I want to wait until some other patches land before landing this patch.

See me on IRC for more details.
Attachment #450006 - Flags: review?(mark.finkle) → review+
Same patch but updated on trunk
Attachment #450006 - Attachment is obsolete: true
Attachment #450242 - Flags: review?(mark.finkle)
Comment on attachment 450242 [details] [diff] [review]
Patch updated on trunk

>diff -r 67517d995cb2 chrome/content/browser-ui.js
>   _savePageAsPDF: function saveAsPDF() {

>+    let browser = Browser.selectedBrowser;
>+    let fileName = getDefaultFileName(null, browser.documentURI, browser.contentDocument, null);

the "contentWindow.document" will need to be changed for real e10s. We will probably need to make our own simple function for making a default fileName based on the contentTitle. Followup bug.

>     fileName = fileName.replace(/[\*\:\?]+/g, " ");
> #endif
>     picker.defaultString = fileName + ".pdf";
>     let dm = Cc[";1"].getService(Ci.nsIDownloadManager);
>     picker.displayDirectory = dm.defaultDownloadsDirectory;
>     let rv =;

I like keeping this whitespace. Don't smash the code all together.

>+    let data = {
>+      type: Ci.nsIPrintSettings.kOutputFormatPDF,
>+      id: newItemId,
>+      referrer: current,
>+      path: picker.file.path

Just a nit, but maybe "filePath" would be a better name.

>diff -r 67517d995cb2 chrome/content/content.js
> // how many milliseconds before the mousedown and the overlay of an element
> const kTapOverlayTimeout = 200;
> let Cc = Components.classes;
> let Ci = Components.interfaces;

Keep the blank line please

>+        //XXX we probably need a preference here, the header can be useful
>+        printSettings.footerStrCenter = '';
>+        printSettings.footerStrLeft   = '';
>+        printSettings.footerStrRight  = '';
>+        printSettings.headerStrCenter = '';
>+        printSettings.headerStrLeft   = '';
>+        printSettings.headerStrRight  = '';

Use double quotes (") for strings please

r+ with nits fixed
Attachment #450242 - Flags: review?(mark.finkle) → review+
I have an updated version of the patch but my build is pretty crashy now. I want to understand what is the root cause of that before land more patches.
The crash came from somewhere else
Closed: 9 years ago
Resolution: --- → FIXED
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110921
Firefox/9.0a1 Fennec/9.0a1
Device: HTC Desire
OS: Android 2.2
You need to log in before you can comment on or make changes to this bug.