Bug 222368 – RFE: Implement width/height attributes on source elements of <picture>

• Component: DOM
• Status: RESOLVED FIXED

[Christian Biesinger]

As described in https://github.com/whatwg/html/pull/5894/files

Maps width/height attributes on the <source> element in a <picture> to the width/height/aspect-ratio properties on the <img> element

[Radar WebKit Bug Importer]

<rdar://problem/74993565>

[cathiechen]

Created attachment 431642 [details]
Patch

[cathiechen]

Comment on attachment 431642 [details]
Patch

Hi,
I think this patch is ready for review now:)

[Rob Buis]

Comment on attachment 431642 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This patch implement

Implement what? :)

> Source/WebCore/ChangeLog:12
> +        is selected. Also add invalidateAttributeMapping() to synchronous the changes of source's attributes

Synchronize with.

[cathiechen]

Comment on attachment 431642 [details]
Patch

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

Thanks, Rob:)

>> Source/WebCore/ChangeLog:8
>> +        This patch implement
> 
> Implement what? :)

Ah, I should remove it!

>> Source/WebCore/ChangeLog:12
>> +        is selected. Also add invalidateAttributeMapping() to synchronous the changes of source's attributes
> 
> Synchronize with.

Done, thanks!

[cathiechen]

Created attachment 431676 [details]
Patch

[cathiechen]

The specification https://html.spec.whatwg.org/#the-source-element

[Darin Adler]

Comment on attachment 431676 [details]
Patch

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

> Source/WebCore/dom/StyledElement.cpp:299
>      for (const Attribute& attribute : attributesIterator())
>          collectPresentationalHintsForAttribute(attribute.name(), attribute.value(), static_cast<MutableStyleProperties&>(*style));
>  
> +    collectExtraStyleForPresentationalHints(static_cast<MutableStyleProperties&>(*style));

This cast is not good, neither in the old call nor the new. I suggest we change the local variable style to be a Ref instead of a RefPtr to another class.

    Ref style = MutableStyleProperties::create(isSVGElement() ? SVGAttributeMode : HTMLQuirksMode);

Then we can write "style" instead of "static_cast<MutableStyleProperties&>(*style)". Is it really OK to call another virtual function every time? I guess probably, but I am a little sad to see it.

> Source/WebCore/html/HTMLImageElement.cpp:145
> +    auto widthAttrFromSource = sourceElement()->attributeWithoutSynchronization(widthAttr);
> +    auto heightAttrFromSource = sourceElement()->attributeWithoutSynchronization(heightAttr);

Could do this with a little bit less reference count churn by writing auto& instead of auto.

> Source/WebCore/html/HTMLImageElement.cpp:837
> +    if (m_sourceElement.get() == sourceElement)

Should not need a ".get()" here because == is overloaded for WeakPtr and raw pointers.

> Source/WebCore/html/HTMLImageElement.h:204
> +    // The source element that selected to provide the source url.

Should write "URL", not "url".

Not sure what "that selected to" means here. Maybe "that was selected to" is what we mean?

> Source/WebCore/html/HTMLPictureElement.h:39
> +    void sourceAttributeChanged(const HTMLSourceElement&);

This function is specifically called only when the source element's width and height attributes change. That is not so clear from the function name. Might want to choose a name that makes that clearer.

> Source/WebCore/html/HTMLSourceElement.cpp:210
> +    HTMLElement::attributeChanged(name, oldValue, newValue, reason);

Usually it’s best to call through to the base class at the end of the function; such "tail calls" can be better optimized.

> Source/WebCore/html/HTMLSourceElement.cpp:213
> +        RefPtr<Element> parent = parentElement();
> +        if (parent && is<HTMLPictureElement>(*parent))

Can just write RefPtr here, don’t need to write RefPtr<Element>. Since we are doing type checking and only want an HTMLPictureElement, we don’t need to use parentElement, can just use parent or parentNode. Also the is function takes pointers and can handle null checking, so we don’t need to both null check and call is<>.

    if (RefPtr parent = parentNode(); is<HTMLPictureElement>(parent.get()))

