1088070 - Instantiate print settings from the content process instead of the parent

Status: RESOLVED FIXED

(Toolkit :: Printing, defect)

Description

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

When bug 1082575 lands, we'll have a content script doing half the work of making print / print preview happen.

That patch passes nsIPrintSettings CPOWs down to the content script, which are likely going to cause explosions when they get passed into the native printing machinery.

We should instantiate the print settings in the content process instead.

Comment 1

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

I found a work-around for this, but we should probably be getting the initial print settings from the content process instead of passing them down from the parent.

Comment 2

[dw-dev]

Hi Mike,

I am the developer of the Print Edit add-on.  

In case you are not aware, in Print Edit I have implemented quite an extensive set of workarounds and fixes which fully implement the Firefox print and print preview functionality in both local and remote browser windows.  This includes code to fully implement print settings.

Regards,

Dave

Comment 3

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

Initially, browser-content.js was receiving print settings via the message manager from printUtils. In the non-e10s case, we used these without difficulty. In the e10s case, the print settings were CPOWs, and were simply discarded. When we discarded them, we passed null into the print and print preview methods for the settings.

Passing null for the settings works for Windows, but it doesn't work for Linux. We need to properly initialize the print settings with platform-specific defaults. This is something that printUtils does, but we toss those defaults away in the e10s case.

So we need to create those print settings and use those defaults in the content process. Therefore, this bug blocks Linux printing with e10s.

Comment 4

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

Attachment: MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=?

Pull down this commit:

hg pull review -r 423e9a2931d7825620430b8431e4a16b68fac2db

Comment 5

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=?

Pull down this commit:

hg pull review -r 51fcfd44547694762dcb7ec346e07eebdeba3365

Comment 6

[dw-dev]

Thanks for the explanation - actually, I knew most of this since I did a detailed analysis of the changes made in printUtils.js and browser-content.js when I updated my Print Edit add-on to be compatible with e10s.

Passing null for the print settings works to an extent for Windows, but not supporting print settings is something that needs fixing pretty soon.  The latest version of Print Edit fully implements print settings in the content process.

Whilst analysing and testing the code in printUtils.js and browser-content.js, I noticed a couple of issues:

1. In printUtils.js, in line 530 in exitPrintPreview(), focusing the element throws an exception.  This can be worked around by changing line 465 in enterPrintPreview() to:  gFocusedElement = document.activeElement;

2. In browser-content.js, in line 463 in print(), the call to print.print() does not catch and ignore exceptions that occur if print is cancelled.

Also, whilst testing the e10s print functionality, I noticed a couple of more 'user-visible' issues, which I have raised as separate bug reports:

1130416 - 'Print' button on print preview toolbar does not work for remote browser

1130697 - Attempting to print preview or print "about:..." page throws an exception

Comment 7

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

Hey dw-dev - glad to have somebody else in here who knows about these printing interfaces! It feels a bit lonely sometimes. :)


