[Webkit-unassigned] [Bug 29529] MathML Support for mover, munder, munderover, msubsup, and mfrac

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=29529





--- Comment #2 from Alexey Proskuryakov <ap at webkit.org>  2009-09-21 10:21:59 PDT ---
(From update of attachment 39859)
Alex asked to a style review pass on IRC, which I'm happy to do.

+ *     * Neither the name of Alex Milowski nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.

Are you sure that you want the third clause? "it's contributors" looks
particularly strange in this context.

+        // If the style is not inline-block, default to styled render object

This is a full sentence, so it needs a dot at the end. Same issue repeated
elsewhere.

+namespace WebCore {
+    
+using namespace MathMLNames;

We usually put using directives before WebCore namespace.

+RenderFraction::RenderFraction(Node *fraction) 
+    Element* fraction = static_cast<Element *>(node());
+    RenderFraction(Node *fraction);
+RenderMathInlineContainer::RenderMathInlineContainer(Node *fenced) 
+RenderMathSubSup::RenderMathSubSup(Node *subsup) 
+    RenderObject *current = this->firstChild();

Stars are misplaced in some places.

+        attName = "numalign";

Might it be better to add these to MathMLNames? That way, you'd avoid repeated
conversions from ASCII to UTF-16 and memory allocations.

+    if (equalIgnoringCase(align, "left")) {
+
+        // left text align
+        rowStyle->setTextAlign(LEFT);
+
+    } else if (equalIgnoringCase(align, "right")) {
+
+        // right text align
+
+        rowStyle->setTextAlign(RIGHT);
+    }

I do not think that these comments add anything. And there's definitely too
much vertical whitespace here.

+        // FIXME: should this be a constant from somewhere?
+        rowStyle->setBorderTopWidth(3);

Yes, we prefer to define constants at the top of cpp files. We currently use
"static const", even though there was some discussion recently, because
"static" is unnecessary here.

+    return child->node() && child->node()->nodeType() == Node::ELEMENT_NODE ?
true : false;

Braces would make this easier to understand.

+#include "RenderMathSubSup.h"
+#include "FontSelector.h"
+#include "MathMLNames.h"
+#include "RenderInline.h"

I'd add an empty line after RenderMathSubSup.h, because otherwise the list
looks incorrectly sorted.

+namespace WebCore {
+    
+class RenderMathSubSup : public RenderMathInlineContainer {

In a header, code inside a namespace should be indented (yes, a lot of existing
code doesn't do that yet).

+    virtual void addChild(RenderObject* child, RenderObject* beforeChild = 0);
+protected:
+    RenderTable* m_scripts;

We usually put a blank line before "protected" or "private".
+    if (expression->localName() == "mover") {
+        m_kind = Over;
+    } else if (expression->localName() == "munderover") {
+        m_kind = UnderOver;
+    }

There should be no braces around single line blocks.

+    double adjust = -0.4;

These magic values should also go to the top of cpp file.

+const UChar descending[] = {static_cast<UChar>('g') , static_cast<UChar>('j')
, static_cast<UChar>('p') , static_cast<UChar>('q') , static_cast<UChar>('y') ,
0 };

There shouldn't be spaces before commas.

+    for (int i=0; !descends && descending[i] > 0; i++) {

Should have spaces around '=' in "i=0".

+        // We shouldn't get here but we will treat this as under

You can use ASSERT_NOT_REACHED().

A final patch must include pixel test results. Unless there's a special case
for MathML in run-webkit-tests that I'm not aware of, there is no way to make a
non-pixel test that dumps a render tree.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.