240154 - Mozilla should warn if Xprint printing is requested but not built at compilation time

Status: RESOLVED WONTFIX

(Core Graveyard :: Printing: Xprint, defect)

Description

[Wilfried Drube]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040316
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040316

The user should be warned if he has configured XPrint printing but the build
does not have support for it built at compilation time. I wrote a patch to deal
with the problem.

Reproducible: Always
Steps to Reproduce:
1. Install XPrint RPM
2. Print


Actual Results:  
Due http://bugzilla.mozilla.org/show_bug.cgi?id=133534 only Postscript/default
appears as printer choice.

Expected Results:  
A warning dialog should be displayed which says that XPrint printing is not
supported in that built.

Comment 1

[Wilfried Drube]

Attachment: Fix

Comment 2

[Roland Mainz]

Reassign to author...

Comment 3

[Roland Mainz]

Comment on attachment 145810 [details] [diff] [review]
Fix

> --- gfx/public/nsIDeviceContext.h       20 Feb 2004 17:28:49 -0000      1.46
> +++ gfx/public/nsIDeviceContext.h       10 Apr 2004 02:01:29 -0000
> @@ -159,6 +159,9 @@
>  /* Cannot load the matching print module */
>  #define NS_ERROR_GFX_COULD_NOT_LOAD_PRINT_MODULE \
>    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_GFX,NS_ERROR_GFX_PRINTER_BASE+34)   
> +/* XPrint support not build */

"XPrint" --> "Xprint", please...

> +#define NS_ERROR_GFX_XPRINT_SUPPORT_NOT_BUILD \
> +  NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_GFX,NS_ERROR_GFX_PRINTER_BASE+35)   
>        
>  /**
>   * Conts need for Print Preview
> Index: gfx/src/gtk/nsDeviceContextSpecG.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/gfx/src/gtk/nsDeviceContextSpecG.cpp,v
> retrieving revision 1.60
> diff -u -r1.60 nsDeviceContextSpecG.cpp
> --- gfx/src/gtk/nsDeviceContextSpecG.cpp        16 Mar 2004 01:07:13 -0000      1.60
> +++ gfx/src/gtk/nsDeviceContextSpecG.cpp        10 Apr 2004 02:01:29 -0000
> @@ -1048,6 +1048,12 @@
>      
>      XpuFreePrinterList(plist);
>    }
> +#else
> +  if (PR_GetEnv("XPSERVERLIST") || PR_GetEnv("XPRINTER")) {

Please check for "PDPRINTER", too...

> +    /* Make sure we do not cache an empty printer list */
> +    FreeGlobalPrinters();
> +    return NS_ERROR_GFX_XPRINT_SUPPORT_NOT_BUILD;
> +  }
>  #endif /* USE_XPRINT */
>  
>  #ifdef USE_POSTSCRIPT