(In reply to dw-dev from comment #6)
> Thanks for the explanation - actually, I knew most of this since I did a
> detailed analysis of the changes made in printUtils.js and
> browser-content.js when I updated my Print Edit add-on to be compatible with
> e10s.

You seem to know your way around the printing components. Perhaps you'd like to help fix up some of the remaining e10s issues?

> 
> Passing null for the print settings works to an extent for Windows, but not
> supporting print settings is something that needs fixing pretty soon.  The
> latest version of Print Edit fully implements print settings in the content
> process.

Cool - so, this patch I've posted changes things a bit; it doesn't change any of the printUtils APIs, but it makes it so that the print settings get initially constructed in the content process (and properly initialized). It's then serialized / sent to the parent for display and editing in the print settings dialogs, and the final print settings are then reserialized and sent back to the child.

> 
> Whilst analysing and testing the code in printUtils.js and
> browser-content.js, I noticed a couple of issues:
> 
> 1. In printUtils.js, in line 530 in exitPrintPreview(), focusing the element
> throws an exception.  This can be worked around by changing line 465 in
> enterPrintPreview() to:  gFocusedElement = document.activeElement;

I've filed bug 1131079 for this. Perhaps you'd be interested in submitting the patch?

> 
> 2. In browser-content.js, in line 463 in print(), the call to print.print()
> does not catch and ignore exceptions that occur if print is cancelled.
> 
> Also, whilst testing the e10s print functionality, I noticed a couple of
> more 'user-visible' issues, which I have raised as separate bug reports:
> 
> 1130416 - 'Print' button on print preview toolbar does not work for remote
> browser
> 

Yikes, and thanks - I've added it to our backlog for triage.

> 1130697 - Attempting to print preview or print "about:..." page throws an
> exception

Cool, and known - dupe'd to bug 1119563.

Comment 8

[dw-dev]

Mike,

Whilst developing and maintaining Print Edit over several years, I have become very familiar with the printing Javascript code - in fact, a lot more familiar than I wanted to be!  I'm happy to review changes and make suggestions, but I would prefer not to get involved in implementing the Firefox fixes, because I have more than enough to do developing and maintaining my add-ons.

I've reviewed the patch you have posted, and compared the code to some of the workarounds currently in my Print Edit add-on.  Generally, I think the patch is okay, but I do have some comments:

1. In printUtils.js, between lines 172 & 173, need to reinstate the following code:

        if (gPrintSettingsAreGlobal && gSavePrintSettings) {  /* missing in Firefox 36.0a1 */  /*???*/
          printEdit.psService.savePrintSettingsToPrefs(printSettings, true,
                                                       printSettings.kInitSaveAll);
          printEdit.psService.savePrintSettingsToPrefs(printSettings, false,
                                                       printSettings.kInitSavePrinterName);

2. In printUtils.js, line 460, as previously mentioned, needs changing to something like:

        gFocusedElement = document.activeElement;

3. In browser-content.js, in line 471, as previously mentioned, need to catch and ignore exceptions that occur if print is cancelled:

        try
        {
            print.print(printSettings, null);
        }
        catch (e)
        {
            /* Throws exception if print is cancelled */
        }

4.  In browser-content.js, in lines 412-428, I think the following would be more correct to handle global print settings, new print setings and possible exceptions:

        getPrintSettings: function () {
          let PSSVC = Cc["@mozilla.org/gfx/printsettings-service;1"]
                        .getService(Ci.nsIPrintSettingsService);

          let printSettings;
          try {
            let pref = Components.classes["@mozilla.org/preferences-service;1"]
                                 .getService(Components.interfaces.nsIPrefBranch);

            if (pref.getBoolPref("print.use_global_printsettings")) {
              printSettings = PSSVC.globalPrintSettings;
              if (!printSettings.printerName)
                printSettings.printerName = PSSVC.defaultPrinterName;

              // First get any defaults from the printer 
              PSSVC.initPrintSettingsFromPrinter(printSettings.printerName, printSettings);
              // now augment them with any values from last time
              PSSVC.initPrintSettingsFromPrefs(printSettings, true, printSettings.kInitSaveAll);
            } else {
              printSettings = PSSVC.newPrintSettings;
            }
          } catch (e) {
            dump("getPrintSettings: "+e+"\n");
          }
          return printSettings;
        }


In actual fact, the changes in printUtils.js for e10s have caused a lot of issues for my Print Edit add-on. The pre-e10s implementation always had to redefine a few of the functions in printUtils.js. However, with e10s some of the printUtils.js code has moved into functions in browser-content.js, and these functions cannot be redefined by an add-on (as far as I am aware). So, I now have to redefine the functions in printUtils.js more extensively, and redirect some of the messages to my own content script. Not nice!

Dave

Comment 9

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=?
/r/3785 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=?
/r/3787 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?
/r/3789 - Bug 1090448 - Enable printing with e10s windows on Linux. r=?
/r/3791 - Bug 1090439 - PPrinting calls from child to parent via ShowProgress and ShowPrintDialog should not be sync. r=?

Pull down these commits:

hg pull review -r efde56ce651f592c70fd4599f3055b9415829505

Comment 10

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=?
/r/3785 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=?
/r/3787 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?
/r/3791 - Bug 1090448 - nsDeviceContextSpecG should not use GtkPrinter until the print job is ready. r=?
/r/3789 - Bug 1090448 - Enable printing with e10s windows on Linux. r=?

Pull down these commits:

hg pull review -r 03757b3aa2ffb7d1bddc2127e717857e5fcad4d4

Comment 11

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

https://reviewboard.mozilla.org/r/3465/#review3327

::: toolkit/content/browser-content.js
(Diff revision 4)
> +      printSettings = this.getPrintSettings();

Assigning to undeclared variable

::: toolkit/content/browser-content.js
(Diff revision 4)
> -    // Bug 1088070 - we should instantiate nsIPrintSettings here in the
> +    printSettings = this.getPrintSettings();

Assigning to undeclared variable

Comment 12

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

https://reviewboard.mozilla.org/r/3787/#review3329

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 2)
> +

Nit - remove extra newline

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 2)
> +

