[webkit-reviews] review granted: [Bug 161300] Move RenderMathMLRoot:RootType and RenderMathMLScripts:ScriptsType to element classes : [Attachment 328225] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 161300: Move RenderMathMLRoot:RootType and RenderMathMLScripts:ScriptsType
to element classes
https://bugs.webkit.org/show_bug.cgi?id=161300

Attachment 328225: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 328225
  --> https://bugs.webkit.org/attachment.cgi?id=328225
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:57
> +    return
static_cast<MathMLRootElement&>(nodeForNonAnonymous()).rootType();

Typically, this kind of cast should be a downcast, not a static_cast; but of
course this requires adding a way to check the type in debug builds and using
some form of the SPECIALIZE_TYPE_TRAITS macro.

It’s more elegant to add a function named element() to a specific renderer
class like this, and do the cast there. Then this function becomes just
element().rootType(). The benefit for future programmers is that anyone who
wants the element for this renderer and has the specific renderer type pointer
or reference, will get a specifically typed reference, is likely to get the
most efficient version of various functions like the ones that are more
efficient in ContainerNode than Node, gets a reference rather than a pointer,
etc. Helps cut down on casting too.

If we wanted to follow the pattern from, say, RenderButton, which adds
RenderButton::formControlElement and deletes the inherited element function
that would be OK, but I think it would also be fine if RenderButton had just
defined a function named element with return type HTMLFormControlElement&.

> Source/WebCore/rendering/mathml/RenderMathMLRoot.h:37
> +class MathMLRootElement;
> +enum class RootType { SquareRoot, RootWithIndex };

Should not paragraph this forward declaration together with an actual
definition of something in this header. Pleas add a blank line.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:37
>  class MathMLScriptsElement;
> +enum class ScriptType { Sub, Super, SubSup, Multiscripts, Under, Over,
UnderOver };

Ditto.