262287 - Mozilla Xprint 2004/Q3 update

Status: RESOLVED FIXED

(Core Graveyard :: Printing: Xprint, defect)

Description

[Julien Lafon (Hitachi)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040817
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040817

Mozilla Xprint 2004/Q3 update.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Julien Lafon (Hitachi)]

Attachment: Patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 160635 [details] [diff] [review]
Patch

when changing an interface, you MUST change its uuid.

that's in gfx/idl/nsIPrintSettings.idl and
gfx/src/xprint/nsIDeviceContextSpecXPrint.h at least.

Comment 3

[Roland Mainz]

> +    /* PostScript module does not support changing the colorspace... */
[snip]
> +    /* gfx/src/ps does not pass the job title to lpr */

Please be consistent with the comments (usually ""Postscript module").

[snip]
> -  nsCOMPtr<nsIDrawingSurfaceXlib> mOffscreenSurface; /* not supported for
printers */
> +  nsCOMPtr<nsIDrawingSurfaceXlib> mOffscreenSurface;

There is a comment in nsRenderingContextXp::Init() about offscreen drawing
surfaces. Please update it, too.

> +    /* This is compliciated: Older versions of Xprt do not always have 
> +     * Grayscale/StaticGray visual for a chosen printer so we only use
> +     * the selected visual when it really represents gray color values.
> +     * If the selected visual does not match that requirement we jump
> +     * into an "emulation" codepath which tries to find a visual which
> +     * matches the requirement for grayscale or b/w output */
> +    if ((cs_class | GrayScale) == GrayScale)
> +    {
> +      PR_LOG(nsXPrintContextLM, PR_LOG_DEBUG, ("using selected gray 
> visual\n"));
> +      xargs.xtemplate_mask     = VisualIDMask;
> +      xargs.xtemplate.visualid = csvid;
> +      mXlibRgbHandle = xxlib_rgb_create_handle(mPDisplay, mScreen, &xargs);
> +    }
> +    else

I do not like that part - we should try to get rid of the Color/Grayscale radio
buttons completely somehow. Lets talk about that on tomorrows phone call...

> +  mPixFormat.mRedMask    = visual_info->red_mask;
> +  mPixFormat.mGreenMask  = visual_info->green_mask;;
> +  mPixFormat.mBlueMask   = visual_info->blue_mask;;
> +  mPixFormat.mAlphaMask  = 0;
> +  mPixFormat.mRedCount   = ConvertMaskToCount(visual_info->red_mask);
> +  mPixFormat.mGreenCount = ConvertMaskToCount(visual_info->green_mask);
> +  mPixFormat.mBlueCount  = ConvertMaskToCount(visual_info->blue_mask);;

Extra ';' at the end. Please remove them.

>   PR_LOG(nsXPrintContextLM, PR_LOG_DEBUG,
> -         ("nsXPrintContext::SetupWindow: mDepth=%d, mScreenNumber=%d, 
> colormap=%lx, mDrawable=%lx\n",
> -         (int)mDepth, (int)mScreenNumber, (long)xattributes.colormap, 
> (long)mDrawable));
> +         ("nsXPrintContext::SetupWindow: mVisual->c_class=%x, mDepth=%d, 
> mScreenNumber=%d, colormap=%lx, mDrawable=%lx\n",
> +         (int)mVisual->c_class, (int)mDepth, (int)mScreenNumber, 
> (long)xattributes.colormap, (long)mDrawable));

The visual ID should be logged here, too...

> +  /* Remember the last used printer as new "default" printer as long this
> +   * Mozilla instance is running */
> +  PR_SetEnv(strdup(nsPrintfCString(256, "XPRINTER=%s", printername).get()));

Please use the nsIEnvironment service for that (see
http://lxr.mozilla.org/seamonkey/ident?i=nsEnvironment).

> +int XpuSetEnableFontDownload( Display *pdpy, XPContext pcontext, Bool
enableFontDownload )
> +{
> +  char *value,
> +       *newvalue;
> +  
> +  value = XpGetOneAttribute(pdpy, pcontext, XPPrinterAttr, 
> "xp-listfonts-modes-supported"); 
> +  if( !value )
> +  {
> +    fprintf(stderr, "XpuSetEnableFontDownload: xp-listfonts-modes-supported
printer attribute not found.\n");
> +    return 0; /* failure */
> +  }
> +printf("XpuSetEnableFontDownload: old value='%s'\n", value);

Please remove that |printf()| or #ifdef it out.

> +char *XpuResourceEncode( const char *s )
> +{

Please document the algorithm being used (escaping format for Xrm database
strings).

> typedef struct {
> -  long dpi;
> -  /* ToDo: Support for Xdpi != Ydpi */
> +  const char *resolution_name;
> +  long        x_dpi;
> +  long        y_dpi;
> } XpuResolutionRec, *XpuResolutionList;

|resolution_name| --> |name|, please.

And then there is the issue biesi listed (the UUIDs).

Comment 4

[Roland Mainz]

Reassign to new (default) owner...

Comment 5

[Roland Mainz]

Comment on attachment 160635 [details] [diff] [review]
Patch

Julien:
I don't do superreviews, read http://www.mozilla.org/hacking/reviewers.html

Comment 6

[Roland Mainz]

I forgot two things:
- Xlib toolkits's gfx/src/xlib/nsDeviceContextSpecXlib.* need to be updated, too
(simply copy nsDeviceContextSpecG.* over to ../xlib/. , rename the classes
s/GTK/Xlib/ and you are done (you can build that toolkit with % configure
--enable-default-toolkit=xlib ; gmake # ...).
- Firefox/Thunderbird have copies of printdialog.js, printdialog.xul,
printjoboptions.xul, printjoboptions.dtd, etc. in mozilla/toolkit/. Basically
they are copies from the files in mozilla/xpfe/ except that they are passed
throught a CPP-like preprocessor first.

Comment 7

[Julien Lafon (Hitachi)]

Attachment: New patch

Comment 8

[Roland Mainz]

Comment on attachment 160738 [details] [diff] [review]
New patch

r=[e-mail]

Comment 9

[Roland Mainz]

roc:
Can you take a look at the new patch, please ?

Comment 10

[Julien Lafon (Hitachi)]

Attachment: New patch including firefox bits

Comment 11

[Roland Mainz]

Comment on attachment 160996 [details] [diff] [review]
New patch including firefox bits

r=[e-mail]

Comment 12

[Simon Montagu :smontagu]

Comment on attachment 160996 [details] [diff] [review]
New patch including firefox bits

Transferring sr request

Comment 13

[David :Bienvenu]

Roland or roc, can you suggest a more appropriate sr'er than me? For a patch of
this size, it's probably better to have someone who has a passing familiarity
with this code - cc'ing dbaron.

Comment 14

[Roland Mainz]

David Bienvenu wrote:
> Roland or roc, can you suggest a more appropriate sr'er than me?

Possibly jst, peterv or Henry Jia (others like attinasi are gone... ;-( ) ...
the problem is to find someone who is not overburned.

> For a patch 
> of this size, it's probably better to have someone who has a passing 
> familiarity with this code

The patch isn't that big, most of it is just 1:1 copying of code between
Seamonkey and Firefox...

Comment 15

[Roland Mainz]

Comment on attachment 160996 [details] [diff] [review]
New patch including firefox bits

Reassign sr= request to Henry Jia...

Comment 16

[Henry Jia]

Comment on attachment 160996 [details] [diff] [review]
New patch including firefox bits

Quite tough to sr. Seems good.

sr=Henry

Please pay extra attention to the tinderbox after patch checked in.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Patch checked in to the trunk.  Roland, please file the followup bug on the leak
issues with string manipulations in this code.

Comment 18

[Roland Mainz]

Boris Zbarsky wrote:
> Patch checked in to the trunk.  Roland, please file the followup bug on the 
> leak issues with string manipulations in this code.

I filed bug bug 265925 ("String usage in nsDeviceContextSpec(GTK|Xlib|Qt).cpp is
wrong" ; I assigned the bug to me for now, if I don't have time in the next week
then I'll reassign it to Julien's queue, OK ?).

Marking bug as FIXED.

Comment 19

[Ian Neal]

Relanding toolkit part of patch following aviary branch landing - previously
approved