Nit - remove extra newline

Comment 13

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

https://reviewboard.mozilla.org/r/3791/#review3331

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 2)
> +  // If we couldn't find a printer, we're sunk.

This comment is wrong. If we _do_ have a printer... that's the case that's unexpected.

Comment 14

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=?
/r/3785 - Bug 1090448 - Add initial serialization and deserialization capabilities for nsPrintOptionsGTK. r=?
/r/3787 - Bug 1090448 - Add GTK-specific PrintData fields and serialization / deserialization. r=?
/r/3791 - Bug 1090448 - nsDeviceContextSpecG should not use GtkPrinter until the print job is ready. r=?
/r/3789 - Bug 1090448 - Enable printing with e10s windows on Linux. r=?

Pull down these commits:

hg pull review -r 1f33b369fef3a3ad0136e6bd010f2632713f4a84

Comment 15

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/3465/#review3421

Comment 16

[Karl Tomlinson (:karlt back Feb 10)]

https://reviewboard.mozilla.org/r/3785/#review3371

::: widget/gtk/nsPrintOptionsGTK.h
(Diff revision 3)
> +#include "mozilla/embedding/PPrinting.h"

Usually we try to minimize includes of headers in headers.  The classes with pointers and references in the parameters to the new methods can be forward declared, I assume.

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 3)
> +using namespace mozilla::embedding;

I don't think this using statement is required.

Comment 17

[Karl Tomlinson (:karlt back Feb 10)]

https://reviewboard.mozilla.org/r/3787/#review3373

::: widget/gtk/nsPrintSettingsGTK.cpp
(Diff revision 3)
> +  SetDuplex(GTK_PRINT_DUPLEX_SIMPLEX);

I guess this is because nsPrintOptions::SerializeToPrintData() doesn't
serialize failure in GetDuplex() and so SetDuplex() asserts when called with
uninitialized values.

I'd prefer to deal with this in GetDuplex() as there is no guarantee that the setting will still be present when GetDuplex() is called.

GetDuplex() could return either a default, or a special value that SetDuplex() might recognise.  With the SetGtkPrintSettings() approach described above, the original set of settings (prior to serialization) will be restored anyway regardless of what GetDuplex returns.

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 3)
> +void
> +nsPrintOptionsGTK::SerializeGTKPrintSettingsToPrintData(const gchar *key,
> +                                                        const gchar *value,
> +                                                        gpointer aData)

Please use file-static internal linkage, or anonymous namespace, for this function, to keep it out of the header file.  It doesn't seem to need access to nsPrintOptionsGTK private members.

::: embedding/components/printingui/ipc/PPrinting.ipdl
(Diff revision 3)
> +struct KeyValue {
> +  nsCString key;
> +  nsCString value;
> +};

Perhaps this could be CStringKeyValue to be more specific.  I don't really mind.

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 3)
> +  GtkPageSetup* gtkPageSetup = settingsGTK->GetGtkPageSetup();
> +  NS_ENSURE_STATE(gtkPageSetup);
> +
> +  GtkPaperSize* gtkPaperSize = settingsGTK->mPaperSize;
> +  NS_ENSURE_STATE(gtkPaperSize);

