Minor cleanup and refactoring

Source/Core/Layout/FormattingContext.cpp

@@ -216,7 +216,7 @@ float FormattingContext::FormatFitContentHeight(ContainerBox* parent_container,
 	const FormattingMode& parent_formatting_mode = parent_container->GetFormattingMode();
 
 	float max_content_height = box.GetSize().y;
-	if (Optional<float> cached_height = (parent_formatting_mode.allow_max_content_cache ? layout_node->GetMaxContentHeightIfCached() : std::nullopt))
+	if (Optional<float> cached_height = (parent_formatting_mode.allow_max_content_cache ? layout_node->GetCommittedMaxContentHeight() : std::nullopt))
 	{
 		max_content_height = *cached_height;
 	}
@@ -262,7 +262,7 @@ void FormattingContext::FormatFitContentWidth(Box& box, Element* element, Format
 	float max_content_width = -1.f;
 	ScopedFormatFitContentWidthDebugTracker debug_tracker{element, type, box};
 
-	if (Optional<float> cached_width = (parent_formatting_mode.allow_max_content_cache ? layout_node->GetMaxContentWidthIfCached() : std::nullopt))
+	if (Optional<float> cached_width = (parent_formatting_mode.allow_max_content_cache ? layout_node->GetCommittedMaxContentWidth() : std::nullopt))
 	{
 		max_content_width = *cached_width;
 		debug_tracker.SetCacheHit();

Source/Core/Layout/FormattingContextDebug.cpp

@@ -28,7 +28,9 @@
 
 #include "FormattingContextDebug.h"
 #include "../../../Include/RmlUi/Core/Element.h"
+#include "../../../Include/RmlUi/Core/ElementUtilities.h"
 #include "../../../Include/RmlUi/Core/StringUtilities.h"
+#include "LayoutNode.h"
 
 namespace Rml {
 #ifdef RMLUI_DEBUG
@@ -195,5 +197,18 @@ int FormattingContextDebugTracker::CountEntries() const
 	return (int)entries.size();
 }
 
+void DebugLogDirtyLayoutTree(Element* root_element)
+{
+	String tree_dirty_state;
+	ElementUtilities::VisitElementsDepthOrder(root_element, [&](Element* element, int tree_depth) {
+		tree_dirty_state += String(size_t(4 * tree_depth), ' ');
+		tree_dirty_state += CreateString("%s.  Self: %d  Child: %d", element->GetAddress(false, tree_depth == 0).c_str(),
+			element->GetLayoutNode()->IsSelfDirty(), element->GetLayoutNode()->IsChildDirty());
+		tree_dirty_state += '\n';
+	});
+
+	Log::Message(Log::LT_INFO, "Dirty layout tree:\n%s\n", tree_dirty_state.c_str());
+}
+
 #endif // RMLUI_DEBUG
 } // namespace Rml

Source/Core/Layout/FormattingContextDebug.h

@@ -133,6 +133,8 @@ private:
 	FormattingContextDebugTracker::Entry* tracker_entry = nullptr;
 };
 
+void DebugLogDirtyLayoutTree(Element* root_element);
+
 #else
 
 class ScopedFormatIndependentDebugTracker {
@@ -150,6 +152,8 @@ public:
 	void CloseEntry(float /*max_content_width*/) {}
 };
 
+inline void DebugLogDirtyLayoutTree(Element* /*root_element*/) {}
+
 #endif // RMLUI_DEBUG
 
 } // namespace Rml

Source/Core/Layout/LayoutEngine.cpp

@@ -33,6 +33,7 @@
 #include "../../../Include/RmlUi/Core/Profiling.h"
 #include "ContainerBox.h"
 #include "FormattingContext.h"
+#include "FormattingContextDebug.h"
 #include "LayoutDetails.h"
 #include "LayoutNode.h"
 
