Bug 29158 - Basic MathML CSS and DOM Support for Rendering
: Basic MathML CSS and DOM Support for Rendering
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 29143 29349
:
  Show dependency treegraph
 
Reported: 2009-09-10 15:45 PST by
Modified: 2009-09-19 11:09 PST (History)


Attachments
Patch to add CSS and MathML DOM support (32.45 KB, patch)
2009-09-10 15:45 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch with change to the ordering in WebCore/Configuration/FeatureDefines.xconfig (32.61 KB, patch)
2009-09-10 15:53 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch with synchronized FeatureDefines.xcconfig (37.47 KB, patch)
2009-09-10 18:09 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch with fixes from previous review (36.63 KB, patch)
2009-09-12 08:07 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch excluding build script changes already applied in 29143 (29.75 KB, patch)
2009-09-15 18:18 PST, Alex Milowski
mjs: review-
Review Patch | Details | Formatted Diff | Diff
Fix for CSS font name and added layout tests (40.18 KB, patch)
2009-09-17 14:54 PST, Alex Milowski
mjs: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated patch to fix conflicts (40.35 KB, patch)
2009-09-18 15:58 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-09-10 15:45:58 PST
Created an attachment (id=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.
------- Comment #1 From 2009-09-10 15:53:12 PST -------
Created an attachment (id=39390) [details]
Updated patch with change to the ordering in WebCore/Configuration/FeatureDefines.xconfig
------- Comment #2 From 2009-09-10 18:09:15 PST -------
Created an attachment (id=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/
------- Comment #3 From 2009-09-10 22:34:49 PST -------
(From update of attachment 39402 [details])
> 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@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.
------- Comment #4 From 2009-09-11 07:12:59 PST -------
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.
------- Comment #5 From 2009-09-11 07:20:36 PST -------
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.
------- Comment #6 From 2009-09-11 08:18:03 PST -------
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.
------- Comment #7 From 2009-09-12 08:07:14 PST -------
Created an attachment (id=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.
------- Comment #8 From 2009-09-14 07:24:00 PST -------
You really need the build script toggle to be able to build the code.
------- Comment #9 From 2009-09-15 18:18:06 PST -------
Created an attachment (id=39627) [details]
Updated patch excluding build script changes already applied in 29143
------- Comment #10 From 2009-09-17 14:00:46 PST -------
(From update of attachment 39627 [details])
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!
------- Comment #11 From 2009-09-17 14:54:44 PST -------
Created an attachment (id=39728) [details]
Fix for CSS font name and added layout tests
------- Comment #12 From 2009-09-17 15:28:53 PST -------
This patch now requires a test for mathml support in the run-webkit-tests script found in 29349
------- Comment #13 From 2009-09-17 18:43:16 PST -------
(From update of attachment 39728 [details])
r=me
------- Comment #14 From 2009-09-18 13:07:01 PST -------
(From update of attachment 39728 [details])
Rejecting patch 39728 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=39728 from bug 29158 failed to download and apply.
------- Comment #15 From 2009-09-18 13:08:28 PST -------
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.
------- Comment #16 From 2009-09-18 15:58:31 PST -------
Created an attachment (id=39802) [details]
Updated patch to fix conflicts
------- Comment #17 From 2009-09-18 15:59:47 PST -------
(From update of attachment 39802 [details])
r=me
------- Comment #18 From 2009-09-19 11:02:43 PST -------
(From update of attachment 39802 [details])
Clearing flags on attachment: 39802

Committed r48559: <http://trac.webkit.org/changeset/48559>
------- Comment #19 From 2009-09-19 11:02:49 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #20 From 2009-09-19 11:09:23 PST -------
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/