There's no need to check that the nsPrintSettingsGTK has GtkPageSetup and GtkPaperSize if they are not used, right?

::: widget/gtk/nsPrintOptionsGTK.cpp
(Diff revision 3)
> +  // nsPrintSettingsGTK constructs and holds a reference to a GtkPrintSettings,
> +  // a GtkPageSetup and a GtkPaperSize on on construction. It also does the
> +  // job of assigning the created GtkPaperSize to the GtkPageSetup and
> +  // GtkPrintSettings. Not having any of these things in the settings that is
> +  // passed in is unexpected.
> +  GtkPrintSettings* gtkPrintSettings = settingsGTK->GetGtkPrintSettings();
> +  NS_ENSURE_STATE(gtkPrintSettings);
> +
> +  GtkPageSetup* gtkPageSetup = settingsGTK->GetGtkPageSetup();
> +  NS_ENSURE_STATE(gtkPageSetup);
> +
> +  NS_ENSURE_STATE(settingsGTK->mPaperSize);
> +
> +  for (uint32_t i = 0; i < data.GTKPrintSettings().Length(); ++i) {
> +    KeyValue pair = data.GTKPrintSettings()[i];
> +    gtk_print_settings_set(gtkPrintSettings,
> +                           (const gchar*)pair.key().get(),
> +                           (const gchar*)pair.value().get());
> +  }
> +
> +  settingsGTK->SaveNewPageSize();

Thanks for this comment.  It helps to explain what is happening.

I'd prefer to avoid using friend classes and instead make some necessary
methods public if necessary, but this is probably not necessary.

IIUC nsPrintOptions::DeserializeToPrintSettings() will call
SetPaperName/Width/Height to save the paper size in various places, including
the GtkPrintSettings.

gtk_print_settings_set() will then change the GtkPrintSettings to have the
values from the GtkPrintSettings in the other process.

And then the explicit SaveNewPageSize() will restore the values in the
GtkPrintSettings to be those from SetPaperName/Width/Height.

If the values from different sources are equivalent, then there is no need for
SaveNewPageSize(), but I suspect there may be variations due to the various
conversions.

I suspect it would be simple and provide a better representation of the
original state if nsPrintOptionsGTK::DeserializeToPrintSettings() were to
use gtk_print_settings_new() and call nsPrintSettingsGTK::SetGtkPrintSettings().

Then I think its fine to drop the early returns for failed checks on
GetGtkPrintSettings() and the private ->mPaperSize.  If you'd like to keep the
GetGtkPageSetup() check, I'd prefer to make it an assertion within #ifdef DEBUG (instead of an early return), to clarify that it is just a check that the code is behaving as expected and the page setup is not being fetched for use.

Comment 18

[Karl Tomlinson (:karlt back Feb 10)]

https://reviewboard.mozilla.org/r/3791/#review3431

This looks close.

