1090448 - Make e10s printing work on Linux

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

nsPrintOptionsGtk needs to serialize and deserialize PrintData with the GTK-specific stuff that the parent/child need to know in order to get a print job going.

Unfortunately, it looks as if nsPrintSettingsGtk on the parent side gets a memory address for the printer that the user selects, and we can't pass that down through IPC. So instead, we should probably serialize the name or some unique identifier for the printer to the child, and have the child enumerate the available printers looking for that unique identifier when reconstructing the nsIPrintSettings on the child side.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Just a note that there seems to be another case for Linux where we display a XUL-based dialog for print settings. I'm not entirely sure under what conditions this is the case, but it exists.

When using the XUL-based dialog, the script for the dialog makes use of nsIPrintingPromptService::ShowPrinterProperties, which will need to get implemented through the printing proxy.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: bz://1090448/mconley

/r/3329 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK
/r/3331 - [WIP] e10s printing on linux

Pull down these commits:

hg pull review -r b930f6163801dc9144c52d9d49e1ba8a37ae893a

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8559407 [details]
MozReview Request: bz://1090448/mconley

/r/3329 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=?
/r/3331 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?

Pull down these commits:

hg pull review -r 9628282c58f8ed84344b1545236df7798b3e9b4a

Comment 5

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/3327/#review2775

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 2)
> +gboolean nsPrintOptionsGTK::FindPrinterWithName(GtkPrinter *aPrinter,

Document this

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 2)
> +  // The following fields are not serialized automatically by the parent
> +  // class, so we need to do it ourselves:
> +  //
> +  // GtkPaperSize
> +  //   displayName

This comment doesn't appear to add much.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8559407 [details]
MozReview Request: bz://1090448/mconley

/r/3329 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=?
/r/3331 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?

Pull down these commits:

hg pull review -r e6ce3bb54e79031c72337247fc33249ed8bfbca7

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Hey karl - I've requested review from you via this new MozReview thing a few of us are playing with. I'm doing that mostly because I broke this into two patches.

If you're uncomfortable using MozReview, I can turn these into a set of traditional patches. Let me know what you'd prefer.

Also, if you want to test, you can try these changes via:

hg pull review -r 9628282c58f8ed84344b1545236df7798b3e9b4a
hg update -r 9628282c58f8ed84344b1545236df7798b3e9b4a

or

hg pull http://reviewboard-hg.mozilla.org/gecko -r 9628282c58f8ed84344b1545236df7798b3e9b4a
hg update -r 9628282c58f8ed84344b1545236df7798b3e9b4a

if you don't have the review alias set up.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8559407 [details]
MozReview Request: bz://1090448/mconley

/r/3329 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=?
/r/3331 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?
/r/3475 - Bug 1090448 - Enable printing with e10s windows on Linux. r=?

Pull down these commits:

hg pull review -r 3306bd0c2ff50c2e5c7f2aed5e53c6413e72f09e

Comment 9

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/3475/#review2817

::: browser/app/profile/firefox.js
(Diff revision 1)
>  pref("print.enable_e10s_testing", true);

why not just remove this pref altogether?

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #1)
> Just a note that there seems to be another case for Linux where we display a
> XUL-based dialog for print settings. I'm not entirely sure under what
> conditions this is the case, but it exists.

I'm not sure exactly when this is used, but I assume it is legacy from before the GTK print dialog.  There may be a pref to re-enable it, but I expect we can drop support for this.  Beware, there remnants of all sorts of things in the printing code, and I've found it difficult to determine what is still used and what is not.

Comment 11

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/3331/#review2931

I wonder why all these details need to be accessed on both the content and
browser sides.  Could there be a cross-process object that is opaque on the
content side?  Is there only a subset of this info that is needed by layout?

I don't have good ideas on how to deal with finding the GtkPrinter
synchronously.  If it needs to pass between processes then perhaps determining
the GtkPrinter can be delayed until printing, which is already an async
process, I assume.

