[webkit-reviews] review denied: [Bug 136883] Changes in the stretchy attribute do not update rendering : [Attachment 238845] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 136883: Changes in the stretchy attribute do not update rendering
https://bugs.webkit.org/show_bug.cgi?id=136883

Attachment 238845: Patch
https://bugs.webkit.org/attachment.cgi?id=238845&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238845&action=review


Looks good.

review- because of the bad cast

> Source/WebCore/mathml/MathMLTextElement.cpp:69
> +    if ((name == stretchyAttr) && renderer())

Don’t need the parentheses here. This is a common enough idiom and we don’t use
the parentheses everywhere else. I understand that sometimes parentheses make
operator precedence clearer, but == vs. && shows up so often that it doesn’t
require the extra emphasis.

> Source/WebCore/mathml/MathMLTextElement.cpp:70
> +	  
toRenderMathMLOperator(renderer())->setOperatorFlagFromAttributeValue(MathMLOpe
ratorDictionary::Stretchy, value, true);

What guarantees the renderer is a RenderMathMLOperator? The
createElementRenderer function creates objects of many different types; if this
isn’t a RenderMathMLOperator then this will be a bad cast.

> Source/WebCore/mathml/MathMLTextElement.cpp:72
> +    else
> +	   MathMLElement::parseAttribute(name, value);

I don’t think this needs to be an else. I think we should call through to the
base class parseAttribute even if we do the setOperatorFlagFromAttributeValue
work.

> Source/WebCore/mathml/MathMLTextElement.h:43
> +protected:
> +    virtual void parseAttribute(const QualifiedName&, const AtomicString&)
override;

Why protected instead of private? I don’t see any classes that derive from this
one. I also suggest we mark this class final for the same reason.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1181
> +    const AtomicString& attributeValue = element().fastGetAttribute(name);
> +
> +    setOperatorFlagFromAttributeValue(flag, attributeValue);

This code would read better without the local variable.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:81
> +    void setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Flag,
const AtomicString&, bool doLayoutIfNeeded = false);

I think we might need the name "attributeValue" or "value" for the AtomicString
argument. It’s not entirely clear what that argument is from the type alone.

We try to avoid using booleans for these kinds of arguments where you pass a
constant, because “true” is not clear at the call site. Instead we normally use
enums or two separate functions for something like this. In this case, I think
we should have two separate functions, an inner one that does not do
setNeedsLayoutAndPrefWidthsRecalc and an outer one that does. I’m not sure that
the right name for that is “layout”. It’s easy for the outer function to get
the operator flags before and after; it can just call the inner function.