@@ -41,12 +42,10 @@ namespace Rml {
 static void FormatElementImpl(Element* element, Vector2f containing_block, Vector2f absolutely_positioning_containing_block,
 	const FormattingMode& formatting_mode)
 {
-	RMLUI_ZoneScoped;
-
 	RootBox absolute_root(Box(absolutely_positioning_containing_block), formatting_mode);
 	RootBox root(Box(containing_block), &absolute_root);
 
-	auto layout_box = FormattingContext::FormatIndependent(&root, element, nullptr, FormattingContextType::Block);
+	UniquePtr<LayoutBox> layout_box = FormattingContext::FormatIndependent(&root, element, nullptr, FormattingContextType::Block);
 	if (!layout_box)
 	{
 		Log::Message(Log::LT_ERROR, "Error while formatting element: %s", element->GetAddress().c_str());
@@ -62,13 +61,17 @@ static void FormatElementImpl(Element* element, Vector2f containing_block, Vecto
 
 void LayoutEngine::FormatElement(Element* layout_root, Vector2f containing_block, bool allow_cache)
 {
+	RMLUI_ZoneScoped;
 	RMLUI_ASSERT(layout_root && containing_block.x >= 0 && containing_block.y >= 0);
 
 	const FormattingMode formatting_mode{FormattingMode::Constraint::None, allow_cache, allow_cache};
 
 	constexpr bool debug_logging = false;
 	if (debug_logging)
+	{
 		Log::Message(Log::LT_INFO, "UpdateLayout start: %s", layout_root->GetAddress().c_str());
+		DebugLogDirtyLayoutTree(layout_root);
+	}
 
 	struct ElementToFormat {
 		Element* element;
@@ -80,6 +83,7 @@ void LayoutEngine::FormatElement(Element* layout_root, Vector2f containing_block
 	bool force_full_document_layout = !allow_cache;
 	if (!force_full_document_layout)
 	{
+		RMLUI_ZoneScopedNC("Search dirty elements", 0x66AACC);
 		ElementUtilities::BreadthFirstSearch(layout_root, [&](Element* candidate) {
 			if (candidate->GetDisplay() == Style::Display::None)
 				return ElementUtilities::CallbackControlFlow::SkipChildren;

Source/Core/Layout/LayoutNode.cpp

@@ -59,25 +59,19 @@ void LayoutNode::PropagateDirtyToParent()
 
 	if (IsSelfDirty())
 	{
-		// We may be able to skip formatting in ancestor elements if this is a layout boundary, in some scenarios. Consider
-		// some scenarios for illustration:
+		// @performance We may be able to skip formatting in ancestor elements if this is a layout boundary, in some
+		// scenarios. Consider the following for illustration:
 		//
-		// 1. Absolute element. `display: block` to `display: none`. This does not need a new layout. Same with margin and
-		//    size, only the current element need to be reformatted, not ancestors.
-		// 2. Absolute element. `display: none` to `display: block`. This *does* need to be layed out, since we don't know
+		// 1. Absolute element. `display: block` to `display: none`. This does not need parents to be layed out.
+		// 2. Absolute element. `width` or `margin`. Same, this does not need parents to be layed out. Only the current
+		//    element needs to be reformatted, not ancestors.
+		// 3. Absolute element. `display: none` to `display: block`. This *does* need parents to be layed out, since we don't know
 		//    our static position or containing block. We could in principle ignore static position in some situations where
-		//    it is not used, and could in principle find our containing block. But it is tricky.
-		// 3. Flex container contents changed. If (and only if) it results in a new layed out size, its parent needs to be
+		//    it is not used, and could in principle find our containing block.
+		// 4. Flex container contents changed. If (and only if) it results in a new layed out size, its parent needs to be
 		//    reformatted again. If so, it should be able to reuse the flex container's layout cache.
 		//
-		// Currently, we don't have all of this information here. So skip this for now.
-		// - TODO: This information could be provided as part of DirtyLayout.
-		// - TODO: Consider if some of this logic should be moved to the layout engine.
-		//
-		// ```
-		//     if (IsLayoutBoundary()) return;
-		// ```
-
+		// We don't attempt to optimize these situations for now, simply continue with dirtying the parent node.
 		DirtyParentNode(element);
 		return;
 	}
@@ -97,7 +91,7 @@ void LayoutNode::CommitLayout(Vector2f containing_block_size, Vector2f absolutel
 	// even if there is nothing to format (for example due to `display: none`), or if the element itself can be cached
 	// despite ancestor changes. This way we can resume the layout here, without formatting its ancestors, if it is
 	// dirtied in a non-parent-mutable way.
-	// - E.g. consider scenario 2 above with Absolute element `display: none` to `display: block`.
+	// - E.g. consider scenario 3 above with Absolute element `display: none` to `display: block`.
 	//
 	// Conversely, the output of the layout passed in here can later be used by when formatting ancestors, when the
 	// current element does not need a new layout by itself.

Source/Core/Layout/LayoutNode.h

@@ -100,10 +100,10 @@ public:
 	bool CommittedLayoutMatches(Vector2f containing_block_size, Vector2f absolutely_positioning_containing_block_size, const Box* override_box,
 		bool layout_constraint) const;
 
-	Optional<float> GetMaxContentWidthIfCached() const { return committed_max_content_width; }
+	Optional<float> GetCommittedMaxContentWidth() const { return committed_max_content_width; }
 	void CommitMaxContentWidth(float width) { committed_max_content_width = width; }
 
-	Optional<float> GetMaxContentHeightIfCached() const { return committed_max_content_height; }
+	Optional<float> GetCommittedMaxContentHeight() const { return committed_max_content_height; }
 	void CommitMaxContentHeight(float height) { committed_max_content_height = height; }
 
 	// TODO: Remove and replace with a better interface.

Tests/Source/UnitTests/LayoutIsolation.cpp

@@ -85,19 +85,6 @@ static void LogLayoutTree(const Vector<ElementLayoutInfo>& layout_info_list)
 	Rml::Log::Message(Rml::Log::LT_DEBUG, "%s", message.c_str());
 }
 
-static void LogDirtyLayoutTree(Element* root_element)
-{
-	String tree_dirty_state;
-	ElementUtilities::VisitElementsDepthOrder(root_element, [&](Element* element, int tree_depth) {
-		tree_dirty_state += String(size_t(4 * tree_depth), ' ');
-		tree_dirty_state += CreateString("%s.  Self: %d  Child: %d", element->GetAddress(false, tree_depth == 0).c_str(),
-			element->GetLayoutNode()->IsSelfDirty(), element->GetLayoutNode()->IsChildDirty());
-		tree_dirty_state += '\n';
-	});
-
-	Log::Message(Log::LT_INFO, "Dirty layout tree:\n%s\n", tree_dirty_state.c_str());
-}
-
 static const String document_isolation_rml = R"(
 <rml>
 <head>
@@ -384,9 +371,9 @@ TEST_CASE("LayoutIsolation.Absolute")
 
 		container->SetProperty("width", "300px");
 
-		LogDirtyLayoutTree(document);
+		DebugLogDirtyLayoutTree(document);
 		document->UpdatePropertiesForDebug();
-		LogDirtyLayoutTree(document);
+		DebugLogDirtyLayoutTree(document);
 
 		TestsShell::RenderLoop();