Consider deCOMtaminating nsIFontMetrics

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
16 years ago
8 months ago

People

(Reporter: bzbarsky, Assigned: kherron+mozilla)

Tracking

(Blocks 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
This may require merging gfx and layout into one library; not sure whether we
want to do that.
There is a bug on merging gfx+widget into gklayout. AFAIK no-one's objected. I'm
all in favour of it.
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.
nsIFontMetrics is already not IDL....
Most of our Gfx and Widget interfaces are not IDL.
Sorry.  I meant to say that they are hidden behind an interface.
Bug 194385 is the bug for merging gfx+widget into gklayout.
Depends on: 194385
(Assignee)

Comment 8

15 years ago
*** Bug 273557 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 9

15 years ago
Posted patch Proposed 1st round of changes (obsolete) — Splinter Review
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.
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?
(Assignee)

Comment 11

15 years ago
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.
Attachment #168177 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Assignee: nobody → kherron+mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 12

15 years ago
Posted patch Make GetFont() non-virtual (obsolete) — Splinter Review
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.
(Assignee)

Updated

15 years ago
Attachment #170036 - Flags: superreview?(bzbarsky)
Attachment #170036 - Flags: review?(blizzard)
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.
Attachment #170036 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 14

15 years ago
(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.
> 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.
(Assignee)

Updated

15 years ago
Attachment #170036 - Attachment is obsolete: true
Attachment #170036 - Flags: review?(blizzard)
(Assignee)

Comment 16

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

Updated

15 years ago
Attachment #170698 - Flags: superreview?(bzbarsky)
Attachment #170698 - Flags: review?(blizzard)
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.
Attachment #170698 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

15 years ago
Attachment #170698 - Flags: review?(blizzard) → review?(bryner)
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.
Attachment #170698 - Flags: review?(bryner) → review+
(Assignee)

Comment 19

14 years ago
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)
Blocks: deCOM
Were the patches in this bug ever checked in?  Should the bug remain open?
nsIFontMetrics is history.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME

Updated

8 months ago
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.