Some cleanup of decorator instancing.

Decorator properties can now be given as either DecoratorList or String. When loading a stylesheet they are first loaded directly as String, but converted to DecoratorList (that is, the specified decorators are instanced) when the stylesheet is applied to an ElementDocument. We can't do it before this because we may reference a decorator or sprite located in another stylesheet, and we shouldn't do it later (e.g. during ComputeValues()) for performance reasons. This way, we avoid instancing new decorators every time we instance a new element.
If a decorator is set on the style of an element, the decorators are instanced during ElementStyle::ComputeValues().

Include/Rocket/Core/ComputedValues.h

@@ -199,7 +199,7 @@ struct ComputedValues
 	TransitionList transition;
 	AnimationList animation;
 
-	// Decorators and font-effects are not computed, retrieve from local property
+	DecoratorList decorator;
 };
 }
 

Include/Rocket/Core/StyleSheet.h

@@ -61,7 +61,7 @@ struct DecoratorSpecification {
 
 struct Sprite {
 	Rectangle rectangle;
-	Spritesheet* sprite_sheet;
+	const Spritesheet* sprite_sheet;
 };
 
 struct Spritesheet {
@@ -201,8 +201,9 @@ public:
 
 	/// Combines this style sheet with another one, producing a new sheet.
 	StyleSheet* CombineStyleSheet(const StyleSheet* sheet) const;
-	/// Builds the node index for a combined style sheet.
-	void BuildNodeIndex();
+	/// Builds the node index for a combined style sheet, and optimizes some properties for faster retrieval.
+	/// Specifically, converts all decorator properties from strings to instanced decorator lists.
+	void BuildNodeIndexAndOptimizeProperties();
 
 	/// Returns the Keyframes of the given name, or null if it does not exist.
 	Keyframes* GetKeyframes(const String& name);
@@ -210,9 +211,11 @@ public:
 	/// Returns the Decorator of the given name, or null if it does not exist.
 	std::shared_ptr<Decorator> GetDecorator(const String& name) const;
 
-	const Sprite* GetSprite(const String& name) const;
+	/// Parses the decorator property from a string and returns a list of instanced decorators
+	DecoratorList InstanceDecoratorsFromString(const String& decorator_string_value, const String& source_file, int source_line_number) const;
 
-	std::shared_ptr<Decorator> GetOrInstanceDecorator(const String& decorator_value, const String& source_file, int source_line_number);
+	/// Get sprite located in any spritesheet within this stylesheet
+	const Sprite* GetSprite(const String& name) const;
 
 	/// Returns the compiled element definition for a given element hierarchy. A reference count will be added for the
 	/// caller, so another should not be added. The definition should be released by removing the reference count.

Include/Rocket/Core/Variant.h

@@ -78,6 +78,7 @@ public:
 		TRANSFORMREF = 't',
 		TRANSITIONLIST = 'T',
 		ANIMATIONLIST = 'A',
+		DECORATORLIST = 'D',
 		VOIDPTR = '*',			
 	};
 
@@ -134,6 +135,7 @@ private:
 	void Set(const TransformRef& value);
 	void Set(const TransitionList& value);
 	void Set(const AnimationList& value);
+	void Set(const DecoratorList& value);
 	void Set(const Colourf& value);
 	void Set(const Colourb& value);
 	void Set(ScriptInterface* value);

Include/Rocket/Core/Variant.inl

@@ -102,6 +102,10 @@ bool Variant::GetInto(T& value) const
 		return TypeConverter< AnimationList, T >::Convert(*(AnimationList*)data, value);
 		break;
 
+	case DECORATORLIST:
+		return TypeConverter< DecoratorList, T >::Convert(*(DecoratorList*)data, value);
+		break;
+
 	case COLOURF:
 		return TypeConverter< Colourf, T >::Convert(*(Colourf*)data, value);
 		break;

Source/Core/ElementDecoration.cpp

@@ -50,27 +50,14 @@ bool ElementDecoration::ReloadDecorators()
 {
 	ReleaseDecorators();
 
-	StyleSheet* stylesheet = element->GetStyleSheet();
-	if (!stylesheet)
-		return true;
+	const DecoratorList& decorator_list = element->GetComputedValues().decorator;
 
-	const Property* property = element->GetLocalProperty(PropertyId::Decorator);
-	if (!property)
-		return true;
-
-	String decorator_value = property->Get<String>();
-
-	// @performance: Can optimize for the case of only one decorator
-	StringList decorator_list;
-	// We use custom quote characters as we don't want to split on any commas inside parenthesis, which may appear in decorator shorthands.
-	StringUtilities::ExpandString(decorator_list, decorator_value, ',', '(', ')');
-
-	for (const String& name : decorator_list)
+	for (const auto& decorator : decorator_list)
 	{
-		std::shared_ptr<Decorator> decorator = stylesheet->GetOrInstanceDecorator(name, property->source, property->source_line_number);
-
 		if (decorator)
-			LoadDecorator(std::move(decorator));
+		{
+			LoadDecorator(decorator);
+		}
 	}
 
 	return true;

Source/Core/ElementDecoration.h

@@ -70,13 +70,13 @@ private:
 		DecoratorDataHandle decorator_data;
 	};
 
-	typedef std::vector< DecoratorHandle > DecoratorList;
+	typedef std::vector< DecoratorHandle > DecoratorHandleList;
 
 	// The element this decorator belongs to
 	Element* element;
 
 	// The list of every decorator used by this element in every class.
-	DecoratorList decorators;
+	DecoratorHandleList decorators;
 
 	// If set, a full reload is necessary
 	bool decorators_dirty;

Source/Core/ElementDefinition.cpp

@@ -44,7 +44,7 @@ ElementDefinition::~ElementDefinition()
 }
 
 // Initialises the element definition from a list of style sheet nodes.
