85527 - Turn xlibrgb into a shared library

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Roland Mainz]

RFE: Turn xlibrgb code (mozilla/gfx/src/xlibrgb/) into a shared library that
both Xlib-toolkit and Xprint print system can share the same code.

This is non-trivial as much code relies on the static vars in xlibrgb which hold
|Display *|, |Screen *| and so on... these variables have to be moved into a 
xlibrgb-"handle" which can be created/destroyed multiple times - and the _same_
code must be able to handle multiple Display/Screen combinations using different
handles at the same time...

Comment 1

[Roland Mainz]

Advantages:
- Dramatically reduction of resulting module size in Xlib/Xprint modules
Disadvantages:
- patch will be horrible _big_ and therefore it will be rotten soon. Adding some
"depends on" bugs to avoid that someone starts this bug while
Xlib-toolkit/Xprint are still under heavy development...
- due the problem above r= and sr= must be fast as hell (less than 2 days if
possible)... ;-(

Setting milestone to Mozilla 1.1...

Comment 2

[Roland Mainz]

More thoughts:
This can be "done" more smoothly:
Step 1: Modify xlibrgb.c code to use handles but preseve compatibility to old
API via macros (#define XLIBRGB_USE_HANDLES to turn "handles" on). The
"compatibility mode" use one global handle during that time.
Step 2: Modify Xlib-toolkit and Xprint-toolkit to use the new handle API
Step 3: Turn "handles" on by default
Step 4: Turn xlibrgb.c into a shared library

I can do everything except step 4 myself. Step 1 is quite easy... steps 2/3 will
be hard... ;-(

Overtaking...

Comment 3

[Roland Mainz]

Last blocker bug died few secs ago, this is now on my list for next week. 
CC:'ing pocemit.

Question to blizzard/tor:
Source compatibility to old xlibrgb API is not required, right ? I'd like to add
some #define's for the first patch in this bug to keep tinderbox machines
"green" but remove this compatibility hacks later after all code has been moved
over to the new API...

Comment 4

[tor]

It would be best if xlibrgb was kept in sync with gdk-pixbuf-xlibrgb.[ch].
That way fixes could be shared and if moz-xlib ever went the gdkpixbuf-xlib
route there would only be one xlibrgb copy floating around in memory.

Comment 5

[Roland Mainz]

tor:
AFAIK gdkrgb does not support more than one |Display *| per application. 
But that's why I filed this bug - xlibrgb should be able to handle more than one
|Display *| at the same time (one per "handle" (which stores all |Display
*|-specific vars) - but multiple handles)...

Comment 6

[Roland Mainz]

Attachment: Patch for 2001-06-29-08-trunk

Comment 7

[Roland Mainz]

Attachment: Patch for 2001-06-29-08-trunk (same as previous patch but without whitespace changes, review ONLY!!)

Comment 8

[Roland Mainz]

Filed patch.

Changes:
- new API: same as old xlibrgb API but starts with xxlib_rgb_*. Each function
takes a "handle" argument to carry all "global vars" in this handle. There can
be any number of handles at the same time. Seperate "handle"s can be managed by
seperate threads. The old xlib_rgb* API is still available for some time until
all code has been moved over to use the new API...
- Bad XFlush() ifndef'ed - this is only "neccesary"[1] for MIT_SHMEM putimage
- No TABs in *.c anymore...

ToDO:
- Add API to create a xxlib_rgb handle with a given visual. May be usefull to
render the skin with a 8bit visual (for speed) but render the content area with
24bit...
[1]=Using the event API is the correct way and removes the need for XFlush()
here...

Requesting r=

Comment 9

[pocemit]

I have looked over the patch, and while I do not see anything strange in it I
don't feel I am qualified to give r= on such a large patch. I personally don't
see anything wrong with it - the old API is still available, the changes do not
introduce any weirdness, etc, but I would rather have more people take a look at
this.
 -pocemit

Comment 10

[Roland Mainz]

Attachment: New patch for 2001-06-29-08-trunk to get r=timeless

Comment 11

[Roland Mainz]

Filed new patch to match r=timeless as discussed in IRC #mozilla

Additional changes:
- disabled alpha image support in Xprint module. It does not work ith 8bit alpha
images and sometimes spamms the users with X error messages. BAD. Fix is on the
way - I just do not like to make users nuts with these error messages...
-- snip --
+/* gisburn: alpha image code disabled. 
+ * it fails for 8bit alpha stuff and spamms the user with X errors sometimes
BAD.
+ */
+#ifdef XPRINT_NOT_NOW  
   // Create gc clip-mask on demand
   if( alphaBits != nsnull )
   {
@@ -829,6 +833,7 @@
     x_image->data = nsnull; /* Don't free the IL_Pixmap's bits. */
     XDestroyImage(x_image);
   }
+#endif /* XPRINT_NOT_NOW */
-- snip --

Requesting r=timeless...

Comment 12

[timeless]

whoops i wasn't clear, you need to fix _all_ else after return instances. fix
that for r=timeless.  let's see what the sr's have to say :)

Comment 13

[Roland Mainz]

timeless: thanks !

Code to fix is
-- snip --
Colormap
xxlib_rgb_get_cmap (XlibRgbHandle *handle)
{
  /* xxlib_rgb_init (); */
  if (handle)
    return handle->cmap;
  else
    return 0;
}
-- snip --

I'll wait for the sr= comments and file a final catch-all/fix-all patch...

----

requesting sr= ... blizzard, tor ?

Comment 14

[tor]

Attach the final version of the patch you want reviewed (plus a -w version).

Comment 15

[tor]

Seperate the unrelated xprint change into a new bug.

Comment 16

[Roland Mainz]

#ifdef XPRINT_NOT_NOW /* alpha code */ #endif ... that are _two_ lines... is
this really worth a complete new bug ??

Comment 17

[tor]

Yes.

Keeping patches contained to fixing a particular bug's problem will speed
up r/sr.

Comment 18

[Roland Mainz]

Attachment: New patch for 2001-06-29-08-trunk, fixing final issue to match r=timeless, removed "disable alpha support from Xprint module" lines from patch (suggested by tor)

Comment 19

[Christopher Blizzard (:blizzard)]

I looked through it.  Most of it is busy work.  rs=blizzard

Please try to touch less whitespace in the future.  It screws up cvsblame.

Comment 20

[Roland Mainz]

blizzard:
thanks !

> Please try to touch less whitespace in the future.  It screws up cvsblame.

I know. But I removed - once and forever - all xx!!@@-TABs from xlibrgb.[ch] ...
there should be no need for such a massive whitespace change in these files
anymore... :-)

Comment 21

[Roland Mainz]

CC:'ing mkaply for checkin, please...

Comment 22

[Mike Kaply [:mkaply]]

Checked in.

Comment 23

[John Morrison]

May God have mercy on us all. The 212 bug spam-o-rama is Now!