89851 - [xlib] nsFontMetricsXlib cleanup part 2

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[pocemit]

Needs to be synched with the GTK version which had some useful langgroup changes
and fixes for bitmap fonts.
patch follows.

Comment 1

[pocemit]

adding xlib people, accepting, setting tracker bug...

Comment 2

[pocemit]

Looking for r, sr

Comment 3

[pocemit]

Attachment: the patch

Comment 4

[pocemit]

gisburn: you might want to test this with your xprint stuff since this is in the
default build.
You should see no visible changes, but things should work as usual.

Comment 5

[Roland Mainz]

Some ToDo stuff:
- make all arrays with read-only data |const| arrays
- implement fix in bug 63831 ("wrong width for 8 bit data rendered with 16
bit font")
- verify that your changes do not break the staticbuild (or cls will
eat me alive). Many of the recent renames like |nsFontLangGroup| -->
|nsFontLangGroupXlib| were done to avoid collisions in the
staticbuild stuff.

Comment 6

[Roland Mainz]

More ToDo stuff:
- Implement GetBoundingMetrics() to fix bug 87285
- Adopt patch to match the changes introduced in patch for bug 89388

pocemit:
Can I overtake this bug, please ?

Comment 7

[pocemit]

Go for it.
The intent of this cleanup was to make the GTK and Xlib fontmetrics code similar
enough to be able to diff -u them and look for changes to update.
If you want to add whatever else to it, go for it.

By the way, if you do add things keep the same coding style, please?

if (something) {
  do it;
} else {
  dont do it ;
}

I know how hard it is for you not to use your coding style but...
Other people have to work with the code, too.

Comment 8

[Roland Mainz]

Accepting bug.

pocemit:
Agreed. It's a simple issue: No r=pocemit without correct |if()|-formatting
stuff. :-)

Comment 9

[Roland Mainz]

Attachment: 2nd patch, requires patch from bug 89388 to be functional

Comment 10

[Roland Mainz]

Attachment: New patch for 2001-07-16-08-trunk

Comment 11

[Roland Mainz]

Filed new patch. This one isn't final, only something from my debug tree. 

Unfortunately I am catched in a self-build mice-cage - the build
"crashes"(=exit(EXIT_FAILURE);) with a X error:
-- snip --
% export NSPR_LOG_MODULES="sync,RenderingContextXlib:5,FontMetricsXlib:5"
NSPR_LOG_FILE=/dev/stdout
% ./mozilla
[snip]
1[404b8]: nsRenderingContxtXlib::CommonInit()
1[404b8]: XGetGeometry(display=179978,drawable=8800046) = {root_win=2e, x=2,
y=2, width=1, height=1. border=0, depth=8}
1[404b8]: nsRenderingContextXlib::~nsRenderingContextXlib()
1[404b8]: nsRenderingContextXlib::PopState()
1[404b8]: nsRenderingContextXlib::nsRenderingContextXlib()
1[404b8]: nsRenderingContextXlib::PushState()
1[404b8]: nsRenderingContextXlib::Init(DeviceContext, Widget)
1[404b8]: nsRenderingContxtXlib::CommonInit()
1[404b8]: XGetGeometry(display=179978,drawable=8800012) = {root_win=2e, x=0,
y=0, width=1, height=1. border=0, depth=8}
1[404b8]: nsRenderingContextXlib::PushState()
1[404b8]: nsRenderingContext::GetDeviceContext()
1[404b8]: nsRenderingContextXlib::SetFont(nsIFontMetrics)
1[404b8]: nsRenderingContextXlib::GetWidth()
1[404b8]: nsRenderingContextXlib::GetWidth()
1[404b8]: nsRenderingContextXlib::SetColor(nscolor)
1[404b8]: Setting color to 255 255 255
1[404b8]: nsRenderingContextXlib::UpdateGC()
1[404b8]: new font 61d3f0
'-linotype-helvetica-medium-r-normal-sans-14-140-72-72-p-76-iso8859-1',
XID=61f290
1[404b8]: mark 6
1[404b8]: mark 7
1[404b8]: mark 10
X Error of failed request:  BadFont (invalid Font parameter)
  Major opcode of failed request:  55 (X_CreateGC)
  Resource id in failed request:  0x8800048
  Serial number of failed request:  2177
  Current serial number in output stream:  2179
-- snip --

Disable the |CHANGE_FONTS_AND_CRASH| CPP symbol in nsRenderingContextXlib.cpp
and the code works.

Nice... ;-(
Unfortunately I can't find _why_ it crashes... the fontXID is the same from
XLoadQueryFont()... looks OK so far... maybe I am simply too tired... :-))

Does anyone here see why it fails ?
Thanks!

Comment 12

[Roland Mainz]

Issue found. Thanks to [e-mail]
It was a cast problem, |XFontStructPtr(mCurrentFont)| is wrong,
|XFontStructPtr(*mCurrentFont)| is the correct way... :-)

Comment 13

[Roland Mainz]

Attachment: New patch for 2001-07-16-08-trunk

Comment 14

[Roland Mainz]

Filed patch, requesting r=, sr=

Comment 15

[Roland Mainz]

This patch will fix bug 85399, too... :-)

