139380 - duplicate code in nsFontMetricsXlib.cpp/nsFontMetricsGTK.cpp

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P2)

Description

[Mike]

from LXR
4473   encodingHyphen = aFFREName.FindChar('-', encodingHyphen + 1);
4474   encodingHyphen = aFFREName.FindChar('-', encodingHyphen + 1);

I have commented out the second line, rebuilt, and have been running fine
without new errors.

Comment 1

[Matthias Versen [:Matti]]

-> Gisburn

This is from bug 80224...

CC [e-mail]

Comment 2

[Roland Mainz]

nsFontMetricsGTK.cpp has the same "flaw" ...

Comment 3

[Roland Mainz]

Attachment: Patch for 2002-04-21-08-trunk

Comment 4

[Roland Mainz]

Requesting r=/sr= ...

Comment 5

[kill this account]

r=[e-mail]

thanks for finding and fixing this

Comment 6

[Marc Attinasi]

Comment on attachment 80841 [details] [diff] [review]
Patch for 2002-04-21-08-trunk

sr=attinasi :)

Comment 7

[Roland Mainz]

Patch checked in
(http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=peterv%25netscape.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1019820540&maxdate=1019820960&cvsroot=%2Fcvsroot),
marking bug as FIXED.

Comment 8

[kill this account]

Doesn't this change the logic?

   PRInt32 encodingHyphen = aFFREName.FindChar('-');
   encodingHyphen = aFFREName.FindChar('-', encodingHyphen + 1);
-  encodingHyphen = aFFREName.FindChar('-', encodingHyphen + 1);
   aFFREName.Truncate(encodingHyphen+1);
   aFFREName.Append(aReplacementEncoding);

Looks to me that we now truncate after the 2nd hyphen instead of the 3rd.

Comment 9

[kill this account]

and saying it explicitly:

Isn't this function now broken?

Comment 10

[kill this account]

and of course my mistake in approving this.

Comment 11

[kill this account]

Roland: when you fix this would you kindly add a comment explaining this and
also add comments to the other places where there is similar login; eg:
FFREToXLFDPattern?

Comment 12

[Mike]

maybe something like this would be better than?
PRInt32 charsetHyphen;
int x;
for(x=0;x<3;x++)
{
charsetHyphen = oPattern.FindChar('-', charsetHyphen + 1); 
}
oPattern.Insert("-*-*-*-*-*-*-*-*-*-*", charsetHyphen);

Comment 13

[kill this account]

A comment would do work.
A loop would work (just be sure to initialize charsetHyphen).

Comment 14

[kill this account]

> A comment would do work.

By this I mean: put back the original code and add a comment so people in the
future do not make get confused and break the logic.

Either one is okay with me.

Comment 15

[kill this account]

> from LXR
> 4473   encodingHyphen = aFFREName.FindChar('-', encodingHyphen + 1);
> 4474   encodingHyphen = aFFREName.FindChar('-', encodingHyphen + 1);
> 
> I have commented out the second line, rebuilt, and have been running fine
> without new errors.

Part of the error occured here. Not seeing an error does not infer that things
are working correctly.

To make the font system tolerant to the vagaries of fonts on different systems
I spent months writing code to find alternate (fallback) fonts. Because the font 
system has fallback depth a bustage like this did not cause a catastrophic 
failure. The code just fell to the next fallback layer.

I guess this means that as people who do not understand the code make changes
it seems likely that they may well bust things.


did not 

Comment 16

[Roland Mainz]

Taking...

Comment 17

[Roland Mainz]

Attachment: Patch for 2002-05-08-08-trunk, revert to old code and add comments to avoid this "trap" in the future

Comment 18

[Roland Mainz]

Requesting r=/sr=/a= ...

Comment 19

[kill this account]

r=[e-mail] for attachment 83286 [details] [diff] [review]

Thanks Roland!

Comment 20

[Marc Attinasi]

Comment on attachment 83286 [details] [diff] [review]
Patch for 2002-05-08-08-trunk, revert to old code and add comments to avoid this "trap" in the future

rsr=attinasi

Comment 21

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

Comment on attachment 83286 [details] [diff] [review]
Patch for 2002-05-08-08-trunk, revert to old code and add comments to avoid this "trap" in the future

a=[e-mail]

Comment 22

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

Patch checked into branch for gisburn.  Still needs to hit trunk.

Comment 23

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

Fix checked into trunk for gisburn