::: widget/gtk/nsDeviceContextSpecG.h
(Diff revision 3)
> -  
> +  static gboolean PrinterEnumerator(GtkPrinter *aPrinter, gpointer aData);
> +  static void StartPrintJob(nsDeviceContextSpecGTK *spec,

Can these new methods be protected or private, please?

::: widget/gtk/nsPrintDialogGTK.cpp
(Diff revision 3)
> +      aNSSettingsGTK->SetAcceptsPDF(canReadAcceptsPDF &&
> +                                    gtk_printer_accepts_pdf(printer));

This is OK if you initialize mAcceptsPDF in the nsPrintSettingsGTK() constructor, but perhaps a simpler approach than serializing AcceptsPDF is to call SetOutputFormat() here or in nsPrintSettingsGTK::SetGtkPrinter().

::: 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.

::: widget/gtk/nsDeviceContextSpecG.h
(Diff revision 3)
>    nsCOMPtr<nsIPrintSettings> mPrintSettings;
> +  nsCOMPtr<nsPrintSettingsGTK> mPrintSettingsGTK;

Please use only one pointer to the PrintSettings, as these are the same object.
nsPrintSettingsGTK is fine, if necessary, and has the nsIPrintSettings methods.  I don't mind whether the pointer is called mPrintSettings or mPrintSettingsGTK, but it may be simpler to keep calling it mPrintSettings.

::: widget/gtk/nsDeviceContextSpecG.cpp
(Diff revision 3)
> +  mTitle = NS_ConvertUTF16toUTF8(aTitle);

NS_ConvertUTF16toUTF8 is a class holding an extra copy of the string, so please use mTitle.Truncate() and AppendUTF16toUTF8(aTitle, mTitle)

Comment 19

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

https://reviewboard.mozilla.org/r/3785/#review3497

> I don't think this using statement is required.

Agreed - but I've opted to keep the using, and just switch my usage of mozilla::embedding::PrintData to just PrintData instead. Hope that's cool.

Comment 20

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

So, I kind of screwed up, had a MozReview fail, and accidentally my review request for bug 1090448 went into this bug. So that's why karlt's feedback is in here.

dw-dev's feedback drew my attention to bug 1136855, which I've recently fixed, but which also makes this bug a little more complicated.

Specifically, if I want to be able to save the print settings, I have to somehow send them back to the parent process. If the print settings are instantiated in the content process, that means they'll be a CPOW in the parent, and a CPOW can't get passed into native code (which the print settings saving stuff is).

I think my plan is to create an nsIPrintSettingsService proxy for the content process, which will do the job of querying the parent for print settings, and sending them off to the parent again when it's time to save. This proxy can then make use of the PrintData struct I already use for serializing print settings for the dialogs.

Comment 21

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=?
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=?
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r aaecdd04279dbc9a851fc2bc8a21eb4ca3a1892c

Comment 22

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=?
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=?
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r aaecdd04279dbc9a851fc2bc8a21eb4ca3a1892c

Comment 23

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

https://reviewboard.mozilla.org/r/3791/#review3711

::: widget/nsPrintOptionsImpl.cpp
(Diff revision 4)
> +

Comment about what we're doing.

::: embedding/components/printingui/ipc/nsPrintingProxy.cpp
(Diff revision 4)
> +/**
> + * nsIPrintingPromptService
> + */

Not necessary at this point.

::: embedding/components/printingui/ipc/nsPrintingProxy.cpp
(Diff revision 4)
> +

Remove extra newline

::: embedding/components/printingui/ipc/nsPrintingProxy.h
(Diff revision 4)
> +#include "nsIPrintSettingsService.h"

Not needed.

Comment 24

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=?
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 7488cca660b66aed3ad1978ccbe7e38ff4522cb9

Comment 25

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=?
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 7488cca660b66aed3ad1978ccbe7e38ff4522cb9

Comment 26

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=?
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 7488cca660b66aed3ad1978ccbe7e38ff4522cb9

Comment 27

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=?
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 7488cca660b66aed3ad1978ccbe7e38ff4522cb9

Comment 28

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=?
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 7488cca660b66aed3ad1978ccbe7e38ff4522cb9

Comment 29

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=?
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 7488cca660b66aed3ad1978ccbe7e38ff4522cb9

Comment 30

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

Something is going pretty wrong with MozReview right now when I try to assign reviewers, hence the spam. Sorry about that. :(

Hoping Mossop can review "Move saving nsIPrintSettings after a print job to browser-content.js. r=?" and smaug can review "Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?".

That last patch, "If saving print settings in the content process, proxy to the parent. r=?", I think I have a few more things I want to fix on it first.

Comment 31

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/3785/#review3723

Comment 32

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=Mossop.
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 5c91ee2a60a6ac5fd0754ca9626c238eca494cfa

Comment 33

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=Mossop.
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 5c91ee2a60a6ac5fd0754ca9626c238eca494cfa

Comment 34

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=Mossop.
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 5c91ee2a60a6ac5fd0754ca9626c238eca494cfa

Comment 35

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=Mossop.
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 5c91ee2a60a6ac5fd0754ca9626c238eca494cfa

Comment 36

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=Mossop.
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r 5c91ee2a60a6ac5fd0754ca9626c238eca494cfa

Comment 37

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

Ok, that last patch ("If saving print settings in the content process, proxy to the parent.") is ready for reviewing now, smaug.

Comment 38

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

Attachment: Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=?

Comment 39

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

Attachment: If saving print settings in the content process, proxy to the parent. r=?

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8571608 [details] [diff] [review]
If saving print settings in the content process, proxy to the parent. r=?

>+PrintingParent::RecvSavePrintSettings(const PrintData& data,
>+                                      const bool& usePrinterNamePrefix,
>+                                      const uint32_t& flags,
>+                                      nsresult* rv)
It would be nice if new code used mozilla coding style. So, aFoo naming for arguments.

>+/* static */
>+already_AddRefed<nsPrintingProxy>
>+nsPrintingProxy::GetInstance()
>+{
>+  if (!sPrintingProxyInstance) {
>+    sPrintingProxyInstance = new nsPrintingProxy();
>+    if (!sPrintingProxyInstance) {
>+      return nullptr;
>+    }
>+    NS_ADDREF(sPrintingProxyInstance);
>+    nsresult rv = sPrintingProxyInstance->Init();
>+    if (NS_FAILED(rv)) {
>+      NS_RELEASE(sPrintingProxyInstance);
>+      return nullptr;
>+    }
>+  }
>+
>+  nsRefPtr<nsPrintingProxy> inst = sPrintingProxyInstance;
>+  return inst.forget();
>+}
Don't we leak sPrintingProxyInstance here?
Did you test this in debug build with
XPCOM_MEM_LEAK_LOG=1

Comment 41

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=Mossop.
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=smaug.
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=?

Pull down these commits:

hg pull review -r f591789194dc1fe04f4c927a3b0ad46b69f4d6dd

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

.h could use Mozilla coding style for arguments too.

Interesting, didn't know about ClearOnShutdown.

Comment 43

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

Comment on attachment 8560642 [details]
MozReview Request: bz://1088070/mconley

/r/3465 - Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.
/r/3785 - Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=Mossop.
/r/3787 - Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=smaug.
/r/3791 - Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=smaug.

Pull down these commits:

hg pull review -r d9a490909b92e9433f1f708c0e8e0a4a430722b4

Comment 44

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

Thanks!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/20a6d6e9c31a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d053fdf3b609
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c1f919b5b8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e04b9c921ad

Comment 45

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7236221&repo=mozilla-inbound

Comment 46

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

Thanks - investigating...

Comment 47

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

It turns out that the getPrintSettings equivalent code in printUtils.js wraps interactions with the nsIPrintSettingsService in a try/catch, and returns null in the event of a failure (which is fine, since calling print / printPreview in nsIWebBrowserPrint with null for the nsIPrintSettings just causes the print engine to default to the global print settings).

I wasn't doing that try/catch wrapping in browser-content.js. I've fixed that, and pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5aa918d76fe

If that goes green, I'll push to inbound - I don't think I need another round of review just for a try/catch wrap.

Comment 48

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

Looks green - relanding:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e58f9c6913b7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9ad6279fa6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8893464d6b2d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe733cadbb6

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e58f9c6913b7
https://hg.mozilla.org/mozilla-central/rev/1c9ad6279fa6
https://hg.mozilla.org/mozilla-central/rev/8893464d6b2d
https://hg.mozilla.org/mozilla-central/rev/dbe733cadbb6

Comment 50

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

Attachment: Bugnotes

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

Comment 52

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

Attachment: MozReview Request: Bug 1088070 - Instantiate print settings from the content process instead of the parent. r=Mossop.

Comment 53

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

Attachment: MozReview Request: Bug 1088070 - Move saving nsIPrintSettings after a print job to browser-content.js. r=Mossop.

Comment 54

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

Attachment: MozReview Request: Bug 1088070 - Rename nsPrintingPromptServiceProxy to nsPrintingProxy. r=smaug.

Comment 55

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

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

Comment 56

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

Attachment: MozReview Request: Bug 1088070 - If saving print settings in the content process, proxy to the parent. r=smaug.