Closed Bug 94569 Opened 23 years ago Closed 23 years ago

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

Categories

(Core :: XUL, enhancement)

All
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla0.9.4

People

(Reporter: roland.mainz, Assigned: roland.mainz)

References

Details

(Keywords: crash, memory-footprint, perf, Whiteboard: want for mozilla 0.9.4)

Attachments

(7 files)

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...
Accepting bug.
Blocks: 79119, 83355
Status: NEW → ASSIGNED
Keywords: footprint, perf
Target Milestone: --- → mozilla0.9.4
Filed patch. 
Requesting r=/sr=
Keywords: patch, review
Looks good to me.

r=mkaply on the non makefile changes
cls:
Wanna r= the Makefile.in changes, please ?
r=cls on the Makefile change
Requesting sr= ...
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.
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 ...
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...
blizzard:
Do you really demand on the final solution ?
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= ...
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=?
+  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.
> +  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.
Requesting sr= ...
Blocks: 95408
Blocks: 95621
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.
Blocks: 92581
Blocks: 79100
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.
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...) ...
Filed (hopefully :-) final patch, r=jag, r=cls (for Makefile.in stuff) and
r=Tomi.Leppikangas@oulu.fi

Requesting sr=
Keywords: mozilla0.9.4
Whiteboard: want for mozilla 0.9.4
Roland says he addressed all of blizzard's concerns, and patch looks ok to me
... sr=kin@netscape.com, unless there are any objections from blizzard.
May God have mercy on us all. The 212 bug spam-o-rama is Now!
QA Contact: aegis → jrgm
Requesting a=drivers ...
CC:'ing asa - requesting a= ...
Keywords: reviewapproval, crash
a=asa on behalf of drivers.
I'll commit this today when I do 94243.
Fix checked in for gisburn
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: