230002 - nsFontMetricsBeOS.cpp cleanup

Status: RESOLVED FIXED

(Core Graveyard :: GFX: BeOS, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

nsIPref -> nsIPrefService, removal of some unnecessary memory allocations, and a
few other things

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Sergei Dolgov]

First notice:
+    isfixed = family.Equals("monospace") || family.Equals("fixed");

I'm unsure if 1) comparison should be case-sensitive 2) maybe family.Contains
(?) is more reliable
Second notice (you didn't know about, though)
-  if (!IsASCIIFontName(aName)) 
+  if (!IsASCII(aName)
Non-ascii utf-8 font names are allowed in BeOS, and even sometimes required.
So i think this check may be omitted at all - see bug 200589
and two patches there. I think it is good idea, as we doing cleanup, to
implement both here.

Comment 3

[Sergei Dolgov]

Also wondering if this patch will fix
http://bugzilla.mozilla.org/show_bug.cgi?id=173116
problem:)
Hope so

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

> 1) comparison should be case-sensitive

Well, the old version was case-sensitive as well

> 2) maybe family.Contains(?) is more reliable

I checked what families are used here, and it was always a single word like
"monospace" "fixed" "Arial" etc. so .Equals seems safe.

Comment 5

[Sergei Dolgov]

>I checked what families are used here, and it was always a single word like
>"monospace" "fixed" "Arial" etc. so .Equals seems safe.

this is safe if we use "generic" families.
But what happens if there is request by page source to use Arial, but we have.
for example "Arial Unicode" installed? Does it matter for this method?

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Arial Unicode is not a fixed font...

Comment 7

[Sergei Dolgov]

ok, in this place it seems ok.
So it is notice for another places for future, when we will rewrite methods like
GetFontsList or such:)

Comment 8

[Sergei Dolgov]

If you think that this patch is wrong place to allow Japanese font names, i will
put review for it.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

yes, I would prefer if the japanese fonts were addressed in a separate bug

Comment 10

[Sergei Dolgov]

Comment on attachment 138361 [details] [diff] [review]
patch

r=[e-mail]

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in gfx/src/beos/nsFontMetricsBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsFontMetricsBeOS.cpp,v  <--  nsFontMetricsBeOS.cpp
new revision: 1.35; previous revision: 1.34
done