get rid of duplicated codes in nsFontFreeType and nsFontPSFreeType



Core Graveyard
14 years ago
3 years ago


(Reporter: Jungshik Shin, Assigned: Jungshik Shin)


Firefox Tracking Flags

(Not tracked)



(1 attachment)

33.28 KB, patch
Jungshik Shin
: review?
Roland Mainz
Details | Diff | Splinter Review


14 years ago
While working on bug 226288, almost identical methods are defined separately for
both nsFontFreeType and nsFontPSFreeType. (bug 226288 comment #6).
They have to be factored out to reduce the footprint and ease the maintenance.

Comment 1

14 years ago
Created attachment 137960 [details] [diff] [review]
a patch


Comment 2

14 years ago
Comment on attachment 137960 [details] [diff] [review]
a patch

asking for r/sr.
Attachment #137960 - Flags: superreview?(rbs)
Attachment #137960 - Flags: review?(Louie.Zhao)

Comment 3

14 years ago
What about making |nsFontPSFreeType| to inherit from |nsFreeTypeFont|. Or
something along those lines, to give a chance to methods of nsFontPSFreeType
(e.g., GetBoundingMetrics) that remain unimplemented so far. Otherwise, I don't
see a significant pay off from your patch and don't mind the little duplication.

Comment 4

14 years ago
I'll see if that's feasible. Note that nsFontPSFreetype inherits nsFontPS and
nsFreetypeFont inherits nsFontGTK so it can get complicated.
BTW, I think the patch as it stands is still worthwhile at least for the easy
maintenance if not for the reduction in the code size.

Comment 5

14 years ago
I keep forgetting things (hmm.....). In bug 228911, I considered that
possibility (I haven't added any comment about that), but making
nsFontPSFreetype inherit nsFreetypeFont is not so simple as it looks because in
addition to the reason I gave in the previous comment, MathML fonts are handled
by a derived class of nsFontFreetype. It might be possible to factor out
something. When I filed this bug, I planned to use mozFT2Utils helper class as a
sorta launching pad for that.

Comment 6

14 years ago
Comment on attachment 137960 [details] [diff] [review]
a patch

Since this has been a while, I am removing it from my sr queue. Re-ask when the
reviewer is happy.
Attachment #137960 - Flags: superreview?(rbs)

Comment 7

14 years ago
Comment on attachment 137960 [details] [diff] [review]
a patch

Roland, do you care to review this? 

I think every bit of the code reduction is worth trying. Besides, as I wrote in
comment #0, it's cumbersome to make exactly the same change twice when I need
to make changes. 

I'll make sure to replace NPL in the patch with MPL.
Attachment #137960 - Flags: review?(Louie.Zhao) → review?(roland.mainz)
Product: Core → Core Graveyard
This bug has been buried in the graveyard and has not been updated in over 5 years. It is probably safe to assume that it will never be fixed, so resolving as WONTFIX.

[Mass-change filter: graveyard-wontfix-2014-09-24]
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.