[cathiechen]

Comment on attachment 431676 [details]
Patch

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

Hi Darin,
Thanks for the review!

>> Source/WebCore/dom/StyledElement.cpp:299
>> +    collectExtraStyleForPresentationalHints(static_cast<MutableStyleProperties&>(*style));
> 
> This cast is not good, neither in the old call nor the new. I suggest we change the local variable style to be a Ref instead of a RefPtr to another class.
> 
>     Ref style = MutableStyleProperties::create(isSVGElement() ? SVGAttributeMode : HTMLQuirksMode);
> 
> Then we can write "style" instead of "static_cast<MutableStyleProperties&>(*style)". Is it really OK to call another virtual function every time? I guess probably, but I am a little sad to see it.

Done, thanks!
I will add an element type check before collectExtraStyleForPresentationalHints.

>> Source/WebCore/html/HTMLImageElement.cpp:145
>> +    auto heightAttrFromSource = sourceElement()->attributeWithoutSynchronization(heightAttr);
> 
> Could do this with a little bit less reference count churn by writing auto& instead of auto.

Done, thanks!

>> Source/WebCore/html/HTMLImageElement.cpp:837
>> +    if (m_sourceElement.get() == sourceElement)
> 
> Should not need a ".get()" here because == is overloaded for WeakPtr and raw pointers.

Done!

>> Source/WebCore/html/HTMLImageElement.h:204
>> +    // The source element that selected to provide the source url.
> 
> Should write "URL", not "url".
> 
> Not sure what "that selected to" means here. Maybe "that was selected to" is what we mean?

Done, thanks!
Yeah, this should be "that was selected to" :)

>> Source/WebCore/html/HTMLPictureElement.h:39
>> +    void sourceAttributeChanged(const HTMLSourceElement&);
> 
> This function is specifically called only when the source element's width and height attributes change. That is not so clear from the function name. Might want to choose a name that makes that clearer.

Done, I will change the function name to sourceDimensionAttributesChanged

>> Source/WebCore/html/HTMLSourceElement.cpp:210
>> +    HTMLElement::attributeChanged(name, oldValue, newValue, reason);
> 
> Usually it’s best to call through to the base class at the end of the function; such "tail calls" can be better optimized.

Done!

>> Source/WebCore/html/HTMLSourceElement.cpp:213
>> +        if (parent && is<HTMLPictureElement>(*parent))
> 
> Can just write RefPtr here, don’t need to write RefPtr<Element>. Since we are doing type checking and only want an HTMLPictureElement, we don’t need to use parentElement, can just use parent or parentNode. Also the is function takes pointers and can handle null checking, so we don’t need to both null check and call is<>.
> 
>     if (RefPtr parent = parentNode(); is<HTMLPictureElement>(parent.get()))

Done! Use auto here instead, for parentNode() returns ContainerNode*.

[cathiechen]

Created attachment 431845 [details]
Patch

[Darin Adler]

Comment on attachment 431845 [details]
Patch

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

Looks good; still have a tiny set of comments but none are mandatory.

> Source/WebCore/dom/StyledElement.cpp:296
> +    Ref style = MutableStyleProperties::create(isSVGElement() ? SVGAttributeMode : HTMLQuirksMode);

This could be auto instead of Ref, by the way; I think Ref is just fine, but auto does the same in this case.

> Source/WebCore/dom/StyledElement.cpp:298
> +        collectPresentationalHintsForAttribute(attribute.name(), attribute.value(), style.get());

You might be able to use "style" here instead of "style.get()" because Ref can convert itself when passed to a function expecting a reference (strangely, unlike passing RefPtr to a function expecting a pointer).

> Source/WebCore/dom/StyledElement.cpp:300
> +    if (is<HTMLImageElement>(this))

Should use *this here.

> Source/WebCore/dom/StyledElement.cpp:301
> +        collectExtraStyleForPresentationalHints(style.get());

You might be able to use "style" here instead of "style.get()" because Ref can convert itself.

