629500 - Remove generic print dialogs and massively refactor that layer of the printing system

Status: RESOLVED DUPLICATE of bug 133787

(Toolkit :: Printing, defect)

Description

[Zack Weinberg (:zwol)]

This patch series removes the generic XUL-based print and page setup dialogs, removes a bunch of support code that existed mostly for their benefit or was not being used at all, and then reorganizes what remains in that layer to some approximation of tidiness.

Specifically: printPageSetup.xul, printdialog.xul, and printjoboptions.xul are removed, as is everything that existed solely to support them.  nsIPrintingPromptService is relieved of responsibility for raising the print and page setup dialogs; that's now done exclusively by nsIPrintDialogService, which is now required of every widget implementation.  The code for the print *progress* dialog boxes is consolidated into one XPCOM interface (nsIPrintProgress) and one location (toolkit/components/printing).  nsIPrintSettingsService is persuaded not to care quite so much about the name of the printer, allowing nsIPrinterEnumerator to be removed.  Finally, I removed a bunch of methods and XPCOM interfaces that weren't being used by *anything*.

I was fairly aggressive about correcting style issues, obsolete logic, and just plain *silly* code en passant.  This code hasn't had anyone blow the dust off it in a good long time, it seems.  And this is by no means all of the cleanup that could be done in this area.  The print progress and print preview logic (now mostly in toolkit/components/printing, with some help from layout/printing) deserves further attention, probably from someone better at XUL and the JS/C++ boundary than I am.  So do the various printing back-ends.  The next thing *I* intend to tackle is the Gtk+ printing back-end, and I expect that this will involve a total rewrite of nsIPrintSettings(Service) with yet more knock-on effects on other platforms; hopefully it will be confined to widget/.

There is an important missing piece which should be implemented before this lands: the Windows implementation of nsIPrintDialogService does not implement the page-setup dialog box.  I attempted this and determined that it was well beyond my Windows-fu.  OS/2 and Qt are left without any print dialogs at all, but not being primary platforms, that can presumably be fixed in follow-up bugs (although I would be happy to add patches to the list in this bug, should they appear).  And I did not touch the BeOS printing code at all, on the assumption that it would be removed shortly after FF4.0 branches (see bug 627277).

Further, this is going to need manual testing on all primary platforms.  There is almost no automated testing of this code AFAIK.  I have verified that the Linux print dialogs are no more broken than they were before I started, but this is not saying very much.  I am particularly concerned about preservation of print settings across print jobs, which has never worked properly on Linux and is therefore hard for me to vet; also, the OSX user experience may have changed in undesirable ways (specifically, there might now be two print-progress boxes shown simultaneously).  The most recent set of try server builds for this series is currently at http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-dad40c5dbe03/ .

One final caveat: due to aggressive rearrangement of files, this might want to wait for Mercurial bug http://mercurial.selenic.com/bts/issue1576 to be fixed before landing, but personally I don't think so.

Please feel free to reassign review and super-review tags to whoever you feel is more appropriate; this being, again, a very dusty part of the codebase, there are not obvious reviewer choices for much of it.

Comment 1

[Matt Brubeck (:mbrubeck)]

Brief manual testing of the try build in comment 0 shows no regressions in printing functionality on Android (where we use it only for the "Save As PDF" command).

Comment 2

[Zack Weinberg (:zwol)]

Attachment: 1/19: Remove the generic XUL-based print and page setup dialogs from toolkit/.

Comment 3

[Zack Weinberg (:zwol)]

Attachment: 2/19: Remove all references to the old print dialogs from all implementations of nsIPrintingPromptService, etc

Comment 4

[Zack Weinberg (:zwol)]

Attachment: 3/19: Remove two totally unused nsIPrintOptions methods.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: 4/19: Remove nsIPrintSettingsService.defaultPrinterName.

Comment 6

[Zack Weinberg (:zwol)]

Attachment: 5/19: Move the backend-specific code behind nsIPrintSettingsService::InitPrintSettingsFromPrinter from nsIPrinterEnumerator to nsIDeviceContextSpec.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: 6/19: Unconditionally run the print preview tests.

Comment 8

[Zack Weinberg (:zwol)]

Attachment: 7/19: Use GetDefaultPrinterW() on Windows instead of nsIPrinterEnumerator (which used GetProfileString!); remove the nsIPrinterEnumerator impl.

Comment 9

[Zack Weinberg (:zwol)]

Attachment: 8/19: Remove the remaining nsIPrinterEnumerator implementations, which are now unused.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: 9/19: Remove no-longer-used code to poke libcups for a printer list.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: 10/19: Remove nsIContentViewerFile and all references to nsIPrintOptions outside widget/.

Comment 12

[Zack Weinberg (:zwol)]

Attachment: 11/19: Fold nsIPrintOptions into nsIPrintSettingsService.

Comment 13

[Zack Weinberg (:zwol)]

Attachment: 12/19: Rename all nsIPrintSettingsService implementation files and class names from nsPrintOptionsWhatever to nsPrintSettingsServiceWhatever. Some cruft removal en passant.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: 13/19: Move printingui/src/win/nsPrintDialogUtil.* to widget/src/windows, and convert it to a nsIPrintDialogService implementation.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: 14/19: Kill another totally unused interface, nsIPrintStatusFeedback.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: 15/19: Move remaining contents of embedding/components/printingui to toolkit/components/printing and merge cross-platform.

Comment 17

[Zack Weinberg (:zwol)]

Attachment: 16/19: Make the PrintDialogService scriptable, and use it directly for print and page setup dialogs instead of going through the PrintingPromptService.

Comment 18

[Zack Weinberg (:zwol)]

Attachment: 17/19: Remove the unnecessary Init method from nsIPrintDialogService, and rename the vague Show method to ShowPrint.

Comment 19

[Zack Weinberg (:zwol)]

Attachment: 18/19: Kill ns(I)PrintingPromptService; use ns(I)PrintProgress directly. En passant chainsaw cleanup of printProgress.js and printPreviewProgress.js.

Comment 20

[Zack Weinberg (:zwol)]

Attachment: 19/19: Fold ns(I)PrintProgressParams into ns(I)PrintProgress.

Comment 21

[Zack Weinberg (:zwol)]

(In reply to comment #1)
> Brief manual testing of the try build in comment 0 shows no regressions in
> printing functionality on Android (where we use it only for the "Save As PDF"
> command).

Good to hear; thanks for the quick turnaround.

I meant to ask, is there a keyword or whiteboard token for labeling bugs as in need of manual testing?

Also, does anyone have a hg extension for bombing bugzilla with a patch series?  ;-)

Comment 22

[Matt Brubeck (:mbrubeck)]

