[webkit-reviews] review granted: [Bug 226461] REGRESSION(r266674) Fixed positioned content not displayed : [Attachment 430352] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Oriol Brufau
<obrufau at igalia.com>'s request for review:
Bug 226461: REGRESSION(r266674) Fixed positioned content not displayed
https://bugs.webkit.org/show_bug.cgi?id=226461

Attachment 430352: Patch

https://bugs.webkit.org/attachment.cgi?id=430352&action=review




--- Comment #16 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 430352
  --> https://bugs.webkit.org/attachment.cgi?id=430352
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430352&action=review

> Source/WebCore/css/CSSProperty.cpp:207
> +std::optional<std::pair<LogicalPropertyGroup, MappingLogic>>
CSSProperty::logicalPropertyInfo(CSSPropertyID propertyID)

The code would read better if this returned an optional struct instead of a
pair.

> Source/WebCore/css/CSSProperty.cpp:214
> +	   return std::make_pair(LogicalPropertyGroup::Border,
MappingLogic::Logical);

These can be written as

return std::pair { LogicalPropertyGroup::Border, MappingLogic::Logical };

or probably just

return { { LogicalPropertyGroup::Border, MappingLogic::Logical } };

> Source/WebCore/css/CSSProperty.cpp:335
> +    if (auto pair = logicalPropertyInfo(propertyID))

'pair' is not a great variable name. 'propertyInfo'?

> Source/WebCore/css/CSSProperty.cpp:336
> +	   return pair.value().second == MappingLogic::Logical;

pair->second

> Source/WebCore/css/StyleProperties.cpp:937
> +	       // If the property is in a logical property group, we can't just
update the value in-place,
> +	       // because afterwards there might be another property of the
same group but different mapping logic.
> +	       // In that case the latter might override the former, so
setProperty would have no effect.
> +	       // Instead, remove the original property, and append it to the
end with the new value.

This things is quite a tower. How about factoring it into a function/lambda?

> Source/WebCore/css/StyleProperties.cpp:938
> +	       if (auto pair = CSSProperty::logicalPropertyInfo(property.id()))
{

'pair' is not a great variable name.

> Source/WebCore/css/StyleProperties.cpp:941
> +		   LogicalPropertyGroup group;
> +		   MappingLogic logic;
> +		   std::tie(group, logic) = pair.value();

auto [group, logic] = *pair;

> Source/WebCore/css/StyleProperties.cpp:948
> +			   LogicalPropertyGroup currentGroup;
> +			   MappingLogic currentLogic;
> +			   std::tie(currentGroup, currentLogic) =
currentPair.value();

auto [currentGroup, currentLogic] = *currentPair;