Skip to content

[GTK] Implement printing using the Print portal#26765

Merged
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
GeorgesStavracas:eng/GTK-Printing-does-not-work-when-running-inside-Flatpak
Apr 18, 2024
Merged

[GTK] Implement printing using the Print portal#26765
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
GeorgesStavracas:eng/GTK-Printing-does-not-work-when-running-inside-Flatpak

Conversation

@GeorgesStavracas
Copy link
Copy Markdown
Contributor

@GeorgesStavracas GeorgesStavracas commented Apr 3, 2024

fd47d13

[GTK] Implement printing using the Print portal
https://bugs.webkit.org/show_bug.cgi?id=192748

Reviewed by Michael Catanzaro.

When running inside a sandboxed environment - which is fairly common
nowadays - printing is severely limited due to using the legacy GTK
print API (GtkPrintUnixDialog & family).

Due to WebKit's multi-process architecture, using the modern alternative,
GtkPrintOperation, is not really possible, because GtkPrintOperation has
the enconded assumption that the rendering of the document pages happens
in the same process as of printing.

Implement manual support for the Print portal. The portal has two well
defined steps:

 1. PreparePrint() is where WebKit sends the settings and page setup
    that are stored in WebKitPrintOperation to the portal. The portal
    presents a dialog to let users choose their print preferences. This
    step returns WebKit a token, which is the "authentication" mechanism
    for the next step.

 2. Print() takes a token, and a file descriptor pointing to the file
    that will be send to the printer. The file descriptor can and often
    does point to an in-memory buffer! In WebKit's case, it's a tmpfile.

This effectively means that webkit_print_operation_print(), which assumes
that step 1 can be skipped - cannot work under a sandbox. Deprecate this
function, and adjust the tests to ignore this deprecation, as the tests
should be adapted separately.

* Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp:
(webkitPrintOperationFinished):
(webkitPrintOperationFailed):
(webkitPrintOperationPrintPagesForFrame):
(findFilePrinter):
(webkitPrintOperationSendPagesToPrintPortal):
(webkitPrintOperationPreparePrint):
(webkitPrintOperationRunPortalDialog):
(webkitPrintOperationRunDialogForFrame):
* Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.h.in:
* Tools/TestWebKitAPI/Tests/WebKitGtk/TestPrinting.cpp:
(testPrintOperationPrint):
(testPrintOperationErrors):

Canonical link: https://commits.webkit.org/277671@main

10a3cdb

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-skia
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv-sim ✅ 🧪 api-gtk
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@GeorgesStavracas GeorgesStavracas requested a review from a team as a code owner April 3, 2024 00:54
@GeorgesStavracas GeorgesStavracas self-assigned this Apr 3, 2024
@GeorgesStavracas GeorgesStavracas added the WebKitGTK Bugs related to the Gtk API layer. label Apr 3, 2024
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Apr 3, 2024

@GeorgesStavracas GeorgesStavracas marked this pull request as draft April 3, 2024 00:59
@GeorgesStavracas
Copy link
Copy Markdown
Contributor Author

I'm making this a draft because there is one unresolved problem with the Print portal that I think needs to be solved first: the Print portal offers various output formats (PDF, PS, and SVG) and Skia cannot handle that.

I gave this a stab at https://gitlab.gnome.org/GNOME/xdg-desktop-portal-gnome/-/merge_requests/148 but I'm not sure if this approach is even acceptable - it's likely just a workaround to the problem. Maybe we'll need a new parameter to the Print portal itself.

But please share your thoughts about the code, I'll apply review comments as usual.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 3, 2024
@GeorgesStavracas
Copy link
Copy Markdown
Contributor Author

The warning this patch introduces is making the test suite unhappy...

@GeorgesStavracas GeorgesStavracas removed the merging-blocked Applied to prevent a change from being merged label Apr 3, 2024
@GeorgesStavracas GeorgesStavracas force-pushed the eng/GTK-Printing-does-not-work-when-running-inside-Flatpak branch from 8348b27 to 003af88 Compare April 3, 2024 16:25
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Apr 3, 2024

Copy link
Copy Markdown
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

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

Nice, this is way simpler than I was expecting it would be. Using a temporary file was smart.

I think you should deprecate webkit_print_operation_print(). There's no point in not doing so; applications shouldn't do things that don't work when sandboxed.

Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
@GeorgesStavracas
Copy link
Copy Markdown
Contributor Author

I don't really know why the style checker skipped so many NULLs and TRUEs and FALSEs

@mcatanzaro
Copy link
Copy Markdown
Contributor

I don't really know why the style checker skipped so many NULLs and TRUEs and FALSEs

The style checker is not smart enough to check for gboolean vs. bool.

It should be able to check for nullptr, though, because using NULL is never allowed. I'm not sure why it doesn't catch that. Maybe it's just something nobody ever bothered to implement....

@GeorgesStavracas GeorgesStavracas force-pushed the eng/GTK-Printing-does-not-work-when-running-inside-Flatpak branch from 003af88 to 0517e8f Compare April 4, 2024 19:23
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
@GeorgesStavracas GeorgesStavracas force-pushed the eng/GTK-Printing-does-not-work-when-running-inside-Flatpak branch from 0517e8f to 6a1de80 Compare April 5, 2024 04:28
@carlosgcampos
Copy link
Copy Markdown
Contributor

Wouldn't it be possible to make the portal print without a dialog? I don't understand why we have to deprecated the feature.

@GeorgesStavracas
Copy link
Copy Markdown
Contributor Author

Wouldn't it be possible to make the portal print without a dialog? I don't understand why we have to deprecated the feature.

