xlib doesnt have gc-cache yet, that slows down it a bit. I have done most of this work already, some problems with alpha images and tiling.

Chris/Pav - who should own this?

Here is new patch that adds gc-cache, and some optimisations to nsWidget and nsWindow. There is bug in nsImage that i didnt find, it crashes in viewer test #10

->xlib guys

This #3 patch works ok, tor helped to find bug which crahed in test #10. I debugged this quite a lot, gc-cache doesnt leak any gc's, but xmon shows that some gc's aren't freed, looks like there is leak of nsImageXlib objects. And when they leak, some gc's are leaked too. TnsImageXlib leak cause xpixmaps to leak too. (but thats not this patchs problem)

ok, after investigating a little bit more it looks like someone leaked a lot of GC refs, since I got a SIGABORT on line 66 of nsGCCache.h which is: PRInt32 AddRef(void) { (66) if(mRefCnt>100) abort(); NS_PRECONDITION(PRInt32(mRefCnt) >= 0, "illegal refcnt"); ++mRefCnt; return mRefCnt; }

Ok, I found out that nsDrawingSurfaceXlib really didn't like being cast to an nsRenderingContextXlib, so I copied the GetGC accessor to nsDrawingSurfaceXlib and called it. This causes the mGC to be returned properly, and not some random adress that makes the recount look like 2^31 or something close All in all, this current patch causes mozilla to run, but alpha, and animated gifs are black This also fixes the bug where you get a refresh flash before loading new pages (with xlib) Discussion on the imaages should move to #76820 The copious redraw is #82322

This patch copies some things from the GTK port related to 1 and 8 bit alpha images, adds (not implemented) function to draw scaled images, updates ::DrawString to draw CJK (multibyte) fonts correctly. Also imoT's patch about DEFAULT_XLIB_FONT breaks default font selection for themes - draws the UI in adobe-helvetica. The original version of using whatever font was given looked better (used ms-tahoma on my system).

Ah, yes another one I missed with the fonts - very important --- /tmp/orig/xlib-gfx-v2/nsFontMetricsXlib.cpp Tue May 29 04:57:49 2001 +++ ../xlib/nsFontMetricsXlib.cpp Tue May 29 05:17:20 2001 @@ -1724,7 +1724,7 @@ if ((mFont->min_byte1 == 0) && (mFont->max_byte1 == 0)) return XTextWidth(mFont, (char*) buf, len); else - return XTextWidth16(mFont, buf, len); + return XTextWidth16(mFont, buf, len / 2); } int

quy, please take a look at bug 16688 ("nsFontMetricsGTK::GetWidth/DrawString ignore beyound 512 chars"). Maybe worth to pull the change over to Xlib-toolkit, too...

I messed up pretty badly cut & pasting v1 patch, this one is better, also includes that multibyte font fix, and cleanups. What does nsImageXlib::DrawImageOffscreen do? It works without it. Gtk version doesn't have it.

Can anyone check if Xlib-toolkit now renders pages like http://people.netscape.com/ftang/demo/utf8form.html correctly (tons of languages in one document)...

Yes, with the CJK font patch from earlier that page loads fine (I have most of those fonts installed, and I have tested it). I can attach a screenshot if needed. :) It does not however, print them. I looked at some of the comments in the bug attached to this, and will probably look over nsFontMetricsXlib to see what can be done.

