998869 - construct gfxFont objects lazily only after performing font matching

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[John Daggett (:jtd)]

When a font family list is defined in CSS, it ends up as a list of
fonts used to construct a gfxFontGroup object.  This object serves as
a factory object for constructing textruns, which transform a string
with associated style data into runs of glyphs and positions.

When a gfxFontGroup object is constructed, gfxFontGroup::BuildFontList
is called.  This instantiates a list of gfxFont objects, one for each
of the fonts associated with entries in the font family list.  This
happens *before* font matching is done, so there's a good chance that
in a font list containing multiple entries, only the first gfxFont
will ever actually be used.

There's really no need to build gfxFont objects until font matching
has been done.  Since font matching is associated with the
gfxFontEntry, we really only need to use gfxFontEntry objects rather
than gfxFont objects.  In a quick test I did, when browsing around
major sites for a few minutes, of the roughly 1400 gfxFont objects
created, only 38% were ever used to shape text.

The solution here is to make the gfxFontEntry object the basis for
font matching and only *after* a given font is chosen, create the
gfxFont.

At the same time, I think the names passed in should be
"prenormalized" within CSS so that we correctly distinguish between
generics/system fonts and named fonts (which is part of bug 280443,
sort of).  We could then eliminate a lot of the re-parsing of family
names that occurs in gfxFontGroup::ForEachFontInternal.  I think this
would lead to much more streamlined font matching, something that
always shows up in profiles with lots of textrun construction.

Comment 1

[John Daggett (:jtd)]

Bug 754215 contained a patch to do a better job of lazily building gfxFont objects, including downloadable fonts. As part of this bug, include the logic of the pt 2 patch after making the changes to the font matching code.

Comment 2

[John Daggett (:jtd)]

Attachment: patch part 1 - build fontlists more lazily

For all the entries in the fontlist, don't create gfxFont objects immediately, wait until they are needed as part of font matching.

Comment 3

[John Daggett (:jtd)]

Attachment: patch part 2 - switch the use of GetFontAt(0) to GetFirstValidFont (wip)

Lots of code within gfx and layout calls GetFontAt but only to read the first valid font in the fontlist. In preparation for loading userfonts more lazily, switch these calls to GetFirstValidFont.

Comment 4

[John Daggett (:jtd)]

Attachment: patch part 3 - rename various methods in userfont code

Cleanup the names used for various userfont class methods to try and align it with what makes sense for the CSS Font Loading API work.

Comment 5

[John Daggett (:jtd)]

Attachment: patch part 4 - rename UpdateFontList

The 'UpdateFontList' method name is used within gfx code for two different things, updating the list of fonts in the gfxFontGroup and updating the list of system fonts in the platform font list objects. To distinguish between these two, rename 'UpdateFontList' for the font group to 'UpdateUserFonts', since that's what it's purpose is/will be.

Comment 6

[John Daggett (:jtd)]

Attachment: patch part 5 - lazily load userfonts (non-linux)

When constructing the fontlist for the gfxFontGroup object, include the userfont entries whether loaded or not. Then, within the font matching code, determine whether to initiate a load or not only when it's needed. This will eliminate the problem we have with Gecko always loading downloadable fonts, whether they are in the first position within the fontlist or not.

Ex:
  @font-face {
    font-family: MyHelvetica;
    src: url(Helvetica.woff2);
  }

  body { font-family: Helvetica, MyHelvetica, sans-serif; }

This should load the downloadable font *only* when Helvetica is not available locally (or there's a system substitute such as on Windows).

Note: the code in gfxPangoFontGroup functions as before, I created a method that is used exclusively by that code that mimics existing behavior. We need to rework gfxPangoFontGroup to use the font matching code used on other platforms, using fontconfig for the initial lookups and prefs/system fallback.

Comment 7

[John Daggett (:jtd)]

Note that these patches are based on Jonathan's refactoring of the font class files, bug 1066043.

Comment 8

[John Daggett (:jtd)]

Note: code commented out in BuildFontList in part 1 is fixed later on in part 5

Comment 9

[John Daggett (:jtd)]

Attachment: patch part 2 - switch the use of GetFontAt(0) to GetFirstValidFont

Comment 10

[John Daggett (:jtd)]

Attachment: patch part 5 - lazily load userfonts (non-linux)

Includes handling of the bad underline code removed in part 1.

Comment 11

[John Daggett (:jtd)]

Running a test by loading a set of pages from various sites, the number of gfxFont objects created and used in shaping or for metrics goes from 32% of the total to 89%!

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8489961 [details] [diff] [review]
patch part 1 - build fontlists more lazily

Review of attachment 8489961 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxTextRun.cpp
@@ +1722,5 @@
> +        }
> +        if (mFonts[i].Font() &&
> +            mFonts[i].Font()->GetFontEntry() == aFontEntry) {
> +            return true;
> +        }

