595919 - Implement PDF printing for "Save As PDF" on Android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

With the patch for bug 595737 applied, saving a page as PDF on Android results in the following exception being thrown from the "Browser:SaveAs" handler in content.js:

CC['@mozilla.org/gfx/printsettings-service;1'] is undefined

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

We need to add an PrintOptions factory to widget/src/nsWidgetFactory.cpp.  This attempt at a minimal implementation does not work yet.  Saving as PDF fails with NS_ERROR_FACTORY_NOT_REGISTERED in newPrintSettings.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2

Still missing a few required factories.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 3

Getting farther.  This fails to build; looks like maybe the Cairo feature flags are not getting picked up for some reason:

/home/mbrubeck/src/mozilla/android/gfx/thebes/gfxPDFSurface.cpp:60: error: '_moz_cairo_pdf_surface_create_for_stream' was not declared in this scope

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 4

This builds and runs, but the PDF it produced was blank.

Comment 5

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note to tester : check Bug 569497 along with this.

Comment 6

[Matt Brubeck (:mbrubeck)]

This won't use the file picker from bug 569497.  The Android file picker is for uploads only.  Save As PDF will save directly to the default download folder (bug 595737).

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this works for printing from the chrome process. However, the content process in Fennec tries to save the pdf to the sdcard's downloads folder, which it doesn't have permissions for, so that fails at the end.

Gotta figure out what to do with that last bit

Comment 8

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

Can the content process save the file to any location? If so, we could save it somewhere temporarily in the content process, send back the file name to chrome, and then do a final move.

Comment 9

[Stuart Parmenter]

Do we just need to ask for permissions to that folder?

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: m-b patch

Comment 11

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

Comment on attachment 490713 [details] [diff] [review]
m-b patch

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+        var appInfo = Cc["@mozilla.org/xre/app-info;1"];

Looks like this is not needed

>+        let tmpDir = 
>+          Components.classes["@mozilla.org/file/directory_service;1"]
>+          .getService(Components.interfaces.nsIProperties)
>+          .get("TmpD", Components.interfaces.nsIFile);  

Use Cc and Ci and it will work OK on a single line. I know the platform guys are line-wrappers :)

>+        let tmpFile = tmpDir.clone();
>+        tmpFile.append(dlFile.leafName);
>+        // we sometimes race with the content process, so make sure its finished
>+        // creating/writing the file
>+        while (!tmpFile.exists());

Add a blank line before the comment

>@@ -1333,6 +1351,8 @@ var PageActions = {
> #ifdef ANDROID

// Create the final destination file location

>     let file = downloadsDir.clone();
>     file.append(fileName);
>+    file.createUnique(file.NORMAL_FILE_TYPE, 0x666);

// The filename is used below to save the file to a temp location in the content process. Make sure it's up to date.

>+    fileName = file.leafName;

Add some comments so we remember what's happening here (use my examples if you want)

>     Services.obs.notifyObservers(download, "dl-start", null);
>-
>+#ifdef ANDROID

Keep the blank line. I like having some separation in the code. Makes it easier to read.

>+    let tmpDir = Components.classes["@mozilla.org/file/directory_service;1"]
>+                   .getService(Components.interfaces.nsIProperties)
>+                   .get("TmpD", Components.interfaces.nsIFile);

Cc and Ci and one line

>+    file.append(fileName);
>+ 
>+#endif
>     let data = {
>       type: Ci.nsIPrintSettings.kOutputFormatPDF,

Put the blank line outside the #endif

r+ with nits. Yay, we have PDF on Android! Thanks guys!

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: m-b patch for checkin

Comment 13

[Michael Wu [:mwu]]

Comment on attachment 490527 [details] [diff] [review]
patch

Sorry I don't feel qualified enough to review this...

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 490527 [details] [diff] [review]
patch

this looks reasonable to me, though would be nice to have a MOZ_PDF_PRINTING define somewhere instead of doing:

-#if defined(MOZ_ENABLE_GTK2) || defined(XP_WIN) || defined(XP_OS2) || defined(MOZ_WIDGET_QT)
+#if defined(MOZ_ENABLE_GTK2) || defined(XP_WIN) || defined(XP_OS2) || defined(MOZ_WIDGET_QT) || defined (ANDROID)

I also think that there's another place where this needs to be added, but I can't find it now.. hm.  If it works then it's probably ok!

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/ffe3abb58260 and http://hg.mozilla.org/mobile-browser/rev/df8e2a5be20a

Comment 16

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101117 Namoroka/4.0b8pre Fennec/4.0b3pre

Comment 17

[Camelia Urian]

Testcases update in litmus:
https://litmus.mozilla.org/show_test.cgi?id=48726
https://litmus.mozilla.org/show_test.cgi?id=48730