230605 - Consider deCOMtaminating nsIFontMetrics

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

There is no real reason for this to be an interface that follows the NS_IMETHOD
stuff as opposed to just a class with virtual methods.  Some things that all
font metrics would really need to have (like mLangGroup, eg) could move up to be
members on the "interface" and get inlined.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This may require merging gfx and layout into one library; not sure whether we
want to do that.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There is a bug on merging gfx+widget into gklayout. AFAIK no-one's objected. I'm
all in favour of it.

Comment 3

[Christopher Blizzard (:blizzard)]

I'm in favour of it, too, assuming certain rules are followed.  I'm a little
concerned that if we drop the idl interfaces that we're going to lose our
XPCOM-enforced wall of cross-platform-ness.  It's much more tempting to add
win32 or linux-specific hacks to layout at that point.  Just a thought.

Comment 4

[Boris Zbarsky [:bzbarsky]]

nsIFontMetrics is already not IDL....

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Most of our Gfx and Widget interfaces are not IDL.

Comment 6

[Christopher Blizzard (:blizzard)]

Sorry.  I meant to say that they are hidden behind an interface.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 194385 is the bug for merging gfx+widget into gklayout.

Comment 8

[Kenneth Herron]

*** Bug 273557 has been marked as a duplicate of this bug. ***

Comment 9

[Kenneth Herron]

Attachment: Proposed 1st round of changes

Here's a proposal for a first set of changes. There are eight different
implementations of this class, so I don't want to change too much at once.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks good.

+  virtual void  GetStrikeout(nscoord& aOffset, nscoord& aSize) = 0;
...
+  virtual void  GetUnderline(nscoord& aOffset, nscoord& aSize) = 0;

Why not define a struct for the offset/size pair, and return it?

Comment 11

[Kenneth Herron]

Attachment: FontMetricsXft getter call counts

I added some code to nsFontMetricsXft to count the number of calls to each of
20 different getters on a per-object basis, then surfed for a while and ended
up with data from about 1800 distinct fontmetrics objects. I loaded the data
into a spreadsheet, sorted it by the GetFont column, and exported it as
tab-separated to produce the attached file.

FontMetrics objects are cached and reused by the device context after being
created, so some getters are called several thousand times per object. Based on
this I think we should make some of these inlined, as Boris suggested, rather
than just changing function signatures like I proposed.

Comment 12

[Kenneth Herron]

Attachment: Make GetFont() non-virtual

This patch makes the GetFont() function non-virtual and changes its signature.

Some fontmetric implementations store their nsFont in an nsFont structure
member, while others dynamically allocate an nsFont structure inside Init().
Converting the dynamic implementations to use a member structure (and possibly
moving that variable into the base class) might provide some benefit.

I've tested the gtk, gtk2/xft, xlib, qt, and ps implementations. I don't have a
means to test any of the non-unix implementations.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170036 [details] [diff] [review]
Make GetFont() non-virtual

>Index: gfx/public/nsIFontMetrics.h
>+  nsIFontMetrics() : mFont(nsnull) {};

No need for that trailing ';'

>+private:
>+  const nsFont *mFont;

If this were protected, couldn't you let the subclasses keep using mFont (and
eliminate SetFont() altogether)?

Also, if mFont is to be guaranteed non-null for an nsIFontMetrics object, the
getter should be Font().  It looks like some of the impls delete it in
Destroy(), though, so we may not have this guarantee.

Other than that, looks good.

Comment 14

[Kenneth Herron]

(In reply to comment #13)
> (From update of attachment 170036 [details] [diff] [review] [edit])
> >Index: gfx/public/nsIFontMetrics.h
> >+  nsIFontMetrics() : mFont(nsnull) {};
> 
> No need for that trailing ';'

Oops. I'll get rid of it (and the on on the SetFont() line).

> >+private:
> >+  const nsFont *mFont;
> 
> If this were protected, couldn't you let the subclasses keep using mFont (and
> eliminate SetFont() altogether)?
> 
> Also, if mFont is to be guaranteed non-null for an nsIFontMetrics object, the
> getter should be Font().  It looks like some of the impls delete it in
> Destroy(), though, so we may not have this guarantee.

I suspect the implementations which are deleting their nsFont in Destroy() don't
really need to do that. In fact, I think we could just declare an nsFont
structure as a member instead of allocating/freeing it; the OS/2, Win, and QT
implementations already do this.

Regarding SetFont(), my personal preference is to provide getters and setters
rather than having a variable as part of the interface, but I'll change it if
you prefer.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> In fact, I think we could just declare an nsFont structure as a member instead
> of allocating/freeing it; the OS/2, Win, and QT implementations already do this.

That sounds like a good idea to me, actually.  That should save us a good bit of
code (allocations, error checks, deletions, etc).

At which point, there is really no reason not to use mFont in the subclasses
directly, imo.

Comment 16

[Kenneth Herron]

Attachment: Move the nsFont structure into the base class

This defines an nsFont structure as part of the base class. GetFont() is
renamed to Font(), which returns a const reference to the nsFont.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170698 [details] [diff] [review]
Move the nsFont structure into the base class

>Index: gfx/src/mac/nsFontUtils.cpp

>+	const nsFont *aFont = &inMetrics.GetFont();

Font(), not GetFont().

With that, sr=bzbarsky.  It may be nice to move some of those callers from
nsFont* to const nsFont&, but that can be a separate patch.

Comment 18

[Brian Ryner (not reading)]

Comment on attachment 170698 [details] [diff] [review]
Move the nsFont structure into the base class

Just a word of warning, we may run into problems on some platforms because
nsIFontMetrics will have destructor code now (calling the nsString destructor).
 We can usually work around this when it comes up.

Looks fine otherwise.

Comment 19

[Kenneth Herron]

Checked in the GetFont() change. Tinderbox (luna) shows no significant
performance change, and a slight increase in memory usage:

  Overall Change in Size
  	Total:	        +84 (+595/-511)
  	Code:	       -216 (+227/-363)
  	Data:	       +300 (+368/-148)

Comment 20

[Zack Weinberg (:zwol)]

Were the patches in this bug ever checked in?  Should the bug remain open?

Comment 21

[Reuben Morais [:reuben]]

nsIFontMetrics is history.