Bug 29158 – Basic MathML CSS and DOM Support for Rendering

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alex Milowski]

Created attachment 39389 [details]
Patch to add CSS and MathML DOM support

This patch adds basic support for MathML by adding the CSS stylesheet based on the element being identified as a MathML element.  A simple set of DOM classes has been added that provide the isMathMLElement() method returning 'true'.

The CSS stylesheet is simple and setup to work with the coming rendering code.

[Alex Milowski]

Created attachment 39390 [details]
Updated patch with change to the ordering in WebCore/Configuration/FeatureDefines.xconfig

[Alex Milowski]

Created attachment 39402 [details]
Updated patch with synchronized FeatureDefines.xcconfig

This patch also contains style conformance tweaks to the code.  It also affects the FeatureDefines.xcconfig in JavaScriptCore and WebKit/mac/

[Mark Rowe (bdash)]

Comment on attachment 39402 [details]
Updated patch with synchronized FeatureDefines.xcconfig

> 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 ([e-mail]).
> + *
> + 
> + 
> + 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.

[Alex Milowski]

In terms of consistency of the term "MathML" translated into variable names, MathML is not all uppercase as an acronym.  Thus, when I translate it to a class name it becomes:

   MathMLElement

When I use it as a variable name and it starts at the beginning, the first 'M' is lowercased, so we
get:

   mathMLSomething

I realized the style guide says acronyms like "HTML" or "XML" should be all upper case but
they are also upper case to begin with.

MATHML  looks strange to me.

[Alex Milowski]

It is really odd that mathtags.in ended up being referenced in the "Copy Bundled Resources" task.  I certainly don't remember adding it there at all.  Is there something I should be aware of in XCode projects that might put it there automatically?

...easy to fix but I like to avoid that kind of thing in the future.

[Alex Milowski]

From the list:

'MathML is a word "math" plus an acronym ML (markup language), so MathML seems like an appropriate casing.'

I would still need to adjust the code a bit.

[Alex Milowski]

Created attachment 39515 [details]
Updated patch with fixes from previous review

In checking with the style guide and the list, MathML is a "word + acronym" and should be "MathML" or "mathML" where used.  The stylesheet variable generated for user agent stylesheets is automatically named "mathmlUserAgentStyleSheet" and does not follow the style properly (i.e. mathMLUserAgentStyleSheet) but I can't really change that as it would affect SVG and other user agent style related code.

[Alex Milowski]

You really need the build script toggle to be able to build the code.

[Alex Milowski]

Created attachment 39627 [details]
Updated patch excluding build script changes already applied in 29143

[Maciej Stachowiak]

Comment on attachment 39627 [details]
Updated patch excluding build script changes already applied in 29143

I think the places that mention "New Times Roman" should be "Times New Roman" instead. Also, this patch is lacking test cases. It should include some basic LayoutTests (even if future patches will update the results). Those are the only problems I see. Please revise and resubmit. Thanks!

[Alex Milowski]

Created attachment 39728 [details]
Fix for CSS font name and added layout tests

[Alex Milowski]

This patch now requires a test for mathml support in the run-webkit-tests script found in 29349

[Maciej Stachowiak]

Comment on attachment 39728 [details]
Fix for CSS font name and added layout tests

r=me

[WebKit Commit Bot]

Comment on attachment 39728 [details]
Fix for CSS font name and added layout tests

Rejecting patch 39728 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=39728 from bug 29158 failed to download and apply.

[Eric Seidel (no email)]

3 out of 7 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej

Looks like the project file is out of date.  We'll need a new patch if you want this committed via the commit-queue.

[Alex Milowski]

Created attachment 39802 [details]
Updated patch to fix conflicts

[Maciej Stachowiak]

Comment on attachment 39802 [details]
Updated patch to fix conflicts

r=me

[WebKit Commit Bot]

Comment on attachment 39802 [details]
Updated patch to fix conflicts

Clearing flags on attachment: 39802

Committed r48559: <http://trac.webkit.org/changeset/48559>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

The bots have been red for the last 17 hours, which is why this was so slow to land:
http://webkit-commit-queue.appspot.com/