Remove FontDescriptionChangeScope, and let FontBuilder partially apply values.

Source/core/css/resolver/FontBuilder.cpp

Index: Source/core/css/resolver/FontBuilder.cpp
diff --git a/Source/core/css/resolver/FontBuilder.cpp b/Source/core/css/resolver/FontBuilder.cpp
index cd475ba7e6c2836987a3e362bab5bed7bb7382e4..bfc0c4ba0a94262aa6b4f18b21138add6ba365d9 100644
--- a/Source/core/css/resolver/FontBuilder.cpp
+++ b/Source/core/css/resolver/FontBuilder.cpp
@@ -35,32 +35,9 @@
 
 namespace blink {
 
-// FIXME: This scoping class is a short-term fix to minimize the changes in
-// Font-constructing logic.
-class FontDescriptionChangeScope {
-    STACK_ALLOCATED();
-public:
-    FontDescriptionChangeScope(FontBuilder* fontBuilder)
-        : m_fontBuilder(fontBuilder)
-        , m_fontDescription(fontBuilder->fontDescription())
-    {
-    }
-
-    ~FontDescriptionChangeScope()
-    {
-        if (m_fontBuilder->fontDirty())
-            return;
-        m_fontBuilder->didChangeFontParameters(m_fontBuilder->fontDescription() != m_fontDescription);
-    }
-
-private:
-    RawPtrWillBeMember<FontBuilder> m_fontBuilder;
-    FontDescription m_fontDescription;
-};
-
 FontBuilder::FontBuilder(const Document& document)
     : m_document(document)
-    , m_fontDirty(false)
+    , m_flags(0)
 {
     ASSERT(document.frame());
 }
@@ -71,23 +48,26 @@ void FontBuilder::setInitial(float effectiveZoom)
     if (!m_document.settings())
         return;
 
-    FontDescriptionChangeScope scope(this);
-
-    m_fontDescription = FontDescription();
     setFamilyDescription(m_fontDescription, FontBuilder::initialFamilyDescription());
     setSize(m_fontDescription, FontBuilder::initialSize());
 }
 
-void FontBuilder::inheritFrom(const FontDescription& fontDescription)
+void FontBuilder::didChangeEffectiveZoom(bool changed)
 {
-    FontDescriptionChangeScope scope(this);
+    if (changed)
+        set(IsSetFlag::EffectiveZoom);
+}
 
-    m_fontDescription = fontDescription;
+void FontBuilder::didChangeTextOrientation(bool changed)
+{
+    if (changed)
+        set(IsSetFlag::TextOrientation);
 }
 
-void FontBuilder::didChangeFontParameters(bool changed)
+void FontBuilder::didChangeWritingMode(bool changed)
 {
-    m_fontDirty |= changed;
+    if (changed)
+        set(IsSetFlag::WritingMode);
 }
 
 FontFamily FontBuilder::standardFontFamily() const
@@ -131,35 +111,31 @@ AtomicString FontBuilder::genericFontFamilyName(FontDescription::GenericFamilyTy
 
 void FontBuilder::setFamilyDescription(const FontDescription::FamilyDescription& familyDescription)
 {
-    FontDescriptionChangeScope scope(this);
-
     setFamilyDescription(m_fontDescription, familyDescription);
 }
 
 void FontBuilder::setWeight(FontWeight fontWeight)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::Weight);
 
     m_fontDescription.setWeight(fontWeight);
 }
 
 void FontBuilder::setSize(const FontDescription::Size& size)
 {
-    FontDescriptionChangeScope scope(this);
-
     setSize(m_fontDescription, size);
 }
 
 void FontBuilder::setStretch(FontStretch fontStretch)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::Stretch);
 
     m_fontDescription.setStretch(fontStretch);
 }
 
 void FontBuilder::setScript(const String& locale)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::Script);
 
     m_fontDescription.setLocale(locale);
     m_fontDescription.setScript(localeToScriptCodeForFontSelection(locale));
