[webkit-reviews] review granted: [Bug 236199] [css-logical] changing the writing-mode via a CSS variable does not update the physical properties : [Attachment 458413] 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 236199: [css-logical] changing the writing-mode via a CSS variable does not
update the physical properties
https://bugs.webkit.org/show_bug.cgi?id=236199

Attachment 458413: Patch

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




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

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

Nice!

> Source/WebCore/style/PropertyCascade.cpp:157
> +    CSSPropertyID relatedID;
> +    if (!CSSProperty::isInLogicalPropertyGroup(propertyID))
> +	   relatedID = getRelatedPropertyId(propertyID);
> +    else if (CSSProperty::isDirectionAwareProperty(propertyID))
> +	   relatedID = CSSProperty::resolveDirectionAwareProperty(propertyID,
direction, writingMode);
> +    else
> +	   relatedID = CSSProperty::unresolvePhysicalProperty(propertyID,
direction, writingMode);

Would read better as a lambda.

> Source/WebCore/style/StyleBuilder.cpp:272
> +	   auto& style = m_state.style();
> +	   CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(id,
style.direction(), style.writingMode());

Maybe either use m_state.style() or cache style to a local on top level of the
function (and use it everywhere).

> Source/WebCore/style/StyleBuilder.cpp:322
> -	       } else if (auto* property =
rollbackCascade->lastDeferredPropertyResolvingRelated(id)) {
> -		   applyRollbackCascadeProperty(*property, linkMatchMask);
> -		   return;
> +	       } else {
> +		   auto& style = m_state.style();
> +		   if (auto* property =
rollbackCascade->lastDeferredPropertyResolvingRelated(id, style.direction(),
style.writingMode())) {

Maybe either use m_state.style() or cache style to a local on top level of the
function (and use it everywhere).