1031206 - split out creation and addition of font faces in gfxUserFontSet/nsUserFontSet

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Cameron McCormack (:heycam)]

In bug 1028497 we'll need to be able to create a gfxProxyFontEntry in response to calling load() on a FontFace object in the DOM that isn't in the FontFaceSet.  Currently the methods on nsUserFontSet/gfxUserFontSet only allow creation and addition at the same time.  We can split the current methods apart.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Split out creation and addition of font faces in gfxUserFontSet.

This splits out gfxUserFontSet::AddFontFace into three parts:

  * gfxUserFontSet::MaybeCreateFontFace, which creates a new font face entry or
    returns an existing matching one in a given family, without adding it to
    the family
  * gfxUserFontSet::CreateFontFace, which just creates a new font face entry
    without regard to family
  * gfxUserFontSet::FindExistingProxyEntry, which does the work of looking for
    the existing match for MaybeCreateFontFace

nsUserFontSet::InsertRule now calls gfxUserFontSet::MaybeCreateFontFace followed by gfxUserFontSet::AddFontFace.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Split out creation of font faces in nsUserFontSet.

This splits out the font face creation (which is mostly the mapping of the descriptor values) from nsUserFontSet::InsertRule into a new function nsUserFontSet::MaybeCreateFontFaceFromRule.

Comment 3

[John Daggett (:jtd)]

Comment on attachment 8447036 [details] [diff] [review]
Part 1: Split out creation and addition of font faces in gfxUserFontSet.

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

r+ with name change noted below.

::: gfx/thebes/gfxUserFontSet.cpp
@@ +587,5 @@
> +                               const nsAString& aFamilyName,
> +                               const nsTArray<gfxFontFaceSrc>& aFontFaceSrcList,
> +                               uint32_t aWeight,
> +                               int32_t aStretch,
> +                               uint32_t aItalicStyle,

As noted in other bugs, I think the use of "Maybe" for method names is poor. In this case "FindOrCreateFontFace" is a more accurate reflection of what the method will do.

::: gfx/thebes/gfxUserFontSet.h
@@ +186,5 @@
> +    // matching entry on the family if there is one
> +    already_AddRefed<gfxProxyFontEntry> MaybeCreateFontFace(
> +                               const nsAString& aFamilyName,
> +                               const nsTArray<gfxFontFaceSrc>& aFontFaceSrcList,
> +                               uint32_t aWeight,

name change

::: layout/style/nsFontFaceLoader.cpp
@@ +729,3 @@
>      if (ruleRec.mFontEntry) {
> +      // Add the entry to the end of the list.  If an existing proxy entry was
> +      // returned by MaybeCreateFontFace that was already stored on the

name change

Comment 4

[John Daggett (:jtd)]

Comment on attachment 8447037 [details] [diff] [review]
Part 2: Split out creation of font faces in nsUserFontSet.

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

r+ with name changes

::: layout/style/nsFontFaceLoader.cpp
@@ +568,5 @@
>  
>    // this is a new rule:
> +  FontFaceRuleRecord ruleRec;
> +  ruleRec.mFontEntry =
> +    MaybeCreateFontFaceFromRule(fontfamily, aRule, aSheetType);

name change

@@ +578,5 @@
> +  ruleRec.mContainer.mRule = aRule;
> +  ruleRec.mContainer.mSheetType = aSheetType;
> +
> +  // Add the entry to the end of the list.  If an existing proxy entry was
> +  // returned by MaybeCreateFontFaceFromRule that was already stored on the

name change

@@ +678,3 @@
>      nsCSSValue::Array* srcArr = val.GetArrayValue();
>      size_t numSrc = srcArr->Count();
>      

trim excess white space

::: layout/style/nsFontFaceLoader.h
@@ +70,5 @@
>                    nsTArray<FontFaceRuleRecord>& oldRules,
>                    bool& aFontSetModified);
>  
> +  already_AddRefed<gfxFontEntry> MaybeCreateFontFaceFromRule(
> +                                                   const nsAString& aFamilyName,

name change

Comment 5

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=52b9a2cf20a9

Comment 6

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b37f7f3e377
https://hg.mozilla.org/integration/mozilla-inbound/rev/601963f474d8

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b37f7f3e377
https://hg.mozilla.org/mozilla-central/rev/601963f474d8