94569 - [xlib] Move Xlib and Xprint over to new xxlibrgb* API

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Roland Mainz]

Starting with the patches in bug 85527 ("Turn xlibrgb into a shared library") a
new API for xlibrgb.so was introduced to allow more than one |Display *| /
|Screen *| / |Visual *| per process...

... 2nd part is now to make use of that API. Patch follows...

Comment 1

[Roland Mainz]

Accepting bug.

Comment 2

[Roland Mainz]

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

Comment 3

[Roland Mainz]

Filed patch. 
Requesting r=/sr=

Comment 4

[Mike Kaply [:mkaply]]

Looks good to me.

r=mkaply on the non makefile changes

Comment 5

[Roland Mainz]

cls:
Wanna r= the Makefile.in changes, please ?

Comment 6

[cls]

r=cls on the Makefile change

Comment 7

[Roland Mainz]

Requesting sr= ...

Comment 8

[Christopher Blizzard (:blizzard)]

This patch is unacceptable.  If you're going to make multiple handles do it the
right way.  Make the handle an argument to the functions that are called and
don't just create more than one static handle in the .c function.

Comment 9

[Roland Mainz]

Blizzard: 
Did you read bug 85527 ? I am planning to implement that exactly in the same
manner as you've suggested it here in a later patch. This is only the step "move
Xlib/Xprint to new API". The next step is to kill that macro wrappers and global
handles in xlibrgb.so (but first I want to "land" the MIT-SHM support and I need
this one "in" CVS first) ...

Again... requesting sr=, please ...

Comment 10

[Roland Mainz]

Quick look at the final solution... such a patch would patch more than 22
files... this won't make it in 0.9.4 as it tooks far too much time for getting
r=/sr= ... I need this one first as a temporary solution to test whether the
"switch" to the new API really does not break anything...

Comment 11

[Roland Mainz]

blizzard:
Do you really demand on the final solution ?

Comment 12

[Roland Mainz]

Attachment: Better all-in-one patch for 2001-08-11-08-trunk

Comment 13

[Roland Mainz]

Filed all-in-one patch which switches all source files (except Motif port) to
new xxlib_rgb API. This patch includes a new API which allows to "register"
|XlibRgbHandle|s via name - and look-up them later using that name...

Requesting r=/sr= ...

Comment 14

[Roland Mainz]

Attachment: Better patch for 2001-08-11-08-trunk per jag's comments

Comment 15

[Roland Mainz]

Attachment: New patch for 2001-08-11-08-trunk per jag's comments

Comment 16

[jag (Peter Annema)]

Yeah, this looks mostly right, don't see anything really out of the ordinary,
but don't know the code that well either. Kinda r=jag, but I would appreciate an
r= or sr= from someone more familiar with this part of Mozilla. Blizzard, would
your sr= validate my r= or would you prefer a second r=?

Comment 17

[Christopher Blizzard (:blizzard)]

+  redScale   = 8-xxlib_get_prec_from_mask(visual->red_mask);

Can you put some space in there?

+  redFill    = 0xff>>xxlib_get_prec_from_mask(visual->red_mask);

And comment that?

+
+  xxlib_deregister_handle_by_handle(handle);
+  free((void *)handle->name);

and in xxlib_deregister_handle_by_handle():

+      free((void *)registered_handles[i].name);

That's a double free.

Looking at xxlib_register_handle the ownership seems kind of strange.  It's
allocated by the caller but is freed by the library?  In fact, looking at the
code it looks like there's no clean strategy for who owns what in the library
calls.  Please make it consistent.
+/* saves #include "xlibrgb.h" */
+typedef struct _XlibRgbHandle XlibRgbHandle;
+

No, include it.  I don't like "extern" type defines.

Comment 18

[Roland Mainz]

> +  redScale   = 8-xxlib_get_prec_from_mask(visual->red_mask);
> 
> Can you put some space in there?