-void ElementDefinition::Initialise(const std::vector< const StyleSheetNode* >& style_sheet_nodes, const PseudoClassList& volatile_pseudo_classes, bool _structurally_volatile, const StyleSheet& style_sheet)
+void ElementDefinition::Initialise(const std::vector< const StyleSheetNode* >& style_sheet_nodes, const PseudoClassList& volatile_pseudo_classes, bool _structurally_volatile)
 {
 	// Set the volatile structure flag.
 	structurally_volatile = _structurally_volatile;
@@ -105,13 +105,6 @@ void ElementDefinition::Initialise(const std::vector< const StyleSheetNode* >& s
 			}
 		}
 	}
-
-	// Turn the decorator properties from String to DecoratorList.
-	// This is essentially an optimization, we could do this just as well in e.g. ComputeValues() or ElementDecoration, but when we do it here, we only need to do it once.
-	// Note, since the user may set a new decorator through its style, we we need to do it again in ComputeValues.
-
-
-
 }
 
 // Returns a specific property from the element definition's base properties.

Source/Core/ElementDefinition.h

@@ -55,7 +55,7 @@ public:
 	virtual ~ElementDefinition();
 
 	/// Initialises the element definition from a list of style sheet nodes.
-	void Initialise(const std::vector< const StyleSheetNode* >& style_sheet_nodes, const PseudoClassList& volatile_pseudo_classes, bool structurally_volatile, const StyleSheet& style_sheet);
+	void Initialise(const std::vector< const StyleSheetNode* >& style_sheet_nodes, const PseudoClassList& volatile_pseudo_classes, bool structurally_volatile);
 
 	/// Returns a specific property from the element definition's base properties.
 	/// @param[in] name The name of the property to return.

Source/Core/ElementDocument.cpp

@@ -188,7 +188,7 @@ void ElementDocument::SetStyleSheet(StyleSheet* _style_sheet)
 	if (style_sheet != NULL)
 	{
 		style_sheet->AddReference();
-		style_sheet->BuildNodeIndex();
+		style_sheet->BuildNodeIndexAndOptimizeProperties();
 	}
 
 	GetStyle()->DirtyDefinition();

Source/Core/ElementStyle.cpp

@@ -939,9 +939,24 @@ DirtyPropertyList ElementStyle::ComputeValues(Style::ComputedValues& values, con
 			values.animation = p->Get<AnimationList>();
 			break;
 
-		// Decorators and font-effects are not computed, they are specified as they are given.
 		case PropertyId::Decorator:
+			values.decorator.clear();
+			// Usually the decorator is converted from string after the style sheet is set on the ElementDocument. However, if the
+			// user sets a decorator on the element's style, we may still get a string here which must be parsed and instanced.
+			if (p->unit == Property::DECORATOR)
+			{
+				values.decorator = p->Get<DecoratorList>();
+			}
+			else if (p->unit == Property::STRING)
+			{
+				if(const StyleSheet* style_sheet = GetStyleSheet())
+				{
+					String value = p->Get<String>();
+					values.decorator = style_sheet->InstanceDecoratorsFromString(value, p->source, p->source_line_number);
+				}
+			}
 			break;
+
 		case PropertyId::FontEffect:
 			break;
 		}

