45737 - nsFontMetricsWin.cpp is in a dire straights, and should be restructured

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[rbs]

nsFontMetricsWin.cpp has been changing a lot since it was first created, with
additions and work-around for various problems that have surfaced.

Unfortunately, due to the fear of breaking anything, most of the changes
were done in a "local" manner (i.e, developers took great care to keep their
changes in a confined section of the file). This has led to duplicate 
data/functionalities (e.g., multiple caches for font objects, various structs
that could be combined, etc.).

Now that most of the functionalities needed for a first release are in the code,
this could be a motivation to completely restructure the code from a global 
perspective as you successfully did in nsFontMetricsGTK.cpp. If viewed as
a perf work, it could well be done in the forthcoming perf stage.

With all the practical aspects in the code known (and nearly frozen), a
better integration of all these aspects could lead to a smaller/faster code
in which all the memory leaks (e.g., bug 29358) are addressed from the outset.

Comment 1

[Frank Tang]

erik is on vacation. 

Comment 2

[Erik van der Poel]

Hi Roger! Thanks for the suggestion. The Unix side was not only restructured but
also fixed to address a number of serious issues. On the Windows side, we still
have a few issues such as memory-related ones, and the code could certainly be
made more maintainable by restructuring it, but my first impression after this
long vacation is to leave it more or less as it is until we ship the first
release, which is quite imminent. Please let me think about it some more, while
I go through my other bugs, some of which may require more immediate attention.

Comment 3

[Kevin McCluskey (gone)]

I agree with Erik. It would be great to restructure it, but there isn't enough 
time before we ship.

Comment 4

[rbs]

[mid-air collision. re-submitting]

Welcome back Erik! When I saw how you reworked the Unix code, and how the new
code was looking rock solid, and so clean and tidy, I couldn't resist filing
this bug, asking for the same care on Windows... But with this tight schedule,
since a major overhaul is a demanding job, it can wait if there are other urgent
fishes to fry on your plate. So just fixing the other reported memory leaks
would be okay for the first release.

Comment 5

[Frank Tang]

erik resign. reassign all his bug to ftang for now.

Comment 6

[Frank Tang]

mark all future new as assigned after move from erik to ftang

Comment 7

[rbs]

There have been several gradual improvements ever since I filed this bug, and I 
did include some clean ups and mlk fixes in my font changes (bug 99010). 
Something that remains and could be investigated could be to see if the handling 
of the fontweight hashtable could somehow be combined with the other tables. But 
I can live with things the way they are. So resolving this bug as FIXED to get 
if off the radars.