Will be fixed.

> +  redFill    = 0xff>>xxlib_get_prec_from_mask(visual->red_mask);
>
> And comment that?

My change(=[a]) or what the code does(=[b]) ?
a) Swicthed over to new xxlib API... the |handle| argument wasn't used in code,
therefore I removed it.
b) No clue... some bit stunts. I didn't write it... ;-(

> +  xxlib_deregister_handle_by_handle(handle);
> +  free((void *)handle->name);
>
> and in xxlib_deregister_handle_by_handle():
>
> +      free((void *)registered_handles[i].name);
>
> That's a double free.

Nope. First is a XlibRgbHandle-|handle|, 2nd is a |name| in the registry...

I removed the name string from XlibRgbHandle because it wasn't used anymore (I
used it as an arguement to xxlib_deregister_handle(), but I added the ability to
register one XlibRgbHandle with multiple names (aliases) and switched
xxlib_rgb_destroy_handle() over to xxlib_deregister_handle_by_handle() instead).

> Looking at xxlib_register_handle the ownership seems kind of strange.  It's
> allocated by the caller but is freed by the library?  In fact, looking at the
> code it looks like there's no clean strategy for who owns what in the library
> calls.  Please make it consistent.

Oh, yes. bad. See new code. xxlib_register_handle() is only public to allow
users to register a handle more than once or if the handle wasn't registered yet
(e.g. NULL-|name| argument to xxlib_rgb_create_handle() ...

> +/* saves #include "xlibrgb.h" */
> +typedef struct _XlibRgbHandle XlibRgbHandle;
> +
> 
> No, include it.  I don't like "extern" type defines.

Will be fixed.

New patch follows.

Comment 19

[Roland Mainz]

Attachment: New patch per blizzard's comments

Comment 20

[Roland Mainz]

Requesting sr= ...

Comment 21

[Roland Mainz]

Attachment: New patch for 2001-08-15-08-trunk

Comment 22

[Roland Mainz]

Filed new patch for 2001-08-15-08-trunk.
This new patch removes two backup files created Nedit and removes unused code
(which was not commented out via |#ifdef| anyway).

The only new thing is nsXPrintContext.cpp:
-- snip --
+  /* workaround for crash in XCloseDisplay() on Solaris 2.7 Xserver - when 
+   * shared memory transport is used XCloseDisplay() tries to free() the 
+   * shared memory segment - causing heap corruption and/or SEGV.
+   */
+  putenv("XSUNTRANSPORT=xxx");
-- snip --

Assuming that that minor change does not require a new r= ...

... requesting sr= for the new patch, please.

Comment 23

[jag (Peter Annema)]

With the fix of a minor nit in indenting I mentioned on irc, and if you only
made the changes listed above, r=jag, with the same "needs second r="
requirement.

Comment 24

[Roland Mainz]

I already fixed all (of blizzard's) issues listed "above". 
Looking for 2nd r=, final patch follows after that (incl. that whitespace nit
and fixes for any issues from 2nd review...) ...

Comment 25

[Tomi Leppikangas]

Patch looks ok r=[e-mail]

Comment 26

[Roland Mainz]

Attachment: Final patch for 2001-08-15-08-trunk

Comment 27

[Roland Mainz]

Filed (hopefully :-) final patch, r=jag, r=cls (for Makefile.in stuff) and
r=[e-mail]

Requesting sr=

Comment 28

[kinmoz]

Roland says he addressed all of blizzard's concerns, and patch looks ok to me
... sr=[e-mail], unless there are any objections from blizzard.

Comment 29

[John Morrison]

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

Comment 30

[Roland Mainz]

Requesting a=drivers ...

Comment 31

[Roland Mainz]

CC:'ing asa - requesting a= ...

Comment 32

[Asa Dotzler [:asa]]

a=asa on behalf of drivers.

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

I'll commit this today when I do 94243.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Fix checked in for gisburn