[Webkit-unassigned] [Bug 34347] MathML Support for mrow and Stretchy Operators

[bugzilla-daemon at webkit.org]

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


Roland Steiner <rolandsteiner at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rolandsteiner at chromium.org




--- Comment #2 from Roland Steiner <rolandsteiner at chromium.org>  2010-03-01 01:06:31 PST ---
Just a few comments from a quick look at your patch (all strictly IMHO, since
I'm not a reviewer):

Index: WebCore/mathml/RenderMathMLOperator.cpp
===================================================================
--- WebCore/mathml/RenderMathMLOperator.cpp    (revision 0)
+++ WebCore/mathml/RenderMathMLOperator.cpp    (revision 0)

+// This is a table of stretchy characters.
+// FIXME: Should this be read from the unicode characteristics somehow?
+// table:  stretchy operator, top char, extension char, bottom char, middle
char
+static const UChar stretchy[9][5] = {
+{ 0x28  , 0x239b, 0x239c, 0x239d, 0x0    }, // left parenthesis
+{ 0x29  , 0x239e, 0x239f, 0x23a0, 0x0    }, // right parenthesis
+{ 0x5b  , 0x23a1, 0x23a2, 0x23a3, 0x0    }, // left square bracket
+{ 0x5d  , 0x23a4, 0x23a5, 0x23a6, 0x0    }, // right square bracket
+{ 0x7b  , 0x23a7, 0x23aa, 0x23a9, 0x23a8 }, // left curly bracket
+{ 0x7c  , 0x23d0, 0x23d0, 0x23d0, 0x0    }, // vertical bar
+{ 0x7d  , 0x23ab, 0x23aa, 0x23ad, 0x23ac }, // right curly bracket
+{ 0x222b, 0x2320, 0x23ae, 0x2321, 0x0    }, // integral sign
+{ 0x0   , 0x0,    0x0,    0x0,    0x0    }
+};

Could you make the elements of this array into a struct with 5 UChar members?
Having explicitly named members would be more readable than an fixed numerical
array index. Also, the name 'stretchy' is the same as the attribute that you
reference later, which is IMHO a bit confusing:

+    bool noStretch = false;
+    if (mo) {
+        AtomicString stretchyAttr =
mo->getAttribute(MathMLNames::stretchyAttr);
+        noStretch = equalIgnoringCase(stretchyAttr, "false");
+    }
+    

Later in the code:

+    bool canStretch = index >= 0 && stretchy[index][0];
+    if (noStretch || !firstChar || m_stretchHeight <= 24 || !canStretch) {

When browsing, the meaning of 'noStretch' vs. 'canStretch' is not immediately
clear without reading the assignments. Renaming your character array to, e.g.
'stretchCharacters', and 'noStretch' to 'stretchY' or 'stretchYValue' (i.e.,
repeating the name of the attribute value it's storing, reversing true and
false in the process) would be clearer IMHO.

Lastly, iterating from 0 to sizeof(array)/sizeof(array[0]) rather than having
an end sentinel may be more succinct, but this may just be my personal
preference. The current usage also causes you to have 2 different values for
index that both mean 'N/A': -1 and index-of-sentinel.

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