Closed Bug 1082579 Opened 5 years ago Closed 5 years ago

Introduce PPrinting.ipdl and proxies for opening printing UI

Categories

(Core :: Printing: Output, defect)

32 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m3+ ---

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(2 files, 7 obsolete files)

66.72 KB, patch
mconley
: review+
Details | Diff | Splinter Review
10.25 KB, application/zip
Details
nsPrintEngine instantiates several XPCOM components whose purpose is to open platform-specific printing dialogs (like page setup, print settings, print progress). When nsPrintEngine is being run from within the child process, these components fail because the content process is not able to open windows.

We should introduce a PPrinting IPDL, as well as proxy components to open the dialogs in the parent process, and return any results back to the child.
Comment on attachment 8512065 [details] [diff] [review]
Introduce PPrinting.ipdl and proxies for opening printing UI. r=?

Ok, I think I'm ready for somebody to kick the tires here.

This patch needs to be applied on top of the patch for bug 1082575, and the following pref needs to be set to true:

print.enable_e10s_testing

With this patch (only tested on debug builds) in e10s windows, here's what works:

1) I can print preview on Windows and Linux
2) I can click print on Windows, and have the print settings dialog come up, and then kick off a print job.

Here's what's broken:

1) The print progress dialog that comes up when getting ready to send data to the printer does not show progress nor go away on its own.
2) Printing on Linux - I'm not populating the nsIPrintSettingsGtk with the printer device pointer on the child side after showing the dialog.

Here's what's untested:

1) Printing on OSX - this was the first one to work (since it didn't require messaging to the parent), but I haven't tried it lately with this patch.
Attachment #8512065 - Flags: review?(blassey.bugs)
Attachment #8512065 - Attachment is obsolete: true
Attachment #8512065 - Flags: review?(blassey.bugs)
Comment on attachment 8512206 [details] [diff] [review]
Introduce PPrinting.ipdl and proxies for opening printing UI. r=?

This fixes a build-time error with a public destructor for MockWebBrowserPrint. Also, adds a PROXY_PRINTING ifdef that special-cases OS X so that we don't use nsPrintingPromptServiceProxy for it (since OS X allows us to open dialogs from the child process, it seems).

This seems to get printing up and running on OS X and Windows then.
Attachment #8512206 - Flags: review?(blassey.bugs)
So one thing that smaug pointed out is that my PPrinting protocol is sync, which means that the content process is fully blocked while waiting for the user to do something with the print settings dialog.

Normally what happens is the print settings dialog has its own inner event loop which feeds events back up to the main loop.

We should probably simulate that in the nsPrintingPromptServiceProxy - otherwise, users with multiple e10s windows will find their web content totally blocked with the printing dialog open.

We can / should probably fix that in a follow-up, however.
Comment on attachment 8512206 [details] [diff] [review]
Introduce PPrinting.ipdl and proxies for opening printing UI. r=?

Review of attachment 8512206 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsPrintOptionsWin.h
@@ +24,5 @@
>  
> +  virtual nsresult
> +  SerializeToPrintData(nsIPrintSettings* settings,
> +                       nsIWebBrowserPrint* wbp,
> +                       mozilla::embedding::PrintData* data);

let's put this in nsIPrintOptions so we can avoid the static_cast in ShowPrintDialog()
Attachment #8512206 - Flags: review?(blassey.bugs) → review-
Attachment #8512206 - Attachment is obsolete: true
Attachment #8512206 - Flags: feedback?(jmathies)
Comment on attachment 8512328 [details] [diff] [review]
Introduce PPrinting.ipdl and proxies for opening printing UI. r=?

I've moved the serialization/deserialization methods into the nsIPrintOptions IDL as requested. Also did a little more clean-up, and added a (temporary) pref to enable printing on OS X and Windows, but keep it disabled on Linux.
Attachment #8512328 - Flags: review?(blassey.bugs)
Attachment #8512328 - Attachment is obsolete: true
Attachment #8512328 - Flags: review?(blassey.bugs)
Comment on attachment 8512328 [details] [diff] [review]
Introduce PPrinting.ipdl and proxies for opening printing UI. r=?

Review of attachment 8512328 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsPrintOptionsWin.cpp
@@ +66,5 @@
> +  devName.Assign(deviceName);
> +  drivName.Assign(driverName);
> +
> +  data->deviceName() = devName;
> +  data->driverName() = drivName;

data->deviceName().Assign(deviceName);
data->driverName().Assign(driverName);

...and get rid of the nsAutoString
Attachment #8512328 - Attachment is obsolete: false
Attachment #8512360 - Flags: review?(blassey.bugs) → review+
Comment on attachment 8512719 [details] [diff] [review]
Introduce PPrinting.ipdl and proxies for opening printing UI (r=blassey)

Got rid of the nsAutoString, and doing smarter string manipulation in nsPrintOptionsWin::SerializeToPrintData.

Try push with patch from bug 1082575:

https://tbpl.mozilla.org/?tree=Try&rev=f1aa062a86f9
Attachment #8512719 - Attachment description: Introduce PPrinting.ipdl and proxies for opening printing UI → Introduce PPrinting.ipdl and proxies for opening printing UI (r=blassey)
Attachment #8512719 - Flags: review+
Landed in fx-team (with an OSX build-time fix that put the nsPrintingPromptServiceProxyConstructor creation behind the PROXY_PRINTING ifdef):

https://hg.mozilla.org/integration/fx-team/rev/a9530d7f005a
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a9530d7f005a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Depends on: 1103412
Depends on: 1117936
My previous followup-patch changed "class" to "struct" in a forward-declare, which seemed reasonable, since PrintData is defined as "struct PrintData {" in PPrintingTypes.ipdlh.

BUT, apparently "struct" in IPDL actually generates "class" in .h files. So, "class" was correct after all.

So, I pushed another followup to revert comment 21, and to change the one forward-declare that used "struct" to use "class" instead:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a89d66fd2b6a
Depends on: 1144751
You need to log in before you can comment on or make changes to this bug.