72087 - Xprint major revamp...

Status: VERIFIED FIXED

(Core Graveyard :: Printing: Xprint, defect)

Description

[Roland Mainz]

(first at all - this is not a real bug, this space in BugZilla database should
be a tracker bug and a mini-mailinglist until the 1st revamp (goal: Mozilla 1.0)
is done)

Changes:
- Printer name can now be a plain printer name ("hplaserjet001") or
"name@host:display" ("hplaserjet001@castor:5"). If no display is given the
XPSERVERLIST variable is used to find a matching display for this printer.
- Xprint is automagically the default print system of the XPSERVERLIST env var
is set
- Attribute "job-owner" is not set manually anymore - this is normally done by
libXp (any changes are overwritten and the attribute get's locked to avoid that
an application can change it anymore)
- XpStartJob() has been moved to BeginDocument() where it really should be - and
to be able to set the "job title"
- "job title" attribute is now set (but it does use a crappy conversion
Unicode-->local charset - which should be COMPOUND_TEXT anyway...)
- Default depth is now 24bit instead of 8bit - there may be a problem with the
8bit pseudocolor visual (number_of_significant_bits==0 !!?). Katakai - is this
really a bug or a feature. If this is a bug - who files the bug report ?
- Xprint module now waits for XP(Start|End)Notify events to be sure to not
change/free any resouces currently used by Xprt (X11 is async and future Xprt
implementations may suffer a lot from such an issue...)
- If DEBUG_gisburn is set you can see the single Xp(Start|End)(Job|Page)() calls
(quite usefull to see what&how&where something goes wrong :-)
- nsXPrintContext::EndDocument now closes the display - it's not sure that the
user may use this particular Xprt anymore - and this avoid that restarting the
Xprt server for reconfiguration crashes Mozilla with an uncatched X error
- nsXPrintContext::mDisplay isn't required anymore (I forgot to remove it - but
the #define mDisplay mPDisplay in nsXPrintContext.cpp speaks for itself... :-)

