[webkit-reviews] review denied: [Bug 15302] Add very-basic CSS-based MathML support into WebKit : [Attachment 20352] fixed improved patch to work on new sources (large size due to DTD inclusion)

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied 's request for review:
Bug 15302: Add very-basic CSS-based MathML support into WebKit
http://bugs.webkit.org/show_bug.cgi?id=15302

Attachment 20352: fixed improved patch to work on new sources (large size due
to DTD inclusion)
http://bugs.webkit.org/attachment.cgi?id=20352&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
A couple comments:
1.  As a general rule, we don't commit commented-out code, or #if 0'd out code.
 so :
 /*
 154	 uncomment and add class MathMLElement to support dynamic CSS loading
 155	 
 156 #if ENABLE(MATHML)
 157	 virtual
 158 #endif
 159	     bool isMathMLElement() const { return false; }
 160 */

shouldn't be commited.

Instead, I think that you should make that method non-virtual and just make it
return false in the non-enabled case and:
return namespaceURI() == mathMLNamespace; in the enabled case.	Even if you
have to define mathMLNamespace as a function-local static AtomicString.  If we
had actual MathML elements, and MathMLNames.* MathMLNames::mathMLNamespace
woudl already have been defined by the make_names.pl autogeneration script.

+		 if (document() && document()->isHTMLDocument()) // mathml.css
is parsed before any document() exists
goes away once you add a real isMathMLElement function (which for now is
non-virtual and just checks the namespaceURI)

Otherwise the patch looks good.  I think we need to decide if MathML
development should go on a branch at the beginning or if it can go on trunk. 
Also I think we should talk (over IRC or otherwise) about the general plan for
MathML is.  What the "end" condition is for the summer, etc.  I assume there is
a MathML test suite or two that we could check in to test our progress?

Oh, and finally, I made a mistake by including the DTD in these patches.  I
should have made a separate patch for the DTD, and then not included it in any
future patch (that would make it easier to read all the smaller patches).  If
either of us posts any more of these patches, we should probably not include
the DTD.

r- for the commented out code (which could be replaced by real code). Before we
can land a fixed version, we'll need to figure out if this is going on a branch
or not.