570745 - [f10s] Save As PDF menu does nothing

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: wip v0.1

Comment 1

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

Attachment: Patch

This patch includes the workaround for loading content.js you provided by Mark on IRC.

Comment 2

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

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.

Comment 3

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

Attachment: Patch updated on trunk

Same patch but updated on trunk

Comment 4

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

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

Comment 5

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

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.

Comment 6

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

The crash came from somewhere else

http://hg.mozilla.org/mobile-browser/rev/3006cfe1a3ce

Comment 8

[Catalin Suciu [:csuciu]]

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