The suggested change to FamilyFace in gfxTextRun.h will simplify this.

::: gfx/thebes/gfxTextRun.h
@@ +719,4 @@
>  
>  class gfxFontGroup : public gfxTextRunFactory {
>  public:
>      class FamilyFace {

We should move FamilyFace from public to protected, to emphasize that it's an internal helper for gfxFontGroup. I don't think we need public visibility anywhere?

@@ +723,5 @@
>      public:
>          FamilyFace() { }
>  
>          FamilyFace(gfxFontFamily* aFamily, gfxFont* aFont)
> +            : mFamily(aFamily), mFont(aFont), mFontEntry(nullptr)

This constructor leaves mNeedsBold uninitialized.

@@ +745,3 @@
>          gfxFontFamily* Family() const { return mFamily.get(); }
>          gfxFont* Font() const { return mFont.get(); }
> +        gfxFontEntry* FontEntry() const { return mFontEntry.get(); }

With the suggestion below, these would become something like

    gfxFont* Font() const
    {
        return mFaceIsGfxFont ? mFace.mFont : nullptr;
    }
    gfxFontEntry* FontEntry() const
    {
        return mFaceIsGfxFont ?
            mFace.mFont->GetFontEntry() : mFace.mFontEntry;
    }

(Making FontEntry() always return the font entry, whichever state we're in, will be convenient during the font-matching code; no need to check FontEntry() and Font() separately.)

@@ +746,5 @@
>          gfxFont* Font() const { return mFont.get(); }
> +        gfxFontEntry* FontEntry() const { return mFontEntry.get(); }
> +        bool NeedsBold() const { return mNeedsBold; }
> +
> +        void SetFont(gfxFont* aFont) { mFont = aFont; }

And this would AddRef() the given font, Release() the existing font entry, and change the union tag to indicate that we're now storing a gfxFont.

@@ +751,5 @@
>  
>      private:
>          nsRefPtr<gfxFontFamily> mFamily;
>          nsRefPtr<gfxFont>       mFont;
> +        nsRefPtr<gfxFontEntry>  mFontEntry;

I'm a bit reluctant to see every FamilyFace carry an additional nsRefPtr; this seems like bloat we could avoid.

At any given time, a FamilyFace will contain *either* a gfxFontEntry *or* a gfxFont, right? It doesn't need both at once. So we could avoid enlarging the object by merging those two into a union (called something like mFace, with mFont and mFontEntry members), and provide a tag field that records which one is currently present; the mNeedsBold bool means that there will be some padding bytes available where the tag can go.

(Using a union would presumably mean you'd have to store raw pointers and call AddRef/Release manually on those objects, as I don't think putting nsRefPtr's in a union can work. But this would be entirely encapsulated within the FamilyFace class constructors, destructor, and assignment operator, so it shouldn't be too hard to manage; it needn't expose refcounting messiness anywhere externally.)

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #11)
> Running a test by loading a set of pages from various sites, the number of
> gfxFont objects created and used in shaping or for metrics goes from 32% of
> the total to 89%!

Sounds good. Out of curiosity, what were the actual total numbers of gfxFont objects created for your sample set of pages?

Have you run perf tests to see whether there's any detectable impact on performance?

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8489965 [details] [diff] [review]
patch part 3 - rename various methods in userfont code

Review of attachment 8489965 [details] [diff] [review]:
-----------------------------------------------------------------

Could you add some assertions at the top of LoadNext and LoadPlatformFont that the current mUserFontLoadState (and maybe mFontDataLoadingState too) has an expected value?  I think that'd help understand the state transitions.

What's the reasoning behind splitting the status out into two fields by the way?  Is it so that you can easily expose the coarse notloaded/loading/loaded/failed status to consumers outside gfx, while keeping the slowly loading status internal?

::: gfx/thebes/gfxUserFontSet.cpp
@@ +478,4 @@
>  }
>  
>  bool
> +gfxUserFontEntry::LoadPlatformFont(const uint8_t* aFontData, uint32_t &aLength)

Move the "&" next to the type while you're here.

@@ +780,5 @@
>      }
>  
>      gfxUserFontEntry* userFontEntry = static_cast<gfxUserFontEntry*> (fe);
>  
> +    if (userFontEntry->LoadState() == gfxUserFontEntry::STATUS_NOT_LOADED) {

This condition is already checked inside gfxUserFontEntry::Load.  Either remove the check here, or remove the one from inside Load and add an assertion that we're not already loading.

::: gfx/thebes/gfxUserFontSet.h
@@ +505,5 @@
> +        if (mUserFontLoadState == STATUS_LOADING &&
> +            mFontDataLoadingState < LOADING_SLOWLY) {
> +                return true;
> +        }
> +        return false;

Unless you plan to add more to this method, just make it

  return mUserFontLoadState == STATUS_LOADING &&
         mFontDataLoadState < LOADING_SLOWLY;

@@ +537,5 @@
>      // helper method for creating a platform font
>      // returns true if platform font creation successful
>      // Ownership of aFontData is passed in here; the font must
>      // ensure that it is eventually deleted with NS_Free().
> +    bool LoadPlatformFont(const uint8_t* aFontData, uint32_t &aLength);

"&" next to type.

::: layout/style/nsFontFaceLoader.cpp
@@ +213,3 @@
>    // This is called even in the case of a failed download (HTTP 404, etc),
>    // as there may still be data to be freed (e.g. an error page),
> +  // and we need load the next source.

"need to load"?

Comment 15

[John Daggett (:jtd)]

Attachment: patch part 1v2 - build fontlists more lazily

Updated based on review comments.

Comment 16

[John Daggett (:jtd)]

Attachment: patch part 5v3 - lazily load userfonts (non-linux)

Updated for changes to part 1 patch

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8491285 [details] [diff] [review]
patch part 1v2 - build fontlists more lazily

Review of attachment 8491285 [details] [diff] [review]:
-----------------------------------------------------------------

Basically looks good, but I think the FamilyFace refcounting needs some more attention. See comments below for the issues I noticed, but let's look it over once more after these are addressed.

::: gfx/thebes/gfxTextRun.cpp
@@ +2465,5 @@
> +        // test the font entry, build font if needed
> +        gfxFontEntry *fe = mFonts[i].FontEntry();
> +        if (fe->HasCharacter(aCh)) {
> +            font = mFonts[i].Font();
> +            if (!font.get()) {

The .get() is redundant here.

@@ +2469,5 @@
> +            if (!font.get()) {
> +                font = fe->FindOrMakeFont(&mStyle, mFonts[i].NeedsBold());
> +                mFonts[i].SetFont(font);
> +            }
> +            if (font && font->Valid()) {

And font is known to be non-null here, so only font->Valid() needs to be checked.

@@ +2478,5 @@
>  
> +        // if italic, test the regular face to see if it supports the character
> +        // only do this for platform fonts, not userfonts
> +        if (mStyle.style != NS_FONT_STYLE_NORMAL && !fe->IsUserFont()) {
> +            font = FindNonItalicFaceForChar(mFonts[0].Family(), aCh);

s/mFonts[0]/mFonts[i]/

::: gfx/thebes/gfxTextRun.h
@@ +902,5 @@
> +    class FamilyFace {
> +    public:
> +        FamilyFace() : mFamily(nullptr), mFontEntry(nullptr),
> +                       mNeedsBold(false), mFontCreated(false)
> +        { }

Do we need this constructor? If not, let's make it private (or can we MOZ_DELETE it?), and require people to use the versions with parameters.

OTOH, I believe nsTArray will use the class's copy constructor, so you need to provide this in order to take care of the refcounting; the compiler's default copy constructor won't do here.

@@ +932,5 @@
> +        {
> +            if (mFontCreated) {
> +                NS_RELEASE(mFont);
> +            } else {
> +                NS_RELEASE(mFontEntry);

NS_IF_RELEASE, in case mFontEntry is null. Unless we make the default constructor private, to ensure nobody uses it.

@@ +945,5 @@
> +        gfxFontEntry* FontEntry() const {
> +            return mFontCreated ? mFont->GetFontEntry() : mFontEntry;
> +        }
> +
> +        bool NeedsBold() const { return mNeedsBold; }

Presumably this should only be called when we're holding a fontEntry, not a created font? So I think you can assert that !mFontCreated here.

@@ +952,5 @@
> +        {
> +            NS_ASSERTION(aFont, "font pointer must not be null");
> +            if (mFontCreated) {
> +                NS_RELEASE(mFont);
> +            }

I think this leaves a leak in the (usual) case where we're replacing a fontEntry with a font; it needs

        else {
            NS_IF_RELEASE(mFontEntry);
        }

because we ADDREF'd the font entry when it was set, right?

@@ +958,5 @@
> +            NS_ADDREF(aFont);
> +            mFontCreated = true;
> +        }
> +
> +    private:

I think we should have a private (or deleted) assignment operator in this class, to prevent inadvertently assigning it (which would fail to update the mFont/mFontEntry refcount appropriately).

@@ +1033,5 @@
>  
>      // Helper for font-matching:
> +    // when matching the italic case, allow use of the regular face
> +    // if it supports a character but the italic one doesn't
> +    // return null if regular face doesn't support aCh

It would help readability to provide sentence capitalization and punctuation in the comment here!

Comment 18

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> >      private:
> >          nsRefPtr<gfxFontFamily> mFamily;
> >          nsRefPtr<gfxFont>       mFont;
> > +        nsRefPtr<gfxFontEntry>  mFontEntry;
> 
> I'm a bit reluctant to see every FamilyFace carry an additional nsRefPtr;
> this seems like bloat we could avoid.
> 
> At any given time, a FamilyFace will contain *either* a gfxFontEntry *or* a
> gfxFont, right? It doesn't need both at once. So we could avoid enlarging
> the object by merging those two into a union (called something like mFace,
> with mFont and mFontEntry members), and provide a tag field that records
> which one is currently present; the mNeedsBold bool means that there will be
> some padding bytes available where the tag can go.
> 
> (Using a union would presumably mean you'd have to store raw pointers and
> call AddRef/Release manually on those objects, as I don't think putting
> nsRefPtr's in a union can work. But this would be entirely encapsulated
> within the FamilyFace class constructors, destructor, and assignment
> operator, so it shouldn't be too hard to manage; it needn't expose
> refcounting messiness anywhere externally.)

The more I play with this idea, the more I dislike it. Using union's makes sense when something is really multi-valued but here we're just using it to save the space of a single nsRefPtr. To do that we have to implement a lot of extra swizzling code that is going to be harder to maintain. After fiddling with this a bit it appears to function correctly on non-linux platforms. However, I run into all sorts of refptr-related assertions under Linux. That's sort of the problem here, this data structure is shared with the gfxPangoFontGroup code and used differently. The version with nsRefPtr's was working fine.

There aren't a lot of font groups in use relative to the size of other content. I think reducing the size of this data structure at the cost of type safety is a bad choice.

I think we can revisit this when we do the eventual reworking of the code in gfxPangoFontGroup but I'd like to forego it for now.

Comment 19

[Jonathan Kew [:jfkthame]]

> However, I run into all sorts
> of refptr-related assertions under Linux. That's sort of the problem here, this data structure is shared
> with the gfxPangoFontGroup code and used differently.

Provided FamilyFace correctly encapsulates the refcounting of its mFontEntry/mFont members, it shouldn't matter exactly how it's used by the rest of the code. The problem on Linux is that you didn't implement a FamilyFace assignment operator (see note in comment 17), which it turns out the gfxPangoFonts code uses. So you get the compiler's automatically-generated one, and it doesn't handle the necessary refcounting.

I pushed a new try job where I added a patch to handle this, together with a couple other cleanups, and it seems to be coming up green: https://tbpl.mozilla.org/?tree=Try&rev=0af8eac86603. If you incorporate this, I think you'll be fine.

FWIW, I also tried instrumenting the code to see how many FamilyFace objects are in existence at any given time, and found that simply launching the browser (on OS X) and loading the nytimes.com front page, for example, results in 750+ live FamilyFace instances. With a few tabs open, or some inter-page navigation, it easily runs into thousands (with a lot of churn, as text frames and the associated font groups, etc., get created, cached, discarded,....).

(And it's not just a question of the additional 4 or 8 bytes per FamilyFace, though that will quickly run into multiple KB of RAM; there's also the overhead of having two nsRefPtrs being managed on every construction/assignment/destruction, where we only need one.)

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #14)
> Comment on attachment 8489965 [details] [diff] [review]
> patch part 3 - rename various methods in userfont code
> 
> What's the reasoning behind splitting the status out into two fields by the
> way?  Is it so that you can easily expose the coarse
> notloaded/loading/loaded/failed status to consumers outside gfx, while
> keeping the slowly loading status internal?

Turns out this status field splitting works well for me in bug 1028497, so no need to explain.

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8489965 [details] [diff] [review]
patch part 3 - rename various methods in userfont code

r=me with the comment 14 comments addressed.

Comment 22

[John Daggett (:jtd)]

(In reply to Cameron McCormack (:heycam) from comment #14)

> What's the reasoning behind splitting the status out into two fields by the
> way?  Is it so that you can easily expose the coarse
> notloaded/loading/loaded/failed status to consumers outside gfx, while
> keeping the slowly loading status internal?

Right. The existing code has these two state variables, so I tried to morph the code into something that made it clearer. The coarse-grained UserFontLoadState reflects the state changes needed by FontFace objects while the fine-grained FontDataLoadingState is used within graphics code to determine whether to draw with fallback fonts or not. I've tried to comment the code related to this, let me know if you think it needs more.

Comment 23

[John Daggett (:jtd)]

Attachment: patch part 1v3 - build fontlists more lazily

Updated based on review comments.

Comment 24

[John Daggett (:jtd)]

Attachment: patch part 5v4 - lazily load userfonts (non-linux)

Updated due to changes in part 1 patch

Comment 25

[Jonathan Kew [:jfkthame]]

Comment on attachment 8494335 [details] [diff] [review]
patch part 1v3 - build fontlists more lazily

Review of attachment 8494335 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxTextRun.cpp
@@ +2409,5 @@
>              return firstFont.forget();
>          }
> +
> +        // if italic, test the regular face to see if it supports the character
> +        // only do this for platform fonts, not userfonts

Split into sentences for readability, please.

@@ +2477,4 @@
>          }
>  
> +        // if italic, test the regular face to see if it supports the character
> +        // only do this for platform fonts, not userfonts

And here.

::: gfx/thebes/gfxTextRun.h
@@ +1072,5 @@
>  
>      // Helper for font-matching:
> +    // When matching the italic case, allow use of the regular face.
> +    // If it supports a character but the italic one doesn't
> +    // return null if regular face doesn't support aCh

Seems like the sentence break should be after "doesn't", not "face".

Comment 26

[Jonathan Kew [:jfkthame]]

Comment on attachment 8490575 [details] [diff] [review]
patch part 2 - switch the use of GetFontAt(0) to GetFirstValidFont

Review of attachment 8490575 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxTextRun.cpp
@@ +1875,1 @@
>      if (font && font->HasCharacter(hyphen)) {

You can drop the check that |font| is non-null here; looks like everywhere else, we assume that GetFirstValidFont() always returns a usable gfxFont.

::: gfx/thebes/gfxTextRun.h
@@ +768,5 @@
>  
>      virtual ~gfxFontGroup();
>  
> +    virtual gfxFont* GetFirstValidFont() {
> +        return GetFontAt(0);

I was going to ask about this - how do we know that GetFontAt(0) will always return a valid font? But I see patch 5 replaces this anyway, so whatever...

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 8494336 [details] [diff] [review]
patch part 5v4 - lazily load userfonts (non-linux)

Review of attachment 8494336 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxTextRun.cpp
@@ +1475,5 @@
>  }
>  
> +gfxFontGroup::~gfxFontGroup()
> +{
> +    mFonts.Clear();

Is this necessary? I realize it's preexisting code, you just moved it, but can we dispense with it entirely and let the nsTArray destructor clean up?

@@ +1671,5 @@
>      return false;
>  }
>  
> +gfxFont*
> +gfxFontGroup::GetFontAt(int32_t i) {

Line-break before the opening brace, please.

@@ +1713,3 @@
>  {
> +    if (mDefaultFont) {
> +        return mDefaultFont;

Maybe explictly write mDefaultFont.get(), for clarity? (And like other return statements that return a gfxFont* from an nsRefPtr variable.)

@@ +1772,5 @@
> +            continue;
> +        }
> +
> +        // already have a font?
> +        nsRefPtr<gfxFont> font = ff.Font();

I think you can use a raw pointer rather than needing an nsRefPtr here (and thus avoid the overhead of an addref/release pair).

@@ +1777,5 @@
> +        if (font) {
> +            return font.get();
> +        }
> +
> +        // need to build a font but avoid loading a userfont if not loaded

I don't understand how this comment relates to the code below, which appears to explicitly call ufe->Load() if we have a non-loaded user font.

@@ +2606,5 @@
>          }
>      }
>  
> +    if (fontListLength == 0) {
> +        nsRefPtr<gfxFont> defaultFont = GetDefaultFont();

Use raw ptr.

::: gfx/thebes/gfxTextRun.h
@@ +731,5 @@
>  
>      virtual ~gfxFontGroup();
>  
> +    // returns first valid font in the fontlist or default font
> +    // does not initiate userfont loads if userfont not loaded

There are really two distinct comments here; please provide capitalization/punctuation so that it's clearer to read.

@@ +1009,5 @@
>      mozilla::FontFamilyList mFamilyList;
> +
> +    // fontlist containing a font entry for each family found. gfxFont objects
> +    // are created as needed and userfont loads are initiated when needed.
> +    // code should be careful about addressing this array directly.

Sentence capitalization for these multi-line comments, please.

@@ +1052,5 @@
>      void BuildFontList();
>  
> +    // get the font at index i within the fontlist
> +    // will initiate userfont load if not already loaded
> +    // may return null if userfont not loaded or if font invalid

Ditto.

::: gfx/thebes/gfxUserFontSet.h
@@ +190,5 @@
>      gfxUserFontFamily* LookupFamily(const nsAString& aName) const;
>  
> +    // Lookup a userfont entry for a given style, loaded or not
> +    // aFamily must be a family returned by our LookupFamily method.
> +    // if only invalid fonts in family, returns null

Sentence capitalization & periods.

@@ +224,5 @@
> +    void IncrementGeneration(bool aIsRebuild = false);
> +
> +    // generation is bumped on font loads but that doesn't affect name-style
> +    // mappings. rebuilds do however affect name-style mappings so need to
> +    // lookup fontlists again when that happens

Ditto.

Comment 28

[Jonathan Kew [:jfkthame]]

r=me for these, but please do have a look at the comments above (many of them re code comments rather than actual code) and tidy up before the patches actually land.

Comment 29

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #28)
> r=me for these, but please do have a look at the comments above (many of
> them re code comments rather than actual code) and tidy up before the
> patches actually land.

Will do.

Comment 30

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #27)

> @@ +2606,5 @@
> >          }
> >      }
> >  
> > +    if (fontListLength == 0) {
> > +        nsRefPtr<gfxFont> defaultFont = GetDefaultFont();
> 
> Use raw ptr.

This one needs to be addref'd before it's passed back so it's better in this case to use nsRefPtr, it's consistent with the surrounding code.

Comment 31

[John Daggett (:jtd)]

Pushed to inbound with changes based on review comments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb5151c8052
https://hg.mozilla.org/integration/mozilla-inbound/rev/650040d02bd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a734f91ec4
https://hg.mozilla.org/integration/mozilla-inbound/rev/140c2b5a89ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/a07df88f972e

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eeb5151c8052
https://hg.mozilla.org/mozilla-central/rev/650040d02bd0
https://hg.mozilla.org/mozilla-central/rev/76a734f91ec4
https://hg.mozilla.org/mozilla-central/rev/140c2b5a89ff
https://hg.mozilla.org/mozilla-central/rev/a07df88f972e