[webkit-reviews] review granted: [Bug 222368] RFE: Implement width/height attributes on source elements of <picture> : [Attachment 431676] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted cathiechen <cathiechen at igalia.com>'s
request for review:
Bug 222368: RFE: Implement width/height attributes on source elements of
<picture>
https://bugs.webkit.org/show_bug.cgi?id=222368

Attachment 431676: Patch

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




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 431676
  --> https://bugs.webkit.org/attachment.cgi?id=431676
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&>(*s
tyle));

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()))