@@ -167,76 +143,72 @@ void FontBuilder::setScript(const String& locale)
 
 void FontBuilder::setStyle(FontStyle italic)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::Style);
 
     m_fontDescription.setStyle(italic);
 }
 
 void FontBuilder::setVariant(FontVariant smallCaps)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::Variant);
 
     m_fontDescription.setVariant(smallCaps);
 }
 
 void FontBuilder::setVariantLigatures(const FontDescription::VariantLigatures& ligatures)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::VariantLigatures);
 
     m_fontDescription.setVariantLigatures(ligatures);
 }
 
 void FontBuilder::setTextRendering(TextRenderingMode textRenderingMode)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::TextRendering);
 
     m_fontDescription.setTextRendering(textRenderingMode);
 }
 
 void FontBuilder::setKerning(FontDescription::Kerning kerning)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::Kerning);
 
     m_fontDescription.setKerning(kerning);
 }
 
 void FontBuilder::setFontSmoothing(FontSmoothingMode foontSmoothingMode)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::FontSmoothing);
 
     m_fontDescription.setFontSmoothing(foontSmoothingMode);
 }
 
 void FontBuilder::setFeatureSettings(PassRefPtr<FontFeatureSettings> settings)
 {
-    FontDescriptionChangeScope scope(this);
+    set(IsSetFlag::FeatureSettings);
 
     m_fontDescription.setFeatureSettings(settings);
 }
 
 void FontBuilder::setFamilyDescription(FontDescription& fontDescription, const FontDescription::FamilyDescription& familyDescription)
 {
-    FixedPitchFontType oldFixedPitchFontType = fontDescription.fixedPitchFontType();
+    set(IsSetFlag::Family);
 
     bool isInitial = familyDescription.genericFamily == FontDescription::StandardFamily && familyDescription.family.familyIsEmpty();
 
     fontDescription.setGenericFamily(familyDescription.genericFamily);
     fontDescription.setFamily(isInitial ? standardFontFamily() : familyDescription.family);
-
-    if (fontDescription.keywordSize() && fontDescription.fixedPitchFontType() != oldFixedPitchFontType)
-        setSize(fontDescription, FontDescription::Size(fontDescription.keywordSize(), 0.0f, false));
 }
 
 void FontBuilder::setSize(FontDescription& fontDescription, const FontDescription::Size& size)
 {
     float specifiedSize = size.value;
 
-    if (!specifiedSize && size.keyword)
-        specifiedSize = FontSize::fontSizeForKeyword(&m_document, size.keyword, fontDescription.fixedPitchFontType());
-
     if (specifiedSize < 0)
         return;
 
+    set(IsSetFlag::Size);
+
     // Overly large font sizes will cause crashes on some platforms (such as Windows).
     // Cap font size here to make sure that doesn't happen.
     specifiedSize = std::min(maximumAllowedFontSize, specifiedSize);
@@ -295,35 +267,27 @@ static void getFontAndGlyphOrientation(const RenderStyle* style, FontOrientation
     }
 }
 
-void FontBuilder::checkForOrientationChange(RenderStyle* style)
+void FontBuilder::updateOrientation(FontDescription& description, RenderStyle* style)
 {
     FontOrientation fontOrientation;
     NonCJKGlyphOrientation glyphOrientation;
     getFontAndGlyphOrientation(style, fontOrientation, glyphOrientation);
 
-    FontDescriptionChangeScope scope(this);
-
-    if (m_fontDescription.orientation() == fontOrientation && m_fontDescription.nonCJKGlyphOrientation() == glyphOrientation)
-        return;
-
-    m_fontDescription.setNonCJKGlyphOrientation(glyphOrientation);
-    m_fontDescription.setOrientation(fontOrientation);
+    description.setNonCJKGlyphOrientation(glyphOrientation);
+    description.setOrientation(fontOrientation);
 }
 
