[Webkit-unassigned] [Bug 111121] getComputedStyle not working with more than one background image.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=111121

Cameron McCormack (:heycam) <heycam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |heycam at apple.com
 Attachment #445160|review?                     |review-
              Flags|                            |

--- Comment #8 from Cameron McCormack (:heycam) <heycam at apple.com> ---
Comment on attachment 445160
  --> https://bugs.webkit.org/attachment.cgi?id=445160
Patch

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

Patch is good but I'd like to see the next version. Some comments and questions below.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4451
> +size_t ComputedStyleExtractor::getLayerCount(CSSPropertyID property)
> +{

You have the assertion about `property` in getBackgroundPropertyShorthandValue, but I'd repeat it in here too.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4452
> +    auto* styledElement = this->styledElement();

Can drop the "this->". (Not sure why some other functions in this file do that.)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4469
> +Ref<CSSValueList> ComputedStyleExtractor::getBackgroundPropertyShorthandValue(CSSPropertyID property, const StylePropertyShorthand& propertiesBeforeSlashSeperator, const StylePropertyShorthand& propertiesAfterSlashSeperator, CSSPropertyID lastLayerProperty)

Maybe "getFillLayerPropertyShorthandValue" (after the naming of the FillLayer type)? I don't really think of "mask" as a being background-ish.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4475
> +    auto bgColor = lastLayerProperty != CSSPropertyInvalid ? propertyValue(CSSPropertyBackgroundColor, DoNotUpdateLayout) : nullptr;

Feels a bit weird to pass in a property for lastLayerProperty but use CSSPropertyBackgroundColor here. Maybe call bgColor "lastValue" and pass in lastLayerProperty to propertyValue()?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4488
> +        // Background color is a singleton, and should only be prepended to the last layer.

WebKit style is to avoid comments that describe what the code is doing, if it's reasonably clear what the code is doing by reading it. (And to make code clear enough that a comment isn't required, if possible.) So I would probably drop this comment.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4494
> +            beforeList->append(layerCount == 1 ? value : *downcast<CSSValueList>(value).item(i));

When layerCount == 1, is value a CSSValueList or a single value? If it's a list, we should append item(i).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4497
> +

Do we need to append the slash in here?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4506
> +        // If there was only a single layer, then don't bother wrapping in the
> +        // layers list.

And I'd drop this one too.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4520
> +    static const CSSPropertyID propertiesBeforeSlashSeperator[4] = { CSSPropertyBackgroundImage, CSSPropertyBackgroundRepeat, CSSPropertyBackgroundAttachment, CSSPropertyBackgroundPosition };
>      static const CSSPropertyID propertiesAfterSlashSeperator[3] = { CSSPropertyBackgroundSize, CSSPropertyBackgroundOrigin, CSSPropertyBackgroundClip };

Do you know what other browsers do in terms of the ordering of the longhand components? The general rule should be to follow the grammar order in the spec, unless there is some other explicit rules about serialization, and this ordering here doesn't match what's in the spec.

Also we can drop the explicit array lengths.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211126/150b1061/attachment-0001.htm>