GtkPrintSettings is basically a hash table of strings keyed by strings.  All
the accessor methods just convert between string and the particular format.
The keys aren't necessarily limited to the strings with GTK_PRINT_SETTINGS\_\*
defines.  "The predefined keys try to use shared values as much as possible so
that moving such a document between systems still works", but I suspect
special printer backends might use other values.  However, strings are
reasonably easy to serialize anyway, I assume, so, if this needs to be passed
between processes, then I suspect it would be simpler to use
gtk_print_settings_foreach() and gtk_print_settings_set(), with an array of
pairs of strings in PrintData.

I haven't yet looked at how GtkPageSetup and GtkPaperSize are used.  I wonder
why the GtkPaperSize doesn't come from gtk_print_settings_get_paper_size().

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 3)
> +                         settingsGTK, nullptr, TRUE);

I think we need to do whatever is necessary to avoid running arbitrary events in DeserializeToPrintSettings().  Nested event loops are evil.  They unroll only in FILO order, and they surprise callers who don't expect the world to change while they weren't watching.

Is it necessary to pass the printer between content and browser processes?  Is it the printer selection or the printing that happens in the content process?  Could they both happen in the browser process?

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8559407 [details]
MozReview Request: bz://1090448/mconley

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> Hey karl - I've requested review from you via this new MozReview thing a few
> of us are playing with. I'm doing that mostly because I broke this into two
> patches.
> 
> If you're uncomfortable using MozReview, I can turn these into a set of
> traditional patches. Let me know what you'd prefer.

I'm happy to use MozReview.  It made looking at the changes easy and the process went well until I tried to paste my comments in the review page.  Middle button events seem to have been prevented and so I don't know how to paste the primary selection without copying to the clipboard.  I also didn't work out how to remove the review request flag from reviewboard.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Thanks for the review karlt!

Thanks for the key/value suggestion for GtkPrintSettings. I wasn't aware that it was all just strings in the background. That's reduced the complexity of the code considerably.

I think I've also found a way of finding the printer on the content process side without spinning yet another nested event loop - I found the code for gtk_enumerate_printers, and I think I can emulate it by just iterating the GtkPrintBackends and calling gtk_print_backend_find_printer on each, passing in the name of the printer from the parent process. Does that sound like an OK approach?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Hrm. So I spoke too soon. All of the gtk_print_backend stuff seems to be private - I can't seem to call into it. It looks like the only publicly exposed mechanism for finding printers is by enumerating them. :/

