Moving dirty properties to ElementStyle

Include/Rocket/Core/Element.h

@@ -61,6 +61,7 @@ class FontFaceHandle;
 class PropertyDictionary;
 class RenderInterface;
 class StyleSheet;
+class DirtyPropertyList;
 struct ElementMeta;
 
 /**
@@ -602,9 +603,7 @@ protected:
 	/// @param[in] changed_properties The properties changed on the element.
 	virtual void OnPropertyChange(const PropertyNameList& changed_properties);
 
-	void DirtyAllProperties() { all_properties_dirty = true; }
-	void DirtyProperties(const PropertyNameList& changed_properties);
-	void UpdateDirtyProperties();
+	void UpdateDirtyProperties(const DirtyPropertyList& dirty_properties);
 
 	/// Called when a child node has been added up to two levels below us in the hierarchy.
 	/// @param[in] child The element that has been added. This may be this element.
@@ -737,8 +736,6 @@ private:
 	bool structure_dirty;
 	bool parent_structure_dirty;
 
-	PropertyNameList dirty_properties;
-	bool all_properties_dirty;
 	bool computed_values_are_default_initialized;
 	bool box_dirty;
 

Source/Core/Element.cpp

@@ -146,7 +146,6 @@ transform_state(), transform_state_perspective_dirty(true), transform_state_tran
 	structure_dirty = false;
 	parent_structure_dirty = false;
 
-	all_properties_dirty = true;
 	computed_values_are_default_initialized = true;
 	box_dirty = false;
 
@@ -218,7 +217,6 @@ void Element::Update()
 	UpdateAnimation();
 	AdvanceAnimations();
 
-	if(all_properties_dirty || dirty_properties.size() > 0)
 	{
 		using namespace Style;
 		const ComputedValues* parent_values = (parent ? &parent->GetComputedValues() : nullptr);
@@ -230,15 +228,16 @@ void Element::Update()
 			if (auto context = doc->GetContext())
 				dp_ratio = context->GetDensityIndependentPixelRatio();
 		}
-		style->ComputeValues(element_meta->computed_values, parent_values, document_values, computed_values_are_default_initialized, dp_ratio);
+		auto dirty_properties = style->ComputeValues(element_meta->computed_values, parent_values, document_values, computed_values_are_default_initialized, dp_ratio);
 
 		computed_values_are_default_initialized = false;
+
+		// Computed values are calculated before OnPropertyChange in UpdateDirtyProperties, thus these can safely be used.
+		// However, new properties set during this call will not be available until the next update loop.
+		// Enable ROCKET_DEBUG to get a warning when this happens.
+		UpdateDirtyProperties(dirty_properties);
 	}
 
-	// Computed values are calculated before OnPropertyChange in UpdateDirtyProperties, thus these can safely be used.
-	// However, new properties set during this call will not be available until the next update loop.
-	// Enable ROCKET_DEBUG to get a warning when this happens.
-	UpdateDirtyProperties();
 
 	if (box_dirty)
 	{
@@ -1355,7 +1354,6 @@ void Element::AppendChild(Element* child, bool dom_element)
 	}
 
 	child->GetStyle()->DirtyDefinition();
-	all_properties_dirty = true;
 
 	Element* ancestor = child;
 	for (int i = 0; i <= ChildNotifyLevels && ancestor; i++, ancestor = ancestor->GetParentNode())
@@ -1402,7 +1400,6 @@ void Element::InsertBefore(Element* child, Element* adjacent_element)
 		children.insert(children.begin() + child_index, child);
 
 		child->GetStyle()->DirtyDefinition();
-		all_properties_dirty = true;
 
 		Element* ancestor = child;
 		for (int i = 0; i <= ChildNotifyLevels && ancestor; i++, ancestor = ancestor->GetParentNode())
@@ -1439,7 +1436,6 @@ bool Element::ReplaceChild(Element* inserted_element, Element* replaced_element)
 	RemoveChild(replaced_element);
 
 	inserted_element->GetStyle()->DirtyDefinition();
-	all_properties_dirty = true;
 
 	Element* ancestor = inserted_element;
 	for (int i = 0; i <= ChildNotifyLevels && ancestor; i++, ancestor = ancestor->GetParentNode())
@@ -1751,13 +1747,8 @@ void Element::OnPropertyChange(const PropertyNameList& changed_properties)
 		changed_properties.find(FONT_STYLE) != changed_properties.end() ||
 		changed_properties.find(FONT_SIZE) != changed_properties.end())
 	{
-		// Store the old em; if it changes, then we need to dirty all em-relative properties.
-		int old_em = -1;
-		if (font_face_handle != NULL)
-			old_em = font_face_handle->GetLineHeight();
-
 		// Fetch the new font face.
-		FontFaceHandle * new_font_face_handle = ElementUtilities::GetFontFaceHandle(element_meta->computed_values);
+		FontFaceHandle* new_font_face_handle = ElementUtilities::GetFontFaceHandle(element_meta->computed_values);
 
 		// If this is different from our current font face, then we've got to nuke
 		// all our characters and tell our parent that we have to be re-laid out.
@@ -1767,19 +1758,6 @@ void Element::OnPropertyChange(const PropertyNameList& changed_properties)
 				font_face_handle->RemoveReference();
 
 			font_face_handle = new_font_face_handle;
-
-			// Our font face has changed; odds are, so has our em. All of our em-relative values
-			// have therefore probably changed as well, so we'll need to dirty them.
-			int new_em = -1;
-			if (font_face_handle != NULL)
-				new_em = font_face_handle->GetLineHeight();
-
-			// However, if all properties are dirty, we don't need to perform this expensive
-			// step as all properties are dirtied below anyway.
-			if (old_em != new_em && !all_dirty)
-			{
-				style->DirtyEmProperties();
-			}
 		}
 		else if (new_font_face_handle != NULL)
 			new_font_face_handle->RemoveReference();
@@ -1793,6 +1771,7 @@ void Element::OnPropertyChange(const PropertyNameList& changed_properties)
 		changed_properties.find(TOP) != changed_properties.end() ||
 		changed_properties.find(BOTTOM) != changed_properties.end())
 	{
+		// TODO: This should happen during/after layout, as the containing box is not properly defined yet
 		UpdateOffset();
 		DirtyOffset();
 	}
@@ -1900,40 +1879,29 @@ void Element::OnPropertyChange(const PropertyNameList& changed_properties)
 	}
 }
 
-void Element::DirtyProperties(const PropertyNameList& changed_properties) 
-{ 
-	if (all_properties_dirty)
-		return;
-
-	dirty_properties.insert(changed_properties.begin(), changed_properties.end());
-}
-
-void Element::UpdateDirtyProperties()
+void Element::UpdateDirtyProperties(const DirtyPropertyList& dirty_properties)
 {
-	if (!all_properties_dirty && dirty_properties.empty())
+	if (dirty_properties.Empty())
 		return;
 
-	if(all_properties_dirty)
+	if(dirty_properties.AllDirty())
 	{
-		// Clear the dirty properties first, so that any new dirty properties during the call are properly added.
-		// They will not actually be evaluated until the next update loop. Thus, setting any properties here should be avoided.
-		all_properties_dirty = false;
-		dirty_properties.clear();
 		OnPropertyChange(StyleSheetSpecification::GetRegisteredProperties());
 	}
+	else if (dirty_properties.AnyInheritedDirty())
+	{
+		OnPropertyChange(StyleSheetSpecification::GetRegisteredInheritedProperties());
+	}
 	else
 	{
-		// Move the underlying dirty properties container to a temporary, so that we can fill it 
-		// with new dirty properties during OnPropertyChange.
-		PropertyNameList properties(std::move(dirty_properties));
-		dirty_properties.clear();
-		OnPropertyChange(properties);
+		OnPropertyChange(dirty_properties.GetList());
 	}
 
-#ifdef ROCKET_DEBUG
-	if (all_properties_dirty || !dirty_properties.empty())
-		Log::Message(Log::LT_WARNING, "One or more properties were set during OnPropertyChange, these will only be evaluated on the next update call and should be avoided.");
-#endif
+	// TODO: Add the following check back
+//#ifdef ROCKET_DEBUG
+//	if (all_properties_dirty || !dirty_properties.empty())
+//		Log::Message(Log::LT_WARNING, "One or more properties were set during OnPropertyChange, these will only be evaluated on the next update call and should be avoided.");
+//#endif
 }
 
 // Called when a child node has been added somewhere in the hierarchy

Source/Core/ElementStyle.cpp

@@ -49,10 +49,9 @@ namespace Rocket {
 namespace Core {
 
 
-ElementStyle::ElementStyle(Element* _element)
+ElementStyle::ElementStyle(Element* _element) : dirty_properties(true)
 {
 	local_properties = NULL;
-	em_properties = NULL;
 	definition = NULL;
 	element = _element;
 
@@ -63,8 +62,6 @@ ElementStyle::~ElementStyle()
 {
 	if (local_properties != NULL)
 		delete local_properties;
-	if (em_properties != NULL)
-		delete em_properties;
 
 	if (definition != NULL)
 		definition->RemoveReference();
@@ -211,6 +208,8 @@ void ElementStyle::UpdateDefinition()
 
 			definition = new_definition;
 			
+			// @performance: It may be faster to dirty all properties
+			//dirty_properties.SetAllDirty();
 			DirtyProperties(properties);
 			element->GetElementDecoration()->DirtyDecorators(true);
 		}
@@ -552,57 +551,10 @@ void ElementStyle::DirtyChildDefinitions()
 		element->GetChild(i)->GetStyle()->DirtyDefinition();
 }
 
-// Dirties every property.
-void ElementStyle::DirtyProperties()
-{
-	const PropertyNameList &properties = StyleSheetSpecification::GetRegisteredProperties();
-	DirtyProperties(properties);
-}
-
 // Dirties em-relative properties.
 void ElementStyle::DirtyEmProperties()
 {
-	if (!em_properties)
-	{
-		// Check if any of these are currently em-relative. If so, dirty them.
-		em_properties = new PropertyNameList;
-		for (auto& property : StyleSheetSpecification::GetRegisteredProperties())
-		{
-			// Skip font-size; this is relative to our parent's em, not ours.
-			if (property == FONT_SIZE)
-				continue;
-
-			// Get this property from this element. If this is em-relative, then add it to the list to
-			// dirty.
-			if (element->GetProperty(property)->unit == Property::EM)
-				em_properties->insert(property);
-		}
-	}
-
-	if (!em_properties->empty())
-		DirtyProperties(*em_properties, false);
-
-	// Now dirty all of our descendant's font-size properties that are relative to ems.
-	int num_children = element->GetNumChildren(true);
-	for (int i = 0; i < num_children; ++i)
-		element->GetChild(i)->GetStyle()->DirtyInheritedEmProperties();
-}
-
-// Dirties font-size on child elements if appropriate.
-void ElementStyle::DirtyInheritedEmProperties()
-{
-	const Property* font_size = element->GetLocalProperty(FONT_SIZE);
-	if (font_size == NULL)
-	{
-		int num_children = element->GetNumChildren(true);
-		for (int i = 0; i < num_children; ++i)
-			element->GetChild(i)->GetStyle()->DirtyInheritedEmProperties();
-	}
-	else
-	{
-		if (font_size->unit & Property::RELATIVE_UNIT)
-			DirtyProperty(FONT_SIZE);
-	}
+	dirty_properties.SetEmRelativeDirty();
 }
 
 // Dirties rem properties.
@@ -619,7 +571,7 @@ void ElementStyle::DirtyRemProperties()
 	}
 
 	if (!rem_properties.empty())
-		DirtyProperties(rem_properties, false);
+		DirtyProperties(rem_properties);
 
 	// Now dirty all of our descendant's properties that use the rem unit.
 	int num_children = element->GetNumChildren(true);
@@ -640,7 +592,7 @@ void ElementStyle::DirtyDpProperties()
 	}
 
 	if (!dp_properties.empty())
-		DirtyProperties(dp_properties, false);
+		DirtyProperties(dp_properties);
 
 	// Now dirty all of our descendant's properties that use the dp unit.
 	int num_children = element->GetNumChildren(true);
@@ -651,107 +603,38 @@ void ElementStyle::DirtyDpProperties()
 // Sets a single property as dirty.
 void ElementStyle::DirtyProperty(const String& property)
 {
-	PropertyNameList properties;
-	properties.insert(String(property));
-
-	DirtyProperties(properties);
+	dirty_properties.SetDirty(property);
 }
 
 // Sets a list of properties as dirty.
-void ElementStyle::DirtyProperties(const PropertyNameList& properties, bool clear_em_properties)
+void ElementStyle::DirtyProperties(const PropertyNameList& properties)
 {
-	if (properties.empty())
-		return;
-
-	bool all_inherited_dirty = 
-		&properties == &StyleSheetSpecification::GetRegisteredProperties() ||
-		StyleSheetSpecification::GetRegisteredProperties() == properties ||
-		StyleSheetSpecification::GetRegisteredInheritedProperties() == properties;
-
-
-	if (all_inherited_dirty)
-	{
-		const PropertyNameList &all_inherited_properties = StyleSheetSpecification::GetRegisteredInheritedProperties();
-		for (int i = 0; i < element->GetNumChildren(true); i++)
-			element->GetChild(i)->GetStyle()->DirtyInheritedProperties(all_inherited_properties);
-	}
-	else
-	{
-		PropertyNameList inherited_properties;
-
-		for (PropertyNameList::const_iterator i = properties.begin(); i != properties.end(); ++i)
-		{
-			// If this property is an inherited property, then push it into the list to be passed onto our children.
-			const PropertyDefinition* property = StyleSheetSpecification::GetProperty(*i);
-			if (property != NULL &&
-				property->IsInherited())
-				inherited_properties.insert(*i);
-		}
-
-		// Pass the list of those properties that are inherited onto our children.
-		if (!inherited_properties.empty())
-		{
-			for (int i = 0; i < element->GetNumChildren(true); i++)
-				element->GetChild(i)->GetStyle()->DirtyInheritedProperties(inherited_properties);
-		}
-	}
-
-	// clear the list of EM-properties, we will refill it in DirtyEmProperties
-	if (clear_em_properties && em_properties != NULL)
-	{
-		delete em_properties;
-		em_properties = NULL;
-	}
-
-	// And send the event.
-	element->DirtyProperties(properties);
+	dirty_properties.SetDirty(properties);
 }
 
 // Sets a list of our potentially inherited properties as dirtied by an ancestor.
 void ElementStyle::DirtyInheritedProperties(const PropertyNameList& properties)
 {
-	bool clear_em_properties = em_properties != NULL;
-
-	PropertyNameList inherited_properties;
-	for (PropertyNameList::const_iterator i = properties.begin(); i != properties.end(); ++i)
-	{
-		const Property *property = GetLocalProperty((*i));
-		if (property == NULL)
-		{
-			inherited_properties.insert(*i);
-			if (!clear_em_properties && em_properties != NULL && em_properties->find((*i)) != em_properties->end()) {
-				clear_em_properties = true;
-			}
-		}
-	}
-
-	if (inherited_properties.empty())
-		return;
-
-	// clear the list of EM-properties, we will refill it in DirtyEmProperties
-	if (clear_em_properties && em_properties != NULL)
-	{
-		delete em_properties;
-		em_properties = NULL;
-	}
-
-	// Pass the list of those properties that this element doesn't override onto our children.
-	for (int i = 0; i < element->GetNumChildren(true); i++)
-		element->GetChild(i)->GetStyle()->DirtyInheritedProperties(inherited_properties);
-
-	element->DirtyProperties(properties);
+	dirty_properties.SetDirty(properties);
+	return;
 }
 
 
 
-
-void ElementStyle::ComputeValues(Style::ComputedValues& values, const Style::ComputedValues* parent_values, const Style::ComputedValues* document_values, bool values_are_default_initialized, float dp_ratio)
+DirtyPropertyList ElementStyle::ComputeValues(Style::ComputedValues& values, const Style::ComputedValues* parent_values, const Style::ComputedValues* document_values, bool values_are_default_initialized, float dp_ratio)
 {
+	ROCKET_ASSERT_NONRECURSIVE;
+
+	if (dirty_properties.Empty())
+		return DirtyPropertyList();
+
 	// Generally, this is how it works (for now, we can probably be smarter about this):
 	//   1. Assign default values (clears any newly dirtied properties)
 	//   2. Inherit inheritable values from parent
 	//   3. Assign any local properties (from inline style or stylesheet)
 
+	const float font_size_before = values.font_size;
+	const float line_height_before = values.line_height.value;
 
 	// The next flag is just a small optimization, if the element was just created we don't need to copy all the default values.
 	if (!values_are_default_initialized)
@@ -760,27 +643,38 @@ void ElementStyle::ComputeValues(Style::ComputedValues& values, const Style::Com
 	}
 
 	// Always do font-size first if dirty, because of em-relative values
-	if (auto p = GetLocalProperty(FONT_SIZE))
-		values.font_size = ComputeFontsize(*p, values, parent_values, document_values, dp_ratio);
-	else if (parent_values)
-		values.font_size = parent_values->font_size;
+	{
+		if (auto p = GetLocalProperty(FONT_SIZE))
+			values.font_size = ComputeFontsize(*p, values, parent_values, document_values, dp_ratio);
+		else if (parent_values)
+			values.font_size = parent_values->font_size;
+		
+		if(font_size_before != values.font_size)
+			dirty_properties.SetEmRelativeDirty();
+	}
 
 	const float font_size = values.font_size;
 	const float document_font_size = (document_values ? document_values->font_size : DefaultComputedValues.font_size);
 
+
 	// Since vertical-align depends on line-height we compute this before iteration
-	if (auto p = GetLocalProperty(LINE_HEIGHT))
-	{
-		values.line_height = ComputeLineHeight(p, font_size, document_font_size, dp_ratio);
-	}
-	else if (parent_values)
 	{
-		// Line height has a special inheritance case for numbers/percent: they inherit them directly instead of computed length, but for lengths, they inherit the length.
-		// See CSS specs for details. Percent is already converted to number.
-		if (parent_values->line_height.inherit_type == Style::LineHeight::Number)
-			values.line_height = Style::LineHeight(font_size * parent_values->line_height.inherit_value, Style::LineHeight::Number, parent_values->line_height.inherit_value);
-		else
-			values.line_height = parent_values->line_height;
+		if (auto p = GetLocalProperty(LINE_HEIGHT))
+		{
+			values.line_height = ComputeLineHeight(p, font_size, document_font_size, dp_ratio);
+		}
+		else if (parent_values)
+		{
+			// Line height has a special inheritance case for numbers/percent: they inherit them directly instead of computed length, but for lengths, they inherit the length.
+			// See CSS specs for details. Percent is already converted to number.
+			if (parent_values->line_height.inherit_type == Style::LineHeight::Number)
+				values.line_height = Style::LineHeight(font_size * parent_values->line_height.inherit_value, Style::LineHeight::Number, parent_values->line_height.inherit_value);
+			else
+				values.line_height = parent_values->line_height;
+		}
+
+		if(line_height_before != values.line_height.value)
+			dirty_properties.SetDirty(VERTICAL_ALIGN);
 	}
 
 
@@ -973,8 +867,18 @@ void ElementStyle::ComputeValues(Style::ComputedValues& values, const Style::Com
 	}
 
 
+	if (dirty_properties.AnyInheritedDirty())
+	{
+		for (int i = 0; i < element->GetNumChildren(true); i++)
+		{
+			auto child = element->GetChild(i);
+			child->GetStyle()->dirty_properties.SetDirty(StyleSheetSpecification::GetRegisteredInheritedProperties());
+		}
+	}
 
-
+	DirtyPropertyList result(std::move(dirty_properties));
+	dirty_properties.Clear();
+	return result;
 }
 
 }

Source/Core/ElementStyle.h

@@ -34,6 +34,81 @@
 namespace Rocket {
 namespace Core {
 
+
+class DirtyPropertyList {
+private:
+	bool all_dirty = false;
+	bool inherited_dirty = false;
+	bool em_relative_dirty = false;
+	PropertyNameList dirty_list;
+
+	inline void CheckInheritance(const String& property_name) {
+		const auto& inherited_properties = StyleSheetSpecification::GetRegisteredInheritedProperties();
+		auto it = inherited_properties.find(property_name);
+		if (it != inherited_properties.end())
+			inherited_dirty = true;
+	}
+
+public:
+	DirtyPropertyList(bool all_dirty = false) : all_dirty(all_dirty) {}
+
+	void SetDirty(const String& property_name) {
+		if (all_dirty) return;
+		auto result = dirty_list.insert(property_name);
+		if (!inherited_dirty && result.second)
+			CheckInheritance(property_name);
+	}
+	void SetDirty(const PropertyNameList& properties) {
+		if (all_dirty) return;
+		// @performance: Can be made O(N+M)
+		dirty_list.insert(properties.begin(), properties.end());
+		for(size_t i = 0; i < properties.size() && !inherited_dirty; i++)
+			CheckInheritance(properties.container()[i]);
+	}
+	void SetAllDirty() {
+		Clear();
+		all_dirty = true;
+	}
+	void SetEmRelativeDirty() {
+		em_relative_dirty = true;
+	}
+
+	void Clear() {
+		all_dirty = false;
+		inherited_dirty = false;
+		em_relative_dirty = false;
+		dirty_list.clear();
+	}
+
+	bool Empty() const {
+		return !(all_dirty || inherited_dirty || em_relative_dirty || !dirty_list.empty());
+	}
+
+	bool IsDirty(const String & name) const {
+		if (all_dirty)
+			return true;
+		auto it = dirty_list.find(name);
+		return (it != dirty_list.end());
+	}
+
+	bool AllDirty() const {
+		return all_dirty;
+	}
+
+	bool IsEmRelativeDirty() const {
+		return (all_dirty || em_relative_dirty);
+	}
+
+	bool AnyInheritedDirty() const {
+		return (all_dirty || inherited_dirty);
+	}
+
+	const PropertyNameList& GetList() const {
+		return dirty_list;
+	}
+};
+
+
 /**
 	Manages an element's style and property information.
 	@author Lloyd Weehuizen
@@ -130,12 +205,8 @@ public:
 	/// Dirty all child definitions
 	void DirtyChildDefinitions();
 
-	// Dirties every property.
-	void DirtyProperties();
 	// Dirties em-relative properties.
 	void DirtyEmProperties();
-	// Dirties font-size on child elements if appropriate.
-	void DirtyInheritedEmProperties();
 	// Dirties rem properties.
 	void DirtyRemProperties();
 	// Dirties dp properties.
@@ -143,13 +214,13 @@ public:
 
 	/// Turns the local and inherited properties into computed values for this element. These values can in turn be used during the layout procedure.
 	/// Must be called in correct order, always parent before its children.
-	void ComputeValues(Style::ComputedValues& values, const Style::ComputedValues* parent_values, const Style::ComputedValues* document_values, bool values_are_default_initialized, float dp_ratio);
+	DirtyPropertyList ComputeValues(Style::ComputedValues& values, const Style::ComputedValues* parent_values, const Style::ComputedValues* document_values, bool values_are_default_initialized, float dp_ratio);
 
 private:
 	// Sets a single property as dirty.
 	void DirtyProperty(const String& property);
 	// Sets a list of properties as dirty.
-	void DirtyProperties(const PropertyNameList& properties, bool clear_em_properties = true);
+	void DirtyProperties(const PropertyNameList& properties);
 	// Sets a list of our potentially inherited properties as dirtied by an ancestor.
 	void DirtyInheritedProperties(const PropertyNameList& properties);
 
@@ -168,12 +239,12 @@ private:
 
 	// Any properties that have been overridden in this element.
 	PropertyDictionary* local_properties;
-	// All properties (including inherited) that are EM-relative.
-	PropertyNameList* em_properties;
 	// The definition of this element; if this is NULL one will be fetched from the element's style.
 	ElementDefinition* definition;
 	// Set if a new element definition should be fetched from the style.
 	bool definition_dirty;
+
+	DirtyPropertyList dirty_properties;
 };
 
 }