Source/Core/PropertyDictionary.cpp

@@ -58,7 +58,7 @@ const Property* PropertyDictionary::GetProperty(PropertyId id) const
 {
 	PropertyMap::const_iterator iterator = properties.find(id);
 	if (iterator == properties.end())
-		return NULL;
+		return nullptr;
 
 	return &(*iterator).second;
 }

Source/Core/StyleSheet.cpp

@@ -112,14 +112,14 @@ StyleSheet* StyleSheet::CombineStyleSheet(const StyleSheet* other_sheet) const
 }
 
 // Builds the node index for a combined style sheet.
-void StyleSheet::BuildNodeIndex()
+void StyleSheet::BuildNodeIndexAndOptimizeProperties()
 {
 	if (complete_node_index.empty())
 	{
 		styled_node_index.clear();
 		complete_node_index.clear();
 
-		root->BuildIndex(styled_node_index, complete_node_index);
+		root->BuildIndexAndOptimizeProperties(styled_node_index, complete_node_index, *this);
 	}
 }
 
@@ -145,59 +145,82 @@ const Sprite* StyleSheet::GetSprite(const String& name) const
 	return spritesheet_list.GetSprite(name);
 }
 
-std::shared_ptr<Decorator> StyleSheet::GetOrInstanceDecorator(const String& decorator_value, const String& source_file, int source_line_number)
+DecoratorList StyleSheet::InstanceDecoratorsFromString(const String& decorator_string_value, const String& source_file, int source_line_number) const
 {
-	// Try to find a decorator declared with @decorator or otherwise previously instanced shorthand decorator.
-	auto it_find = decorator_map.find(decorator_value);
-	if (it_find != decorator_map.end())
-	{
-		return it_find->second.decorator;
-	}
+	// Decorators are declared as
+	//   decorator: <decorator-value>[, <decorator-value> ...];
+	// Where <decorator-value> is either a @decorator name:
+	//   decorator: invader-theme-background, ...;
+	// or is an anonymous decorator with inline properties
+	//   decorator: tiled-box( <shorthand properties> ), ...;
+	
+	DecoratorList decorator_list;
+	if (decorator_string_value.empty() || decorator_string_value == NONE)
+		return decorator_list;
 
-	// The decorator does not exist, try to instance a new one from a shorthand decorator value declared as:
-	//   decorator: <type>( <shorthand> );
-	// where <type> is the decorator type and the value <shorthand> is applied to its "decorator"-shorthand.
+	// Make sure we don't split inside the parenthesis since they may appear in decorator shorthands.
+	StringList decorator_string_list;
+	StringUtilities::ExpandString(decorator_string_list, decorator_string_value, ',', '(', ')');
 
-	// Check syntax
-	size_t shorthand_open = decorator_value.find('(');
-	size_t shorthand_close = decorator_value.rfind(')');
-	if (shorthand_open == String::npos || shorthand_close == String::npos || shorthand_open >= shorthand_close)
-		return nullptr;
+	decorator_list.reserve(decorator_string_list.size());
 
-	String type = StringUtilities::StripWhitespace(decorator_value.substr(0, shorthand_open));
+	// Get or instance each decorator in the comma-separated string list
+	for (const String& decorator_string : decorator_string_list)
+	{
+		const size_t shorthand_open = decorator_string.find('(');
+		const size_t shorthand_close = decorator_string.rfind(')');
+		const bool invalid_parenthesis = (shorthand_open == String::npos || shorthand_close == String::npos || shorthand_open >= shorthand_close);
 
-	// Check for valid decorator type
-	const PropertySpecification* specification = Factory::GetDecoratorPropertySpecification(type);
-	if (!specification)
-		return nullptr;
+		if (invalid_parenthesis)
+		{
+			// We found no parenthesis, that means the value must be a name of a @decorator rule, look it up
+			std::shared_ptr<Decorator> decorator = GetDecorator(decorator_string);
+			if (decorator)
+				decorator_list.emplace_back(std::move(decorator));
+			else
+				Log::Message(Log::LT_WARNING, "Decorator name '%s' could not be found in any @decorator rule, declared at %s:%d", decorator_string.c_str(), source_file.c_str(), source_line_number);
+		}
+		else
+		{
+			// Since we have parentheses it must be an anonymous decorator with inline properties
+			const String type = StringUtilities::StripWhitespace(decorator_string.substr(0, shorthand_open));
 
-	String shorthand = decorator_value.substr(shorthand_open + 1, shorthand_close - shorthand_open - 1);
+			// Check for valid decorator type
+			const PropertySpecification* specification = Factory::GetDecoratorPropertySpecification(type);
+			if (!specification)
+			{
+				Log::Message(Log::LT_WARNING, "Decorator type '%s' not found, declared at %s:%d", type.c_str(), source_file.c_str(), source_line_number);
+				continue;
+			}
 
-	// Parse the shorthand properties
-	PropertyDictionary properties;
-	if (!specification->ParsePropertyDeclaration(properties, "decorator", shorthand, source_file, source_line_number))
-	{
-		Log::Message(Log::LT_WARNING, "Could not parse decorator value '%s' at %s:%d", decorator_value.c_str(), source_file.c_str(), source_line_number);
-		return nullptr;
-	}
+			const String shorthand = decorator_string.substr(shorthand_open + 1, shorthand_close - shorthand_open - 1);
 
-	specification->SetPropertyDefaults(properties);
+			// Parse the shorthand properties
+			PropertyDictionary properties;
+			if (!specification->ParsePropertyDeclaration(properties, "decorator", shorthand, source_file, source_line_number))
+			{
+				Log::Message(Log::LT_WARNING, "Could not parse decorator value '%s' at %s:%d", decorator_string.c_str(), source_file.c_str(), source_line_number);
+				continue;
+			}
 
-	std::shared_ptr<Decorator> decorator = Factory::InstanceDecorator(type, properties, *this);
-	if (!decorator)
-		return nullptr;
+			specification->SetPropertyDefaults(properties);
 
-	// Insert decorator into map
-	auto result = decorator_map.emplace(decorator_value, DecoratorSpecification{ type, properties, decorator });
-	
-	if (!result.second)
-	{
-		return nullptr;
+			std::shared_ptr<Decorator> decorator = Factory::InstanceDecorator(type, properties, *this);
+
+			if (decorator)
+				decorator_list.emplace_back(std::move(decorator));
+			else
+			{
+				Log::Message(Log::LT_WARNING, "Decorator name '%s' could not be found in any @decorator rule, declared at %s:%d", decorator_string.c_str(), source_file.c_str(), source_line_number);
+				continue;
+			}
+		}
 	}
 
-	return decorator;
+	return decorator_list;
 }
 
+
 // Returns the compiled element definition for a given element hierarchy.
 ElementDefinition* StyleSheet::GetElementDefinition(const Element* element) const
 {
@@ -299,7 +322,7 @@ ElementDefinition* StyleSheet::GetElementDefinition(const Element* element) cons
 	// Create the new definition and add it to our cache. One reference count is added, bringing the total to two; one
 	// for the element that requested it, and one for the cache.
 	ElementDefinition* new_definition = new ElementDefinition();
-	new_definition->Initialise(applicable_nodes, volatile_pseudo_classes, structurally_volatile, *this);
+	new_definition->Initialise(applicable_nodes, volatile_pseudo_classes, structurally_volatile);
 
 	// Add to the address cache.
 //	address_cache[element_address] = new_definition;

Source/Core/StyleSheetNode.cpp

@@ -152,7 +152,7 @@ bool StyleSheetNode::MergeHierarchy(StyleSheetNode* node, int specificity_offset
 }
 
 // Builds up a style sheet's index recursively.
-void StyleSheetNode::BuildIndex(StyleSheet::NodeIndex& styled_index, StyleSheet::NodeIndex& complete_index)
+void StyleSheetNode::BuildIndexAndOptimizeProperties(StyleSheet::NodeIndex& styled_index, StyleSheet::NodeIndex& complete_index, const StyleSheet& style_sheet)
 {
 	// If this is a tag node, then we insert it into the list of all tag nodes. Makes sense, neh?
 	if (type == TAG)
@@ -181,12 +181,30 @@ void StyleSheetNode::BuildIndex(StyleSheet::NodeIndex& styled_index, StyleSheet:
 			else
 				(*iterator).second.insert(tag_node);
 		}
+
+		// Turn any decorator properties from String to DecoratorList.
+		// This is essentially an optimization, it will work fine to skip this step and let ElementStyle::ComputeValues() do all the work.
+		// However, when we do it here, we only need to do it once.
+		// Note, since the user may set a new decorator through its style, we still do the conversion as necessary again in ComputeValues.
+		if (const Property* property = properties.GetProperty(PropertyId::Decorator))
+		{
+			if (property->unit == Property::STRING)
+			{
+				const String string_value = property->Get<String>();
+				DecoratorList decorator_list = style_sheet.InstanceDecoratorsFromString(string_value, property->source, property->source_line_number);
+
+				Property new_property = *property;
+				new_property.value.Reset(std::move(decorator_list));
+				new_property.unit = Property::DECORATOR;
+				properties.SetProperty(PropertyId::Decorator, new_property);
+			}
+		}
 	}
 
 	for (int i = 0; i < NUM_NODE_TYPES; i++)
 	{
 		for (NodeMap::iterator j = children[i].begin(); j != children[i].end(); ++j)
-			(*j).second->BuildIndex(styled_index, complete_index);
+			(*j).second->BuildIndexAndOptimizeProperties(styled_index, complete_index, style_sheet);
 	}
 }
 

Source/Core/StyleSheetNode.h

@@ -70,8 +70,8 @@ public:
 
 	/// Merges an entire tree hierarchy into our hierarchy.
 	bool MergeHierarchy(StyleSheetNode* node, int specificity_offset = 0);
-	/// Builds up a style sheet's index recursively.
-	void BuildIndex(StyleSheet::NodeIndex& styled_index, StyleSheet::NodeIndex& complete_index);
+	/// Builds up a style sheet's index recursively and optimizes some properties for faster retrieval.
+	void BuildIndexAndOptimizeProperties(StyleSheet::NodeIndex& styled_index, StyleSheet::NodeIndex& complete_index, const StyleSheet& style_sheet);
 
 	/// Returns the name of this node.
 	const String& GetName() const;

Source/Core/StyleSheetParser.h

@@ -88,7 +88,7 @@ private:
 	// Attempts to parse a @keyframes block
 	bool ParseKeyframeBlock(KeyframesMap & keyframes_map, const String & identifier, const String & rules, const PropertyDictionary & properties);
 
-	// Attempts to parse a @keyframes block
+	// Attempts to parse a @decorator block
 	bool ParseDecoratorBlock(const String& at_name, DecoratorSpecificationMap& decorator_map, const StyleSheet& style_sheet);
 
 	// Attempts to find one of the given character tokens in the active stream

Source/Core/Variant.cpp

@@ -41,6 +41,7 @@ Variant::Variant() : type(NONE)
 	static_assert(sizeof(TransformRef) <= LOCAL_DATA_SIZE, "Local data too small for TransformRef");
 	static_assert(sizeof(TransitionList) <= LOCAL_DATA_SIZE, "Local data too small for TransitionList");
 	static_assert(sizeof(AnimationList) <= LOCAL_DATA_SIZE, "Local data too small for AnimationList");
+	static_assert(sizeof(DecoratorList) <= LOCAL_DATA_SIZE, "Local data too small for DecoratorList");
 }
 
 Variant::Variant( const Variant& copy ) : type(NONE)