I don't know for sure, but I don't think this would be possible. The challenge here is that, in a tight sandbox, apps cannot "just" send a print job to the Print portal, because they cannot choose a printer - they don't have access to the hardware in a tight sandbox.

The only printer available will effectively be the file printer, but even in that case, the app won't have an output file path. That's selected by the user through the PreparePrint method.

So, all in all, I don't see how an tightly sandboxed app would be able to skip the PreparePrint method. Any alternative would require opening sandbox holes - be it the CUPS permission, or add more filesystem permissions for printing into a file.

@carlosgcampos
Copy link
Copy Markdown
Contributor

What I meant is whether it would be possible to add api to the portal to allow printing without showing a dialog.

@GeorgesStavracas
Copy link
Copy Markdown
Contributor Author

What I meant is whether it would be possible to add api to the portal to allow printing without showing a dialog.

The lack of a dialog is the problem. How would WebKit, or any sandboxed app, tell the portal which printer or output file it should print to?

@mcatanzaro
Copy link
Copy Markdown
Contributor

I don't see the point of ever allowing an app to print without first showing the print dialog. Why would an app ever want to do that?

And Debian codesearch indicates the only user of this API is our own API test, so it won't be missed.

@GeorgesStavracas GeorgesStavracas force-pushed the eng/GTK-Printing-does-not-work-when-running-inside-Flatpak branch from 6a1de80 to be461e0 Compare April 5, 2024 20:27
@GeorgesStavracas GeorgesStavracas marked this pull request as ready for review April 5, 2024 20:27
@GeorgesStavracas
Copy link
Copy Markdown
Contributor Author

I took the liberty to take a few executive decisions about this PR:

  • Removed the code that depended on newer portal features. I'll create a new issue, and submit a separate PR for that.
    • This code was the Skia-related stuff, so this PR should be good to backport again
  • Reverted back to iterating the current main context. Pushing a new main context freezes the UI and triggers the unresponsive app dialog.
  • Fixed all remaining FIXMEs in code

I think this is good to go now.

Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
@GeorgesStavracas GeorgesStavracas force-pushed the eng/GTK-Printing-does-not-work-when-running-inside-Flatpak branch from be461e0 to 7c14592 Compare April 5, 2024 21:42
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 6, 2024
Copy link
Copy Markdown
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

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

Please check with @carlosgcampos before landing, because he might want to review this first. But you pass my review.

@mcatanzaro mcatanzaro removed the merging-blocked Applied to prevent a change from being merged label Apr 9, 2024
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it woulld be easier if this is a struct instead of a class, and you can create instances using { } without adding explicit constructors

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, it this what you had in mind?

Comment thread Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp Outdated
@GeorgesStavracas GeorgesStavracas force-pushed the eng/GTK-Printing-does-not-work-when-running-inside-Flatpak branch from 7c14592 to 10a3cdb Compare April 15, 2024 18:05
@GeorgesStavracas GeorgesStavracas added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Apr 18, 2024
https://bugs.webkit.org/show_bug.cgi?id=192748

Reviewed by Michael Catanzaro.

When running inside a sandboxed environment - which is fairly common
nowadays - printing is severely limited due to using the legacy GTK
print API (GtkPrintUnixDialog & family).

Due to WebKit's multi-process architecture, using the modern alternative,
GtkPrintOperation, is not really possible, because GtkPrintOperation has
the enconded assumption that the rendering of the document pages happens
in the same process as of printing.

Implement manual support for the Print portal. The portal has two well
defined steps:

 1. PreparePrint() is where WebKit sends the settings and page setup
    that are stored in WebKitPrintOperation to the portal. The portal
    presents a dialog to let users choose their print preferences. This
    step returns WebKit a token, which is the "authentication" mechanism
    for the next step.

 2. Print() takes a token, and a file descriptor pointing to the file
    that will be send to the printer. The file descriptor can and often
    does point to an in-memory buffer! In WebKit's case, it's a tmpfile.

This effectively means that webkit_print_operation_print(), which assumes
that step 1 can be skipped - cannot work under a sandbox. Deprecate this
function, and adjust the tests to ignore this deprecation, as the tests
should be adapted separately.

* Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.cpp:
(webkitPrintOperationFinished):
(webkitPrintOperationFailed):
(webkitPrintOperationPrintPagesForFrame):
(findFilePrinter):
(webkitPrintOperationSendPagesToPrintPortal):
(webkitPrintOperationPreparePrint):
(webkitPrintOperationRunPortalDialog):
(webkitPrintOperationRunDialogForFrame):
* Source/WebKit/UIProcess/API/gtk/WebKitPrintOperation.h.in:
* Tools/TestWebKitAPI/Tests/WebKitGtk/TestPrinting.cpp:
(testPrintOperationPrint):
(testPrintOperationErrors):

Canonical link: https://commits.webkit.org/277671@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/GTK-Printing-does-not-work-when-running-inside-Flatpak branch from 10a3cdb to fd47d13 Compare April 18, 2024 14:40
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 277671@main (fd47d13): https://commits.webkit.org/277671@main

Reviewed commits have been landed. Closing PR #26765 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit fd47d13 into WebKit:main Apr 18, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Apr 18, 2024
@GeorgesStavracas GeorgesStavracas deleted the eng/GTK-Printing-does-not-work-when-running-inside-Flatpak branch April 18, 2024 14:55
@aperezdc
Copy link
Copy Markdown
Contributor

Backported as 2de9573 into the 2.44 release branch 👏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebKitGTK Bugs related to the Gtk API layer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants