[f10s] Save As PDF menu does nothing

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 449890 [details] [diff] [review]
wip v0.1
Created attachment 450006 [details] [diff] [review]
Patch

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]
Patch

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+
Created attachment 450242 [details] [diff] [review]
Patch updated on trunk

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.

> #ifdef MOZ_PLATFORM_MAEMO
>     fileName = fileName.replace(/[\*\:\?]+/g, " ");
> #endif
>     picker.defaultString = fileName + ".pdf";
>-
>     let dm = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
>     picker.displayDirectory = dm.defaultDownloadsDirectory;
>-
>     let rv = picker.show();

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

http://hg.mozilla.org/mobile-browser/rev/3006cfe1a3ce
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Duplicate of this bug: 569635
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.