1411589 - [flatpak] No printer in the list of printers

Status: NEW

Description

[Jan Horak [:jhorak]]

The sandboxed application does not have direct access to the printers which current implementation requires. We need to use GtkPrintOperation for this instead:
https://developer.gnome.org/gtk3/stable/gtk3-High-level-Printing-API.html

Comment 1

[Jed Davis [:jld] (⏰UTC-6)]

Is this about Firefox's content sandboxing, or running the entire browser under flatpak, or both?

Comment 2

[Jan Horak [:jhorak]]

It has nothing to do with Firefox sandbox implementation. It's because the flatpak deny direct access to the printers. I'm going to look into it how this can be fixed. Sorry for not being clear.

Comment 3

[Mike Kaply [:mkaply]]

Is still an issue with the Flatpak?

Comment 4

[Jan Horak [:jhorak]]

Yes, non trivial changes needs to be done to have this fixed. Still on my radar.

Comment 5

[Jan Horak [:jhorak]]

Created attachment 8960096 [details]
Bug 1411589 - Export ShouldUseFlatpakPortal();

We need to detect the flatpak environment on multiple places.
Making the ShouldUseFlatpakPortal() exported to the nsIGIOService
allows us to reuse it.

Review commit: https://reviewboard.mozilla.org/r/228890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228890/

Comment 6

[Jan Horak [:jhorak]]

Created attachment 8960097 [details]
Bug 1411589 - Pass print-to-file-finished observer topic from the child process to the parent

When printing with e10s enabled, we need to propagate a print-to-file-finished topic to the parent
process, because the nsFlatpakPrintPortal instances, which accept the topic, are created in the parent.
Otherwise portal will never be notified that writing to the file has been finished and the content
will never make it to the printer.

Review commit: https://reviewboard.mozilla.org/r/228892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228892/

Comment 7

[Jan Horak [:jhorak]]

Created attachment 8960098 [details]
Bug 1411589 - Implement printing support for the flatpak portal,

In the flatpak environment the applications do not have access to the printers.
They need to use printing portal implemented by DBUS interface. The patch
implements support for printing portal by introducing nsFlatpakPrintPortal class.
1. it request print portal to show the print dialog
2. waits until print dialog is finished
3. setup observer for 'print-to-file-finished' topic
4. pass file descriptor of the printed file to the portal when the observer is notified

Review commit: https://reviewboard.mozilla.org/r/228894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228894/

Comment 8

[Martin Stránský [:stransky]]

Comment on attachment 8960096 [details]
Bug 1411589 - Export ShouldUseFlatpakPortal();

https://reviewboard.mozilla.org/r/228890/#review235496

Comment 9

[Martin Stránský [:stransky]]

Comment on attachment 8960098 [details]
Bug 1411589 - Implement printing support for the flatpak portal,

https://reviewboard.mozilla.org/r/228894/#review235508

::: layout/base/nsDocumentViewer.cpp:4507
(Diff revision 1)
>    // nsPrintJob::PrintPreview(), mPrintJob is already nullptr here.
>    // So, the following clean up does nothing in such case.
>    // (Do we need some of this for that case?)
>    if (mPrintJob) {
>      RefPtr<nsPrintJob> printJob = mPrintJob;
> +

This needs to be reviewed by layout peer.

::: widget/gtk/mozgtk/mozgtk.c:619
(Diff revision 1)
>  STUB(gtk_color_chooser_dialog_get_type)
>  STUB(gtk_color_chooser_get_type)
>  STUB(gtk_color_chooser_set_rgba)
>  STUB(gtk_color_chooser_get_rgba)
>  STUB(gtk_color_chooser_set_use_alpha)
> +STUB(gdk_threads_leave)

Those two are marked as obsoleted. We need to make sure we call Gtk code from main thread.

::: widget/gtk/nsPrintDialogGTK.cpp:981
(Diff revision 1)
> +  // Check for the flatpak portal first
> +  nsCOMPtr<nsIGIOService> giovfs =
> +    do_GetService(NS_GIOSERVICE_CONTRACTID);
> +  bool shouldUsePortal;
> +  giovfs->ShouldUseFlatpakPortal(&shouldUsePortal);
> +  if (shouldUsePortal && !gtk_check_version(3, 22, 0)) {

Please use == nullptr here isntead of !gtk_check_version()

Comment 10

[Martin Stránský [:stransky]]

Comment on attachment 8960098 [details]
Bug 1411589 - Implement printing support for the flatpak portal,

https://reviewboard.mozilla.org/r/228894/#review235516

This patch does not build. Please fix that and also separate the layout part for layout peer review.

Comment 11

[Martin Stránský [:stransky]]

Comment on attachment 8960098 [details]
Bug 1411589 - Implement printing support for the flatpak portal,

https://reviewboard.mozilla.org/r/228894/#review235740

::: widget/gtk/nsPrintDialogGTK.cpp:904
(Diff revision 1)
> +    return GTK_PRINT_OPERATION_RESULT_ERROR;
> +  }
> +  gdk_threads_leave();
> +  // Calling g_main_loop_run stops current code until g_main_loop_quit is called
> +  g_main_loop_run(mLoop);
> +  gdk_threads_enter();

gdk_threads_leave/enter here does not look right. Why is it here? (beside it's deprecated).

Comment 12

[Martin Stránský [:stransky]]

Comment on attachment 8960098 [details]
Bug 1411589 - Implement printing support for the flatpak portal,

https://reviewboard.mozilla.org/r/228894/#review235742

::: widget/gtk/nsPrintDialogGTK.cpp:535
(Diff revision 1)
> +typedef void (*GtkWindowHandleExported) (GtkWindow  *window,
> +                                         const char *handle,
> +                                         gpointer    user_data);
> +#ifdef MOZ_WAYLAND
> +typedef struct {
> +        GtkWindow *window;

Don't mix 8/2 spaces indentation. Use just 4 spaces everywhere.

::: widget/gtk/nsPrintDialogGTK.cpp:556
(Diff revision 1)
> +        g_free (handle_str);
> +}
> +#endif
> +
> +static void
> +free_exported_data (gpointer data)

This is not needed. Just pass g_free to s_gdk_wayland_window_export_handle() directly.

::: widget/gtk/nsPrintDialogGTK.cpp:583
(Diff revision 1)
> +      callback (window, handle_str, user_data);
> +      g_free (handle_str);
> +      return TRUE;
> +    }
> +#ifdef MOZ_WAYLAND
> +  if (GDK_IS_WAYLAND_DISPLAY (gtk_widget_get_display (GTK_WIDGET (window))))

Use just else instead of "GDK_IS_WAYLAND_DISPLAY (gtk_widget_get_display (GTK_WIDGET (window)))" check. We don't support other non-x11 displays right now.

nit: mozilla style is without space between function name and braces ().

::: widget/gtk/nsPrintDialogGTK.cpp:593
(Diff revision 1)
> +      data = g_new0 (WaylandWindowHandleExportedData, 1);
> +      data->window = window;
> +      data->callback = callback;
> +      data->user_data = user_data;
> +
> +      static auto s_gdk_wayland_window_export_handle =

80 chars/column here.

::: widget/gtk/nsPrintDialogGTK.cpp:596
(Diff revision 1)
> +      data->user_data = user_data;
> +
> +      static auto s_gdk_wayland_window_export_handle =
> +        reinterpret_cast<gboolean (*)(GdkWindow*, GdkWaylandWindowExported, gpointer, GDestroyNotify)>
> +        (dlsym(RTLD_DEFAULT, "gdk_wayland_window_export_handle"));
> +      if (!s_gdk_wayland_window_export_handle ||

You're missing matching gdk_wayland_window_unexport_handle() call to release the window handle. see gdkwindow-wayland.c for details.

::: widget/gtk/nsPrintDialogGTK.cpp:601
(Diff revision 1)
> +      if (!s_gdk_wayland_window_export_handle ||
> +          !s_gdk_wayland_window_export_handle(gdk_window,
> +                                              wayland_window_handle_exported,
> +                                              data, free_exported_data))
> +        {
> +          return FALSE;

You need to free data explicitly here (see gtkwindow.c/gtk_window_export_handle().

::: widget/gtk/nsPrintDialogGTK.cpp:603
(Diff revision 1)
> +                                              wayland_window_handle_exported,
> +                                              data, free_exported_data))
> +        {
> +          return FALSE;
> +        }
> +      else

"} else {" and brace { after if () is mozilla style.