Could you please patch gfx/src/xlib/nsDeviceContextSpecXlib.cpp - that's the
same code, only for the Xlib toolkit (no need to compile, just patch it and
I'll check...).

> Index: layout/html/base/src/printing.properties
> ===================================================================
> RCS file: /cvsroot/mozilla/layout/html/base/src/printing.properties,v
> retrieving revision 3.22
> diff -u -r3.22 printing.properties
> --- layout/html/base/src/printing.properties    19 Feb 2004 21:58:40 -0000      3.22
> +++ layout/html/base/src/printing.properties    10 Apr 2004 02:01:32 -0000
> @@ -90,7 +90,7 @@
>  NS_ERROR_GFX_PRINTER_DOC_IS_BUSY=The browser cannot print the document while it is being loaded.
>  NS_ERROR_GFX_PRINTING_NOT_IMPLEMENTED=Printing is not implemented.
>  NS_ERROR_GFX_COULD_NOT_LOAD_PRINT_MODULE=The requested print module cannot be loaded.
> -
> +NS_ERROR_GFX_XPRINT_SUPPORT_NOT_BUILD=XPrint printing was disabled at compile time. Please contact the author of this build and request an XPrint-enabled binary.

1. Please add a \n after the first sentence to make the dialog text little bit
more readable.
2. "XPrint" --> "Xprint", please...
From the Xprint-FAQ (http://xprint.mozdev.org/docs/Xprint_FAQ.html).
-- snip --
Q: Which spelling is correct - "Xprint", "XPrint", "Xprinter" or Xprt" ?
A: "Xprint" is the correct one - "XPrint" is just a typo, "Xprinter" is a
   complety different product not related to X11/Xprint and "Xprt" is
   only the "X11 print server"(=the server side of Xprint).
-- snip --

Comment 4

[Andrew Schultz]

does not block Mozilla development

doesn't the patch prevent all printing, (even by the PS module) when Xprint is
not built?  At most, I would expect to see a dummy entry in the dropdown list,
something like "Xprint -- not supported", perhaps a bit more descriptive.

Comment 5

[Roland Mainz]

Andrew Schultz wrote:
> doesn't the patch prevent all printing, (even by the PS module) when Xprint is
> not built? 

No, you can always unset XPSERVERLIST and then everthing works.

> At most, I would expect to see a dummy entry in the dropdown list,
> something like "Xprint -- not supported", perhaps a bit more descriptive.

This doesn't work without doing major surgery in the print engine code
(content/), Platform print coe (gfx/) and the print dialogs (page setup, main,
print job options).
They all expect that a given printer name is something useable. If this isn't
true some functions will fail... and at those points there is usually no error
checking to deal with the problem (the user visible result of such a confusion
is usually a print dialog which has no widgets filled-in... and a crash will
follow if you hit "OK").

Either we add giant bloat to the printing code to handle such cases (=check
EVERY error code and do proper error handling, propagation of the error code to
the caller and resource cleanup) OR start to support C++ exceptions (even across
XPCOM module boundaries) to limit the amount of error checking required. I doubt
that any of these two solutions will ever be supported unless the hell
freezes... and therefore I'll take Wilfried's solution since it seems to
be a quite clean one. The change is isolated and doesn't touch much code.

And moreover - it's not part of the default build (actually --disable-xprint was
NEVER thought to be used by anyone except for debugging purposes. I only added
it on dauphin's request to make him happy... and blizzard started to misuse that
switch for his own purposes):

Comment 6

[Wilfried Drube]

(In reply to comment #4)
> does not block Mozilla development
> 
> doesn't the patch prevent all printing, (even by the PS module) when Xprint is
> not built?
If I install the Xprint RPM then I WANT Xprint. And I WANT a error message to
tell me that the build not useable for me.

Comment 7

[Wilfried Drube]

Attachment: Fix take 2

Comment 8

[Roland Mainz]

Comment on attachment 145815 [details] [diff] [review]
Fix take 2

r=[e-mail]

Comment 9

[Andrew Schultz]

> If I install the Xprint RPM then I WANT Xprint. And I WANT a error message to
> tell me that the build not useable for me.

If my sysadmin installs Xprint (or just incorrectly sets up the environment
variables) and an Xprint-disabled (already half-crippled) Mozilla, I would not
expect Mozilla to cripple itself completely and disable printing altogether.  I
would want to be informed of the problem, but not locked out.

unsetting the env variables is a workaround, but not one that end-users will
know about.

Comment 10

[Andrew Schultz]

> No, you can always unset XPSERVERLIST and then everthing works.

Actually, no you can't.  run-mozilla.sh sets it back (if /etc/init.d/xprint works)

Comment 11

[Wilfried Drube]

(In reply to comment #10)
> > No, you can always unset XPSERVERLIST and then everthing works.
> 
> Actually, no you can't.  run-mozilla.sh sets it back (if /etc/init.d/xprint works)

run-mozilla.sh sets XPSERVERLIST only when is not defined. I do not see a
problem with that.

Comment 12

[Roland Mainz]

Wilfried Drube wrote:
> (In reply to comment #10)
> > > No, you can always unset XPSERVERLIST and then everthing works.
> > 
> > Actually, no you can't.  run-mozilla.sh sets it back (if /etc/init.d/xprint
> works)
>
> run-mozilla.sh sets XPSERVERLIST only when is not defined. I do not see a
> problem with that.

No, ajschult is right. There is a small difference between Unix and Window. On
Windows you can unset an environment variable via setting it to en empty string,
on Unix it still exists with an empty string as value.
"run-mozilla.sh" checks whether using XPSERVERLIST variable results in a
non-empty string, not whether the variable itself was defined yet. Fixing that
will solve that issue.
I wrote a patch for that last week, I'll attach it in a few secs...

Comment 13

[Roland Mainz]

Attachment: Patch for 2004-04-18-trunk to address ajschult's comments

Comment 14

[Brendan Eich [:brendan]]

First, let's talk about this before patching and going for r/sr=.  Second, get
module owners to review -- kyle is not owner or peer of all the files affected.

I've been hacking Unix since 1983, and I can't see a good reason for so bossy
and intrusive a patch.  It's wrong to guess what the user wants.

Here are some counterproposals:

A.  Do nothing, WONTFIX this bug.

B.  Add install-time checks so that if XPrint is installed but Mozilla without
XPrint is installed, the user gets a warning from the installer.

C.  Make the patch affect run-mozilla.sh *only* and have it spit diagnostics to
stderr if XPrint envars are set but the mozilla being launched does not support
XPrint.

D.  (roc's generous idea, but I am not agreeing to it yet) Do all of the
following steps:

D1.  Do not disable printing unless the user unsets XPSERVERLIST.
D2.  Do not ask the user to "contact the author of this build and request ..."
-- delete that whole sentence.
D3.  Add a "warn me again" checkbox, defaulting to unchecked, to the dialog.

/be

Comment 15

[Brendan Eich [:brendan]]

Sorry, D was confusingly written:

D.  (roc's generous idea, but I am not agreeing to it yet) Do all of the
following steps to the latest patch in this bug:

D1.  Do not require the user to unset XPSERVERLIST to prevent printing from
being disabled -- do not disable printing on account of a hunch that the user
wants XPrint and XPRint only!
D2.  Do not ask the user to "contact the author of this build and request ..."
-- delete that whole sentence.
D3.  Add a "warn me again" checkbox, defaulting to unchecked, to the dialog.

/be

Comment 16

[Henry Jia]

Comment on attachment 146566 [details] [diff] [review]
Patch for 2004-04-18-trunk to address ajschult's comments

Cancel the sr request.

Roland, any opinions on Brendan's opinions?

Comment 17

[Andrew Schultz]

Xprint is no longer built (and CVS removed, I think)