Bug 153023 – Cleanup in font loading code

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Cleanup in font loading code

[Myles C. Maxfield]

Created attachment 268783 [details]
Patch

[Myles C. Maxfield]

Created attachment 268794 [details]
Patch

[Myles C. Maxfield]

Committed r194923: <http://trac.webkit.org/changeset/194923>

[Said Abou-Hallawa]

Comment on attachment 268794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268794&action=review

> Source/WebCore/css/CSSFontFace.cpp:60
>  void CSSFontFace::addedToSegmentedFontFace(CSSSegmentedFontFace* segmentedFontFace)

The name of this function is a little bit confusing. We have a CSSSegmentedFontFace and we are calling addToSegmentedFontFace to add it to a collection of CSSSegmentedFontFaces. right? Would not be clearer if this function is called

void CSSFontFace::addedToSegmentedFontFaces(CSSSegmentedFontFace* segmentedFontFace)

Since we are adding a singular to plural.

> Source/WebCore/css/CSSFontFace.cpp:62
> +    m_segmentedFontFace.add(segmentedFontFace);

I do not think this change makes the code clearer. m_segmentedFontFaces is a HashSet<CSSSegmentedFontFace*> which is a collection of CSSSegmentedFontFaces. How can it be called m_segmentedFontFace and it has a function to add a CSSSegmentedFontFace?

> Source/WebCore/css/CSSFontSelector.cpp:451
> +        }

Would not be cleaner if this loop is written like this:

for (unsigned w = 1 << FontWeight100Bit; !(m_desiredTraitsMaskForComparison & w); w <<= 1)
    ruleSetIndex++;

> Source/WebCore/css/CSSFontSelector.cpp:453
> +        ASSERT_WITH_SECURITY_IMPLICATION(ruleSetIndex < fallbackRuleSets);

It is is weird to have this assertion here. Can't we check at the beginning of this function/class that m_desiredTraitsMaskForComparison will not end up firing this assertion; something like

unsigned mask = (1 << fallbackRuleSets) - 1;
if (!(m_desiredTraitsMaskForComparison & mask)) {
   ASSERT(false);
   return false;
}

[Myles C. Maxfield]

Comment on attachment 268794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268794&action=review

>> Source/WebCore/css/CSSFontFace.cpp:60
>>  void CSSFontFace::addedToSegmentedFontFace(CSSSegmentedFontFace* segmentedFontFace)
> 
> The name of this function is a little bit confusing. We have a CSSSegmentedFontFace and we are calling addToSegmentedFontFace to add it to a collection of CSSSegmentedFontFaces. right? Would not be clearer if this function is called
> 
> void CSSFontFace::addedToSegmentedFontFaces(CSSSegmentedFontFace* segmentedFontFace)
> 
> Since we are adding a singular to plural.

I think the existing name makes sense. The CSSSegmentedFontFace is a higher level object which can be thought of to own these CSSFontFace objects (though a particular CSSFontFace may be "owned" by multiple CSSSegmentedFontFaces). This function represents the handshake between when an owner gains an ownee. When this function is called, the lower-level object is being registered as having a new higher-level object which refers to itself. So, it is being added to the SegmentedFontFace.

>> Source/WebCore/css/CSSFontFace.cpp:62
>> +    m_segmentedFontFace.add(segmentedFontFace);
> 
> I do not think this change makes the code clearer. m_segmentedFontFaces is a HashSet<CSSSegmentedFontFace*> which is a collection of CSSSegmentedFontFaces. How can it be called m_segmentedFontFace and it has a function to add a CSSSegmentedFontFace?

Yes, you are right. I reverted this part of the patch before committing.

>> Source/WebCore/css/CSSFontSelector.cpp:451
>> +        }
> 
> Would not be cleaner if this loop is written like this:
> 
> for (unsigned w = 1 << FontWeight100Bit; !(m_desiredTraitsMaskForComparison & w); w <<= 1)
>     ruleSetIndex++;

I just un-indented this code, but I think you're right. I'll make a follow-up patch.

How about:
unsigned ruleSetIndex = 0;
for (; !(m_desiredTraitsMaskForComparison & (1 << (FontWeight100Bit + ruleSetIndex))); ++ruleSetIndex) { }

>> Source/WebCore/css/CSSFontSelector.cpp:453
>> +        ASSERT_WITH_SECURITY_IMPLICATION(ruleSetIndex < fallbackRuleSets);
> 
> It is is weird to have this assertion here. Can't we check at the beginning of this function/class that m_desiredTraitsMaskForComparison will not end up firing this assertion; something like
> 
> unsigned mask = (1 << fallbackRuleSets) - 1;
> if (!(m_desiredTraitsMaskForComparison & mask)) {
>    ASSERT(false);
>    return false;
> }

It seems to me that this could be simplified to just ASSERT_WITH_SECURITY_IMPLICATION(m_desiredTraitsMaskForComparison & FontWeightMask) up in the constructor.