Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GeckoView] Bundle the Firefox printing code in the viewer (bug 1810111) #16163

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Mar 16, 2023

This may not be enough, on its own, to completely fix bug 1810111 however it's impossible for printing to work in GeckoView without this patch.

@Snuffleupagus Snuffleupagus force-pushed the bug-1810111 branch 2 times, most recently from 6b4e2e9 to 3eea0c1 Compare March 22, 2023 19:22
@mozilla mozilla deleted a comment from pdfjsbot Mar 31, 2023
@mozilla mozilla deleted a comment from pdfjsbot Mar 31, 2023
@mozilla mozilla deleted a comment from pdfjsbot Mar 31, 2023
@mozilla mozilla deleted a comment from pdfjsbot Mar 31, 2023
@mozilla mozilla deleted a comment from pdfjsbot Mar 31, 2023
@mozilla mozilla deleted a comment from pdfjsbot Mar 31, 2023
@mozilla mozilla deleted a comment from pdfjsbot Mar 31, 2023
@mozilla mozilla deleted a comment from pdfjsbot Mar 31, 2023
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/09842f7fe593918/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/93876ed2dd470d9/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/93876ed2dd470d9/output.txt

Total script time: 4.31 mins

  • Integration Tests: FAILED

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/09842f7fe593918/output.txt

Total script time: 16.08 mins

  • Integration Tests: FAILED

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally and it doesn't work.
The printerContainer is missing in viewer-geckoview.html and in viewer-geckoview.js.
I can share with you how I setup an android dev environment on windows 11 if you need.

@calixteman
Copy link
Contributor

When printing, Fenix is generating a pdf and then it's sent to whatever app to print it.
So we probably don't need to really "print" it but we could just run the beforeprint callbacks, save it, send it to fenix and use the result as the print result.
It should be a way faster like this which means that we don't consume too much resources (especially power).

@Snuffleupagus
Copy link
Collaborator Author

I tested locally and it doesn't work.
The printerContainer is missing in viewer-geckoview.html and in viewer-geckoview.js.

D'oh, that's of course obvious in hindsight; didn't think about that :-)

When printing, Fenix is generating a pdf and then it's sent to whatever app to print it.
So we probably don't need to really "print" it but we could just run the beforeprint callbacks, save it, send it to fenix and use the result as the print result.
It should be a way faster like this which means that we don't consume too much resources (especially power).

Ah, that makes total sense! In that case this PR obviously isn't even in the correct direction, since none of this is necessary if we just call PDFDocumentProxy.saveDocument() and send its result to some native API.

Given that I don't know anything about the Fenix platform-code, and it seemingly being written in Java (which I've not used for many years), I'm unfortunately not sure if I'll be able to fix this bug anytime soon.

@calixteman
Copy link
Contributor

My plan was to merge this patch and try to be smarter later.

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Apr 5, 2023

My plan was to merge this patch and try to be smarter later.

Sorry, I figured we should go directly for the "proper" solution.

I've updated the patch to address #16163 (review), so now the DOM element should be found correctly.

@Snuffleupagus
Copy link
Collaborator Author

@calixteman Ping, do we want to move forward with these changes?

Should we perhaps also add a GeckoView toolbar-button for this, and if so could you please help find a suitable one?

@calixteman
Copy link
Contributor

It was on my today todo-list :).
IIRC, I need to write something in m-c to have it working correctly.

@Snuffleupagus Snuffleupagus force-pushed the bug-1810111 branch 2 times, most recently from 30f04f0 to 1ea9118 Compare May 5, 2023 14:39
@calixteman
Copy link
Contributor

So for now the window.print is plugged on the save to pdf stuff:

which isn't what we want when we've for example a filled form and especially if we've something to execute before printing.
For now, the only way to trigger a window.print is to have an embedded script calling it (or call it from a devtools console connected to the device/emulator).
As far as I can tell there is no UI to print something in Fenix, so I'll see with Android UI/UX people what we want and then they'll provide us an icon.

So for the moment, I'm waiting for Fenix people to know what we want to do.
Could you add a pref (off by default), something like pdfjs.enablePrinting ? and then I'll land your patch or we can keep it in the oven until we've more info. What do you prefer ?

@Snuffleupagus
Copy link
Collaborator Author

Landing something that doesn't work anyway seems kind of pointless. at least to me, so let's just wait until we actually know exactly how this should be integrated on the platform side.

@calixteman
Copy link
Contributor

@Snuffleupagus, I want to merge this patch asap to fix pdf form printing on Android (bug 1842685).
Would you mind to have a look on it to see if everything is ok for you ?

This may not be enough, on its own, to completely fix [bug 1810111](https://bugzilla.mozilla.org/show_bug.cgi?id=1810111) however it's impossible for printing to work in GeckoView without this patch.
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Thank you.

@calixteman calixteman merged commit cfd179f into mozilla:master Jul 28, 2023
3 checks passed
@Snuffleupagus Snuffleupagus deleted the bug-1810111 branch October 12, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants