Implement PDF printing for "Save As PDF" on Android

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: mbrubeck, Assigned: blassey)

Tracking

Trunk
All
Android
Bug Flags:
in-litmus +

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
tracking-fennec: --- → ?
(Reporter)

Comment 1

7 years ago
Created attachment 474774 [details] [diff] [review]
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.
(Reporter)

Comment 2

7 years ago
Created attachment 475440 [details] [diff] [review]
WIP 2

Still missing a few required factories.
Assignee: nobody → mbrubeck
Attachment #474774 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Reporter)

Comment 3

7 years ago
Created attachment 475506 [details] [diff] [review]
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
Attachment #475440 - Attachment is obsolete: true
(Reporter)

Comment 4

7 years ago
Created attachment 475531 [details] [diff] [review]
WIP 4

This builds and runs, but the PDF it produced was blank.
Attachment #475506 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
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
(Assignee)

Updated

7 years ago
tracking-fennec: ? → 2.0b2+
tracking-fennec: 2.0b2+ → 2.0+
Note to tester : check Bug 569497 along with this.
(Reporter)

Comment 6

7 years ago
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).

Updated

7 years ago
Assignee: mbrubeck → blassey.bugs
tracking-fennec: 2.0+ → 2.0b3+
Created attachment 490527 [details] [diff] [review]
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
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.

Comment 9

7 years ago
Do we just need to ask for permissions to that folder?
Created attachment 490713 [details] [diff] [review]
m-b patch
Attachment #490713 - Flags: review?(mark.finkle)
(Assignee)

Updated

7 years ago
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+
Created attachment 490799 [details] [diff] [review]
m-b patch for checkin
Attachment #490713 - Attachment is obsolete: true
Attachment #490799 - Flags: review+

Comment 13

7 years ago
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
Last Resolved: 7 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?

Comment 17

6 years ago
Testcases update in litmus:
https://litmus.mozilla.org/show_test.cgi?id=48726
https://litmus.mozilla.org/show_test.cgi?id=48730
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.