Bug 161300 – Move RenderMathMLRoot:RootType and RenderMathMLScripts:ScriptsType to element classes

• Component: MathML
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

We have some mapping from tag names to these enums. This could just be performed directly in the constructors of the corresponding element classes.

[Frédéric Wang (:fredw)]

*** Bug 180161 has been marked as a duplicate of this bug. ***

[Frédéric Wang (:fredw)]

Created attachment 328225 [details]
Patch

[Darin Adler]

Comment on attachment 328225 [details]
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.

[Frédéric Wang (:fredw)]

Comment on attachment 328225 [details]
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&.

OK, that seems to be a generic problem in the MathML code. For now, I'll just introduce override element() and I opened the follow-up bug 180346.

>> Source/WebCore/rendering/mathml/RenderMathMLRoot.h:37
>> +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.

Done.

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

Done.

[Frédéric Wang (:fredw)]

Committed r225475: <https://trac.webkit.org/changeset/225475>

[Radar WebKit Bug Importer]

<rdar://problem/35826875>