(In reply to Karl Tomlinson (:karlt) from comment #11)
> https://reviewboard.mozilla.org/r/3331/#review2931
> 
> I don't have good ideas on how to deal with finding the GtkPrinter
> synchronously.  If it needs to pass between processes then perhaps
> determining
> the GtkPrinter can be delayed until printing, which is already an async
> process, I assume.

Perhaps - I'll investigate this next.

> 
> Is it necessary to pass the printer between content and browser processes? 
> Is it the printer selection or the printing that happens in the content
> process?  Could they both happen in the browser process?

Printer selection happens in the parent process, printing happens in the content process.

There is a long term goal of proxying the draw commands from the child so that all printing is kicked off in the parent (bug 1090454), but not something we're aiming on solving for the first version of e10s.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Note that the patches from bug 1134891 are needed to test this for the non-e10s case, as currently printing from non-e10s Nightly is busted for Linux and Windows.

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #12)
> I tried to paste my comments in the review page. 
> Middle button events seem to have been prevented

The workaround here is to uncheck "Enable Markdown".

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8559407 [details]
MozReview Request: bz://1090448/mconley

/r/3329 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=karlt.
/r/3331 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?
/r/4729 - Bug 1090448 - nsDeviceContextSpecG should not use GtkPrinter until the print job is ready. r=?
/r/3475 - Bug 1090448 - Enable printing with e10s windows on Linux. r=felipe.

Pull down these commits:

hg pull review -r 6d0cb15676be0d93fe5c2749d527e267e8b22b46

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Hey karlt - I messed up last week and accidentally posted my review request to bug 1088070, so that's where your old review feedback went. Sorry about that.

I believe I've addressed all but one of your issues:

(In reply to Karl Tomlinson (:karlt) from comment #18)
> https://reviewboard.mozilla.org/r/3791/#review3431
> ::: widget/gtk/nsDeviceContextSpecG.cpp
> (Diff revision 3)
> > +      gtk_enumerate_printers(&nsDeviceContextSpecGTK::PrinterEnumerator, this,
> 
> EndDocument() is called from the nsPrintData destructor, so I don't think we
> can assume that this is a safe place to run the event loop.  Instead dispatch
> an event that will call gtk_enumerate_printers().  There could be issues with
> asynchronicity if GetSurfaceForPrinter() calls are repeated after
> EndDocument(), so either assert this doesn't happen, or pass mSpoolFile
> to the event.
> 

Since I'm passing a nsDeviceContextSpecGTK in for the nsIRunnable, and the nsDeviceContextSpecGTK holds a reference to mSpoolFile... am I OK?

Comment 19

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/3329/#review2933

(In reply to Mike Conley (:mconley) - Needinfo me\! from bug 1088070 comment #19)
\> I've opted to keep the using, and just switch my usage of
\> mozilla::embedding::PrintData to just PrintData instead. Hope that's cool.

Yes, that's fine.  I don't think we have too many classes called PrintData.

Comment 20

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/3331/#review3855

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 4)
> +                           (const gchar*)pair.key().get(),
> +                           (const gchar*)pair.value().get());

gchar is just an typedef for char, so these casts should not be necessary.

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 4)
> +  GtkPrintSettings* newGtkPrintSettings = gtk_print_settings_new();

Need to move this to after all the early returns to avoid leaking.

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 4)
> +serialize_gtk_printsettings_to_printdata(const gchar *key,
> +                                         const gchar *value,
> +                                         gpointer aData)
> +{
> +  PrintData* data = (PrintData*)aData;
> +  KeyValue pair;
> +  pair.key() = *key;
> +  pair.value() = *value;

\*key and \*value or of type char, i.e. a single character, so I don't follow this.  Can you remove the \* or explain, please?

I don't know what type pair.key() has but I assumed it was nsCString&

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 4)
> +  if (!newGtkPrintSettings) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }

No need to null check here and creating new GObjects like GtkPrintSettings never returns null, but aborts on allocation failure.

The docs will say "or null" in the "returns" section if this is not the case.
https://developer.gnome.org/gtk2/2.24/GtkPrintSettings.html#gtk-print-settings-new

::: widget/gtk/nsPrintSettingsGTK.h
(Diff revision 4)
> +  friend class nsPrintOptionsGTK;

Can this be removed now?

::: widget/gtk/nsPrintSettingsGTK.cpp
(Diff revision 4)
> +  // Default values

Remove this comment now, please.
It's not strictly a default value, but means use what the destination prefers.

Comment 21

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/4729/#review3857

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> Since I'm passing a nsDeviceContextSpecGTK in for the nsIRunnable, and the
> nsDeviceContextSpecGTK holds a reference to mSpoolFile... am I OK?

You're OK as long as GetSurfaceForPrinter() is not called again.  It's not
clear from the API that this can't happen.
Please add an assert that mSpoolFile is null when GetSurfaceForPrinter()
is called, so we know if this ever happens.

::: widget/gtk/nsDeviceContextSpecG.cpp
(Diff revision 1)
> +  gtk_enumerate_printers(&nsDeviceContextSpecGTK::PrinterEnumerator, this,
> +                         nullptr, FALSE);

Now that this is run off an event, there is no other code on the stack below the previous event loop, so it is safe to run another event loop.

I think we need to pass wait=TRUE to detect network printers.

::: widget/gtk/nsPrintSettingsGTK.h
(Diff revision 1)
> +  bool GetAcceptsPDF() { return mAcceptsPDF; };
> +  void SetAcceptsPDF(bool aAcceptsPDF) {
> +    mAcceptsPDF = aAcceptsPDF;
> +  };

No longer needed.

::: widget/gtk/nsPrintSettingsGTK.h
(Diff revision 1)
> +  bool mAcceptsPDF;