@@ -85,6 +86,11 @@ void Variant::Clear()
 			AnimationList* animation_list = (AnimationList*)data;
 			animation_list->~AnimationList();
 		}
+		case DECORATORLIST:
+		{
+			DecoratorList* decorator_list = (DecoratorList*)data;
+			decorator_list->~DecoratorList();
+		}
 		break;
 		default:
 		break;
@@ -120,6 +126,10 @@ void Variant::Set(const Variant& copy)
 		Set(*(AnimationList*)copy.data);
 		break;
 
+	case DECORATORLIST:
+		Set(*(DecoratorList*)copy.data);
+		break;
+
 	default:
 		Clear();
 		memcpy(data, copy.data, LOCAL_DATA_SIZE);
@@ -238,6 +248,19 @@ void Variant::Set(const AnimationList& value)
 	}
 }
 
+void Variant::Set(const DecoratorList& value)
+{
+	if (type == DECORATORLIST)
+	{
+		*(DecoratorList*)data = value;
+	}
+	else
+	{
+		type = DECORATORLIST;
+		new(data) DecoratorList(value);
+	}
+}
+
 void Variant::Set(const Colourf& value)
 {
 	type = COLOURF;
@@ -295,6 +318,8 @@ bool Variant::operator==(const Variant & other) const
 		return DEFAULT_VARIANT_COMPARE(TransitionList);
 	case ANIMATIONLIST:
 		return DEFAULT_VARIANT_COMPARE(AnimationList);
+	case DECORATORLIST:
+		return DEFAULT_VARIANT_COMPARE(DecoratorList);
 	case COLOURF:
 		return DEFAULT_VARIANT_COMPARE(Colourf);
 	case COLOURB: