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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 238919: Patch
https://bugs.webkit.org/attachment.cgi?id=238919&action=review

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


I’m going to say review+ although it’s not good to have a bug fix here (case
sensitivity on true and false) with no test coverage.

> LayoutTests/mathml/presentation/mo-stretch-update.html:14
> +	   if (window.testRunner) {
> +	       testRunner.waitUntilDone();
> +	   }

No need for braces here.

> LayoutTests/mathml/presentation/mo-stretch-update.html:19
> +	       window.setTimeout(function()

No need for "window." here.

> LayoutTests/mathml/presentation/mo-stretch-update.html:26
> +	   window.addEventListener('load', updatePageAfterRendering, false);

No need for "window." here.

> Source/WebCore/mathml/MathMLTextElement.cpp:70
> +    if (name == stretchyAttr && renderer() &&
renderer()->isRenderMathMLOperator())
> +	  
toRenderMathMLOperator(renderer())->setOperatorFlagAndScheduleLayoutIfNeeded(Ma
thMLOperatorDictionary::Stretchy, value);

Given our conversation about not calling through, this could also be:

    if (name == stretchyAttr) {
	if (renderer() && renderer()->isRenderMathMLOperator())
	   
toRenderMathMLOperator(renderer())->setOperatorFlagAndScheduleLayoutIfNeeded(Ma
thMLOperatorDictionary::Stretchy, value);
	return;
    }

But it’s also fine the way you have it.

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

This needs to be marked override.

This *class* should be marked final. Then we would not bother marking this
particular function final, since all functions would be final.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1179
> +    if (equalIgnoringCase(attributeValue, "true"))

Good bug fix. But no test coverage for this!

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1181
> +    else if (equalIgnoringCase(attributeValue, "false"))

Good bug fix. But no test coverage for this!

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:83
>  protected:

Almost everything below this point should be private rather than protected.
Things can be protected if they need to be used in RenderMathMLRadicalOperator,
but as much as possible should be private instead for better understanding of
the class and easier refactoring later.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:188
>      void setOperatorFlagFromAttribute(MathMLOperatorDictionary::Flag, const
QualifiedName&);

As with most everything above, this should be private rather than protected.

Also, function members should typically appear before data members in the class
definition, which typically are listed all at the end. It’s peculiar to have
this paragraph of four functions at the bottom after the data members. Not
caused by the new patch, but could be better.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:189
> +    void setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Flag,
const AtomicString& attributeValue);

This new function should be private rather than protected.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:191
>      virtual void SetOperatorProperties();

Incorrect capitalization. Not related to this patch, but not right either.