No longer needed.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Bah - sloppy defects on my part. I'll do a proper self-review for the next set.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8559407 [details]
MozReview Request: bz://1090448/mconley

/r/3329 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=karlt.
/r/3331 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?
/r/4729 - Bug 1090448 - nsDeviceContextSpecG should not use GtkPrinter until the print job is ready. r=?
/r/3475 - Bug 1090448 - Enable printing with e10s windows on Linux. r=felipe.

Pull down these commits:

hg pull review -r 9cc8995e8800da0775e73c9a84f7bc4f245f5116

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8559407 [details]
MozReview Request: bz://1090448/mconley

/r/3329 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=karlt.
/r/3331 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?
/r/4729 - Bug 1090448 - nsDeviceContextSpecG should not use GtkPrinter until the print job is ready. r=?
/r/3475 - Bug 1090448 - Enable printing with e10s windows on Linux. r=felipe.

Pull down these commits:

hg pull review -r 9cc8995e8800da0775e73c9a84f7bc4f245f5116

Comment 25

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/3331/#review3909

::: widget/gtk/nsPrintSettingsGTK.h
(Diff revision 5)
> +  void SaveNewPageSize();

So this change will be unnecessary.

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 5)
> +  GtkPrintSettings* newGtkPrintSettings = gtk_print_settings_new();
> +  settingsGTK->SetGtkPrintSettings(newGtkPrintSettings);
> +  // nsPrintSettingsGTK is holding a reference to newGtkPrintSettings, so
> +  // we're OK to unref it here.
> +  g_object_unref(newGtkPrintSettings);
> +
>    nsresult rv = nsPrintOptions::DeserializeToPrintSettings(data, settings);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  for (uint32_t i = 0; i < data.GTKPrintSettings().Length(); ++i) {
> +    CStringKeyValue pair = data.GTKPrintSettings()[i];
> +    gtk_print_settings_set(newGtkPrintSettings,
> +                           pair.key().get(),
> +                           pair.value().get());
> +  }
> +
> +  settingsGTK->SaveNewPageSize();

Sorry I didn't spot this in the previous revision, but I'd like SetGtkPrintSettings() to be called after DeserializeToPrintSettings(), and after the gtk_print_settings_set calls.

That way we have the same GtkPrintSettings as we got from the other process, and SetGtkPrintSettings will call SaveNewPageSize() for us, so that there is no need to call again.

That also means the g_object_unref will be after all use in this method, instead of depending on the the nsPrintSettingsGTK to hold a ref.

Comment 26

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/4729/#review3913

Still the wait=TRUE issue to resolve.

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 8559407 [details]
MozReview Request: bz://1090448/mconley

Almost there, but the Deserialize ordering is a bit fiddly.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8559407 [details]
MozReview Request: bz://1090448/mconley

/r/3329 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=karlt.
/r/3331 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?
/r/4729 - Bug 1090448 - nsDeviceContextSpecG should not use GtkPrinter until the print job is ready. r=?
/r/3475 - Bug 1090448 - Enable printing with e10s windows on Linux. r=felipe.

Pull down these commits:

hg pull review -r 321d0ba027216e01471e8ff606c35a0d7f031f04

Comment 29

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/3331/#review4029

Ship It!

Comment 30

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/4729/#review4031

Ship It!

Comment 31

[Mike Conley (:mconley) (:⚙️)]

\o/ Thanks karlt!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0c609cf34814
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fc5e797ee40a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3cbae160df
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3dce86af200f

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c609cf34814
https://hg.mozilla.org/mozilla-central/rev/fc5e797ee40a
https://hg.mozilla.org/mozilla-central/rev/fb3cbae160df
https://hg.mozilla.org/mozilla-central/rev/3dce86af200f

Comment 34

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 39.0a1 (2015-03-04) using Ubuntu 14.04 32bit.

Verified fixed on Firefox 39.0a1 (2015-03-18) using Ubuntu 14.04 32bit.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1090448.html

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=karlt.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1090448 - Enable printing with e10s windows on Linux. r=felipe.

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1090448 - nsDeviceContextSpecG should not use GtkPrinter until the print job is ready. r=?