(In reply to comment #21)
> Also, does anyone have a hg extension for bombing bugzilla with a patch series?

Ted has a bzexport extension for hg that can help:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/bzexport/

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 507646 [details] [diff] [review]
9/19: Remove no-longer-used code to poke libcups for a printer list.

Thank you, Zack for diving into this.  It definitely needs this clean-out.

This was fairly easy to review because
print.tmp.printerfeatures.can_change_spoolercommand and
.supports_spoolercommand_change and nsIPrintSettings::printCommand are only
used in printjoboptions.js, which is not used for this backend.

Would this be a good place to remove the print_command prefs from all.js (or
at least the postscript.print_command variants)?  And maybe the references to
print_command in nsPrintOptionsImpl.cpp?

Comment 24

[Zack Weinberg (:zwol)]

(In reply to comment #23)
> Comment on attachment 507646 [details] [diff] [review]
> 
> This was fairly easy to review because
> print.tmp.printerfeatures.can_change_spoolercommand and
> .supports_spoolercommand_change and nsIPrintSettings::printCommand are only
> used in printjoboptions.js, which is not used for this backend.

You may not have noticed, but an earlier patch in the series deletes printjoboptions.js ;-)  *Nobody* used that one.

> Would this be a good place to remove the print_command prefs from all.js (or
> at least the postscript.print_command variants)?  And maybe the references to
> print_command in nsPrintOptionsImpl.cpp?

Possibly.  I didn't look very hard at nsPrintSettings* in this cycle (note: nsPrintOptionsImpl.cpp becomes nsPrintSettingsService.cpp at some point in the series) but I can look for obviously dead settings in there tomorrow and maybe throw a 20th patch on the queue.

Comment 25

[Philip Chee]

CC OS/2 people

Comment 26

[Zack Weinberg (:zwol)]

FYI, an up-to-date try server batch (trunk as of yesterday afternoon + the patches as posted to this bug, which was not quite the case for the previous link I gave) is at

http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-a021a26d725f/

and the build results are here:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=a021a26d725f

It looks like all green with a few unrelated known oranges.  I'm not sure what's going on with maemo M2 and M3 (the logs are uninformative) but other people seem to get that too and maemo's hidden on trunk, so I assume it is not my fault.

Comment 27

[Zack Weinberg (:zwol)]

(In reply to comment #24)
> I didn't look very hard at nsPrintSettings* in this cycle (note:
> nsPrintOptionsImpl.cpp becomes nsPrintSettingsService.cpp at some 
> point in the series) but I can look for obviously dead settings
> in there tomorrow and maybe throw a 20th patch on the queue.

Figuring out exactly which settings are dead is hairy enough that I'd rather do it in the next patchset, with the anticipated total overhaul of ns*PrintSettings*.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 507654 [details] [diff] [review]
17/19: Remove the unnecessary Init method from nsIPrintDialogService, and rename the vague Show method to ShowPrint.

I think showPrintDialog would be an even better name, but I don't mind either way.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I recommend changing the sr for all the rest of the patches to Olli.

Comment 30

[Zack Weinberg (:zwol)]

(In reply to comment #28)
> Comment on attachment 507654 [details] [diff] [review]
> 17/19: Remove the unnecessary Init method from nsIPrintDialogService, and
> rename the vague Show method to ShowPrint.
> 
> I think showPrintDialog would be an even better name, but I don't mind either
> way.

I did it that way for consistency with ShowPageSetup.  I wouldn't mind making them ShowPrintDialog and ShowPageSetupDialog, but I think either both or neither should have the suffix.

(In reply to comment #29)
> I recommend changing the sr for all the rest of the patches to Olli.

Done.  It makes sense for the same person to sr the whole thing.  (Also, stuck you with the r? for something that I'd asked vlad to do the basic review on.)

Comment 31

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

I wish http://mercurial.selenic.com/bts/issue1576 was finally fixed.

Comment 32

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

Zack, do you have some preferred release to which you'd like to get these
changes in? Fx5 or Fx6
(I'm just trying to figure out whether to spend few days
during next two weeks or so to review this, or can I postpone
reviewing to happen after that.)

Though, since hg is still broken, I wouldn't want the
file moves to be pushed to m-c anyway.

Comment 33

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

Zack, do you have some preferred release to which you'd like to get these
changes in? Fx5 or Fx6
(I'm just trying to figure out whether to spend few days
during next two weeks or so to review this, or can I postpone
reviewing to happen after that.)

Though, since hg is still broken, I wouldn't want the
file moves to be pushed to m-c anyway.

Comment 34

[Zack Weinberg (:zwol)]

They're not that interesting without follow-up code that has yet to be written, and I still need to bait someone into doing the Windows page setup dialog before it can land at all.  So I think Fx6 is a more reasonable timeframe for this stuff.

I still object to gating it on a Mercurial bug, though.

Comment 35

[Zack Weinberg (:zwol)]

Frank Yan suggested that either Rob Strong or Jim Matthies might be the person who could write the missing Windows page setup dialog box for this bug.

Comment 36

[Zack Weinberg (:zwol)]

also cc:ing the ux team per offline discussion.

Comment 37

[Zack Weinberg (:zwol)]

Comment on attachment 507633 [details] [diff] [review]
1/19: Remove the generic XUL-based print and page setup dialogs from toolkit/.

Boriss and Limi suggested that I bounce review of all as-yet-unreviewed patches to Justin Dolske.  Justin: these are all front-end XUL stuff except for the occasional piece of horribly low-level OS-specific goo.  Please feel free to re-reassign to anyone you think is more appropriate.

Comment 38

[Robert Strong (they/them - no direct email)]

(In reply to comment #35)
> Frank Yan suggested that either Rob Strong or Jim Matthies might be the person
> who could write the missing Windows page setup dialog box for this bug.
It is extremely unlikely that I will have time to do so. Not sure if it is possible to use the native one but here is a link
http://msdn.microsoft.com/en-us/library/ms646962%28v=vs.85%29.aspx

Comment 39

[Zack Weinberg (:zwol)]

Comment on attachment 507656 [details] [diff] [review]
19/19: Fold ns(I)PrintProgressParams into ns(I)PrintProgress.

Note that patches 18 and 19 restructure code that is now slated to be totally removed per bug 650960, so perhaps there is no point to them anymore.

Comment 40

[Zack Weinberg (:zwol)]

(In reply to comment #38)
> (In reply to comment #35)
> > Frank Yan suggested that either Rob Strong or Jim Matthies might be the person
> > who could write the missing Windows page setup dialog box for this bug.
> It is extremely unlikely that I will have time to do so. Not sure if it is
> possible to use the native one but here is a link
> http://msdn.microsoft.com/en-us/library/ms646962%28v=vs.85%29.aspx

That is exactly the thing that I think we should use, as a new patch on top of the existing series in this bug (the code would go in widget/src/windows/nsPrintDialogWin.cpp, formerly known as embedding/components/printingui/src/win/nsPrintDialogUtil.cpp) but I am not good enough at Windows GUI programming to do it myself, even cribbing liberally from MSDN.

Comment 41

[Justin Dolske [:Dolske]]

Before I dive into this, what exactly would the state of Windows be wrt the current "Page Setup" knobs/settings? Sounds like they're totally gone, and in need of someone to implement a new replacement? (But Mac/Linux have some replacement?)

Comment 42

[Zack Weinberg (:zwol)]

With the patch series as is, I believe the "Page Setup" menu item would be nonfunctional on Windows and there would be no way to access those settings.  I have no intention of landing this patch series until someone reimplements the Windows page setup dialog; however, review would still be useful before that happens, as I could then address any other problems in advance of the missing piece getting coded.  It is a "simple" matter of writing raw Windows GUI code to implement nsIPrintDialogService::ShowPageSetup().

Mac and Linux already implement that function, and therefore are already not using the XUL-based page setup dialog that this patch series removes.

Comment 43

[Justin Dolske [:Dolske]]

I think I know a guy who may be up to implementing the Windows site of this. :) I'll start skimming the patches soon to better understand what needs implemented and work on making that happen.

Comment 44

[Zack Weinberg (:zwol)]

Cool beans.  FYI, I am not going to have much time for this bug or any related work until August-ish.

Comment 45

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

Sorry about not-reviewing this, but it isn't quite clear to me when
the review is needed.
"There is an important missing piece which should be implemented before this lands: the Windows implementation of nsIPrintDialogService does not implement the page-setup dialog box."
Should I wait until dolske finds someone to do that?

Note, I should start fixing printing for e10s soon, and it may or may not
overlap with this all.

Comment 46

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

(In reply to Olli Pettay [:smaug] from comment #45)
> Sorry about not-reviewing this, but it isn't quite clear to me when
> the review is needed.
Zack, any comments to this?

Comment 47

[Zack Weinberg (:zwol)]

(In reply to Olli Pettay [:smaug] from comment #46)
> (In reply to Olli Pettay [:smaug] from comment #45)
> > Sorry about not-reviewing this, but it isn't quite clear to me when
> > the review is needed.
> Zack, any comments to this?

I'm sorry, I thought I had replied to this, but apparently I didn't.

The patch series cannot *land* until someone implements the missing piece.  However, the missing piece is self-contained and slots into an existing API, so it is very unlikely to necessitate changes to the rest of the patch series.

Therefore, review / super-review of the remainder of the series does not need to wait for the missing piece.

I will not be able to revise the patch series till September, though, so please prioritize accordingly.

Comment 48

[Justin Dolske [:Dolske]]

(In reply to Zack Weinberg (:zwol) from comment #47)

> I will not be able to revise the patch series till September, though, so
> please prioritize accordingly.

When you next do, could you attach / linkto an Hg bundle (or repo) that includes all the parts of this patch? I'm going to start going through these, but for the first pass of reviews I'll just be doing code inspection since downloading 19 attachments is a PITA. ;-)

Comment 49

[Justin Dolske [:Dolske]]

Comment on attachment 507633 [details] [diff] [review]
1/19: Remove the generic XUL-based print and page setup dialogs from toolkit/.

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

::: toolkit/components/printing/content/printdialog.js
@@ -44,5 @@
> -var printService       = null;
> -var gOriginalNumCopies = 1;
> -
> -var paramBlock;
> -var gPrefs             = null;

Note to future self: Audit final patch to remove any now-dead default prefs.

::: toolkit/components/printing/content/printdialog.xul
@@ -55,5 @@
> -  screenX="24" screenY="24">
> -
> -  <script type="application/javascript" src="chrome://global/content/printdialog.js"/>
> -
> -  <stringbundle id="printingBundle" src="chrome://global/locale/printing.properties"/>

There are presumably unused strings in the .properties file now that this .xul is being removed (perhaps another patch in this series blows away this while file?).

::: toolkit/components/printing/content/printjoboptions.xul
@@ -52,5 @@
> -  screenX="24" screenY="24">
> -
> -  <script type="application/javascript" src="chrome://global/content/printjoboptions.js"/>
> -
> -  <stringbundle id="printBundle" src="chrome://global/locale/printPageSetup.properties"/>

This file can... Ah-ha. It already doesn't exist in the tree. Lovely.

Comment 50

[Justin Dolske [:Dolske]]

Comment on attachment 507634 [details] [diff] [review]
2/19: Remove all references to the old print dialogs from all implementations of nsIPrintingPromptService, etc

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

I feel a little bit dirtier for having waded through this code, but am increasingly excited by how much cleanup is happening here.

<3

::: embedding/browser/webBrowser/Makefile.in
@@ -91,5 @@
>  		$(NULL)
>  
>  ifdef NS_PRINTING
>  SDK_XPIDLSRCS += nsIWebBrowserPrint.idl
> -XPIDLSRCS     += nsIPrintingPrompt.idl nsIPrintingPromptService.idl

A quick search on MXR shows 4 AMO-hosted addons that reference nsIPrintingPrompt. (https://mxr.mozilla.org/addons/)

16 addons contain the string "nsIPrint". Not a lot, but we'll want to do some outreach to let them know that (1) they're going to break and (2) how they might be able to do what they want in the brave new world after this patch lands.

::: embedding/browser/webBrowser/nsIPrintingPrompt.idl
@@ -50,5 @@
> -#include "nsIPrintProgressParams.idl"
> -#include "nsIObserver.idl"
> -
> -[scriptable, uuid(44E314CA-75B1-4f3d-9553-9B3507912108)]
> -interface nsIPrintingPrompt : nsISupports

Note to self: Mozilla consumers of this are mostly C++ (some JS) in the expected places. Didn't see any oddball call sites that were likely to be missed.

::: embedding/browser/webBrowser/nsIPrintingPromptService.idl
@@ +50,1 @@
>  interface nsIPrintingPromptService : nsISupports

Random comment: It _might_ make sense to just move all the printing-related goop from embedding to some other central place. We've basically given up exposing an easily-embeddable Gecko, so keeping things in /embedding isn't so useful anymore. That's basically what I did as part of the nsIPrompt cleanup in bug 563274.

...oh, it looks like that's exactly what's happening later in this patch series. Carry on!

::: embedding/components/printingui/src/os2/nsPrintingPromptService.cpp
@@ +34,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "nsPrintingPromptService.h"

This file (nsPrintingPromptService.cpp) seems mostly identical to the ones in unixshared/win/osx. Hmm, looks like later patches are indeed collapsing all this code down to a single implementation. I hope it ends up being JS! :)

::: embedding/components/printingui/src/unixshared/nsPrintingPromptService.cpp
@@ +95,5 @@
>      NS_ENSURE_ARG(notifyOnOpen);
>  
>      *notifyOnOpen = PR_FALSE;
>  
> +    mProgress = new nsPrintProgress(printSettings);

Hmm. What happens if the user prints multiple things in rapid succession? Seems to be a problem with the original code too. Do we need a followup to explicitly handle this, or can we just fail here if mProgress != null without breaking anything that already works?

@@ +103,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    // this should never fail
> +    nsCOMPtr<nsIDOMWindowInternal> parentDOMIntl(do_QueryInterface(parent));
> +    if (parentDOMIntl)

Hmm, if it can't fail then why bother with the if check?

I suppose parent could be null? I'm not sure that's intended to be allowed. The regular nsIPrompt stuff allows it (eg, so a component not tied to a specific window can ask a question), but it seems less likely that one would print something without a window being involved. Though if it's just being used for window hierarchy/anchoring, then I guess there's no harm in supporting it. Should probably update the IDL, though.

::: embedding/components/printingui/src/win/nsPrintDialogUtil.cpp
@@ +1441,5 @@
> +  HWND hWnd = NULL;
> +
> +  // We might be embedded so check this path first
> +  // XXXzw Is this logic appropriate anymore? Particularly when caller
> +  // unconditionally destroys the window?

Umm, I don't know. Ask one of the DOM reviewers for this part... jst or sicking?

I'll still mark r+ since this looks to just be moving from the Windows nsPrintingPromptService.cpp change in this same patch.

Comment 51

[Steffen Wilberg]

nsIDOMWindowInternal has been removed by bug 670235 (and reintroduced as an empty interface by bug 675075 for compatibility). Use nsIDOMWindow instead.

Comment 52

[Steffen Wilberg]

Of course that already happened in the tree. You could update the "parentDOMIntl" pointers though:
http://mxr.mozilla.org/mozilla-central/search?string=parentDOMIntl

Comment 53

[Philip Chee]

> 16 addons contain the string "nsIPrint". Not a lot, but we'll want to do some outreach > to let them know that (1) they're going to break and (2) how they might be able to do
> what they want in the brave new world after this patch lands.

Two consumers in comm-central/mailnews. Please remember to reach out to the Thunderbird devs as well.

Comment 54

[Justin Dolske [:Dolske]]

Comment on attachment 507636 [details] [diff] [review]
4/19: Remove nsIPrintSettingsService.defaultPrinterName.

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

::: dom/base/nsGlobalWindow.cpp
@@ +5066,2 @@
>          }
> +        printSettingsService->InitPrintSettingsFromPrefs(printSettings,

This feels like a slightly odd pattern. If it doesn't just go away entirely, I wonder if it would make sense to have InitPrintSettingsFromPrefs() take care of doing this (followup bug material, if so).

::: widget/public/nsIPrintSettingsService.idl
@@ -73,5 @@
>  
>    /**
> -   * The name of the last printer used, or else the system default printer.
> -   */
> -  readonly attribute wstring defaultPrinterName;

(Random comment, no action needed)

Removing the need for callers to deal with this is righteous, but I'm a little ambivalent about removing the generic ability for something to determine what printer will be used by default.

OTOH, I'm not sure why something external to the printing code would really want to know this. And I see other patches are cleaning up the printer enumerator stuff, so maybe this comes back anyway.

::: widget/src/xpwidgets/nsPrintOptionsImpl.cpp
@@ -829,5 @@
> -
> -  // Look up the printer from the last print job
> -  nsAutoString lastPrinterName;
> -  ReadPrefString(kPrinterName, lastPrinterName);
> -  if (!lastPrinterName.IsEmpty()) {

(Another random comment)

I wonder if this was the right thing to have been doing anyway -- I can see arguments for making the default printer specific to the app, as well just a global OS-managed setting. Might depend on platform conventions?

Comment 55

[Justin Dolske [:Dolske]]

Comment on attachment 507638 [details] [diff] [review]
5/19: Move the backend-specific code behind nsIPrintSettingsService::InitPrintSettingsFromPrinter from nsIPrinterEnumerator to nsIDeviceContextSpec.

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

This patch probably ought to be reviewed by a widget peer, and perhaps needs some consideration into the "XXX NS_NOT_IMPLEMENTED instead?" comments.

But from the general viewpoint of shuffling code around and hitting print dialogs but a Big Pretty Stick, looks fine to me.

Comment 56

[Justin Dolske [:Dolske]]

Comment on attachment 507641 [details] [diff] [review]
7/19: Use GetDefaultPrinterW() on Windows instead of nsIPrinterEnumerator (which used GetProfileString!); remove the nsIPrinterEnumerator impl.

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

I'm marking r+ here, but this probably ought to be looked at by someone with more win32 experience. I can help make that happen for the next review pass.

::: embedding/components/printingui/src/win/nsPrintDialogUtil.cpp
@@ +793,2 @@
>  {
> +#ifndef WINCE

We've dropped support for working (or even compiling) on Windows CE / Windows Mobile, so any such #ifdefs can be removed.

@@ +803,5 @@
> +    // retry with resized buffer
> +    length = aDefaultPrinterName.GetMutableData(&bufptr, length);
> +    if (::GetDefaultPrinterW(bufptr, &length)) {
> +      aDefaultPrinterName.Truncate(length - 1);
> +      return;

I wonder if that ends up causing other problems should a code flow encounter this. Maybe this function wants to return a nsresult? Or always return "UNKNOWN" / empty-string if there's a problem?

Alternatively, it would feel a bit safer to always make an initial query to ::InitPrintSettingsFromPrefs() just to get the length, and then use that for the GetMutableData() call -- and just bail out in the unlikely case that it changes between calls.

At least that way there's no risk of the <= 1024 path working but the > 1024 path being untested / accidentally broken / insecure.

::: widget/src/windows/nsDeviceContextSpecWin.cpp
@@ +192,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  // XXXzw shouldn't this be aWhichPrinters again?
> +  // Also, should we be using "level" 4 here to avoid blocking?

mmm, sure seems like aWhichPrinters should be used (without looking at what callers actually want). Ditto, at first MSDN glance, for the level change.

Since this is an existing issue, just file followups for them?

@@ +215,5 @@
>  
> +static void
> +GetDefaultPrinterName(nsXPIDLString &aDefaultPrinterName)
> +{
> +#ifndef WINCE

Same comments as above re ifdef/errors/length.

Comment 57

[Justin Dolske [:Dolske]]

Comment on attachment 507644 [details] [diff] [review]
8/19: Remove the remaining nsIPrinterEnumerator implementations, which are now unused.

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

r+ based on ratio of removed-to-added/modified lines. ;)

Comment 58

[Justin Dolske [:Dolske]]

Comment on attachment 507652 [details] [diff] [review]
15/19: Move remaining contents of embedding/components/printingui to toolkit/components/printing and merge cross-platform.

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

Mmm, consolidation.

Comment 59

[Justin Dolske [:Dolske]]

Comment on attachment 507655 [details] [diff] [review]
18/19: Kill ns(I)PrintingPromptService; use ns(I)PrintProgress directly.  En passant chainsaw cleanup of printProgress.js and printPreviewProgress.js.

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

Mostly just skimming through this, but it looks fine.

::: toolkit/components/printing/nsIPrintProgress.idl
@@ +61,5 @@
> +  void openDialog(in nsIDOMWindow parent,
> +                  in nsIWebBrowserPrint webBrowserPrint,
> +                  in nsIPrintSettings printSettings,
> +                  in nsIObserver openDialogObserver,
> +                  in boolean isForPrinting,

Two tiny nits:

* boolean params generally suck, because it's not obvious from looking at the caller what's happening. But if you must, you must...

* I'd probably reverse this as isForPreview, which makes the purpose a little more direct.

::: toolkit/components/printing/nsPrintProgress.cpp
@@ +168,3 @@
>  {
> +  if (mObserver) {
> +    mObserver->Observe(nsnull, nsnull, nsnull);

*shudder*

If this isn't outright removed, worth a followup to, y'know, have this actually send a topic...

::: toolkit/components/printing/printPreviewProgress.js
@@ +55,3 @@
>      return aStr;
>  
> +  if (aStr.substr(0, 3) == "..." || aStr.substr(aStr.length-4, 3) == "...")

Eww. These are not ellipses. They are 3 periods. Worth changing (followup?) to "…"? [Actually, more than that, since that needs I18N'd] Or, better yet, since these seem to just get shoved into the UI use text-overflow: ellipsis.

@@ +95,2 @@
>    {
> +    if (aMessage != "")

Nit: We seem to prefer |if (aMessage)| in JS, which handles this fine.

::: toolkit/components/printing/printProgress.js
@@ +59,2 @@
>  
> +  if (aStr.substr(0, 3) == "..." || aStr.substr(aStr.length-4, 3) == "...")

This looks oddly familiar. :/

Comment 60

[Justin Dolske [:Dolske]]

Comment on attachment 507656 [details] [diff] [review]
19/19: Fold ns(I)PrintProgressParams into ns(I)PrintProgress.

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

Another mostly skim, but things look fine.

::: toolkit/components/printing/nsIPrintProgress.idl
@@ +67,1 @@
>  

Eww. I guess you're just collapsing interfaces, but it would sure be nice to make these be nsStrings, or at internally manage anything that wants prunichars, so that callers don't have to convert and manage the alloc.

Comment 61

[Justin Dolske [:Dolske]]

Zach: Any idea when you might be able to get around to updating this patch to trunk?

Comment 62

[Steffen Wilberg]

After this is fixed, would it still be possible to implement a combined print-preview tab with the controls of the print dialog (selection of printer, pages, copies, orientation, color, Print/Cancel buttons, etc.)?

See bug 650966 comment 22, where I attached and described a Chrome screenshot (attachment 552728 [details]).

Comment 63

[[e-mail]]

(In reply to Justin Dolske from comment #59)
> (From update of attachment 507655 [details] [diff] [review])
> > +  if (aStr.substr(0, 3) == "..." || aStr.substr(aStr.length-4, 3) == "...")
> Eww. These are not ellipses. They are 3 periods. Worth changing (followup?)
> to "…"? [Actually, more than that, since that needs I18N'd] Or, better yet,
> since these seem to just get shoved into the UI use text-overflow: ellipsis.
Or, since this is XUL, crop="end".

Comment 64

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

(In reply to Steffen Wilberg from comment #62)
> After this is fixed, would it still be possible to implement a combined
> print-preview tab with the controls of the print dialog (selection of
> printer, pages, copies, orientation, color, Print/Cancel buttons, etc.)?
> 
> See bug 650966 comment 22, where I attached and described a Chrome
> screenshot (attachment 552728 [details]).
This has nothing to do with that. Print preview is already its own tab, we just happen to hide
the normal UI when it is visible.

Comment 65

[Zack Weinberg (:zwol)]

I am preparing a new patch series now.  Some notes on review feedback:

(In reply to Justin Dolske [:Dolske] from comment #49)
> 1/19: Remove the generic XUL-based print and page setup dialogs from
> toolkit/.
>
> Note to future self: Audit final patch to remove any now-dead
> default prefs.

I would prefer to do that in the follow-up bug in which I rewrite the
PrintSettingsService (possibly I will recycle one of the dependent
bugs that amount to "print settings are not remembered" for this,
possibly I'll file a new one).

> ::: toolkit/components/printing/content/printdialog.xul
> @@ -55,5 @@
> > -  screenX="24" screenY="24">
> > -
> > -  <script type="application/javascript" src="chrome://global/content/printdialog.js"/>
> > -
> > -  <stringbundle id="printingBundle" src="chrome://global/locale/printing.properties"/>
>
> There are presumably unused strings in the .properties file now that this
> .xul is being removed (perhaps another patch in this series blows away this
> while file?).

Good catch; it hadn't occurred to me to check.  Most of the strings in that file are still in use (error messages for nsPrintEngine, "page X of Y" used by nsSimplePageSequence, etc) but some of them can go.

(In reply to Justin Dolske [:Dolske] from comment #50)
> 2/19: Remove all references to the old print dialogs from all
> implementations of nsIPrintingPromptService, etc
>
> I feel a little bit dirtier for having waded through this code, but am
> increasingly excited by how much cleanup is happening here.
>
> <3

Thank you for your kind words!

> A quick search on MXR shows 4 AMO-hosted addons that reference
> nsIPrintingPrompt. (https://mxr.mozilla.org/addons/)
>
> 16 addons contain the string "nsIPrint". Not a lot, but we'll want to do
> some outreach to let them know that (1) they're going to break and (2) how
> they might be able to do what they want in the brave new world after this
> patch lands.

I do not have access to that search, so I can't assess what they are
doing...  I'll be happy to summarize the addon-relevant changes, but
it'd be nice to know if these addons do anything with the print
preferences, which are certain to change again in follow-ups.

> This file (nsPrintingPromptService.cpp) seems mostly identical to the ones
> in unixshared/win/osx. Hmm, looks like later patches are indeed collapsing
> all this code down to a single implementation. I hope it ends up being JS! :)

I don't think we get quite that far, but printing progress bars are
slated to go away anyway, so I also don't think it matters so much.

> Hmm. What happens if the user prints multiple things in rapid succession?
> Seems to be a problem with the original code too. Do we need a followup to
> explicitly handle this, or can we just fail here if mProgress != null
> without breaking anything that already works?

Again, this is going to get nuked later.

> Hmm, if it can't fail then why bother with the if check?
> I suppose parent could be null? I'm not sure that's intended to be allowed.

I think I remember auditing all callers at the time I originally wrote
the patch and determining that parent can never be null.  I kept the
if because of the nsIDOMWindowInternal conversion, that theoretically
could fail. Current revision-under-test enforces having a parent and
eliminates the if.

> > +  // We might be embedded so check this path first
> > +  // XXXzw Is this logic appropriate anymore? Particularly when caller
> > +  // unconditionally destroys the window?
>
> Umm, I don't know. Ask one of the DOM reviewers for this part... jst or
> sicking?

Ok.

> I'll still mark r+ since this looks to just be moving from the Windows
> nsPrintingPromptService.cpp change in this same patch.

Yah.  I presume it works or our Windows users would be unhappy, but it
sure looks weird.

> > +        printSettingsService->InitPrintSettingsFromPrefs(printSettings,
>
> This feels like a slightly odd pattern. If it doesn't just go away entirely,
> I wonder if it would make sense to have InitPrintSettingsFromPrefs() take
> care of doing this (followup bug material, if so).

That would logically go in the revamp of print settings.

> Removing the need for callers to deal with this is righteous, but I'm a
> little ambivalent about removing the generic ability for something to
> determine what printer will be used by default.

That was actually a *goal* of this patch series.  The modern
high-level OS printing APIs on all three of Windows, OSX, and Gtk+
specifically do not want applications to ask that question.  You're
supposed to let the print dialog box figure it out for you.  They
don't really want you enumerating all available printers, either.

> 5/19: Move the backend-specific code behind
> nsIPrintSettingsService::InitPrintSettingsFromPrinter from
> nsIPrinterEnumerator to nsIDeviceContextSpec.
>
> This patch probably ought to be reviewed by a widget peer, and perhaps needs
> some consideration into the "XXX NS_NOT_IMPLEMENTED instead?" comments.

Ok.  Could you suggest a reviewer, please?

> 7/19: Use GetDefaultPrinterW() on Windows instead of nsIPrinterEnumerator
> (which used GetProfileString!); remove the nsIPrinterEnumerator impl.
>
> I'm marking r+ here, but this probably ought to be looked at by someone with
> more win32 experience. I can help make that happen for the next review pass.

Again, please suggest a reviewer?  I don't know who does win32 stuff.

> ::: embedding/components/printingui/src/win/nsPrintDialogUtil.cpp
> > +#ifndef WINCE
>
> We've dropped support for working (or even compiling) on Windows CE /
> Windows Mobile, so any such #ifdefs can be removed.

Done.

> @@ +803,5 @@
> > +    // retry with resized buffer
> > +    length = aDefaultPrinterName.GetMutableData(&bufptr, length);
> > +    if (::GetDefaultPrinterW(bufptr, &length)) {
> > +      aDefaultPrinterName.Truncate(length - 1);
> > +      return;
>
> I wonder if that ends up causing other problems should a code flow encounter
> this. Maybe this function wants to return a nsresult? Or always return
> "UNKNOWN" / empty-string if there's a problem?
>
> Alternatively, it would feel a bit safer to always make an initial query to
> ::InitPrintSettingsFromPrefs() just to get the length, and then use that for
> the GetMutableData() call -- and just bail out in the unlikely case that it
> changes between calls.

According to MSDN, if ::GetDefaultPrinterW fails with
GetLastError() == ERROR_INSUFFICIENT_BUFFER, it writes to &length the
amount of space required.  So we're kinda already doing that.  The
second call should not truncate unless GetMutableData fails to
allocate enough space.

I have restructured both copies of this function to make an
unconditional call to ::GetDefaultPrinterW with a null buffer, which
is documented to report the necessary length, rather than guessing
1024 characters.  The logic should also be clearer.

> > +  // XXXzw shouldn't this be aWhichPrinters again?
> > +  // Also, should we be using "level" 4 here to avoid blocking?
>
> mmm, sure seems like aWhichPrinters should be used (without looking at what
> callers actually want). Ditto, at first MSDN glance, for the level change.
>
> Since this is an existing issue, just file followups for them?

Filed bug 701479.

> 18/19: Kill ns(I)PrintingPromptService; use ns(I)PrintProgress directly.  En
> passant chainsaw cleanup of printProgress.js and printPreviewProgress.js.

Given that print progress bars are slated to be nuked in bug 650960, I
have not addressed any of your nits in either 18/ or 19/.

Comment 66

[Zack Weinberg (:zwol)]

Attachment: current patchset in one bundle

Here is an updated patchset in hg bundle format.  This applies on top of http://hg.mozilla.org/mozilla-central/rev/a41fa578cfc1 and has review comments addressed as described above.  Broken-down patch series to follow.  Try job running: https://tbpl.mozilla.org/?tree=Try&rev=2453e71f42ec

Comment 67

[Zack Weinberg (:zwol)]

Attachment: 1/19: Remove the generic XUL-based print and page setup dialogs from toolkit/.

Comment 68

[Zack Weinberg (:zwol)]

Attachment: 2/19: Remove all references to the old print dialogs from all implementations of nsIPrintingPromptService, etc

jst: dolske said a DOM peer should comment on embedding/components/printingui/src/win/nsPrintDialogUtil.cpp::GetHWNDForDOMWindow and the XXX comment therein.

Comment 69

[Zack Weinberg (:zwol)]

Attachment: 3/19: Remove two totally unused nsIPrintOptions methods and some archaic debugging code in nsPrintOptionsImpl.cpp.

Requesting re-review here because I decided to go a little further than the previous iteration, and remove a bunch of old #ifdefed-out debugging logic from nsPrintOptionsImpl.cpp.

Comment 70

[Zack Weinberg (:zwol)]

Attachment: 4/19: Remove nsIPrintSettingsService.defaultPrinterName.

Comment 71

[Zack Weinberg (:zwol)]

Attachment: 5/19: Move the backend-specific code behind nsIPrintSettingsService::InitPrintSettingsFromPrinter from nsIPrinterEnumerator to nsIDeviceContextSpec.

roc: dolske thought this was more appropriately reviewed by a widget peer, and you've had a look at several of the other patches in this bug.

Comment 72

[Zack Weinberg (:zwol)]

Attachment: 6/19: Unconditionally run the print preview tests

Comment 73

[Zack Weinberg (:zwol)]

Attachment: 7/19: Use GetDefaultPrinterW() on Windows instead of nsIPrinterEnumerator (which used GetProfileString!); remove the nsIPrinterEnumerator impl.

jimm: dolske thought this deserved a look-over from a Windows widget peer.  The XXX comments are bug 701479.

Comment 74

[Zack Weinberg (:zwol)]

Attachment: 8/19: Remove the remaining nsIPrinterEnumerator implementations, which are now unused.

Comment 75

[Zack Weinberg (:zwol)]

Attachment: 9/19: Remove no-longer-used code to poke libcups for a printer list.

Comment 76

[Zack Weinberg (:zwol)]

Attachment: 10/19: Remove nsIContentViewerFile and all references to nsIPrintOptions outside widget/.

Comment 77

[Zack Weinberg (:zwol)]

Attachment: 11/19: Fold nsIPrintOptions into nsIPrintSettingsService.

Comment 78

[Zack Weinberg (:zwol)]

Attachment: 12/19: Rename all nsIPrintSettingsService implementation files and class names from nsPrintOptionsWhatever to nsPrintSettingsServiceWhatever.

Comment 79

[Zack Weinberg (:zwol)]

Attachment: 13/19: Move printingui/src/win/nsPrintDialogUtil.* to widget/src/windows, and convert it to a nsIPrintDialogService implementation. Add a stub nsIPrintDialogService implementation for OS/2.

Comment 80

[Zack Weinberg (:zwol)]

Attachment: 14/19: Kill another totally unused interface, nsIPrintStatusFeedback.

Comment 81

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 573659 [details] [diff] [review]
5/19: Move the backend-specific code behind nsIPrintSettingsService::InitPrintSettingsFromPrinter from nsIPrinterEnumerator to nsIDeviceContextSpec.

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

::: widget/public/nsIDeviceContextSpec.h
@@ +46,5 @@
>  
> +// b00eb598-68f2-485e-a403-cd4baf0734c4
> +#define NS_IDEVICE_CONTEXT_SPEC_IID \
> +{ 0xb00eb598, 0x68f2, 0x485e,       \
> +  { 0xa4, 0x03, 0xcd, 0x4b, 0xaf, 0x07, 0x34, 0xc4 } }

I don't think you need to rev this IID since you're only changing a static function.

Comment 82

[Zack Weinberg (:zwol)]

Attachment: 15/19: Move remaining contents of embedding/components/printingui to toolkit/components/printing and merge cross-platform.

Comment 83

[Zack Weinberg (:zwol)]

Attachment: 16/19: Make the PrintDialogService scriptable, and use it directly for print and page setup dialogs instead of going through the PrintingPromptService.

Comment 84

[Zack Weinberg (:zwol)]

Attachment: 17/19: Remove the unnecessary Init method from nsIPrintDialogService, and rename the vague Show method to ShowPrint.

Comment 85

[Zack Weinberg (:zwol)]

Attachment: 18/19: Kill ns(I)PrintingPromptService; use ns(I)PrintProgress directly. En passant chainsaw cleanup of printProgress.js and printPreviewProgress.js.

Comment 86

[Zack Weinberg (:zwol)]

Attachment: 19/19: Fold ns(I)PrintProgressParams into ns(I)PrintProgress.

Comment 87

[Zack Weinberg (:zwol)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #81)
> Comment on attachment 573659 [details] [diff] [review] [diff] [details] [review]
> 5/19: Move the backend-specific code behind
> nsIPrintSettingsService::InitPrintSettingsFromPrinter from
> nsIPrinterEnumerator to nsIDeviceContextSpec.

That was fast!  Before I even finished posting all the patches. :-)

> > +// b00eb598-68f2-485e-a403-cd4baf0734c4
> > +#define NS_IDEVICE_CONTEXT_SPEC_IID \
> > +{ 0xb00eb598, 0x68f2, 0x485e,       \
> > +  { 0xa4, 0x03, 0xcd, 0x4b, 0xaf, 0x07, 0x34, 0xc4 } }
> 
> I don't think you need to rev this IID since you're only changing a static
> function.

I try to err on the side of safety when updating IIDs.  Will revert in my copy.

Comment 88

[Mozilla RelEng Bot]

Try run for 2453e71f42ec is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2453e71f42ec
Results (out of 121 total builds):
    success: 109
    warnings: 7
    failure: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2453e71f42ec

Comment 89

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

So is the missing part for Windows now implemented or being implemented?

Comment 90

[Zack Weinberg (:zwol)]

(In reply to Olli Pettay [:smaug] from comment #89)
> So is the missing part for Windows now implemented or being implemented?

It's assigned to Jared Wein in bug 693230.  (It doesn't make sense to land that bug separately from this one.)  I reiterate that super-review can go ahead independently of the missing piece, which will only fill in an unimplemented nsIPrintDialogService method.

I was going to post a new bundle with corrections to the merge botches that caused build failures on OSX and Android, but there are conflicts with the just-landed bug 311007 that require me to go back through all the patches very carefully and make sure I didn't introduce _new_ merge botches, and I won't have time to do that today.  I'm also not sure why the print preview tests are hanging on Windows XP; I probably messed up patch 6 somehow.

Comment 91

[Jim Mathies [:jimm]]

Comment on attachment 573662 [details] [diff] [review]
7/19: Use GetDefaultPrinterW() on Windows instead of nsIPrinterEnumerator (which used GetProfileString!); remove the nsIPrinterEnumerator impl.

(In reply to Zack Weinberg (:zwol) from comment #73)
> Created attachment 573662 [details] [diff] [review] [diff] [details] [review]
> 7/19: Use GetDefaultPrinterW() on Windows instead of nsIPrinterEnumerator
> (which used GetProfileString!); remove the nsIPrinterEnumerator impl.
> 
> jimm: dolske thought this deserved a look-over from a Windows widget peer. 
> The XXX comments are bug 701479.

I'll take bug 701479. I bet we can grab the default printer name via a background thread and cache the value. Then refresh it on settings changed events. The enum printer things looks bad too, I bet this code has been around since the beginning. :/

Comment 92

[Zack Weinberg (:zwol)]

(In reply to Jim Mathies [:jimm] from comment #91)
>
> I'll take bug 701479.

Cool.  I'll update the XXX comments in my copy to mention that bug.

> I bet we can grab the default printer name via a
> background thread and cache the value. Then refresh it on settings changed
> events. The enum printer things looks bad too, I bet this code has been
> around since the beginning. :/

Yeah, it's got dates circa 2001 and @netscape.com handles and stuff like that.

You should be aware that my longer-term goal (past this bug) is to get us to the point where we can use "modern" printing APIs on all platforms (I'm not sure which those are for Windows, though) and it is my understanding that looking up the name of the default printer and getting a list of all available printers are discouraged operations for all such "modern" APIs.

Comment 93

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Zack Weinberg (:zwol) from comment #90)
> (In reply to Olli Pettay [:smaug] from comment #89)
> > So is the missing part for Windows now implemented or being implemented?
> 
> It's assigned to Jared Wein in bug 693230.  (It doesn't make sense to land
> that bug separately from this one.)  I reiterate that super-review can go
> ahead independently of the missing piece, which will only fill in an
> unimplemented nsIPrintDialogService method.
> 

I currently have Bug 697683 higher on my priority list. I hope to get to this bug when bug 697683 is fixed, but I'm not sure how long that will take.

Comment 94

[Zack Weinberg (:zwol)]

Attachment: updated bundle

This has the aforementioned merge botches fixed and also updates the patchset for bug 311007, which turned out to be much easier than I was expecting (hg diff --mq is awesome).  I'll repost patch 17, which is the only one that changed in an interesting way (I accidentally deleted an entire class declaration).

Comment 95

[Zack Weinberg (:zwol)]

Attachment: 17/19: Remove the unnecessary Init method from nsIPrintDialogService, and rename the vague Show method to ShowPrint.

This revision restores two accidentally-deleted ObjC interface definitions, which were the cause of the build failures on OSX in the previous cycle.

Comment 96

[Mozilla RelEng Bot]

Try run for 6e1df89d632a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6e1df89d632a
Results (out of 211 total builds):
    success: 191
    warnings: 20
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6e1df89d632a

Comment 98

[Zack Weinberg (:zwol)]

I think I'm going to need some help debugging these Windows XP test failures.  Representative logs:

https://tbpl.mozilla.org/php/getParsedLog.php?id=7393101&tree=Try#error0
https://tbpl.mozilla.org/php/getParsedLog.php?id=7377982&tree=Try#error0
https://tbpl.mozilla.org/php/getParsedLog.php?id=7375888&tree=Try#error0

The interesting failures are the test_printpreview*.xul timeouts; I think everything after that is just fallout.  They appear at patch 4 (which removes nsIPrintSettingsService.defaultPrinterName) and they *only* affect our XP testers.  Updating the tests to go through docshell.printPreview as the warnings suggest, does not help.  Comments in the test imply that something about print preview doesn't work correctly on a computer with no printer configured, but I'd expect a failure rather than a hang in that case.  Does anyone have any idea?

Comment 99

[Zack Weinberg (:zwol)]

cc:ing bhearsum: do the XP testers have any printers configured?

Comment 100

[David Dahl :ddahl]

Without reading through these patches and all 100 comments - is it not possible to at least test the dialogs via browser chrome tests? And - perhaps test print to file automatically?

Comment 101

[Zack Weinberg (:zwol)]

Attachment: updated bundle

This bundle is the patches rebased against current trunk.  No substantive changes except the removal of yet more #ifdef DEBUG_rodsX cruft.

http://tbpl.mozilla.org/?tree=Try&rev=1437387551c4 running now; based on a previous cycle, this should be good everywhere except maybe Windows; I *think* I fixed the Windows build bustage this time but of course I can't test that locally.

Comment 102

[Mozilla RelEng Bot]

Try run for 1437387551c4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1437387551c4
Results (out of 164 total builds):
    success: 137
    warnings: 23
    failure: 3
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1437387551c4

Comment 103

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mozilla RelEng Bot from comment #102)
> Try run for 1437387551c4 is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=1437387551c4
> Results (out of 164 total builds):
>     success: 137
>     warnings: 23
>     failure: 3
>     other: 1
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 1437387551c4

All of the windows builds failed. I think the merge might have been bad?

Comment 104

[Mozilla RelEng Bot]

Try run for d4de1d602b23 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d4de1d602b23
Results (out of 47 total builds):
    success: 44
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d4de1d602b23

Comment 105

[Zack Weinberg (:zwol)]

Attachment: corrected updated bundle

Yeah, bad merge.  This one should be good.

Comment 106

[Zack Weinberg (:zwol)]

(In reply to David Dahl :ddahl from comment #100)
> Without reading through these patches and all 100 comments - is it not
> possible to at least test the dialogs via browser chrome tests? And -
> perhaps test print to file automatically?

I have no idea whether it is possible to test the OS native print/page setup dialog boxes (which we use on most platforms already, and will use on all platforms after this patch series) with a browserchrome test.  Do you know?

In any case I call mission creep on doing that (or testing print-to-file) in _this_ bug, although I agree it would be nice.

Comment 107

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Zack Weinberg (:zwol) from comment #106)
> In any case I call mission creep on doing that (or testing print-to-file) in
> _this_ bug, although I agree it would be nice.

A big change like this is scary to me, as it has the potential to break printing in ways that won't be noticed until the changes have made it to Beta.

Adding more tests should help us, even if it just means that the investigations in to how to add such tests are front-loaded.

Comment 108

[Zack Weinberg (:zwol)]

I agree this does have quite a bit of risk.  I just don't want to make this bug more complicated than it already is, or go down rabbit holes because of the large number of existing bugs in this area.  I'd be fine with making new bugs to add UI tests and having this bug depend on those (like the existing bug 702678), provided we do not attempt to fix any _existing_ bugs in the process (such as the bugs related to print settings getting lost, that this bug blocks).

Comment 109

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

So bug 693230 is still open. I know I've been very slow with this bug, but I'd like to know that
bug 693230 gets fixed before I spend days to review the patches in this bug.

Comment 110

[Zack Weinberg (:zwol)]

(In reply to Olli Pettay [:smaug] from comment #109)
> So bug 693230 is still open. I know I've been very slow with this bug, but
> I'd like to know that bug 693230 gets fixed before I spend days to review
> the patches in this bug.

Bug 693230 fills in the Windows implementation of an existing nsIPrintDialogService method.  The presence or absence of that code should not affect super-review of this code.  Therefore, it would be a better use of everyone's time if you started reviewing now, so that I could fix any problems you found in parallel with the code getting written for that bug.

Anyway, Jared Wein said he'd be working on it this week.

Comment 111

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

Comment on attachment 573652 [details] [diff] [review]
1/19: Remove the generic XUL-based print and page setup dialogs from toolkit/.

Removing this stuff from my review queue until the bugs blocking this
are fixed.

Comment 112

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

(In reply to Olli Pettay [:smaug] from comment #111)
> Removing this stuff from my review queue until the bugs blocking this
> are fixed.
...since I don't want to spend a week reviewing this stuff without knowing the
code will actually land soon.

Comment 113

[Zack Weinberg (:zwol)]

I specifically asked on at least two occasions for you to go ahead and super-review this stuff before its dependencies are finished.  I can't do anything about the dependencies except nag the people who are assigned, but I could have addressed any problems you found by now if you'd done the sr already, and then we'd be in better shape for getting this landed quickly.

Comment 114

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

But you do realize that it takes several days, probably a week, to review this all.
It would feel waste of time if I couldn't know that the patches can land soon after reviews.
(I'm just trying to prioritize my time, sorry about that.)

Comment 115

[Zack Weinberg (:zwol)]

I do appreciate the time it would take.  I'm just unable to make any forward progress here until the sr happens.

Comment 116

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

Could you push bug 693230 to get fixed?
And bug 702678 needs some fixes.

Comment 117

[Zack Weinberg (:zwol)]

I'll see what I can do (I finally have a few cycles for Mozilla again in the next few weeks) but my personal ability to do *anything* on Windows is very, very limited, so it may come down to trying to find more help :-/

Comment 118

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

Unfortunately I don't have Windows dev environment either.

Comment 119

[Justin Dolske [:Dolske]]

Comment on attachment 573655 [details] [diff] [review]
3/19: Remove two totally unused nsIPrintOptions methods and some archaic debugging code in nsPrintOptionsImpl.cpp.

Clearing review request; until bug 693230 happens there really isn't anything to do here.

Comment 120

[Benjamin Smedberg]

Resetting owner to default per Zack's request.

Comment 121

[Daniel Holbert [:dholbert]]

Seems like there was a lot of work covered here, but at a high level, this seems like it's approximately a dupe of bug 133787 (which is a metabug for a massive printing rewrite, which recently shipped to release via the pref-flip in bug 1692232).

--> Closing this as a dupe of bug 133787.

Comment 122

[Zack Weinberg (:zwol)]

I'm pleased to hear someone finally picked up this ball after I dropped it all those years ago.