[Webkit-unassigned] [Bug 29158] Basic MathML CSS and DOM Support for Rendering

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Mark Rowe (bdash) <mrowe at apple.com>  2009-09-10 22:34:49 PDT ---
(From update of attachment 39402)
> Index: JavaScriptCore/Configurations/FeatureDefines.xcconfig
> ===================================================================
> --- JavaScriptCore/Configurations/FeatureDefines.xcconfig	(revision 48273)
> +++ JavaScriptCore/Configurations/FeatureDefines.xcconfig	(working copy)
> @@ -44,6 +44,7 @@ ENABLE_FILTERS = ;
>  ENABLE_GEOLOCATION = ;
>  ENABLE_ICONDATABASE = ENABLE_ICONDATABASE;
>  ENABLE_JAVASCRIPT_DEBUGGER = ENABLE_JAVASCRIPT_DEBUGGER;
> +ENABLE_MATHML = ENABLE_MATHML;

I think it is best for MathML to be disabled by default until the
implementation is more complete and tested.


> Index: WebCore/WebCore.xcodeproj/project.pbxproj
> ===================================================================
> --- WebCore/WebCore.xcodeproj/project.pbxproj	(revision 48273)
> +++ WebCore/WebCore.xcodeproj/project.pbxproj	(working copy)
> @@ -17900,6 +17938,7 @@
>  				85136CA80AED665900F90A3D /* westResizeCursor.png in Resources */,
>  				1AB1AE7A0C051FDE00139F4F /* zoomInCursor.png in Resources */,
>  				1AB1AE7B0C051FDE00139F4F /* zoomOutCursor.png in Resources */,
> +				FABE72FA1059C1EB00D999DD /* mathtags.in in Resources */,
>  			);
>  			runOnlyForDeploymentPostprocessing = 0;
>  		};

mathtags.in shouldn't be in the Copy Bundle Resources build phase.  That would
result in mathtags.in being placed inside WebCore.framework which wouldn't
serve any useful purpose.

> Index: WebCore/css/CSSStyleSelector.cpp
> ===================================================================
> --- WebCore/css/CSSStyleSelector.cpp	(revision 48273)
> +++ WebCore/css/CSSStyleSelector.cpp	(working copy)
> @@ -1139,6 +1139,17 @@ PassRefPtr<RenderStyle> CSSStyleSelector
>      }
>  #endif
>  
> +#if ENABLE(MATHML)
> +    static bool loadedMathMLUserAgentSheet;
> +    if (e->isMathMLElement() && !loadedMathMLUserAgentSheet) {
> +        // MathML rules.
> +        loadedMathMLUserAgentSheet = true;
> +        CSSStyleSheet* mathmlSheet = parseUASheet(mathmlUserAgentStyleSheet, sizeof(mathmlUserAgentStyleSheet));
> +        defaultStyle->addRulesFromSheet(mathmlSheet, screenEval());
> +        defaultPrintStyle->addRulesFromSheet(mathmlSheet, printEval());
> +    }
> +#endif

There's a bit of inconsistency in naming style here, and in other places.  In
some places the ML is uppercase and in others it is lowercase.  It seems to me
that it should be consistently uppercase.


> Index: WebCore/mathml/MathMLElement.cpp
> ===================================================================
> --- WebCore/mathml/MathMLElement.cpp	(revision 0)
> +++ WebCore/mathml/MathMLElement.cpp	(revision 0)
> @@ -0,0 +1,59 @@
> +/*
> + *  MathMLElement.cpp
> + *  WebCore
> + *
> + *  Copyright (C) 2009 Alex Milowski (alex at milowski.com).
> + *
> + 
> + 
> + This file is part of the WebKit project
> + 
> + This library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Library General Public
> + License as published by the Free Software Foundation; either
> + version 2 of the License, or (at your option) any later version.
> + 
> + This library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + Library General Public License for more details.
> + 
> + You should have received a copy of the GNU Library General Public License
> + along with this library; see the file COPYING.LIB.  If not, write to
> + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + Boston, MA 02110-1301, USA.
> + 
> + 
> + */

This license header block is formatted a little strangely.  Can we format it
consistently with other files, here and in other places?

> +
> +#include "config.h"
> +
> +#if ENABLE(MATHML)
> +
> +#include "RenderObject.h"
> +#include "MathMLElement.h"
> +#include "MathMLNames.h"

Our style is to include the header corresponding to the .cpp file first,
followed by a blank line, then the remaining includes in alphabetical order. 
We love alphabetical order!


Sorry for the nit-picky comments focused on style.  I'm not going to mark the
patch r- so as to not deter any reviewers that want to focus on the substance
of the patch.

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