> Source/WebCore/html/HTMLImageElement.cpp:152
> +        addPropertyToPresentationalHintStyle(style, CSSPropertyWidth, CSSValueAuto);

Is this necessary? If it is necessary, then I don’t understand why it’s OK to skip if both attributes are null.

> Source/WebCore/html/HTMLImageElement.cpp:157
> +        addPropertyToPresentationalHintStyle(style, CSSPropertyHeight, CSSValueAuto);

Is this necessary? If it is necessary, then I don’t understand why it’s OK to skip if both attributes are null.

> Source/WebCore/html/HTMLImageElement.cpp:159
> +    if (!widthAttrFromSource.isNull() && !heightAttrFromSource.isNull())

This if statement will always be true. Earlier in the function we do an early return if both attributes are null.

> Source/WebCore/html/HTMLSourceElement.cpp:211
> +        if (auto parent = parentNode(); is<HTMLPictureElement>(parent))

This local variable type should probably be RefPtr, not auto. We want to hold a reference to the object when calling sourceDimensionAttributesChanged on it. Yes, it will be RefPtr<ContainerNode>, but we should be able to just write RefPtr.

[cathiechen]

Comment on attachment 431845 [details]
Patch

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

Thanks, Darin!

>> Source/WebCore/dom/StyledElement.cpp:296
>> +    Ref style = MutableStyleProperties::create(isSVGElement() ? SVGAttributeMode : HTMLQuirksMode);
> 
> This could be auto instead of Ref, by the way; I think Ref is just fine, but auto does the same in this case.

Done!

>> Source/WebCore/dom/StyledElement.cpp:298
>> +        collectPresentationalHintsForAttribute(attribute.name(), attribute.value(), style.get());
> 
> You might be able to use "style" here instead of "style.get()" because Ref can convert itself when passed to a function expecting a reference (strangely, unlike passing RefPtr to a function expecting a pointer).

Done!

>> Source/WebCore/dom/StyledElement.cpp:300
>> +    if (is<HTMLImageElement>(this))
> 
> Should use *this here.

Done!

>> Source/WebCore/dom/StyledElement.cpp:301
>> +        collectExtraStyleForPresentationalHints(style.get());
> 
> You might be able to use "style" here instead of "style.get()" because Ref can convert itself.

Done, thanks!

>> Source/WebCore/html/HTMLImageElement.cpp:152
>> +        addPropertyToPresentationalHintStyle(style, CSSPropertyWidth, CSSValueAuto);
> 
> Is this necessary? If it is necessary, then I don’t understand why it’s OK to skip if both attributes are null.

Yeah, this is necessary. I think this is to avoid get one attribute from <source> and get the other attribute from <img>.
We have some cases to test this scenario in LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/picture-aspect-ratio.html
I think it's necessary to add a comment to explain this:)

>> Source/WebCore/html/HTMLImageElement.cpp:157
>> +        addPropertyToPresentationalHintStyle(style, CSSPropertyHeight, CSSValueAuto);
> 
> Is this necessary? If it is necessary, then I don’t understand why it’s OK to skip if both attributes are null.

Ditto.

>> Source/WebCore/html/HTMLImageElement.cpp:159
>> +    if (!widthAttrFromSource.isNull() && !heightAttrFromSource.isNull())
> 
> This if statement will always be true. Earlier in the function we do an early return if both attributes are null.

I think this is to distinguish the scenarios that both attributes are set and the one only single attribute is set.
Anyway, I will add a comment to explain it too:)

>> Source/WebCore/html/HTMLSourceElement.cpp:211
>> +        if (auto parent = parentNode(); is<HTMLPictureElement>(parent))
> 
> This local variable type should probably be RefPtr, not auto. We want to hold a reference to the object when calling sourceDimensionAttributesChanged on it. Yes, it will be RefPtr<ContainerNode>, but we should be able to just write RefPtr.

Done, thanks for the explanation:)

[cathiechen]

Created attachment 431946 [details]
Patch

[EWS]

Committed r279108 (239025@main): <https://commits.webkit.org/239025@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431946 [details].