timeless: Screenshots are always good (large one >=1152x900, please)... :-) Sounds I will pull over these changes to Xprint, too (at least because I killed some locales with my latest code-copies Xlib-toolkit-->Xprint (nsFontmetrics*.*)... Is it possible to set a milestone for this bug, please (what about 0.9.2) ?

I've integrated the CJK-fixes into fixes for bug 77210... timeless... are the CJK fonts in Xlib-toolkit screen output mainly on the left side, leaving the right side blank ? It looks like the fonts are only using the left half of the page... ;-(

Hmm, i am "pocemit" on mozilla.org, Secondly, what do you mean on the left side? The screenshot shows them all centered, like supposed to be? I have some other Japanese test pages like www.goo.ne.jp and it renders them correctly. Did you also apply the 1-line fix that was printed in a comment? That one is important, otherwise it returns the widths incorrectly and causes things to look bad.

Beep... yes... I missed that one-line patch in this comment... BAD... ;-( Going to fix that...

This removes the slow smooth scaling code and adds newly ported XIE.c and scale.c from the GTK toolkit. Makefile changes aren't included so you'd have to add in your Makefile.in: diff -u -r1.27 Makefile.in --- Makefile.in 2001/05/17 18:25:21 1.27 +++ Makefile.in 2001/05/30 00:03:35 @@ -44,9 +44,18 @@ nsRenderingContextXlib.cpp \ nsScreenXlib.cpp \ nsScreenManagerXlib.cpp \ - nsPrintOptionsXlib.cpp \ + nsPrintOptionsXlib.cpp \ + nsGCCache.cpp \ $(NULL) +ifdef HAVE_XIE +CSRCS += XIE.c +endif + +CSRCS += scale.c + +EXPORTS = nsGCCache.h + include $(topsrcdir)/config/rules.mk ifndef MOZ_MONOLITHIC_TOOLKIT @@ -57,10 +66,16 @@ CXXFLAGS += $(TK_CFLAGS) endif +ifdef HAVE_XIE +DEFINES += -DHAVE_XIE +GFX_XIE_LIBS += $(MOZ_XIE_LIBS) +endif + EXTRA_DSO_LDOPTS += \ $(NSPR_LIBS) \ - -lxpcom \ + -lxpcom \ -lgkgfx \ + $(GFX_XIE_LIBS) \ $(NULL) DEFINES += -D_IMPL_NS_GFXNONXP Also would someone want to look this bug over and assign a milestone or something for this?

Before adding XIE support to the Xlib port please read news://news.mozilla.org:119/3B13BE4A.5060608@mozilla.org and the thread following.

And please read my reply to that posting...

He makes sense. XIE scaler does a nice fallback to the software scaling if XIE was for some strange reason compiled in, it is #ifdef'd for those who don't have XIE like mentioned in that post, and gdkpixbuf makes things depend on Gdk. These comments are about gfx/gtk implementation of XIE scaler, since it has been proven to work so far.

tim copperfield: 1. Could you please file a all-in-one patch (gdiff -r -u -N) 2. Uhm... what about overtaking this bug (e.g. reassign to you) ? 3. After step [2] please set a milestone - I really like to see this in 0.9.2...

This one includes the original gc-cache patch plus Myth's fixes to the current tree plus Myth's fixes to the gc cache segfault plus pocemit's fixes to the images plus pocemit's fixes to scaled images plus pocemit's fixes to the native scrollbar in nswidget.

If you end up building gtk and xlib in the same tree this is going to conflict: +EXPORTS = nsGCCache.h Also, are you just copying those files? Shouldn't you pull them out of the gtk port if you can? + mSurface = nsnull; + if(mDefaultFont) + XFreeFont(mDisplay, mDefaultFont); + If the default font handle is the same for all instances of the device context why not share it in between all of them instead of making it a member of each one? + /* GC gc = XCreateGC(mDisplay, (Drawable) mWidget, 0, - NULL); - + NULL);*/ If you're going to remove code then please actually remove it instead of just commenting it out. + { + char *fontName = (char *)NULL; + unsigned long pr = 0; + + ::XGetFontProperty(mDefaultFont, XA_FULL_NAME, &pr); + if(pr) + { + fontName = XGetAtomName(mDisplay, pr); + aInfo->mFont->name.AssignWithConversion(fontName); + ::XFree(fontName); + } All of this stuff needs to be cached. The XGet* are usually server round trips. + // if (mGC) + // XFreeGC(mDisplay, mGC); Once again, please remove this code if it's commented out and not needed for something. + //mGC = XCreateGC(mDisplay, mDrawable, 0, NULL); Same thing. - mGC = XCreateGC(mDisplay, mDrawable, 0, NULL); + //mGC = XCreateGC(mDisplay, mDrawable, 0, NULL); Same thing. + xGC * GetGC() { mGC->AddRef(); return mGC; } Are you sure that's going to work every time? Is mGC always non-null? + fprintf(stderr, "Loading: %s\n", mName); Spam. Please remove it. +#include "drawers.h" Are you copying this, too? + PRInt32 j = aSX + aSWidth; + PRInt32 z; Can you use some self-describing variables there? It makes the code later harder to read. + 0, /* x offset, XXX fix this */ + // no, it's still MSB XXX check on this!! + // x_image->byte_order = LSBFirst; You've got comments like this. What's the deal? + // Write into the pixemap that is underneath gdk's mAlphaPixmap + // the image we just created. I thought this was xlib? :) + /* XXX Why? DrawImageOffscreen(validX, validY, validWidth, validHeight); */ ? -class nsDrawingSurfaceXlib; +// class nsDrawingSurfaceXlib; + //PRUint8 *mConvertedBits; You should remove that if you are commenting it out. +#define MAX(a, b) (((a) > (b)) ? (a) : (b)) +#define MIN(a, b) (((a) < (b)) ? (a) : (b)) I'm pretty sure these are defined in system headers somewhere. + float mXScale; + float mYScale; Watch your tabs. - attr.background_pixel = mBackgroundPixel; - attr.border_pixel = mBorderPixel; + /*attr.background_pixel = mBackgroundPixel;*/ + /*attr.border_pixel = mBorderPixel;*/ - attr_mask = CWBitGravity | CWEventMask | CWBackPixel | CWBorderPixel; + attr_mask = CWBitGravity | CWEventMask /*| CWBackPixel | CWBorderPixel*/; More comments that should just be removed. - attr.background_pixel = mBackgroundPixel; - attr.border_pixel = mBorderPixel; + /*attr.border_pixel = mBorderPixel; */ - attr_mask = CWBitGravity | CWEventMask | CWBackPixel | CWBorderPixel ; + attr_mask = CWBitGravity | CWEventMask /*| CWBorderPixel*/ ; *ahem* + PRBool nNeedToShow = PR_FALSE; What is the 'n' prefix? + + /* if (aRepaint) + Invalidate(PR_FALSE);*/ Is that supposed to work? I didn't review the XIE or GC cache code since I'm assuming that's copied directly.

Attachment 1 [details] [diff]: cleaned up patch with all changes that actually applies All printfs are now either removed, or put in ifdefs nsGCCache is no longer exported Attachment 2 [details] [diff]: is an adapted scale.c Attachment 3 [details] [diff]: is an adapted drawers.h

Patches seams to work ok. Xmon shows that CreateGC/ChangeGC counts are near gtk ones. I hope to get thease in and open new bugs for remaining issues.

I am sorry that things are posted under this bug, but most of these changes more or less depend on the gc-cache addition... Plus its harder to keep track of things otherwise. Anyway, highlights of this patch: * add Xlocale setting in AppShell, so that currently selected locale is properly found by PlatformCharsetService and friends * window title code adapted from the GTK version - correctly handles multibyte titles * cleanups per blizzards comment * Context menus now work correctly * Mouse/Keyboard grab fixed for popup menus + autocomplete window * nsWidget now supports nsIWeakReference (part of context menu fix) * rearranged nsWidget + nsWindow a little bit

Patch 37516 works ok. I hope we get this in soon so it doesn't rot anymore, gc patch has been hangin here over 4 months..

Here is some build info: Get latest patch from attachment 37516 [details] [diff] [review] and apply it. Get attachments and name them as follows, all in mozilla/gfx/src/xlib/ http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23077 -> nsGCCache.cpp http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23078 -> nsGCCache.h http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36963 -> scale.c http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36964 -> drawers.h http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36965 -> XIE.c configure --enable-toolkit=xlib --with-xlib

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

The patch looks very good... :-) timecop: Wanna integrate bug 16688 ("nsFontMetricsGTK::GetWidth/DrawString ignore beyound 512 chars") and bug 34242 ("X font lookups on -*-*-*-*-*-*-*-*-*-*-*-*-*-*") into that patch, too ?

I would love to do that (the -* fix looks interesting), as well as the buffer bug, but can this PLEASE get reviewed and checked in before it becomes un-appliable again? Thank you.

Tomi: Can you change the target milestone on this to something like 0.9.2 or so? Perhaps more people would be willing to look into this then.

Setting target milestone to 0.9.2 and added "want for 0.9.2" to status_whiteboard.

Just some quick comments: + if (eventType == NS_MOUSE_RIGHT_BUTTON_DOWN) + eventType = NS_CONTEXTMENU; + Move that up into the ButtonPress: case above. You are just resetting it after it's already been set once and this violates the principal of least surprise. - // XXX fix this pevent.time = 0; - //pevent.time = PR_Now(); - //pevent.region = nsnull; Don't even bother. What events use time, anyway? +/* Create scrollbar widget */ +void nsScrollbar::CreateNative(Window aParent, nsRect aRect) +{ + XSetWindowAttributes attr; + unsigned long attr_mask; + + // on a window resize, we don't want to window contents to + // be discarded... + attr.bit_gravity = SouthEastGravity; Don't you want NorthWest gravity instead of SouthEast? +#define XLIB_DEFAULT_FONT1 "-*-helvetica-medium-r-*--*-140-*-*-*-*-iso8859-1" Aren't you missing a '*' there in between r-* and *-140? + XFontStruct *mDefaultFont; That font is used all over and you're loading it from every class. Why don't you make it a static and just destroy it when you are about to shut down? I could have nickel and dimed this patch a lot more but it's mostly infastructure so in the interests of expediency I'm not going to. Fix those and you have an r=blizzard

I'll look into the other comments but the mDefaultFont thing, I really dont get it - the font is only created once during DeviceContext::Init, then a bunch of metrics is extracted off it (only happens 2 or 3 times), and then its freed at shutdown. What other classes are using it?

pocemit: DeviceContext::Init() may be called multiple times in Mozilla's life (uhm... once per window... I don't know anymore... ;-( ) - and then it does that stuff multiple times, too. BAD.

pocemit: If you are going to file a new patch to match r=blizzard please wait for bug 80224 - blizzard will r= it soon. It may be a good idea to migrate both patches. BTW: bug 16688 ("nsFontMetricsGTK::GetWidth/DrawString ignore beyound 512 chars") got r=/sr=, too (currently waiting for a=) - if you're going to touch nsFontMetricsXlib.cpp: wanna integrate that patch, too ?

The new patch is _great_ !! Requesting sr= ...

asa: Requesting a=drivers@mozilla.org The risk associated with this patch is low as it patches only Xlib-toolkit sources - which are all _not_ part of the default build...

a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)

Things to be checked in: Get latest patch from attachment 38436 [details] [diff] [review] and apply it. Get attachments and name them as follows, all in mozilla/gfx/src/xlib/ http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23077 -> nsGCCache.cpp http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23078 -> nsGCCache.h http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36963 -> scale.c http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36964 -> drawers.h http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36965 -> XIE.c That's it.

Fix checked in for timecop by mkaply@us.ibm.com Glad to be of service. Marking fixed.

Thanks for all who helped with this. All seems to work ok now, marking as verified.