From 891dd46a175ba70468b8b51277d1f7bdcfd3b797 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Thu, 4 Nov 2021 17:38:11 +0000 Subject: [PATCH] LibWeb: Store Repeat values directly in BackgroundRepeatStyleValue ...as opposed to storing StyleValues, which we have to later check are IdentifierStyleValues, which store identifiers that we can convert to Repeat values later. It's fewer allocations, and we can't end up with invalid values by mistake. :^) --- .../Libraries/LibWeb/CSS/Parser/Parser.cpp | 23 +++++++++++--- .../CSS/ResolvedCSSStyleDeclaration.cpp | 21 ++----------- .../Libraries/LibWeb/CSS/StyleProperties.cpp | 23 ++------------ Userland/Libraries/LibWeb/CSS/StyleValue.h | 30 ++++++++++++++----- 4 files changed, 47 insertions(+), 50 deletions(-) diff --git a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp index 37a95d07d4..01003415aa 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp +++ b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp @@ -2702,6 +2702,21 @@ RefPtr Parser::parse_single_background_repeat_value(ParsingContext c return value_id == ValueID::RepeatX || value_id == ValueID::RepeatY; }; + auto as_repeat = [](ValueID identifier) { + switch (identifier) { + case ValueID::NoRepeat: + return Repeat::NoRepeat; + case ValueID::Repeat: + return Repeat::Repeat; + case ValueID::Round: + return Repeat::Round; + case ValueID::Space: + return Repeat::Space; + default: + VERIFY_NOT_REACHED(); + } + }; + auto& token = tokens.next_token(); auto maybe_x_value = parse_css_value(context, token); if (!maybe_x_value || !property_accepts_value(PropertyID::BackgroundRepeat, *maybe_x_value)) @@ -2711,8 +2726,8 @@ RefPtr Parser::parse_single_background_repeat_value(ParsingContext c if (is_directional_repeat(*x_value)) { auto value_id = x_value->to_identifier(); return BackgroundRepeatStyleValue::create( - IdentifierStyleValue::create(value_id == ValueID::RepeatX ? ValueID::Repeat : ValueID::NoRepeat), - IdentifierStyleValue::create(value_id == ValueID::RepeatX ? ValueID::NoRepeat : ValueID::Repeat)); + value_id == ValueID::RepeatX ? Repeat::Repeat : Repeat::NoRepeat, + value_id == ValueID::RepeatX ? Repeat::NoRepeat : Repeat::Repeat); } // See if we have a second value for Y @@ -2720,13 +2735,13 @@ RefPtr Parser::parse_single_background_repeat_value(ParsingContext c auto maybe_y_value = parse_css_value(context, second_token); if (!maybe_y_value || !property_accepts_value(PropertyID::BackgroundRepeat, *maybe_y_value)) { // We don't have a second value, so use x for both - return BackgroundRepeatStyleValue::create(x_value, x_value); + return BackgroundRepeatStyleValue::create(as_repeat(x_value->to_identifier()), as_repeat(x_value->to_identifier())); } tokens.next_token(); auto y_value = maybe_y_value.release_nonnull(); if (is_directional_repeat(*y_value)) return error(); - return BackgroundRepeatStyleValue::create(x_value, y_value); + return BackgroundRepeatStyleValue::create(as_repeat(x_value->to_identifier()), as_repeat(y_value->to_identifier())); } RefPtr Parser::parse_background_repeat_value(ParsingContext const& context, Vector const& component_values) diff --git a/Userland/Libraries/LibWeb/CSS/ResolvedCSSStyleDeclaration.cpp b/Userland/Libraries/LibWeb/CSS/ResolvedCSSStyleDeclaration.cpp index a995bc7f3e..f4f40b868c 100644 --- a/Userland/Libraries/LibWeb/CSS/ResolvedCSSStyleDeclaration.cpp +++ b/Userland/Libraries/LibWeb/CSS/ResolvedCSSStyleDeclaration.cpp @@ -374,21 +374,6 @@ static CSS::ValueID to_css_value_id(CSS::Overflow value) VERIFY_NOT_REACHED(); } -static CSS::ValueID to_css_value_id(CSS::Repeat value) -{ - switch (value) { - case Repeat::NoRepeat: - return CSS::ValueID::NoRepeat; - case Repeat::Repeat: - return CSS::ValueID::Repeat; - case Repeat::Round: - return CSS::ValueID::Round; - case Repeat::Space: - return CSS::ValueID::Space; - } - VERIFY_NOT_REACHED(); -} - static CSS::ValueID to_css_value_id(CSS::ListStyleType value) { switch (value) { @@ -662,8 +647,8 @@ RefPtr ResolvedCSSStyleDeclaration::style_value_for_property(Layout: return ColorStyleValue::create(layout_node.computed_values().background_color()); case CSS::PropertyID::BackgroundRepeat: return BackgroundRepeatStyleValue::create( - IdentifierStyleValue::create(to_css_value_id(layout_node.computed_values().background_repeat().repeat_x)), - IdentifierStyleValue::create(to_css_value_id(layout_node.computed_values().background_repeat().repeat_y))); + layout_node.computed_values().background_repeat().repeat_x, + layout_node.computed_values().background_repeat().repeat_y); case CSS::PropertyID::Background: { auto maybe_background_color = property(CSS::PropertyID::BackgroundColor); auto maybe_background_image = property(CSS::PropertyID::BackgroundImage); @@ -677,7 +662,7 @@ RefPtr ResolvedCSSStyleDeclaration::style_value_for_property(Layout: value_or_default(maybe_background_color, InitialStyleValue::the()), value_or_default(maybe_background_image, IdentifierStyleValue::create(CSS::ValueID::None)), value_or_default(maybe_background_position, PositionStyleValue::create(PositionEdge::Left, Length::make_px(0), PositionEdge::Top, Length::make_px(0))), - value_or_default(maybe_background_repeat, BackgroundRepeatStyleValue::create(IdentifierStyleValue::create(CSS::ValueID::Repeat), IdentifierStyleValue::create(CSS::ValueID::Repeat))), + value_or_default(maybe_background_repeat, BackgroundRepeatStyleValue::create(CSS::Repeat::Repeat, CSS::Repeat::Repeat)), value_or_default(maybe_background_attachment, IdentifierStyleValue::create(CSS::ValueID::Scroll)), value_or_default(maybe_background_origin, IdentifierStyleValue::create(CSS::ValueID::PaddingBox)), value_or_default(maybe_background_clip, IdentifierStyleValue::create(CSS::ValueID::BorderBox))); diff --git a/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp b/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp index 35221e1384..d782ab8909 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp +++ b/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp @@ -693,28 +693,9 @@ Optional StyleProperties::background_repeat() const auto value = property(CSS::PropertyID::BackgroundRepeat); if (!value.has_value() || !value.value()->is_background_repeat()) return {}; + auto& background_repeat = value.value()->as_background_repeat(); - - auto to_repeat = [](auto value) -> Optional { - switch (value->to_identifier()) { - case CSS::ValueID::NoRepeat: - return CSS::Repeat::NoRepeat; - case CSS::ValueID::Repeat: - return CSS::Repeat::Repeat; - case CSS::ValueID::Round: - return CSS::Repeat::Round; - case CSS::ValueID::Space: - return CSS::Repeat::Space; - default: - return {}; - } - }; - auto repeat_x = to_repeat(background_repeat.repeat_x()); - auto repeat_y = to_repeat(background_repeat.repeat_y()); - if (repeat_x.has_value() && repeat_y.has_value()) - return BackgroundRepeatData { repeat_x.value(), repeat_y.value() }; - - return {}; + return BackgroundRepeatData { background_repeat.repeat_x(), background_repeat.repeat_y() }; } Optional StyleProperties::box_shadow() const diff --git a/Userland/Libraries/LibWeb/CSS/StyleValue.h b/Userland/Libraries/LibWeb/CSS/StyleValue.h index fbf3d8f32f..5bafe34347 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleValue.h +++ b/Userland/Libraries/LibWeb/CSS/StyleValue.h @@ -181,6 +181,22 @@ enum class Repeat : u8 { Space, }; +constexpr StringView to_string(Repeat value) +{ + switch (value) { + case Repeat::NoRepeat: + return "no-repeat"sv; + case Repeat::Repeat: + return "repeat"sv; + case Repeat::Round: + return "round"sv; + case Repeat::Space: + return "space"sv; + default: + VERIFY_NOT_REACHED(); + } +} + enum class TextAlign { Left, Center, @@ -462,30 +478,30 @@ private: class BackgroundRepeatStyleValue final : public StyleValue { public: - static NonnullRefPtr create(NonnullRefPtr repeat_x, NonnullRefPtr repeat_y) + static NonnullRefPtr create(Repeat repeat_x, Repeat repeat_y) { return adopt_ref(*new BackgroundRepeatStyleValue(repeat_x, repeat_y)); } virtual ~BackgroundRepeatStyleValue() override { } - NonnullRefPtr repeat_x() const { return m_repeat_x; } - NonnullRefPtr repeat_y() const { return m_repeat_y; } + Repeat repeat_x() const { return m_repeat_x; } + Repeat repeat_y() const { return m_repeat_y; } virtual String to_string() const override { - return String::formatted("{} {}", m_repeat_x->to_string(), m_repeat_y->to_string()); + return String::formatted("{} {}", CSS::to_string(m_repeat_x), CSS::to_string(m_repeat_y)); } private: - BackgroundRepeatStyleValue(NonnullRefPtr repeat_x, NonnullRefPtr repeat_y) + BackgroundRepeatStyleValue(Repeat repeat_x, Repeat repeat_y) : StyleValue(Type::BackgroundRepeat) , m_repeat_x(repeat_x) , m_repeat_y(repeat_y) { } - NonnullRefPtr m_repeat_x; - NonnullRefPtr m_repeat_y; + Repeat m_repeat_x; + Repeat m_repeat_y; }; class BorderStyleValue final : public StyleValue {