Closed Bug 1078432 Opened 5 years ago Closed 4 years ago

Use Android print service to enable cloud printing

Categories

(Firefox for Android :: General, defect)

35 Branch
x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox43 + fixed
firefox44 --- verified
relnote-firefox --- 44+
fennec 44+ ---

People

(Reporter: wesj, Assigned: mfinkle)

References

Details

Attachments

(1 file, 4 obsolete files)

We should hook into the Android print service. That would let us use their cloud printing (and bring better parity with Chrome). I think this should be fairly "simple". i.e. We need a PrintDocumentAdapter that overrides its onWrite to have Gecko print the speicified page range to a file:

http://developer.android.com/reference/android/print/PrintDocumentAdapter.html#onWrite%28android.print.PageRange%5B%5D,%20android.os.ParcelFileDescriptor,%20android.os.CancellationSignal,%20android.print.PrintDocumentAdapter.WriteResultCallback%29
This looks horrible: support is patchy, especially going further back in API time.
http://www.doubleencore.com/2013/11/android-gets-a-print-framework/

How far back can we give up support?

Assume unacceptable:
 * URL goes off the device, rendered in the cloud (can't print from behind logins)
 * Using current Webview/Javascript way as described with PrintDocumentAdapter https://developer.android.com/training/printing/html-docs.html

The two PrintDocumentAdapters provided are a PDFDocumentAdapter and one from WebView.

Options
-------
 * Implement our own PrintDocumentAdapter
 * Use existing ones (I'm most partial to sending a Bitmap screen shot).

What do we print?
-----------------
The most webby thing to allow would be the print media style sheet. Other options: 

 * Send raw html (perhaps from reader mode)?
 * Send long screen shot of mobile page as BitMap? (easiest)
(In reply to James Hugman [:jhugman] [@jhugman] from comment #1)
> This looks horrible: support is patchy, especially going further back in API
> time.
> http://www.doubleencore.com/2013/11/android-gets-a-print-framework/
> 
> How far back can we give up support?

Only add support for well supported Android versions. We aren't trying to bring printing to Gingerbread, for example. I did have an old add-on that worked with the Google web APIs for printing [1], but I think we only want to start with the new built-in support for Printing.

> Assume unacceptable:
>  * URL goes off the device, rendered in the cloud (can't print from behind
> logins)
>  * Using current Webview/Javascript way as described with
> PrintDocumentAdapter
> https://developer.android.com/training/printing/html-docs.html

Let's try to avoid those.
 
> The two PrintDocumentAdapters provided are a PDFDocumentAdapter and one from
> WebView.

Can we try to use our Gecko support for generating PDFs and the PDFDocumentAdapter to make something work?

[1] https://addons.mozilla.org/en-US/mobile/addon/cloud-printer/
Attached patch android-printing WIP (obsolete) — Splinter Review
Work in progress.

1. Saves a temp PDF to the TmpD folder
2. Uses the PDF to print using the Android printing service

Notes:

1. Android 4.4+ only
2. Gecko is displaying an "error" popup for some reason. It does not stop the print, but we need to make it go away.
3. No progress or other UX in place yet (if we need it) to show work happening while the PDF is being generated.
Assignee: nobody → mark.finkle
Attached patch android-printing v0.1 (obsolete) — Splinter Review
This patch creates a temporary PDF file by "printing to PDF" using Gecko. We do the same kind of thing for SaveAsPDF. That temporary PDF file is then passed to the Android Printing Service.

The patch adds a "Page > Print" menu to the main menu. It only works on Android 4.4 and higher:
http://developer.android.com/training/printing/index.html

Just looking for some feedback right now. Might not be ready for real review yet.
Attachment #8548912 - Attachment is obsolete: true
Attachment #8633648 - Flags: feedback?(s.kaspari)
This should include a NIGHTLY_BUILD flag before landing as well.
Cool! That's a nice feature. It seems like at least PrintHelper.java is missing in the attached patch though.
Flags: needinfo?(mark.finkle)
Attached patch android-printing v0.2 (obsolete) — Splinter Review
Now with PrintHelper.java
Attachment #8633648 - Attachment is obsolete: true
Attachment #8633648 - Flags: feedback?(s.kaspari)
Flags: needinfo?(mark.finkle)
Attachment #8634055 - Flags: feedback?(s.kaspari)
Attached patch android-printing v0.3 (obsolete) — Splinter Review
Now with whitespace cleanup
Attachment #8634055 - Attachment is obsolete: true
Attachment #8634055 - Flags: feedback?(s.kaspari)
Attachment #8634057 - Flags: feedback?(s.kaspari)
Comment on attachment 8634057 [details] [diff] [review]
android-printing v0.3

Review of attachment 8634057 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure what feedback you are exactly looking for, so here are some random ideas I had while using the feature:

* We do not apply the print CSS right now I guess? E.g. printing a Wikipedia page will print the table of contents and wastes a lot of space in general. Printing the same page in Chrome has certain parts removed from the page.

* Generating the PDF takes some time for very long pages. We probably will need to have some progress UI (You already mentioned that)

* Do we need to take care of the generated files? Can we remove them as soon as they are processed by the printing service?

Except from some cleanup and error handling - is there anything else that you consider "not be ready for real review yet"?

::: mobile/android/chrome/content/PrintHelper.js
@@ +35,5 @@
> +    let printSettings = Cc["@mozilla.org/gfx/printsettings-service;1"].getService(Ci.nsIPrintSettingsService).newPrintSettings;
> +    printSettings.printSilent = true;
> +    printSettings.showPrintProgress = false;
> +    printSettings.printBGImages = true;
> +    printSettings.printBGColors = true;

Do we really want to print background images and background colors? Desktop doesn't as default afaik. Do we need a print UI that is shown before the cloud print UI to setup things like this?
Attachment #8634057 - Flags: feedback?(s.kaspari) → feedback+
Comment on attachment 8634057 [details] [diff] [review]
android-printing v0.3

Review of attachment 8634057 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/PrintHelper.js
@@ +31,5 @@
> +    let file = Services.dirsvc.get("TmpD", Ci.nsIFile);
> +    file.append(fileName);
> +    file.createUnique(file.NORMAL_FILE_TYPE, parseInt("666", 8));
> +
> +    let printSettings = Cc["@mozilla.org/gfx/printsettings-service;1"].getService(Ci.nsIPrintSettingsService).newPrintSettings;

For reference, I did wind up wrapping a lot of this in Downloads.jsm at one point. You might reuse it in order to keep things consistent (but just not add the Download to a list)

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1403
(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Comment on attachment 8634057 [details] [diff] [review]
> android-printing v0.3
> 
> Review of attachment 8634057 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure what feedback you are exactly looking for, so here are some
> random ideas I had while using the feature:
> 
> * We do not apply the print CSS right now I guess? E.g. printing a Wikipedia
> page will print the table of contents and wastes a lot of space in general.
> Printing the same page in Chrome has certain parts removed from the page.

We should be using print CSS. In my tests, desktop "print to PDF" on Mac and "print" on Android, yielded nearly the same output.

> * Generating the PDF takes some time for very long pages. We probably will
> need to have some progress UI (You already mentioned that)

I added a NIGHTLY_BUILD flag on the menu for now. It will give us some time to test this out and see what kind of progress UI we might need.

> * Do we need to take care of the generated files? Can we remove them as soon
> as they are processed by the printing service?

I added code to delete the generated files once the streams close.

> Except from some cleanup and error handling - is there anything else that
> you consider "not be ready for real review yet"?

I found that the PrintManager adapter methods were called on the UI thread, so I move the runnable to just the "onWrite" method, where all the heavy file handling was happening. Now I don't see "strict mode" warnings anymore.

> ::: mobile/android/chrome/content/PrintHelper.js
> @@ +35,5 @@
> > +    let printSettings = Cc["@mozilla.org/gfx/printsettings-service;1"].getService(Ci.nsIPrintSettingsService).newPrintSettings;
> > +    printSettings.printSilent = true;
> > +    printSettings.showPrintProgress = false;
> > +    printSettings.printBGImages = true;
> > +    printSettings.printBGColors = true;
> 
> Do we really want to print background images and background colors? Desktop
> doesn't as default afaik. Do we need a print UI that is shown before the
> cloud print UI to setup things like this?

I disabled printing BG colors and images.
(In reply to Wesley Johnston (:wesj) from comment #10)

> For reference, I did wind up wrapping a lot of this in Downloads.jsm at one
> point. You might reuse it in order to keep things consistent (but just not
> add the Download to a list)
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#1403

The wrapped code is a little too specifc to downloads right now. We need to pass different options (BG colors and images) and we don't want the file to appear in downloads. We might be able to do these, but I'll leave that to a different bug.
As I mentioned, the menu is behind a NIGHTLY_BUILD flag.
Attachment #8634057 - Attachment is obsolete: true
Attachment #8654660 - Flags: review?(s.kaspari)
Comment on attachment 8654660 [details] [diff] [review]
android-printing v0.4

Review of attachment 8654660 [details] [diff] [review]:
-----------------------------------------------------------------

r+: It's behind a Nightly flag and it's good enough to test and iterate. My only concern is having no progress feedback but this is something for a follow-up (with UX help).

::: mobile/android/base/PrintHelper.java
@@ +34,5 @@
> +import android.print.PrintManager;
> +import android.print.PageRange;
> +import android.util.Log;
> +
> +

NIT: Two empty lines.

@@ +50,5 @@
> +
> +            @Override
> +            public void onError(NativeJSObject error) {
> +                // Gecko didn't respond due to state change, javascript error, etc.
> +                Log.d(LOGTAG, "No response from Gecko on request to generate a PDF");

NIT: Log as error? Is there helpful information in the error object?

@@ +54,5 @@
> +                Log.d(LOGTAG, "No response from Gecko on request to generate a PDF");
> +            }
> +
> +            private void finish(final Context context, final String filePath, final String title) {
> +                Log.d(LOGTAG, "Made it to finish");

NIT: Should we get rid of these debug logs?

@@ +66,5 @@
> +                    public void onWrite(final PageRange[] pages, final ParcelFileDescriptor destination, final CancellationSignal cancellationSignal, final WriteResultCallback callback) {
> +                        ThreadUtils.postToBackgroundThread(new Runnable() {
> +                            @Override
> +                            public void run() {
> +                                Log.d(LOGTAG, "onWrite");

NIT: Another debug log?

@@ +89,5 @@
> +                                } catch (FileNotFoundException ee){
> +                                    //Catch exception
> +                                } catch (Exception e) {
> +                                    //Catch exception
> +                                } finally {

Should we log these exceptions?

Personally I'm not really a fan of catching "Exception" because it just catches too much stuff that is actually a code error (e.g. NullPointerException, ClassCastException). Do we need it here?

@@ +92,5 @@
> +                                    //Catch exception
> +                                } finally {
> +                                    try {
> +                                        input.close();
> +                                        output.close();

As painful as it is we might to wrap every close() in their own try block. input.close() might throw an IOException and then we never close output. I've often seen this kind of pain hidden in a IOUtils method in other projects. But it seems like we don't have something like that?

@@ +94,5 @@
> +                                    try {
> +                                        input.close();
> +                                        output.close();
> +                                    } catch (IOException e) {
> +                                        e.printStackTrace();

NIT: Log.foo() is more controllable than printStackTrace()

@@ +103,5 @@
> +                    }
> +
> +                    @Override
> +                    public void onLayout(PrintAttributes oldAttributes, PrintAttributes newAttributes, CancellationSignal cancellationSignal, LayoutResultCallback callback, Bundle extras){
> +                        Log.d(LOGTAG, "onLayout");

NIT: debug log?

@@ +114,5 @@
> +                        callback.onLayoutFinished(pdi, true);
> +                    }
> +                };
> +
> +                Log.d(LOGTAG, "start printing");

NIT: debug log?

::: mobile/android/chrome/content/PrintHelper.js
@@ +11,5 @@
> +
> +  observe: function (aSubject, aTopic, aData) {
> +    let browser = BrowserApp.selectedBrowser;
> +
> +    dump("XXX got a message")

debug log?

@@ +15,5 @@
> +    dump("XXX got a message")
> +    switch (aTopic) {
> +      case "Print:PDF":
> +        Messaging.handleRequest(aTopic, aData, (data) => {
> +          dump("XXX calling generate")

debug log?

@@ +50,5 @@
> +          // We get two STATE_STOP calls, one for STATE_IS_DOCUMENT and one for STATE_IS_NETWORK
> +          if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP && stateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK) {
> +            if (Components.isSuccessCode(status)) {
> +              // Send the details to Java
> +              dump("XXX success")

debug log?

@@ +53,5 @@
> +              // Send the details to Java
> +              dump("XXX success")
> +              resolve({ file: file.path, title: fileName });
> +            } else {
> +              dump("XXX failure")

debug log?
Attachment #8654660 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #14)

> > +            public void onError(NativeJSObject error) {
> > +                // Gecko didn't respond due to state change, javascript error, etc.
> > +                Log.d(LOGTAG, "No response from Gecko on request to generate a PDF");
> 
> NIT: Log as error? Is there helpful information in the error object?

Not much we get from the error object, iirc. It has a message which is already logged in JS.

> > +                                } catch (FileNotFoundException ee){
> > +                                    //Catch exception
> > +                                } catch (Exception e) {
> > +                                    //Catch exception
> > +                                } finally {
> 
> Should we log these exceptions?

Yes, I added logging.

> Personally I'm not really a fan of catching "Exception" because it just
> catches too much stuff that is actually a code error (e.g.
> NullPointerException, ClassCastException). Do we need it here?

I changed it to an IOException

> > +                                    try {
> > +                                        input.close();
> > +                                        output.close();
> 
> As painful as it is we might to wrap every close() in their own try block.
> input.close() might throw an IOException and then we never close output.
> I've often seen this kind of pain hidden in a IOUtils method in other
> projects. But it seems like we don't have something like that?

Done

> > +                                    } catch (IOException e) {
> > +                                        e.printStackTrace();
> 
> NIT: Log.foo() is more controllable than printStackTrace()

Done
https://hg.mozilla.org/mozilla-central/rev/c72acc2daca6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
tracking-fennec: --- → Nightly+
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:  Use Android print service to enable cloud printing
[Links (documentation, blog post, etc)]:

This seems relnote-worthy!  Mark, do you have a nicer way to phrase the release note, or anything we might link to? Thanks. 

I can also ask QE to give this some testing during aurora or beta.
Flags: needinfo?(mark.finkle)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> Release Note Request (optional, but appreciated)
> [Why is this notable]:
> [Suggested wording]:  Use Android print service to enable cloud printing
> [Links (documentation, blog post, etc)]:
> 
> This seems relnote-worthy!  Mark, do you have a nicer way to phrase the
> release note, or anything we might link to? Thanks. 

Your suggested wording looks good to me.

> 
> I can also ask QE to give this some testing during aurora or beta.

Not quite yet. We have this feature behind a NIGHTLY_BUILD flag for now. We're not sure it's ready yet. 

We use "tracking-fennec=Nightly+" to indicate this state, but is there a better way to let Release Management know?
Flags: needinfo?(mark.finkle)
Thanks Mark, setting the relnote flag back to ? should mean this gets picked up again next time around. 
Good to know about your tracking flags - I didn't realize that's what it meant.
I don't see any other bugs here... what else is left to do before this is ready to ship?
Flags: needinfo?(mark.finkle)
Flags: needinfo?(bbermes)
Depends on: 1208203
I added bug 1208203 for showing a printing indicator
Flags: needinfo?(mark.finkle)
I think the comment that was made in the funnel was that the experience might not be as quick, too many steps to get to the actual printing action. 

Just tried it out (well didn't print actually), I like it actually, so no concerns from my end except for it got stuck on "Preparing preview" -- I selected cloud print Google, I assume I need to set this up first? Should this give me an error or a message in this case?
Flags: needinfo?(bbermes)
(In reply to Barbara Bermes [:barbara] from comment #23)
> I think the comment that was made in the funnel was that the experience
> might not be as quick, too many steps to get to the actual printing action. 
> 
> Just tried it out (well didn't print actually), I like it actually, so no
> concerns from my end except for it got stuck on "Preparing preview" -- I
> selected cloud print Google, I assume I need to set this up first? Should
> this give me an error or a message in this case?

I need more context. Does this happen while Fennec is active? Or is the Android cloud printing active? Can you make some screenshots?

We likely have some work to do, but I don't know how much.
Depends on: 1210082
Noted. Anything you want me to link to in the release note for 43 beta?
Flags: needinfo?(mark.finkle)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> Noted. Anything you want me to link to in the release note for 43 beta?

I don't think we have anything specific. I don't see any general google/android pages for "printing" either, so I guess not.
Flags: needinfo?(mark.finkle)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #27)
> This should be noted in 44 not 43; see bug 1210082.

Yes, should be in Fx44. Thanks.
tracking-fennec: Nightly+ → 44+
Barbara, could you please update the tracking flag relnote-firefox to "?" and fill out the relnote template? This will help me add it to Beta44 release notes and also help you familiarize with RelMan team's process on relnoting bugs. Thank you!
Flags: needinfo?(bbermes)
Release Note Request (optional, but appreciated)
[Why is this notable]: Added print functionality in Menu->Page->print
[Suggested wording]: Use Android print service to enable cloud printing
[Links (documentation, blog post, etc)]: ? Joni, do we already have a SUMO for this?
Flags: needinfo?(bbermes) → needinfo?(jsavage)
New article for print to Google Cloud Print: https://support.mozilla.org/kb/print-web-page-firefox-android
Flags: needinfo?(jsavage)
i couldn't get this to work on my Note 5 Lollipop 5.0 or Nexus 6P Marshmall6.0 but I will assume that QA got it working on some devices. The error message I get is "printer not available right now" and "spooler stopped"
CORRECTION: "This printer isn't available right now"
Depends on: 1240534
it should be noted that this is broken in ff44 for android, if bug 1240534 gets uplifted to FF44 for android then this will unbreak it!
Verified as fixed in Firefox 44 RC Build 2;
Device: LG G4 (Android 5.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.