Cleanup in font loading code
Created attachment 268783 [details] Patch
Created attachment 268794 [details] Patch
Committed r194923: <http://trac.webkit.org/changeset/194923>
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; }
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.