Closed Bug 595919 Opened 9 years ago Closed 9 years ago

Implement PDF printing for "Save As PDF" on Android

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set

Tracking

(fennec2.0b3+)

VERIFIED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: mbrubeck, Assigned: blassey)

Details

Attachments

(2 files, 5 obsolete files)

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
tracking-fennec: --- → ?
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch WIP 2 (obsolete) — Splinter Review
Still missing a few required factories.
Assignee: nobody → mbrubeck
Attachment #474774 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch WIP 3 (obsolete) — Splinter Review
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
Attachment #475440 - Attachment is obsolete: true
Attached patch WIP 4 (obsolete) — Splinter Review
This builds and runs, but the PDF it produced was blank.
Attachment #475506 - Attachment is obsolete: true
Summary: "Save As PDF" fails on Android: CC['@mozilla.org/gfx/printsettings-service;1'] is undefined → Implement PDF printing for "Save As PDF" on Android
tracking-fennec: ? → 2.0b2+
tracking-fennec: 2.0b2+ → 2.0+
Note to tester : check Bug 569497 along with this.
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).
Assignee: mbrubeck → blassey.bugs
tracking-fennec: 2.0+ → 2.0b3+
Attached patch patchSplinter Review
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
Attachment #475531 - Attachment is obsolete: true
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.
Do we just need to ask for permissions to that folder?
Attached patch m-b patch (obsolete) — Splinter Review
Attachment #490713 - Flags: review?(mark.finkle)
Attachment #490527 - Flags: review?(mwu)
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!
Attachment #490713 - Flags: review?(mark.finkle) → review+
Attachment #490713 - Attachment is obsolete: true
Attachment #490799 - Flags: review+
Comment on attachment 490527 [details] [diff] [review]
patch

Sorry I don't feel qualified enough to review this...
Attachment #490527 - Flags: review?(mwu) → review?(vladimir)
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!
Attachment #490527 - Flags: review?(vladimir) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/ffe3abb58260 and http://hg.mozilla.org/mobile-browser/rev/df8e2a5be20a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101117 Namoroka/4.0b8pre Fennec/4.0b3pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.