-void FontBuilder::checkForGenericFamilyChange(RenderStyle* style, const RenderStyle* parentStyle)
+void FontBuilder::checkForGenericFamilyChange(const FontDescription& oldDescription, FontDescription& newDescription)
 {
-    FontDescriptionChangeScope scope(this);
-
-    if (m_fontDescription.isAbsoluteSize() || !parentStyle)
+    if (newDescription.isAbsoluteSize())
         return;
 
-    const FontDescription& parentFontDescription = parentStyle->fontDescription();
-    if (m_fontDescription.fixedPitchFontType() == parentFontDescription.fixedPitchFontType())
+    if (newDescription.fixedPitchFontType() == oldDescription.fixedPitchFontType())
         return;
 
     // For now, lump all families but monospace together.
-    if (m_fontDescription.genericFamily() != FontDescription::MonospaceFamily
-        && parentFontDescription.genericFamily() != FontDescription::MonospaceFamily)
+    if (newDescription.genericFamily() != FontDescription::MonospaceFamily
+        && oldDescription.genericFamily() != FontDescription::MonospaceFamily)
         return;
 
     // We know the parent is monospace or the child is monospace, and that font
@@ -331,26 +295,31 @@ void FontBuilder::checkForGenericFamilyChange(RenderStyle* style, const RenderSt
     // If the font uses a keyword size, then we refetch from the table rather than
     // multiplying by our scale factor.
     float size;
-    if (m_fontDescription.keywordSize()) {
-        size = FontSize::fontSizeForKeyword(&m_document, m_fontDescription.keywordSize(), m_fontDescription.fixedPitchFontType());
+    if (newDescription.keywordSize()) {
+        size = FontSize::fontSizeForKeyword(&m_document, newDescription.keywordSize(), newDescription.fixedPitchFontType());
     } else {
         Settings* settings = m_document.settings();
         float fixedScaleFactor = (settings && settings->defaultFixedFontSize() && settings->defaultFontSize())
             ? static_cast<float>(settings->defaultFixedFontSize()) / settings->defaultFontSize()
             : 1;
-        size = parentFontDescription.fixedPitchFontType() == FixedPitchFont ?
-            m_fontDescription.specifiedSize() / fixedScaleFactor :
-            m_fontDescription.specifiedSize() * fixedScaleFactor;
+        size = oldDescription.fixedPitchFontType() == FixedPitchFont ?
+            newDescription.specifiedSize() / fixedScaleFactor :
+            newDescription.specifiedSize() * fixedScaleFactor;
     }
 
-    m_fontDescription.setSpecifiedSize(size);
-    updateComputedSize(m_fontDescription, style);
+    newDescription.setSpecifiedSize(size);
 }
 
-void FontBuilder::updateComputedSize(RenderStyle* style, const RenderStyle* parentStyle)
+void FontBuilder::updateSpecifiedSize(FontDescription& fontDescription, RenderStyle* style)
 {
-    FontDescriptionChangeScope scope(this);
-    updateComputedSize(m_fontDescription, style);
+    float specifiedSize = fontDescription.specifiedSize();
+
+    if (!specifiedSize && fontDescription.keywordSize())
+        specifiedSize = FontSize::fontSizeForKeyword(&m_document, fontDescription.keywordSize(), fontDescription.fixedPitchFontType());
+
+    fontDescription.setSpecifiedSize(specifiedSize);
+
+    checkForGenericFamilyChange(style->fontDescription(), fontDescription);
 }
 
 void FontBuilder::updateComputedSize(FontDescription& fontDescription, RenderStyle* style)
@@ -362,17 +331,53 @@ void FontBuilder::updateComputedSize(FontDescription& fontDescription, RenderSty
     fontDescription.setComputedSize(computedSize);
 }
 
-void FontBuilder::createFont(PassRefPtrWillBeRawPtr<FontSelector> fontSelector, RenderStyle* style, const RenderStyle* parentStyle)
+void FontBuilder::createFont(PassRefPtrWillBeRawPtr<FontSelector> fontSelector, RenderStyle* style)
 {
-    if (!m_fontDirty)
+    if (!m_flags)
         return;
 
-    updateComputedSize(style, parentStyle);
-    checkForGenericFamilyChange(style, parentStyle);
-    checkForOrientationChange(style);
-    style->setFontDescription(m_fontDescription);
+    FontDescription description = style->fontDescription();
+
+    if (isSet(IsSetFlag::Family)) {

[andersr, 2014/12/18 13:01:59]

Note: I've verified with a microbenchmark that all these 'if' statements do not
degrade font building performance measurably. (Actually, it increased slightly).

+        description.setGenericFamily(m_fontDescription.genericFamily());
+        description.setFamily(m_fontDescription.family());
+    }
+    if (isSet(IsSetFlag::Size)) {
+        description.setKeywordSize(m_fontDescription.keywordSize());
+        description.setSpecifiedSize(m_fontDescription.specifiedSize());
+        description.setIsAbsoluteSize(m_fontDescription.isAbsoluteSize());
+    }
+    if (isSet(IsSetFlag::Weight))
+        description.setWeight(m_fontDescription.weight());
+    if (isSet(IsSetFlag::Stretch))
+        description.setStretch(m_fontDescription.stretch());
+    if (isSet(IsSetFlag::FeatureSettings))
+        description.setFeatureSettings(m_fontDescription.featureSettings());
+    if (isSet(IsSetFlag::Script)) {
+        description.setLocale(m_fontDescription.locale());
+        description.setScript(m_fontDescription.script());
+    }
+    if (isSet(IsSetFlag::Style))
+        description.setStyle(m_fontDescription.style());
+    if (isSet(IsSetFlag::Variant))
+        description.setVariant(m_fontDescription.variant());
+    if (isSet(IsSetFlag::VariantLigatures))
+        description.setVariantLigatures(m_fontDescription.variantLigatures());
+    if (isSet(IsSetFlag::TextRendering))
+        description.setTextRendering(m_fontDescription.textRendering());
+    if (isSet(IsSetFlag::Kerning))
+        description.setKerning(m_fontDescription.kerning());
+    if (isSet(IsSetFlag::FontSmoothing))
+        description.setFontSmoothing(m_fontDescription.fontSmoothing());
+    if (isSet(IsSetFlag::TextOrientation) || isSet(IsSetFlag::WritingMode))
+        updateOrientation(description, style);
+
+    updateSpecifiedSize(description, style);
+    updateComputedSize(description, style);
+
+    style->setFontDescription(description);
     style->font().update(fontSelector);
-    m_fontDirty = false;
+    m_flags = 0;
 }
 
 void FontBuilder::createFontForDocument(PassRefPtrWillBeRawPtr<FontSelector> fontSelector, RenderStyle* documentStyle)
@@ -383,6 +388,7 @@ void FontBuilder::createFontForDocument(PassRefPtrWillBeRawPtr<FontSelector> fon
 
     setFamilyDescription(fontDescription, FontBuilder::initialFamilyDescription());
     setSize(fontDescription, FontDescription::Size(FontSize::initialKeywordSize(), 0.0f, false));
+    updateSpecifiedSize(fontDescription, documentStyle);
     updateComputedSize(fontDescription, documentStyle);
 
     FontOrientation fontOrientation;