1. I'll attach a diff and two source files for my current tree with all changes
2. Some problems: The resulting binary has some serious flavs which need to be
discussed:
a) nsXPrintContext objects and related display, screen and fonts can only be
used _once_. This is a result of the problem that users can use _different_
printers on _different_ Xprts'. Current code in nsDeviceContextXP tries to reuse
the old object including old display, screen and _font_ objects - which do not
exist anymore
b) XpuWaitForPrintNotify( Display *pdpy, int detail ) in xprint.c has serious
problems to get the "right" event_base. event_based returned by XpQueryExtension
is "9" - but it seems that the correct one is "91" (see #if 0 in xprintutil.c).
This is either a bug or I am too tired to remember the "correct" way... ;-((
c) The "usual" rendering problems including images, "jumpy" text layout,
BeginDocument() called after EndDocument() and so on...

Comment 1

[Roland Mainz]

Sorry for SPAM... [e-mail] wasn't added to CC: automagically... ;-(

Comment 2

[Roland Mainz]

Attachment: 2001-03-12-08-Mtrunk to gisburn's 2001-03-15 tree

Comment 3

[Roland Mainz]

Attachment: /mozilla/gfx/src/xprint/xprintutil.c 2001-03-15

Comment 4

[Roland Mainz]

Attachment: /mozilla/gfx/src/xprint/xprintutil.h 2001-03-15

Comment 5

[Roland Mainz]

Attachment: /mozilla/gfx/src/xprint/Makefile.in 2001-03-15

Comment 6

[Roland Mainz]

Attachment: GIF from PostScript file viewing http://www.yahoo.de/ using my todays build - best snapshot we ever had... :-)

Comment 7

[Roland Mainz]

Attachment: GIF from PostScript 1st page viewing http://www.openoffice.org/ - Looks nice except the embedded images... ;-(

Comment 8

[Roland Mainz]

Attachment: diff 2001-03-12-08-Mtrunk to gisburn's 2001-03-16 tree

Comment 9

[Roland Mainz]

Attachment: /mozilla/gfx/src/xprint/xprintutil.c 2001-03-16

Comment 10

[Roland Mainz]

Todays changes:
* Fonts are now rendered correctly on Xprt (see todays screenshots) !!!!
- replaced XPRINT_ON_SCREEN and replaced it's functionality with a "magic"
printer name ("xprint_preview"). Still needs minor adjustments and a way to
prevent that multiple pages overwrite each other
- minor code cleanup
- more debugging code
- XMapWindow() is now called on Xprt
- fixed tons of possible NULLptr crashes in xprintutil.c
- no more crashes when page title is a NULLptr - in such cases "Mozilla document
without title" is used a title (should be used if strlen(title)==0, too) (still
leaks the title string if a unicode string as passed...)

Todo:
- XpuWaitForPrintNotify still uses the _wrong_ event base (this is _not_ good...
if more than one event comes back before the XP(Page/Job)Event arrives the
Xprint code continues too fast ;-(( )
- top, bottom, left, right border values need to be honored correctly (todays
GIFs from PostScript were made with top=bottom=left=right set to 0 in print
dialog)
- crash: HTML layout code crashes when printing large pages like
http://puck.informatik.med.uni-giessen.de/people/gisburn/work/rfe_solaris.html
with Xprint. Needs investigation WHY it crashes...
- crash: still crashed due 2nd invocation of nsDeviceContextXP::BeginDocument().
Need comment from Don Cone if this is a bug or a legal behaviour
- crash: crashes on 2nd use of Xprint module - nsXPrintContext objects have to
be deleted after use - each cycle needs it's own object
- page size seems not to be fully used, see issue about top/bottom/left/right
above.
- we need a better value for mTextZoom (see nsXPrintContext::Init) - what about 
mDevUnitsToAppUnits !?
- pages with multiple images are not rendered correctly, it looks like that the
first image rendered is replicated on other locations. Do we have a bug for this
?

Finally:
Time to review&check-in these changes to have something more or less useable for
milestone 0.9 (train for 0.8.1 is gone, right ? Setting REVIEW keyword to grab
the next available train...) - we have everything except one thing: _TIME_ !!

Comment 11

[Christopher Blizzard (:blizzard)]

OK, some comments:

|XpuCheckExtension| is noisy.  There's more |printf| noise in |XpuGetPrinter2|
and probably other places.  That should all be #ifdef DEBUG.

The namespace here is strange.  Shouldn't these functions start with moz or ns
or something?  Starting with 'X' is usually the domain of the X libraries.

You're using things like |strtok_r|.  You should really be using PR equivalents.

You're using free/malloc vs. PR_Free() PR_Malloc().

 /* BUG: This does not work for quoted attribute values */

Are you going to clean that up?

 printf("XpuGetOneLongAttribute: '%s' got '%s'\n", X(attribute_name), X(s));

What the heck does X() do?

there's this code, too:

while( !(
    (ev.type ==
#if 0     
    (event_base_return+XPPrintNotify)
#else
    91
#endif
    )
    && (((XPPrintEvent *)(&ev))->detail == detail)) );

Ahh, can you fix that so it's a little more readable? :)

Comment 12

[Roland Mainz]

Attachment: diff 2001-03-12-08-Mtrunk to gisburn's 2001-03-17 tree

Comment 13

[Roland Mainz]

Attachment: /mozilla/gfx/src/xprint/xprintutil.c 2001-03-17

Comment 14

[Roland Mainz]

> OK, some comments:

Blizzard: Thank you for reviewing!! :-)

> |XpuCheckExtension| is noisy.  There's more |printf| noise in |XpuGetPrinter2|
> and probably other places.  That should all be #ifdef DEBUG.

Done&&fixed.

> The namespace here is strange.  Shouldn't these functions start with moz or ns
> or something?  Starting with 'X' is usually the domain of the X libraries.

Uhm... Xpu* functions should make it into libXpu.so (libXprintUtils :-) or
(better) into libXp.so (itself).

> You're using things like |strtok_r|.  You should really be using PR 
equivalents.

In theory yes. But isn't strtok_r() defined by POSIX ? For example
/mozilla/include/libc_r.h itself makes native use of strtok_r(). I can't find a
system (Solaris >= 2.5.1, Linux etc.) have strtok_r() - even the old crappy AIX
box has it...
...and I didn't find a NSPR-equiv. to strtok_r()... ;-(
(Someone else suggested to use nsString* - but AFAIK this looks like a huge
overkill for this tiny strtok_r() function. if this is really an issue I can
grab the strtok_r() from old Amiga's DICE cc sources to get rid of this issue...
=:-)

> You're using free/malloc vs. PR_Free() PR_Malloc().

Fixed. If USE__MOZILLA_TYPES is set it uses the NSPR memory functions (but I
still have to use XFree() to free mem. from XpGet*() functions...

> /* BUG: This does not work for quoted attribute values */
> Are you going to clean that up?

_YES_ - but currentit it does not hurt. The page size attribute is the only one
on my radar which uses quoted attribute values - but first I'd like to hunt&kill
other more urgend bugs before I start to hack my own strtok_r() version to
handle quoted values...

> printf("XpuGetOneLongAttribute: '%s' got '%s'\n", X(attribute_name), X(s));
> What the heck does X() do?

printf() on Solaris is a little bit "picky" when you pass a NULL ptr as an
argument - usually a core dump follows. X() is my one-char-kill-this-xxxxx-issue
solution for debugging-printf()s. Only used for debugging to avoid that debug
code crashes the builds...

> there's this code, too:
>
> while( !(
>    (ev.type ==
> #if 0     
>    (event_base_return+XPPrintNotify)
> #else
>    91
> #endif
>    )
>    && (((XPPrintEvent *)(&ev))->detail == detail)) );
> Ahh, can you fix that so it's a little more readable? :)

Added comment in code. The real problem is that XpQueryExtension() returns
another event base (event_base_return+XPPrintNotify(=9)) than used by events
send by Solaris Xprt (91). 
Either my code is wrong or Xprt source  has a typo - "9" vs. "91"... uhm...

Todays changes:
- Fixed memory leak in xprint.c/XpuGetOneLongAttribute() for attribute value
strings with zero length
- nsDeviceContextXP::EndDocument() now deletes the nsXPContext object as these
objects cannot be reused safely for different printers (better: Xprint printer
models) and/or Xprint servers. This is somehow incomplete as some font refereces
are still cached somewhere - and a 2nd attempt to print fails due these refs...
;-( See below...
- Debugging output is now only present if DEBUG and/or DEBUG_gisburn are defined
- xprintutil.c now uses NSPR memory functions instead of libc memory calls

Issues:
- nsDeviceContextXP.h contains a nice comment:
-- snip --
nsVoidArray mFontMetrics;  // we are not using the normal font cache, this is
special for PostScript.
-- snip ---
Does that mean that mFontMetrics can somehow be "removed"  ?
/mozilla/gfx/src/xlib/ does not use it... mhhh... blizzard... do you have any
ideas if mFontMetrics can be removed ?
- 2nd invocation of Xprint module still does not work (see reason above...)
- paper selection needs to be implemented - but this requires a redesign of the
Unix print dialog as Xprint _knows_ which paper size(s) are supported by the
printer - including "unusual" paper sizes like DIN-A0 (poster-size)...
- images are not scaled to fit text scaling

Question to blizzard: Can we check-in this update, please ?

Comment 15

[Roland Mainz]

Typo...
-- snip --
I can't find a system (Solaris >= 2.5.1, Linux etc.) have strtok_r() - even the
old crappy AIX box has it...
-- snip --
should be
-- snip --
I can't find a system (Solaris >= 2.5.1, Linux etc.) which does not have
strtok_r() - even the old crappy AIX box has it...
-- snip --
I really should read the stuff twice before posting it... sorry... ;-(

Comment 16

[Roland Mainz]

Filed bug 72372 for the crash in HTML layout code.

More issues:
- Xprint module does not wait properly until app page content has been rendered
- PS module does... but _why_ ?

Comment 17

[Roland Mainz]

More issues:
- PostScript output at 600DPI (all previous examples were made in 300DPI
resolution) shows only the left half of the page - it seems that the left border
is far too large... ;-(

Comment 18

[Jim Dunn]

The latest set of diffs 3/17 compile on hp & aix.
On HP (my aix system doesn't have -lXp) I did do a print
(not sure when/if it runs through this code) but was able 
to print out my homepage.

Comment 19

[Roland Mainz]

To Jim Dunn: Thank you very much for testing... :-))

> not sure when/if it runs through this code

Please check if the XPSERVERLIST env var is set (some HP-UX machines set it). If
the var is set Xprint module is used for printing, if not native PostScript
module is used.

If XPSERVERLIST is not set try the following:
% Xprt -audit 4 :1 &
% export XPSERVERLIST=":1"
% mozilla

This will start an Xprt server (X11 print server) in it's default configuration
(which is - unfortunately - the worst imaginable config as Xprt assumes all
printers are dumb and do not have any build-in fonts, see bug 68779) and tells
Mozilla to print to it (XPSERVERLIST can contain a list of available Xprt
servers, seperated by blanks. You can either use the plain printer name (like
"hplaserjet001"; Xprt's in XPSERVERLIST are searched for this printer) or
printer@display, e.g. "hplaserjet001@puck:1"). For remote Xprt's: be sure to
allow that your host/user can access the Xprt, e.g. "xhost +myhost" or "xhost
+myuser@"(=when using SUN-DES-1 or MIT-KERBEROS-5 authentifiaction, plain
MIT-MAGIC-COOKIE1 is not a user2user authentification!!) for that specific Xprt
display...
(my personal recommendation is to grab a Solaris 7/8 machine with latest
Xsun/Xprt patch, start Xprt on it and set XPSERVERLIST to include that server...
:-)

I am going to add a new patch file because the old one was accidently created
with diff's ".-w" option. Patched build correctly in progress, will report when
finished.

To jdunn: Should I attach the full files for you, too ?

Comment 20

[Roland Mainz]

Attachment: moz_2001-03-22-08-trunk to gisburn's 2001-03-23 xprint tree diff

Comment 21

[Roland Mainz]

Attachment: /mozilla/gfx/src/xprint/xprintutil.c 2001-03-23

Comment 22

[Roland Mainz]

Todays changes:
- XpuGetPrinter() no longer creates a print context for non-existing printers
and returns success in such cases
- nsXPrintContext::SetupPrintContext now returns an error if
"default-printer-resolution" DocAttr is not available (e.g. Xprt misconfigured
to a non-supported default DPI value)
- killed some dead code
- removed some unused vars from headers
- debug: nsDeviceContextXP::CheckFontExistence now logs queries to
XListFontsWithInfo showing that CheckFontExistence looks for 75/100DPI fonts
instead for fonts in Xprt's DPI (katakai... is this maybe one of the causes for
bug 68779 ?? (at least the quality problems may be caused by this insanity...
;-( ))...
- debug: Rearranged XpuWaitForPrintNotify() problem&workaround with "wrong
event_base returned by X API" little bit to be more understandable
- debug: Added assert in nsRenderingContextXP::DrawImage to catch bug 73178
- Killed TABs in all */xprint/ source files (wasn't my fault !! =:-)
- created diffs without "-w", e.g. % diff -r -c src/ dest/ # (that was _my_
fault... ;-( )

Comment 23

[Roland Mainz]

Attachment: moz_2001-03-22-08-trunk to gisburn's 2001-03-23 xprint tree "unified" diff created with "gdiff -u"

Comment 24

[Roland Mainz]

Attached diffs from "moz_2001-03-22-08-trunk to gisburn's 2001-03-23" created
with GNU diff (diff (GNU diffutils) 2.7.2) - no changes, only new diff format...

Anyone interested in r= && sr= ?

Comment 25

[Roland Mainz]

Adding reference to bug 73855 - we need a "better" unix print dialog for Xprint
support...

Comment 26

[Roland Mainz]

Adding prabhat.hegde based on IRC request by richb...

----

Newer patches on demand (mainly support for "print to file" functionality using
a consumer thread for XpGetDocumentData(3Xp)).
I post the patches here if someone has time to review them...

Comment 27

[Roland Mainz]

Going to file new patches for current tip, recompile and rework of code need's
few hours, will report back in 22h...

Comment 28

[Christopher Blizzard (:blizzard)]

Comments:

Instead of using +#define WHEREAMI(s) use NSPR logging.  It's clean and then
anyone can use it.  Look in prlog.h for instruction and examples.  It's really
easy to use.

Waiting for a diff -uw patch.

Comment 29

[Roland Mainz]

Attachment: mozilla/gfx/src/xprint/xprintutil.c 2001-04-06

Comment 30

[Roland Mainz]

Attachment: mozilla/gfx/src/xprint/xprintutil.h 2001-04-06

Comment 31

[Roland Mainz]

Attachment: mozilla/gfx/src/xprint/xprintutil_printtofile.c 2001-04-06

Comment 32

[Roland Mainz]

Attachment: Xprint-2001-04-06 diffs for 2001-04-05-08-trunk (gdiff -u)

Comment 33

[Roland Mainz]

Attachment: Xprint-2001-04-06 diffs for 2001-04-05-08-trunk (gdiff -u -w) REVIEW ONLY !!

Comment 34

[Roland Mainz]

Some comments on todays patches:
1. Resulting binary does only work on pages with no images. This is not my fault
nor caused by this patches - someone else killed this functionality. This is
known as bug 73178. I added NS_ASSERT() to "mark" the point of crash to avoid
duplicate bug reports for this issue.
2. Code now supports print-to-file functionality, see
mozilla/gfx/src/xprint/xprintutil_printtofile.c for details
3. Code know uses nsLogging facilities except Xpu*-stuff. I'll fix this later
(please please)
4. Printing does only work _once_ as currently nsFontmetrics&co. cache  stuff to
current display/screen/print context - which is not _legal_ as user may choose
another printer on another display for printing - and even the current printer
may use another set of fonts based on it's config.
5. left/right/top/bottom border must be set to 0.0 before printing - somehow the
border calculations are totally out of control (bustage due careless hacking in
other area without keeping Xprint in sync... ;-(( )

Testing:
1. Start Xprt:
% Xprt -audit :1
2. Set XPSERVERLIST to tell Mozilla where to find some Xprint servers:
% export XPSERVERLIST=":1" # as much as you want, seperated by blanks
% ./mozilla

Print to file functionality requires top select a legal printer _and_ a legal
filename. The printer name is required because the print job is exactly
generated for _this_ printer - only the destination is changed from spooler
system to a file...

Legal printer names are either plain name - or plain_name@display, e.g. "foo" ==
"foo" or "foo@ganja:5"...

Comment 35

[Mike Shaver (:shaver emeritus)]

Super-review comments:

   nsCOMPtr<nsIPref> pPrefs = do_GetService(NS_PREF_CONTRACTID, &rv);
   if (NS_SUCCEEDED(rv) && pPrefs) {
-    PRInt32 method = 0;
+    PRInt32 method = NS_DEFAULT_PRINT_METHOD;
     (void) pPrefs->GetIntPref("print.print_method", &method);
     aMethod = method;
   } else {
-    aMethod = 0;
+    aMethod = NS_DEFAULT_PRINT_METHOD;
   }
   return NS_OK;
 }

Should we be checking for success here?  This code seems to assume that
GetIntPref can return without mutating its parameter, which seems kinda
suspect: what happens when it's called from JS, for example?

-DEFINES         += -D_IMPL_NS_GFXONXP -DUSE_MOZILLA_TYPES
+DEFINES         += -D_IMPL_NS_GFXONXP -DUSE_MOZILLA_TYPES -DXPU_USE_THREADS

What is XPU_USE_THREADS for?

+  PR_LOG(nsDeviceContextXPLM, PR_LOG_DEBUG, ("nsDeviceContextXP::SetSpec()\n"));
+
+  nsIDeviceContextSpecXP *xpSpec;

It looks like the rest of this file keeps lines < 80 columns, so it might be
nice if you were to as well.

   mSpec = aSpec;
   NS_ADDREF(aSpec);
   if (mPrintContext == nsnull) {
      mPrintContext = new nsXPrintContext();
-     res = mSpec->QueryInterface(kIDeviceContextSpecXPIID, (void **) &psSpec);
+     res = mSpec->QueryInterface(kIDeviceContextSpecXPIID, (void **) &xpSpec);
      if (res == NS_OK) {

This code screams out for use of nsCOMPtrs (and it looks like it doesn't
release the old mSpec when it over-writes it!) like:

  nsCOMPtr<nsIDeviceContext> xpSpec;
  mSpec = aSpec;
  if (!mPrintContext) {
    mPrintContext = new XPrintContext();
    xpSpec = do_QueryInterface(mSpec);
    if (xpSpec)
      mPrintContext->Init(xpSpec);
  }

Also, should mPrintContext be an nsCOMPtr?  I realize that you inherited this
code in its current state, but since it seems that you'll be the owner of this
code going forward, I'm going to ask you to assert ownership over it and bring
it into the 21st century with nsCOMPtrs, removal of NS_DEFINE_IID, and the
like.  This means that you get to pick whether you'll test null pointers
implicitly or explicitly, and whether you'll use nsnull or 0 for them (please
don't use NULL), and it means that you get to choose the indentation increment
and brace placement, but it also means that I'll expect you to make the files
universally follow your rules.

+#ifdef DEBUG
+  printf("nsDeviceContextXP::CheckFontExistence: XListFontsWithInfo '%s'=%d\n",
+         wildstring, numnames);
+#endif

Please use DEBUG_gisburn or whatever, or better yet use the PR_LOG
calls.

+   // gisburn: try to catch bug 73178] - crash in
nsTransform2D::TransformNoXLateCoord
+   NS_ASSERTION(nsnull != mTMatrix,   "mTMatrix is null.");
+   NS_ASSERTION(nsnull != mTranMatrix,"mTranMatrix is null.");
+
    sr = aSRect;
    mTMatrix->TransformCoord(&sr.x, &sr.y, &sr.width, &sr.height);

In the null mTMatrix or mTranMatrix case, you're going to crash after the
assertion.  If you're coming from a traditional C programming background, it
may surprise you discover that our assertions are generally non-fatal.  Do you
want to return early, or crash to see what's on the stack when you get a null
matrix?  (Either is reasonable, I'm just not sure if you intend the current
situation.)

-static int xerror_handler(Display *display, XErrorEvent *ev) {
+extern "C" /* Make Sun Workshop and other conformant compilers happy... :-) */
+static 
+int xerror_handler(Display *display, XErrorEvent *ev) 

Is this a case where you need PR_CALLBACK for better portability?  I'm not sure
what the conformance issue is, so forgive me if I'm barking up the wrong tree.

+{
+    /* this should _never_ be happen... but if this happens - debug mode or not
- scream !!! */
     char errmsg[80];
-    XGetErrorText(display, ev->error_code, errmsg, 80);
+    XGetErrorText(display, ev->error_code, errmsg, sizeof(errmsg));
     fprintf(stderr, "lib_xprint: Warning (X Error) -  %s\n", errmsg);
-    return 0;
+    return(0);
 }

Are we sure we want to spew console error unconditionally?  I guess that's the
traditional XError behaviour, but I'm generally against it.  Up to you, in the
end.  (Why did you add the parens in |return (0);|?  That doesn't seem to be
the general pattern in this code.)

 nsXPrintContext::nsXPrintContext()
 {
+   PR_LOG(nsXPrintContextLM, PR_LOG_DEBUG,
("nsXPrintContext::nsXPrintContext()\n"));
+   
+   mPDisplay    = (Display *)NULL;
    mPContext = (XPContext )0;
-   mPrintServerName = (char *)0;
-   mPrinterName     = (char *)0;
-   mScreen = (Screen *)0;
-   mVisual = (Visual *)0;
+   mScreen      = (Screen *)NULL;
+   mVisual      = (Visual *)NULL;
    mGC     = (GC )0;
    mDrawable = (Drawable )0;
    mDepth = 0;

You shouldn't be using C-style casts, but luckily in C++ you can just use 0
without casting for any pointer type.  What about using initializer lists here
instead of assignments?  It's more standard C++ behaviour.

+// debug: "print" on display server for quick debugging
+// see sleep() in nsXPrintContext::EndDocument()
+#define XprintOnScreen (!strcmp("xprint_preview",(aSpec->GetCommand(&buf),buf)))
+

We prefer macros to be UPPERCASE_AND_USING_UNDERSCORES, so that potential
re-evaluation side-effects are easier to spot.

Also, if it's ``debug'', should it (and the XPRINT_ON_SCREEN check below it) be
#if DEBUG?

+    if( T(SetupPrintContext(aSpec)) != NS_OK )
+      return NS_ERROR_FAILURE;
+    

What's |T|?  (And don't check against NS_OK explicitly unless you have a very
good reason; NS_SUCCEEDED() is the Better Way.)

+  PR_LOG(nsXPrintContextLM, PR_LOG_DEBUG, ("nsXPrintContext::SetupWindow()\n"));
+  
+  PR_LOG(nsXPrintContextLM, PR_LOG_DEBUG, ("nsXPrintContext::SetupWindow: x=%d
y=%d width=%d height=%d\n", x, y, width, height));
+

Are both of those needed?

-  mGC     = XCreateGC(mDisplay, mDrawable, gcmask, &gcvalues);
+  mGC     = XCreateGC(mPDisplay, mDrawable, gcmask, &gcvalues);
  return NS_OK;

Can XCreateGC fail?  Should you report an error if it does?  (You might want to
look over the files as a whole and make sure you have a consistent error
reporting story.)

+  /* Are we "printing" to a file instead to the print queue ? */
+  if (mIsAPrinter != PR_TRUE) 
+  {

Explicit comparison against PR_TRUE adds, IMO, noise without any additional
clarity.  What about just |if (mIsAPrinter)| ?

+    /* ToDo: Guess a matching file extension of user did not set one - Xprint 
+     * currrently may use %s.ps, %s.pcl, %s.xwd, %s.pdf etc.
+     * Best idea would be to ask a "datatyping" service (like CDE's datatyping 
+     * functions) what to-do...

Is the MIME extension service what you're looking for?

+/* ConvertUCS2ToLocalEncoding: stolen from 
+ * mozilla/webshell/embed/xlib/qt/QMozillaContainer.cpp
+ * XXX Dont forget to delete this 'C' String since we create it here
+ * Note that this function is _only_ a _hack_ until RFE 73446 
+ * (http://bugzilla.mozilla.org/show_bug.cgi?id=73446 - "RFE: 
+ * Need NS_ConvertUCS2ToLocalEncoding() and 
+ * NS_ConvertLocalEncodingToUCS2()") gets implemented...
+ */

Good comment, and good explanation of when to update this code.

+static 
+char* makeCString( const PRUnichar *aString )

Could you name it something like convertUCS2ToLocalEncoding?  (Or Convert...,
because we try to name C/C++ functions with leading capitals.)

+    char *cstring = new char[ ++len ];
+

You should use the shared allocator, I think.

+  // DEBUG: sleep 15secs if we're displaying to an "display" server
+  // see nsXPrintContext::Init and XprintOnScreen macro above...
+  if( XpuCheckExtension(mPDisplay) != 1 )
+  {
+    sleep(15);
+  }
+

Dude, you can't just sleep for 15 seconds on the layout thread.  Even if you're
in DEBUG mode, which you don't check here.  Please find another way to do
whatever you're trying to do here.

   // XXX kipp: this is temporary code until we eliminate the
   // width/height arguments from the draw method.
-  if ((aWidth != width) || (aHeight != height)) {
+  if ((aWidth != width) || (aHeight != height)) 

Ah, kipp.  Temporary indeed. =)

+      mAlphaPixmap = XCreatePixmap(mPDisplay, 
+                                RootWindow(mPDisplay, mScreenNumber),

Looks like mismatched indentation there.

-  NS_IMETHOD BeginDocument();
+  NS_IMETHOD BeginDocument(PRUnichar * aTitle);

In general, you probably want to use more ``modern'' strings, such as
nsAString.

So, Roland, it looks pretty good, though there are some serious and
less-serious isses enumerated above.  I won't insist on you normalizing the
code style everywhere before you check in, but I would like you to do that in
the near future, and all of your changes should follow the new,
gisburn-approved style.  Can you take a look at the issues above and post
another patch.

Thanks for your patience with this, and my apologies, on behalf of mozilla.org,
for the unpleasant time you've had getting this reviewed.

Comment 36

[Roland Mainz]

>    nsCOMPtr<nsIPref> pPrefs = do_GetService(NS_PREF_CONTRACTID, &rv);
>    if (NS_SUCCEEDED(rv) && pPrefs) {
> -    PRInt32 method = 0;
> +    PRInt32 method = NS_DEFAULT_PRINT_METHOD;
>      (void) pPrefs->GetIntPref("print.print_method", &method);
>      aMethod = method;
>    } else {
> -    aMethod = 0;
> +    aMethod = NS_DEFAULT_PRINT_METHOD;
>    }
>    return NS_OK;
>  }
> 
> Should we be checking for success here?  This code seems to assume that
> GetIntPref can return without mutating its parameter, which seems kinda
> suspect: what happens when it's called from JS, for example?

Simply unknown. This is currently the only place where the default for "aMethod"
is set - and currently the only place where it can be safely "done" without
moving/changing much bigger pieces of code... - I still wonder why this part of
the patch still works without being "killed" by other checkins... =:-)

> -DEFINES         += -D_IMPL_NS_GFXONXP -DUSE_MOZILLA_TYPES
> +DEFINES         += -D_IMPL_NS_GFXONXP -DUSE_MOZILLA_TYPES -DXPU_USE_THREADS
> 
> What is XPU_USE_THREADS for?

See xprintutil* sources. This tells XprintUtil* stuff to use "threads" instead
of pthread-threads.

> +  PR_LOG(nsDeviceContextXPLM, PR_LOG_DEBUG,
("nsDeviceContextXP::SetSpec()\n"));
> +
> +  nsIDeviceContextSpecXP *xpSpec;
> 
> It looks like the rest of this file keeps lines < 80 columns, so it might be
> nice if you were to as well.

Sure... an accident... I usually work with 132 columns... :-)
BTW: Is there any Mozilla.org document which defines source layout (some
companies have such specs...)

>    mSpec = aSpec;
>    NS_ADDREF(aSpec);
>    if (mPrintContext == nsnull) {
>       mPrintContext = new nsXPrintContext();
> -     res = mSpec->QueryInterface(kIDeviceContextSpecXPIID, (void **)
&psSpec);
> +     res = mSpec->QueryInterface(kIDeviceContextSpecXPIID, (void **)
&xpSpec);
>       if (res == NS_OK) {
> 
> This code screams out for use of nsCOMPtrs (and it looks like it doesn't
> release the old mSpec when it over-writes it!) like:

See below...

>   nsCOMPtr<nsIDeviceContext> xpSpec;
>   mSpec = aSpec;
>   if (!mPrintContext) {
>     mPrintContext = new XPrintContext();
>     xpSpec = do_QueryInterface(mSpec);
>     if (xpSpec)
>       mPrintContext->Init(xpSpec);
>   }

I don't know much about XPCOM stuff nor about nsCOMPtrs... trying to cut&paste
your example above only spews tons of errors... see below...

> Also, should mPrintContext be an nsCOMPtr?  I realize that you inherited this
> code in its current state, but since it seems that you'll be the owner of this
> code going forward, I'm going to ask you to assert ownership over it and bring
> it into the 21st century with nsCOMPtrs, removal of NS_DEFINE_IID, and the
> like.  This means that you get to pick whether you'll test null pointers
> implicitly or explicitly, and whether you'll use nsnull or 0 for them (please
> don't use NULL), and it means that you get to choose the indentation increment
> and brace placement, but it also means that I'll expect you to make the files
> universally follow your rules.

Ahhhh... big... got the problem. Do you think XPContext is XPCOM stuff ? No...
XPContext is _xprint_context_ - a X11 XID... try
-- snip --
% cat /usr/include/X11/extensions/Print.h | fgrep "XID"
typedef XID XPContext;
-- snip --

> +#ifdef DEBUG
> +  printf("nsDeviceContextXP::CheckFontExistence: XListFontsWithInfo
'%s'=%d\n",
> +         wildstring, numnames);
> +#endif
> 
> Please use DEBUG_gisburn or whatever, or better yet use the PR_LOG
> calls.

Oups... which slipped through my claws... ;-(

> +   // gisburn: try to catch bug 73178] - crash in
> nsTransform2D::TransformNoXLateCoord
> +   NS_ASSERTION(nsnull != mTMatrix,   "mTMatrix is null.");
> +   NS_ASSERTION(nsnull != mTranMatrix,"mTranMatrix is null.");
> +
>     sr = aSRect;
>     mTMatrix->TransformCoord(&sr.x, &sr.y, &sr.width, &sr.height);
> 
> In the null mTMatrix or mTranMatrix case, you're going to crash after the
> assertion.  If you're coming from a traditional C programming background, it
> may surprise you discover that our assertions are generally non-fatal. 

Yup... I knew as a wrote the code...

> Do you
> want to return early, or crash to see what's on the stack when you get a null
> matrix?  (Either is reasonable, I'm just not sure if you intend the current
> situation.)

No... this is a new problem caused by changes somewhere else. Printting images
did work - now it does not anymore... ;-( I only added the NS_ASSSERT stuff to
remind me and others that those crashes are bug 73178 and do not require to spam
me with additional stack traces from coredumps. It looks that there is no easy
fix for this - at least I did not see one while comparing PS module source
changes comapared to current Xprint source. I assume toolkit wizards like
blizzards can fix this better than me...

> -static int xerror_handler(Display *display, XErrorEvent *ev) {
> +extern "C" /* Make Sun Workshop and other conformant compilers happy... :-)
*/
> +static 
> +int xerror_handler(Display *display, XErrorEvent *ev) 
> 
> Is this a case where you need PR_CALLBACK for better portability?  I'm not 
sure
> what the conformance issue is, so forgive me if I'm barking up the wrong tree.

After looking into include/prtypes.h ... no... PR_CALLBACK won't help... and
after testing: It does not help. (This was the only warning I was getting for
this source file (Sun Workshop is very picky - I was suprised to get only _one_)
- therefore I killed this little beast... :-)
This fix should really work for all compilers...

> +{
> +    /* this should _never_ be happen... but if this happens - debug mode or
not
> - scream !!! */
>      char errmsg[80];
> -    XGetErrorText(display, ev->error_code, errmsg, 80);
> +    XGetErrorText(display, ev->error_code, errmsg, sizeof(errmsg));
>      fprintf(stderr, "lib_xprint: Warning (X Error) -  %s\n", errmsg);
> -    return 0;
> +    return(0);
>  }
> 
> Are we sure we want to spew console error unconditionally?  I guess that's the
> traditional XError behaviour, but I'm generally against it.  Up to you, in the
> end. 

As I wrote in source - those errors should never never appear. If they occur
some really worse worse worse things are going on. I don't like to hide such
problems...

> (Why did you add the parens in |return (0);|?  That doesn't seem to be
> the general pattern in this code.)

I used return(xzy) the last ~15 years of C programming - this simply slipped
through my fingers...

>  nsXPrintContext::nsXPrintContext()
>  {
> +   PR_LOG(nsXPrintContextLM, PR_LOG_DEBUG,
> ("nsXPrintContext::nsXPrintContext()\n"));
> +   
> +   mPDisplay    = (Display *)NULL;
>     mPContext = (XPContext )0;
> -   mPrintServerName = (char *)0;
> -   mPrinterName     = (char *)0;
> -   mScreen = (Screen *)0;
> -   mVisual = (Visual *)0;
> +   mScreen      = (Screen *)NULL;
> +   mVisual      = (Visual *)NULL;
>     mGC     = (GC )0;
>     mDrawable = (Drawable )0;
>     mDepth = 0;
> 
> You shouldn't be using C-style casts, but luckily in C++ you can just use 0
> without casting for any pointer type.  What about using initializer lists here
> instead of assignments?  It's more standard C++ behaviour.

What about using matching #defines for initalisation ? I know that NULL or
nsnull should be used for pointers - but what about X11 resource IDs (XIDs)
required for XPContext/GC/etc. ?

> +// debug: "print" on display server for quick debugging
> +// see sleep() in nsXPrintContext::EndDocument()
> +#define XprintOnScreen
(!strcmp("xprint_preview",(aSpec->GetCommand(&buf),buf)))
> +
> 
> We prefer macros to be UPPERCASE_AND_USING_UNDERSCORES, so that potential
> re-evaluation side-effects are easier to spot.

I agree... stupid name... :-)
I will fix this...

> Also, if it's ``debug'', should it (and the XPRINT_ON_SCREEN check below it)
be
> #if DEBUG?

In theory yes... there's still the question if this may be a quick&dirty way to
hack-up a "print previre" functionality for Xprint...
I'll hack DEBUG around it...

> +    if( T(SetupPrintContext(aSpec)) != NS_OK )
> +      return NS_ERROR_FAILURE;
> +    
> 
> What's |T|? 

Short form for "TRACE" - print function name before execution, see xprintutil.h

> (And don't check against NS_OK explicitly unless you have a very
> good reason; NS_SUCCEEDED() is the Better Way.)

Why ?

> +  PR_LOG(nsXPrintContextLM, PR_LOG_DEBUG,
("nsXPrintContext::SetupWindow()\n"));
> +  
> +  PR_LOG(nsXPrintContextLM, PR_LOG_DEBUG, ("nsXPrintContext::SetupWindow:
x=%d
> y=%d width=%d height=%d\n", x, y, width, height));
> +
> 
> Are both of those needed?

Not really. But it may be possible that the print(x,y,w,h) will be removed
soon... I did the PR_LOG stuff in two steps, first log function names, then move
printf/puts to PR_LOG stuff...
Should I really fix this ?

> -  mGC     = XCreateGC(mDisplay, mDrawable, gcmask, &gcvalues);
> +  mGC     = XCreateGC(mPDisplay, mDrawable, gcmask, &gcvalues);
>   return NS_OK;
> 
> Can XCreateGC fail?  Should you report an error if it does?  (You might want
to
> look over the files as a whole and make sure you have a consistent error
> reporting story.)

I agree that this required better error checking - most of the code needs better
error checking. There are much more stupid issues with lame error checking. I'd
like to care later of all those issues if possible.
BTW: Methods which calls Xprint stuff to not care about returns codes either -
they simply continue if I return an error... ;-((((

> +  /* Are we "printing" to a file instead to the print queue ? */
> +  if (mIsAPrinter != PR_TRUE) 
> +  {
> 
> Explicit comparison against PR_TRUE adds, IMO, noise without any additional
> clarity.  What about just |if (mIsAPrinter)| ?

Mhhh. Some compilers do not like if(expr) statements where "expr" is not an
"int" value. Is data type for "mIsAPrinter" (_always_) an "int" type ?

> +    /* ToDo: Guess a matching file extension of user did not set one - Xprint 
> +     * currrently may use %s.ps, %s.pcl, %s.xwd, %s.pdf etc.
> +     * Best idea would be to ask a "datatyping" service (like CDE's
datatyping 
> +     * functions) what to-do...
> 
> Is the MIME extension service what you're looking for?

Nope. CDE datatyping adds a layer above mime types. Mime types are good for
transport issues - but on the desktop they're a a stupid&&_bad_ idea - for
example one datatype can have multiple valid mime types... ;-(
CDE dataypting can return an extension pattern for a given datatype like "%.ps":
Example ("dttypes" application queries CDE's datatyping database for given
files):
-- snip --
% dttypes -type Xpjob_010406143022 
Xpjob_010406143022 is of type POSTSCRIPT
=============== POSTSCRIPT ===============
loaded from castor:/usr/dt/appconfig/types/C/datatypes.dt
        ACTIONS :       Open,Print
        DESCRIPTION :   This file contains postscript data. Its data type is
named PS.  PS file have names ending with '.ps' or '.PS', or contain the
characters "%!".
        DATA_HOST :
        ICON :  Dtps
        NAME_TEMPLATE : %s.ps
        MIME_TYPE :     application/postscript
        TYPE_LABEL :    POSTSCRIPT
        SUNV3_TYPE :    postscript-file
-- snip --
Take a look at the NAME_TEMPLATE field...

Mozilla should really use CDE datatyping if available instead of it's own
mime-type-only stuff in such cases when available...

[snip]
> +static 
> +char* makeCString( const PRUnichar *aString )
> 
> Could you name it something like convertUCS2ToLocalEncoding?  (Or Convert...,
> because we try to name C/C++ functions with leading capitals.)

I'd like to preserve the name makeCString() if possible as the body is a 1:1
copy (except source reformatting) from the source listed in the comment. I agree
that your naming is better - but the function should be removed anyway in the
near future... scc is working on it...

> +    char *cstring = new char[ ++len ];
> +
> 
> You should use the shared allocator, I think.

Gllwww... OK... but I don't like to touch this part of the source anyway... it's
a 1:1 copy... I'll kill the whole function if the replacement is ready...

> +  // DEBUG: sleep 15secs if we're displaying to an "display" server
> +  // see nsXPrintContext::Init and XprintOnScreen macro above...
> +  if( XpuCheckExtension(mPDisplay) != 1 )
> +  {
> +    sleep(15);
> +  }
> 
> Dude, you can't just sleep for 15 seconds on the layout thread.  Even if
you're
> in DEBUG mode, which you don't check here.  Please find another way to do
> whatever you're trying to do here.

I hack #ifdef DEBUG around it, OK ?
The other option would be to keep the window open all the time and destruct it
somewhere else... but where... I don't like to invenst more into this DEBUG code
unless we find a real use for it (preview maybe) - that's the only reason why
this code is still "in"...

BTW: Why is it dangerous to wait 15secs in the layout thread ?

>    // XXX kipp: this is temporary code until we eliminate the
>    // width/height arguments from the draw method.
> -  if ((aWidth != width) || (aHeight != height)) {
> +  if ((aWidth != width) || (aHeight != height)) 
> 
> Ah, kipp.  Temporary indeed. =)

I only changed the "{" - I don't know what the code is for... blizzard/kipp may
know... ;-(

> +      mAlphaPixmap = XCreatePixmap(mPDisplay, 
> +                                RootWindow(mPDisplay, mScreenNumber),
> 
> Looks like mismatched indentation there.
> 
> -  NS_IMETHOD BeginDocument();
> +  NS_IMETHOD BeginDocument(PRUnichar * aTitle);
> 
> In general, you probably want to use more ``modern'' strings, such as
> nsAString.

PS/Xlib/GTK+ code does not use it, too. It would be usefull to change this for
all toolkits in a consistent way...

> So, Roland, it looks pretty good, though there are some serious and
> less-serious isses enumerated above.  I won't insist on you normalizing the
> code style everywhere before you check in, but I would like you to do that in
> the near future, and all of your changes should follow the new,
> gisburn-approved style.  Can you take a look at the issues above and post
> another patch.

Can you look over the comments above and tell me which of the issues listed in
your review are mandatory and which of them can be done later, please ?

> Thanks for your patience with this, and my apologies, on behalf of
mozilla.org,
> for the unpleasant time you've had getting this reviewed.

No problem... :-)

Thank you very much for reviewing...

Comment 37

[Roland Mainz]

blizzard: Would it be possible to reuse most of Xlib-toolkit in Xprint-toolkit 
- maybe making Xprint a subclass of Xlib-toolkit or create a common superclass
for both ?
AFAIK Xprint only needs to handle the "creation" of the display (e.g.
XpuGetPrinter() instead of XOpenDisplay()) and
BeginDocument()/BeginPage()/EndPage()/EndDocument/cleanup(=close display
connection as next XpuGetPrinter() may return a $DISPLAY to a totally different
Xprt server and get rid of any references (cached font info etc.) to this
$DISPLAY) - the other things may be the same as in Xlib-toolkit...
This should save a lots of code in the distribution...

Comment 38

[Roland Mainz]

Attachment: Xprint-2001-04-08 diffs for 2001-04-05-08-trunk (gdiff -u)

Comment 39

[Roland Mainz]

Attachment: Xprint-2001-04-08 diffs for 2001-04-05-08-trunk (gdiff -u -w) REVIEW ONLY !!

Comment 40

[Roland Mainz]

Attachment: Xprint-2001-04-09 diffs for 2001-04-05-08-trunk (gdiff -u)

Comment 41

[Roland Mainz]

Attachment: Xprint-2001-04-09 diffs for 2001-04-05-08-trunk (gdiff -u -w) REVIEW ONLY !!

Comment 42

[Roland Mainz]

Todays 2001-04-09 patches include&superset pavlov's patch fort bug 73178.
I have added some NS_ASSERTs to track possible use of XLoad/XList*Fonts()
functions before XpSetContext() was made - any call to those functions before
XpSetContext() will result in wrong data returned by these functions !!
The remaining changes take care of shaver's review comments and should fix most
issues listed there...

Issues:
- Printing images still does not work - but it is unclear which part is
Mozilla's fault (client-side) and which part is Xprt's fault (server side).
Upcoming X11R6.6 Xprt may solve some of these issues...

Can anyone review the latest paches, please ? Thanks !!

Comment 43

[Mike Shaver (:shaver emeritus)]

> Mhhh. Some compilers do not like if(expr) statements where "expr" is not an
> "int" value. Is data type for "mIsAPrinter" (_always_) an "int" type ?

That amazes me, because it's a violation of 20+ years of C standards and
behaviour.  What compiler are you thinking of?

> Ahhhh... big... got the problem. Do you think XPContext is XPCOM stuff ? No...
> XPContext is _xprint_context_ - a X11 XID... try
> -- snip --
> % cat /usr/include/X11/extensions/Print.h | fgrep "XID"
> typedef XID XPContext;

I don't believe that mPrintContext is an XPContext, then, because you call
->Init(nsIDeviceContextSpec) on it.  But I could believe that it's not
reference-counted and doesn't inherit from nsISupports, in which case you can't
use a COMPtr.

But mSpec is certainly an XPCOM interface pointer, and xpSpec as well: please
use nsCOMPtrs to avoid refcounting errors and to unclutter the code.  For more
information about nsCOMPtr usage, you might like to peek at 
http://www.mozilla.org/projects/xpcom/nsCOMPtr/ .

> Gllwww... OK... but I don't like to touch this part of the source anyway... 
> it's a 1:1 copy... I'll kill the whole function if the replacement is ready...

I don't much care if it's a copy of another routine (though I certainly would if
I were reviewing that code; sadly for that code, I did not).  This is your code
in your sub-module, and you should clean it up. (Use of the shared allocator is
an XPCOM correctness issue, not just a stylistic one.)

> I'd like to preserve the name makeCString() if possible as the body is a 1:1
> copy (except source reformatting) from the source listed in the comment.

See above: I don't think readers of this code will benefit more from symmetry
with some other bad name in another source file than they will from a name that
more clearly explains the purpose of the function.  (I see that you fixed this
-- thank you -- but I wanted to make the point that copying code from another
source file doesn't justify any bad practices.)

> BTW: Why is it dangerous to wait 15secs in the layout thread ?

Because, unless I miss my guess, you'll starve the rest of the app: all UI
operations, pretty much, happen on the main/layout thread, and if you're
sleeping in it, nothing else is getting events or being redrawn.

What do we do for preview in the other print mechanisms?  I'm pretty sure we
don't sleep for a fixed time in the layout thread!  If you're going to leave it
there, please DEBUG_gisburn it.

> > (And don't check against NS_OK explicitly unless you have a very
> > good reason; NS_SUCCEEDED() is the Better Way.)
>
> Why ?

Because a method can succeed and correctly signal success with a return other
than NS_OK.

> What about using matching #defines for initalisation ? I know that NULL or
> nsnull should be used for pointers - but what about X11 resource IDs (XIDs)
> required for XPContext/GC/etc. ?

You can use a #define in an initialization expression just fine, don't worry.

+#if 0 /* gisburn: this cannot work - Xprint usually operates at >= 300dpi */
     if (abs(dpi - 75) < abs(dpi - 100))
     dpi = 75;
   else
     dpi = 100;
+#endif

Should you just remove that code, rather than #if 0ing it?

+  /* Use strdup() to avoid any silly effects... 
+   * should be portable across all ANSI C platforms 

strdup isn't part of ANSI C, BTW: can you use PL_strdup here?

Mandatory things before I will sign off on this checkin:

  - use of nsCOMPtrs for locals and strong-reference members that contain XPCOM
    interface pointers
  - use of NS_GET_IID or do_CreateInstance/do_QueryInterface and removal of
    the NS_DEFINE_IID code bloat.
  - use of NS_SUCCEEDED
  - verification of error handling
  - blizzard-or-other review of xprintutil.* for X-correctness and related items

The normalization of coding style and such can wait until a future check-in, if
you prefer.  (Is this code affected by dcone's upcoming frame-printing landing?)

Comment 44

[Roland Mainz]

First at all... thank you very much for helping me and reviewing the code...

Sad to say... I didn't know anything about XPCOM until the last 60mins... and
now it's too late to learn this in the remaining time - XPCOM _looks_ too
complex to be easily be swallowed&absorbed in 12hours. Our instutite here is
going to be dismanteled, including infratructure (computers etc.). There's a
small change keeping some infratructure here to continue, but currently it looks
that after Wednesday I don't have the neccesary hardware to continue my work
here. Simple math tells me that additional review cycle would require more time
than I have now - and the patches here will be rotten&busted in a few weeks
until I get new hardware... ;-(

I am sorry that it ends like this... ;-(((((

Comment 45

[Christopher Blizzard (:blizzard)]

Comments about xprintutil.c:

Don't use macros like X() and D().  Use self-describing macros like TRACE() or
DEBUG().

Don't leave debug-only code in there if it's never used.

Please add some vertical whitespace in XpuSetContext().

Lines like this are hard to read:

if( xp_printer_model == NULL ) XpuError("XpuSetContext: could not get
xp-model-identifier printer attribute", NULL);

Please break that up some into more obvious thoughts.

Comment 46

[Roland Mainz]

Reopen bug based on discussion&requests&rants... there's no way to get usefull
international printing in Linux/Unix without Xprint... I have two machines for
development for ~~two months... after that another solution is somehow
required...

I am going to file new patches which fixes most of the issues listed in review
except the follwing two:
1. XPCOM issues - bbaetz offered to hunt these issues
2. Error handling is not perfect. I'd like to resolve this later as some of the
classes currently used may be replaced by Xlib-toolkit classes if
usefull/possible...

Comment 47

[Roland Mainz]

The issue with "images are rendered _after_ EndDocument()" has been called" may
be fixed by patches in bug 6074 (said Pavlov). Adding dependicy...

Comment 48

[Roland Mainz]

Attachment: Xprint-2001-04-12 diffs for 2001-04-10-08-trunk (gdiff -u)

Comment 49

[Roland Mainz]

Attachment: Xprint-2001-04-12 diffs for 2001-04-10-08-trunk (gdiff -u -w) REVIEW ONLY !!

Comment 50

[Roland Mainz]

Attachment: 2001-04-12 xprintutil.c

Comment 51

[Roland Mainz]

Attachment: 2001-04-12 xprintutil.h

Comment 52

[Roland Mainz]

Attachment: 2001-04-12 xprintutil_printtofile.c

Comment 53

[Roland Mainz]

1. Reply to shaver's review comments:

> > Mhhh. Some compilers do not like if(expr) statements where "expr" is not an
> > "int" value. Is data type for "mIsAPrinter" (_always_) an "int" type ?
> 
> That amazes me, because it's a violation of 20+ years of C standards and
> behaviour.  What compiler are you thinking of?

AFAIK this was SAS/C which prints warnings on this issue...

[snip] 
> > I'd like to preserve the name makeCString() if possible as the body is a 1:1
> > copy (except source reformatting) from the source listed in the comment.
> 
> See above: I don't think readers of this code will benefit more from symmetry
> with some other bad name in another source file than they will from a name
that
> more clearly explains the purpose of the function.  (I see that you fixed this
> -- thank you -- but I wanted to make the point that copying code from another
> source file doesn't justify any bad practices.)

Agreed. The code was only thought to hack this functionality working. It isn't
fixed yet - but the current solution works little bit better...

> > BTW: Why is it dangerous to wait 15secs in the layout thread ?
> 
> Because, unless I miss my guess, you'll starve the rest of the app: all UI
> operations, pretty much, happen on the main/layout thread, and if you're
> sleeping in it, nothing else is getting events or being redrawn.

Uhm... did you notice that this code is _only_ used if user uses a "special"
printer name to turn this debug feature "OK" ? Usually Xprt supports the
"XpExtension" - and the sleep() isn't called in those cases. sleep() get's only
called when the print $DISPLAY is directed to a "normal" framebuffer Xserver...

> What do we do for preview in the other print mechanisms?  I'm pretty sure we
> don't sleep for a fixed time in the layout thread!  If you're going to leave
it
> there, please DEBUG_gisburn it.

Done. #ifdef'ed it with it's own cpp symbol...

> > > (And don't check against NS_OK explicitly unless you have a very
> > > good reason; NS_SUCCEEDED() is the Better Way.)
> >
> > Why ?
> 
> Because a method can succeed and correctly signal success with a return other
> than NS_OK.

Fixed.

> > What about using matching #defines for initalisation ? I know that NULL or
> > nsnull should be used for pointers - but what about X11 resource IDs (XIDs)
> > required for XPContext/GC/etc. ?
> 
> You can use a #define in an initialization expression just fine, don't worry.

Fixed, see code. I am now using the matching #define's from X11 headers - the
"casts" are now only "reminders" anymore... :-)

> +#if 0 /* gisburn: this cannot work - Xprint usually operates at >= 300dpi */
>      if (abs(dpi - 75) < abs(dpi - 100))
>      dpi = 75;
>    else
>      dpi = 100;
> +#endif
> 
> Should you just remove that code, rather than #if 0ing it?

This is just a reminder that this method is totally wrong.
But I'd like to remove the code... the _whole_ class... and replace it with the
matching class from Xlib-toolkit if possible...

> +  /* Use strdup() to avoid any silly effects... 
> +   * should be portable across all ANSI C platforms 
> 
> strdup isn't part of ANSI C, BTW: can you use PL_strdup here?

Fixed.

> Mandatory things before I will sign off on this checkin:
> 
>   - use of nsCOMPtrs for locals and strong-reference members that contain
XPCOM
>     interface pointers

Hopefully bbaetz can do this...

>   - use of NS_GET_IID or do_CreateInstance/do_QueryInterface and removal of
>     the NS_DEFINE_IID code bloat.

Hopefully bbaetz can do this...

>   - use of NS_SUCCEEDED

Done.

>   - verification of error handling

Partially done. Better error checking is on my radar, bt if we really start to
reuse XLib-toolkit the whole code will need a 2nd revamp. And currently error
checking on the "calling" method is missing, too - even if I return NS_ERROR the
underlying code doesn't care about that... ;-(

>   - blizzard-or-other review of xprintutil.* for X-correctness and related
items

See comments below.

> The normalization of coding style and such can wait until a future check-in,
if
> you prefer. 

> (Is this code affected by dcone's upcoming frame-printing landing?)

I don't know. I do not even _know_ which plans dcone currently has...

----

2. Reply to blizzard's review of xprintutil*.[ch] 

> Don't use macros like X() and D().  Use self-describing macros like TRACE() or
> DEBUG().

Fixed. Macros now have more or less self-describing names (at least you can
search for them with lxr... =:-)

> Don't leave debug-only code in there if it's never used.

Fixed.

> Please add some vertical whitespace in XpuSetContext().
> Lines like this are hard to read:
>
> if( xp_printer_model == NULL ) XpuError("XpuSetContext: could not get
> xp-model-identifier printer attribute", NULL);
>
> Please break that up some into more obvious thoughts.

Code in
#ifdef XXX
...
XpuSetContext()&co
...
#endif /* XXX */
was only a "demonstrator" for bugs in Xprt - nothing more. Quick dirty hack... I
removed it...

----

3. Accepting bug.

Comment 54

[Bradley Baetz]

You need error checking :) It crashes if:

1. The print server isn't running
2. The printer name is incorrect (the dialog box needs to be reworked, but that
shouldn't hold this up)
3. You try to print more than one document (I get X font errors, and eventually
it crashes)

Also, I had to clobber gfx/src/gtk when I tried building without these patches -
it seems that the fact that you're using --with-xprint=yes rather than
--enable-xprint isn't causing a full rebuild, and so the hooks in gtk don't get
picked up. (With your patches, the file changes, and so its rebuilt, and works
without problems)

> > Should you just remove that code, rather than #if 0ing it?

> This is just a reminder that this method is totally wrong.
> But I'd like to remove the code... the _whole_ class... and replace it with
the
> matching class from Xlib-toolkit if possible...

This, to me, is the biggest issue. A lot of the code seems copied from the xlib
port. gtk doesn't allow someone to tell it to use a user-supplied Drawable*, but
there are comments in the gtk list archives to indicate that 2.1 may have that.
(Of course, those comments + patches were first made in 1998 :) When it does,
then adding "native" xprint support to gtk would seem to be simple.

I'd suggest having an nsDeviceContextXP (and nsXPrintContext) like you have now,
but just defer the rendering stuff to the "real" gfx stuff. Either that, or gfx
libraries which can use xprint interit off an nsRenderingContextXPrintable or
something. Just a comment - its an academic issue at the moment, and I'm
certainly not suggesting it for this checkin.

Now, let me try modernising this code :)

Comment 55

[Bradley Baetz]

OK, patch (made using interdiff) coming up. I can't get images to display, with
or without my patches, and the header/footer fonts are very large.

shaver: gfx as a whole needs to be brought into the modern world. There may be
more that can be done on this, but I don't think xprint is any worse than the
other toolkits. nsFont doesn't have an interface behind it, etc. Theres a limit
to what can reasonably done without going insane :)

Bring on gfx2!

Comment 56

[Bradley Baetz]

Attachment: interdiff patch with XPCOM changes

Comment 57

[Roland Mainz]

> You need error checking :) It crashes if:
> 
> 1. The print server isn't running

Lame error checking in calling code. I return an error if this happens.
NOT MY FAULT...

> 2. The printer name is incorrect 

Lame error checking in calling code. I return an error if this happens.
NOT MY FAULT...

> (the dialog box needs to be reworked, but that shouldn't hold this up)

Please take a look at bug 73855 which asks for "print dialog" enhancements. The
d*vil himself or dcone may know why this has been marked as "invalid". I don't
understand the reason listed there - I'd like to share the same print dialog
with native PostScript module (changes needed for Xprint can be usefull for
PostScript module, too)...

> 3. You try to print more than one document (I get X font errors, and
eventually
> it crashes)

Uhm... known problem. Some stupid code here caches X resources (e.g. XIDs (fonts
etc.)) which are only valid for the current display pointer on the current Xprt.
After XCloseDislay() these resources referenes are _invalid_. We need a cleanup
method which wipes-out all these references...

> Also, I had to clobber gfx/src/gtk when I tried building without these patches 
> it seems that the fact that you're using --with-xprint=yes rather than
> --enable-xprint isn't causing a full rebuild, and so the hooks in gtk don't
get
> picked up. (With your patches, the file changes, and so its rebuilt, and works
> without problems)

Mhhh, I never noticed these problems... I always do a full rebuild with
--with-xprint after download&patching the source...

[snip]
> > But I'd like to remove the code... the _whole_ class... and replace it with
> the
> > matching class from Xlib-toolkit if possible...
> 
> This, to me, is the biggest issue. A lot of the code seems copied from the
xlib
> port. gtk doesn't allow someone to tell it to use a user-supplied Drawable*,
but
> there are comments in the gtk list archives to indicate that 2.1 may have
that.
> (Of course, those comments + patches were first made in 1998 :) When it does,
> then adding "native" xprint support to gtk would seem to be simple.

GTK+ people are strongly against Xprint (this is going to be a "religions" issue
for them... ;-((( ) - mainly because they suffer from some old "myths" like that
"Xprint sends gfx to printer"[1], "Xprint does not support anti-alised fonts"[2]
or "Xprint does not support the RENDER extension"[3] and so on...
Last put not least: X11 is something like a "platform" for them - therefore
"Xprint" is not "platform-independant [enought]" and requires to add a
platform-independant layer above Xprint.... ahhhglll... ;-(((

Looking at the technical problems... does GTK+ still use real X11 API to render
text or do they use their custom functions ? If they use images to draw text
there will be no change to get GTK+ working with the Xprint toolkit.
And: GTK+ does not support multiple ${DIPLAY}s in one application - this is
annouced for V2.x - but I my fear is that this "feature" will not be bug-free
enought to work with it - and that "bug reports" will end-up in the response
"the source is open, fix it and send us the patch, please" (grrrr... ;-(((( )...

[1]=Not true unless misconfiguration of Xprt
[2]=Anti-alised fonts are usefull for low-resolution displays - please tell me
one reason why to use anti-aliasing for fonts at 600DPI... printer-buildin fonts
(which are already high-quality (vector) fonts... =:-)
[3]=The only reason why to implement RENDER on Xprt would be alpha-channel
support (or does anyone here know another reason why Xprt needs RENDER ?)...

> I'd suggest having an nsDeviceContextXP (and nsXPrintContext) like you have
now,
> but just defer the rendering stuff to the "real" gfx stuff. Either that, or
gfx
> libraries which can use xprint interit off an nsRenderingContextXPrintable or
> something. Just a comment - its an academic issue at the moment, and 

Yes, my opinion, too. I'd like to make Xprint module a subclass of Xlib toolkit
if possible as this would avoid any _uncontrollable_ (we do not have control
over this code nor would I expect that it is possible to get GTK+ V2.x
Xprint-friendly (unless blizzard speaks some magic words with those guys...
:-))) side-effects of GTK+ or other toolkits.
Basically current Xlib toolkit is suffient for this except the following points:
- Xlib-toolkit must not use a global $DISPLAY pointer in it's code
- Xlib-toolkit should support subclasses
- Xlib-toolkit needs support for cleaning-up all resources (XIDs) from current
$DISPLAY (Xprint may connect to another Xprint server or to a different printer
model with different printers)
- Xlib-toolkit should be rearranged that it can work without having a $DISPLAY
before nsXPrintContext::Init() nor making any assumptions about fonts before
this point...
- Xlib-toolkit needs to have support for "paged" devices, e.g. devices which is
pages instead of having "sliders" to scroll things...
- Xlib-toolkit needs a way to move the state of GIF/MNG-animations (and other
stuff - what about plugins !?) to the print toolkit...

> I'm certainly not suggesting it for this checkin.

Me, too. Let's checkin the current patches and _then_ start to fix remaining
bugs (like "crash when attempting to print more than once")... and then start to
create a seperate mozilla/gfx/src/xprint2/ to have one stable module and one
development module.... .... Oh, and: It's "Xp", not "XP" - the lowercase 'p'"
may be usefull here to avoid confusion with XPCOM stuff (it would be better to
rename "XP" to "MXP" (Mozilla XPCOM))...

----

> OK, patch (made using interdiff) coming up.

Thank you very much... :-)))))))

> I can't get images to display, 
> with or without my patches, 

Pavlov said that this is bug 6074 - but I saw that he removed the dependicy. And
his patches in bug 6074 did not solve the issue (images are rendered after
EndDocument() - ouch... ;-(()... I assume we need a seperate bug for this -
native PostScript module has the same problem...

> and the header/footer fonts are very large.

Set left/right/top/bottom border to "0". Something with font or resolution
calculations is going wrong... ;-(
I didn't found a way to turn either this feature off or "find the author and ask
him for assistance"... ;-(

> shaver: gfx as a whole needs to be brought into the modern world. There may be
> more that can be done on this, but I don't think xprint is any worse than the
> other toolkits. nsFont doesn't have an interface behind it, etc. Theres a 
> limit to what can reasonably done without going insane :)

I think I don't need to add the comment that shaver's
xpcom-must-be-fixed-before-checkin was going to drive me insane... (there's the
question why mozilla/gfx/src/gtk/ has recent checkins without fixing XPCOM
issues... =:-))))))

> Bring on gfx2!

What is "gfx2" ??

Comment 58

[Roland Mainz]

Attachment: Xprint-2001-04-12 diffs for 2001-04-10-08-trunk (gdiff -u)

Comment 59

[Roland Mainz]

Attachment: Xprint-2001-04-19 diffs for 2001-04-17-cvs (gdiff -u)

Comment 60

[Roland Mainz]

Attachment: Xprint-2001-04-19 diffs for 2001-04-17-cvs (gdiff -u -w) REVIEW ONLY

Comment 61

[Roland Mainz]

Added new patch (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=31361)
which integrates Bradley's XPCOM changes (THANKS !!) with my patches.
Be sure to recompile both mozilla/gfx/src/xprint/ _and_ mozilla/gfx/src/gtk/ (or
mozilla/gfx/src/xlib/) after appyling these patches.

----

As requested via email/IRC:
Build instructions:
- Download&unpack mozilla source
- Apply patch (current one is
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=31361) to source tree:
% gpatch -p0 --dry-run <thePatch.gudiff # check if patch works
% gpatch -p0 <thePatch.gudiff # patch it
- Download xprintutil.h
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=30592), xprintutil.c
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=30591) and
xprintutil_printtofile.c
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=30593) and put them
into mozilla/gfx/src/xprint/
- Compile tree:
% configure --with-xprint
% make

Usage:
- Start Xprt server (as "root" or normal user)
% Xprt -audit 4 :6 &
- tell mozilla where to find Xprt server(s - XPSERVERLIST can contain multiple
servers seperated via blanks)
% export XPSERVERLIST=":6"
- Run Mozilla
% ./mozilla

Running the Xprt server and setting the XPSERVERLIST should be done globally per
machine or network. Be sure to use "xhost" to grant other machines (xhost
+machine) the access to Xprt on demand...

More details on demand...

Comment 62

[Asa Dotzler [:asa]]

a=asa for checkin to 0.9 (This is not part of the default build, correct?). 
Assuming this gets review we'll take it for 0.9

Comment 63

[Roland Mainz]

To Asa: No Xprint is not part of the default build (yet - but this is on my
radar...).
But if we get working Xlib-toolkit+Xprint for 0.9/0.9.1 I'll contribute
milestone builds for sparc/64bit-sparc with Xprint/MathML/etc. enabled...

Comment 64

[Mike Shaver (:shaver emeritus)]

+  NS_NewISupportsArray(getter_AddRefs(mFontMetrics));

You don't check the error state there, and, in fact, have no way or reporting
that error.  Do you have/need an Init method you can put that in?

-/*
-static NS_DEFINE_IID(kDeviceContextIID, NS_IDEVICE_CONTEXT_IID);
-*/
 static NS_DEFINE_IID(kIDeviceContextSpecXPIID, NS_IDEVICE_CONTEXT_SPEC_XP_IID);

So close!  Lose the other DEFINE_IID, please.

+#if 0 /* gisburn: this cannot work - Xprint usually operates at >= 300dpi */
     if (abs(dpi - 75) < abs(dpi - 100))
     dpi = 75;
   else
     dpi = 100;
+#endif

Can we just delete that block?

+  if (mIsAPrinter != PR_TRUE) 

Mmm, explicit comparison against PR_TRUE.  Let's not do that, OK?

Don't sweat the nits for 0.9, I guess (I'm getting soft!).  The XPCOM-fu makes
me much more comfortable.  sr=shaver, though I will bother you endlessly until
you fix those things, once 0.9.1 opens.

Comment 65

[Christopher Blizzard (:blizzard)]

OK.  This looks OK to me.  Assuming that you have at least built the code on
linux you should be OK to check this in.  r=blizzard

Comment 66

[Bradley Baetz]

I checked this in, with the PR_FALSE comparisons fixed, but not the other stuff.
gisburn, its probably better if you open another bug on that.

Comment 67

[Roland Mainz]

*** Bug 66737 has been marked as a duplicate of this bug. ***

Comment 68

[Roland Mainz]

All new functionality introduced by this bug/RFE seems to work. Rubberstamping
"verified" - bug 78548 is the follow-up to this bug...