Bug 160542 – Rename MathMLTextElement to MathMLTokenElement

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

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

MathMLTextElement is not a very good name: these elements may contain non-text content, the text-only element annotation is handled in a separate class after 160540 and the name does not match the associated renderer RenderMathMLToken. We should probably rename it MathMLTokenElement.

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

Created attachment 285310 [details]
Patch

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

Comment on attachment 285310 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:2002
> +    mathml/MathMLTokenElement.cpp

I forgot to update Source/WebCore/mathml/MathMLAllInOne.cpp here too.

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

Created attachment 286564 [details]
Patch

[Darin Adler]

Comment on attachment 286564 [details]
Patch

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

> Source/WebCore/mathml/MathMLTokenElement.cpp:31
> +#if ENABLE(MATHML)
> +#include "MathMLTokenElement.h"

There should be a blank line between these two lines. I know sometimes we do a different style, but I think we should do this consistently; easier to mentally chunk when the #if is not grouped with one line of what’s inside the #if.

> Source/WebCore/mathml/MathMLTokenElement.cpp:55
> +    if (is<RenderMathMLToken>(renderer()))
> +        downcast<RenderMathMLToken>(*renderer()).updateTokenContent();

We should not make a habit of calling renderer() multiple times; it does include a branch. Maybe the compiler is smart enough to not do the work twice, but generally I put it into a local variable for code like this (and the similar code in the next two functions).

> Source/WebCore/mathml/MathMLTokenElement.h:31
> +#if ENABLE(MATHML)
> +#include "MathMLElement.h"

There should be a blank line between these two lines.

> Source/WebCore/mathml/MathMLTokenElement.h:38
> +    bool acceptsMathVariantAttribute() final { return true; }

Seems to me this should be private. If it’s a final function, no one would have a reason to call it on a MathMLTokenElement& since it would just always return true and thus no need for it to be a public function. Unless there is some template idiom where this actually needs to be called.

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

(In reply to comment #4)
> > Source/WebCore/mathml/MathMLTokenElement.cpp:31
> > +#if ENABLE(MATHML)
> > +#include "MathMLTokenElement.h"
> 
> There should be a blank line between these two lines. I know sometimes we do
> a different style, but I think we should do this consistently; easier to
> mentally chunk when the #if is not grouped with one line of what’s inside
> the #if.

For that one, check-webkit-style complains with the following message:

ERROR: Source/WebCore/mathml/MathMLTokenElement.cpp:32:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 11 files

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

Committed r204715: <http://trac.webkit.org/changeset/204715>

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #4)
> > > Source/WebCore/mathml/MathMLTokenElement.cpp:31
> > > +#if ENABLE(MATHML)
> > > +#include "MathMLTokenElement.h"
> > 
> > There should be a blank line between these two lines. I know sometimes we do
> > a different style, but I think we should do this consistently; easier to
> > mentally chunk when the #if is not grouped with one line of what’s inside
> > the #if.
> 
> For that one, check-webkit-style complains with the following message:
> 
> ERROR: Source/WebCore/mathml/MathMLTokenElement.cpp:32:  You should not add
> a blank line before implementation file's own header.  [build/include_order]
> [4]
> Total errors found: 1 in 11 files

Two thoughts:

1) That error message from check-webkit-style is wrong.

2) We don't need to fix that, though, because the preferred style is probably this:

#include "config.h"
#include "MathMLTokenElement.h"

#if ENABLE(MATHML)

#include "OtherHeader.h"
...

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

(In reply to comment #7)
> 2) We don't need to fix that, though, because the preferred style is
> probably this:
> 
> #include "config.h"
> #include "MathMLTokenElement.h"
> 
> #if ENABLE(MATHML)
> 
> #include "OtherHeader.h"
> ...

OK, that does not seem to be what is done in the MathML code so I'll open a follow-up bug to fix that.