Comment 16

[Tomi Leppikangas]

Patch seems to work ok, r=[e-mail]

Comment 17

[Roland Mainz]

Thanks. 
For the final patch I'd like to integrate the fix from bug 88444 ("asserts now
coming from nsFontMetricsGTK.cpp", r=[e-mail]).

Requesting sr= ...

Comment 18

[tor]

We've told you before to not include unrelated changes in a patch, 
as they just slow down the review process.  Yet you're talking 
about including changes from bug 88444 and bug 20860.

Make up your mind.  Either (recommended) seek super review for the 
already attached, reviewed patch; or (not recommended) include a 
bunch of unrelated changes, attach a new patch, get it reviewed 
again, then seek super review.

Comment 19

[Roland Mainz]

tor:
There are only two unrelated changes which are not part of the initial bug:
1. Add comment to get rid of spam from users who did not have a patches Solaris
system:
--- mozilla_original/gfx/src/xprint/nsXPrintContext.cpp Tue Jul 17 17:33:49 2001
+++ mozilla/gfx/src/xprint/nsXPrintContext.cpp  Wed Jul 18 23:54:06 2001
@@ -22,6 +22,11 @@
  *
  */
 
+/* Note: Sun Workshop 6 may have problems when mixing <string,h> 
+ * and X includes. This was fixed by recent Xsun patches which 
+ * are part of the "recommended patches" for Solaris. 
+ * See bug 88703...
+ */
 #include <stdio.h>
 #include <stdlib.h>
 #include <errno.h>

2. Add |#ifdef PR_LOG| to nsImageXlib.cpp
diff -r -u mozilla_original/gfx/src/xlib/nsImageXlib.cpp
mozilla/gfx/src/xlib/nsImageXlib.cpp
--- mozilla_original/gfx/src/xlib/nsImageXlib.cpp       Mon Jul 16 04:38:54 2001
+++ mozilla/gfx/src/xlib/nsImageXlib.cpp        Wed Jul 18 23:46:55 2001
@@ -36,7 +36,9 @@
 
 #define IsFlagSet(a,b) ((a) & (b))
 
+#ifdef PR_LOGGING 
 static PRLogModuleInfo *ImageXlibLM = PR_NewLogModule("ImageXlib");
+#endif /* PR_LOGGING */ 
 
 /* XXX we are simply creating a GC and setting its function to Copy.
    we shouldn't be doing this every time this method is called.  this creates

That's all...

The theory is to file bugs for each single issue. The real life shows that even
minor patches took ~two weeks or more until r=/sr= unless I bundle them with
parts of patches to get them in _quicky_.
Sorry... but that's my current experience here... ;-((

You are right for the other issues... I file a new patch in a few mins...

Comment 20

[Roland Mainz]

Attachment: All-in-one patch for 2001-07-16-08-trunk

Comment 21

[Roland Mainz]

Filed new-all-in-one patch, incl. patch from bug 88444 (r=[e-mail])
and a ported fix from patch to nsFontmetricsGTK.cpp from bug 20860.

Comment 22

[pocemit]

looks good.
r=pocemit

Comment 23

[Roland Mainz]

Requesting sr= ...

Comment 24

[Roland Mainz]

This bug is fixing or blocking some 0.9.3 milestone bugs. Tree freezes in a few
hours, requesting quick sr= ... pleeaassee...

Comment 25

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 26

[Roland Mainz]

blizzard:
THANKS!!

----

CC:'ing [e-mail] for checkin, please...

Comment 27

[Mike Kaply [:mkaply]]

I am out of town right now, so if someone else could pick this up, it would be 
very helpful.

Comment 28

[Daniel Roberts]

I'd be willing to check this in tonight for you, as soon as the tree is open,
and I have a block of time available when I can watch the tree.

Comment 29

[Roland Mainz]

zuperdee:
Thanks!

I just tested it today... it _should_ work. The only unknown factor is the
xx@@xx!!-StaticBuild stuff... ;-(

Comment 30

[Daniel Roberts]

Well, even if xx@@xx!!-StaticBuild stuff doesn't work, I would think that this 
patch will still at least be making things better than they were before...  I 
mean, it isn't like it's actually breaking something that worked before, 
right?  If that is the case, then I say we should file a seperate bug on that 
issue later.

Anyway, unless we find that it will indeed cause a regression, I will check it 
in tonight when I get home from work, when the tree is open, and I have some 
free time available to watch the tree.

Comment 31

[tor]

a=tor, but make sure you're around to fix any problems with static builds.

Comment 32

[Roland Mainz]

Patch has been checked-in by timeless
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=996457680&maxdate=996458040&who=timeless%25mac.com).

Marking bug as FIXED.

----

pocemit:
Wanna verify this bug, please ?

Comment 33

[Roland Mainz]

Attachment: Fix for StaticBuild bustage

Comment 34

[timeless]

fwiw, this broke nondebug builds. I checked in nsRenderingContextXlib.cpp 1.58 
to fix that.

Comment 35

[Roland Mainz]

Ouch... looks I missed a simple #include "nsdebug.h" ... ;-(

timeless: 
thanks!

Comment 36

[John Morrison]

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