Bug 96960 - Turn on ENABLE_MATHML for Chromium
Summary: Turn on ENABLE_MATHML for Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Barton
URL:
Keywords: WebExposed
Depends on: 96843 97606
Blocks: mathml-in-mathjax
  Show dependency treegraph
 
Reported: 2012-09-17 15:15 PDT by Eric Seidel (no email)
Modified: 2012-10-10 13:33 PDT (History)
19 users (show)

See Also:


Attachments
Patch (3.59 KB, patch)
2012-10-10 10:26 PDT, Dave Barton
no flags Details | Formatted Diff | Diff
Patch (3.78 KB, patch)
2012-10-10 12:52 PDT, Dave Barton
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-09-17 15:15:03 PDT
Turn on ENABLE_MATHML for Chromium

After bug 96843, Dave believes most (if not all) of the known crashers for MathML are resolved, and it's ready to enable for Chromium.

We'll flip the switch after M23 has branched, which should be tomorrow.  We'll probably need to inform the current gardener that there may be some massive MathML pixel rebaselines incoming. :)
Comment 1 Eric Seidel (no email) 2012-09-17 15:16:11 PDT
Once its on, it the chromium fuzzer bots should tell us if we have any stability issues left to worry about.  If so, we can always turn it off again before branching for m24 in another 6 weeks.
Comment 2 Abhishek Arya 2012-09-17 15:18:06 PDT
Sounds good!!
Comment 3 Eric Seidel (no email) 2012-09-17 15:26:07 PDT
I've CC'd the upcoming WebKit gardeners for this week so they're aware of this impending change (feel free to remove yourself if you're not interested!)
Comment 4 Eric Seidel (no email) 2012-09-17 18:40:25 PDT
http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/features.gypi is the file to change, btw.
Comment 5 Eric Seidel (no email) 2012-09-17 18:41:52 PDT
We'll also want to remove this line once MATHML is turned back on:
http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations#L1651

or perhaps as part of turning it on.
Comment 6 Dave Barton 2012-09-18 12:36:34 PDT
Other than checking for crashers, I think our two main issues will be:

1. Most of the MathML tests are pixel tests. I still need to tweak MathML layout sometimes, and when I do, it often triggers a ~1MB patch (sigh). This is almost all test results in LayoutTests/platform/*/mathml/presentation. Actually, so far the only platforms are mac, efl, and gtk. I verify the results in platform/mac, and lately Christophe Dumez usually checks the efl and gtk ones. I'd be happy to check results on various platforms if gardeners want to upload them or e-mail a zip file to me. Actually WebKit MathML isn't used yet in the wild too much, so we don't have to be too paranoid about small changes. If you check the .png files quickly and they look similar to the platform/mac ones, then I'd just rebaseline (.png and .txt) if I were you. If your .png files look bad, then please upload or send your results to me.

2. The biggest platform issue for MathML will doubtless be fonts. We can minimize this problem by ensuring the STIX fonts are on all the bots - then we might even be able to combine some pixel test baselines. On Mac bots the OS must be at least OS X 10.7.4, or else the STIX fonts come out always italic. Can someone verify the OS version on the Chromium Mac bots? There are also some issues on linux - search LayoutTests/platform/efl/Skipped for "mathml"; and see bug 47744; and bug 38377, comment 2 (I need to fix that, I think). But the biggest issue is probably -webkit-line-box-contain on Windows - see <http://code.google.com/p/chromium/issues/detail?id=77095>. Christophe Dumez was able to fix this on linux fairly quickly in bug 93073 - see its comments 8, 11 and 12, and its patch. It'd be great if a Windows font expert could do this on Windows also. Bug 48124 may also be an issue on Windows.

Note the Apple Mac port doesn't enable subpixel layout yet I believe, so its MathML baselines may be slightly different from Chromium.

I think the summary is that it'll be great to test MathML for crashes, and hopefully we can also get it looking good on Mac, Linux, and Windows soon. If not, we may have to turn it off on some platforms for now. But MathML in Chrome is still exciting!
Comment 7 Dave Barton 2012-09-23 19:40:52 PDT
(In reply to comment #4)
> http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/features.gypi is the file to change, btw.

Apparently you also have to add an include path of Source/WebCore/rendering/mathml somewhere. Does anyone know where?

Has anyone been able to compile chromium with ENABLE(MATHML) locally? Is there anything else I need to do?

Thanks in advance!
Comment 8 Eric Seidel (no email) 2012-09-23 20:05:02 PDT
http://trac.webkit.org/browser/trunk/Source/WebCore/WebCore.gypi is where the files go, http://trac.webkit.org/browser/trunk/Source/WebCore/WebCore.gyp/WebCore.gyp is where the include paths go, I believe.  Tony Chang is the many I ask all my gyp questions to, he'll correct me if I'm wrong. :)
Comment 9 Eric Seidel (no email) 2012-09-23 20:05:52 PDT
I'm away from my work machine, otherwise I'd test this build for you.  update-webkit-chromium && build-webkit --chromium should also work if you need to try yourself.
Comment 10 Eric Seidel (no email) 2012-09-23 20:07:02 PDT
update-webkit-chromium is just the fancy script that update-webkit --chromium (note the space) calls to update the generated project files (in your case, XCode file, presumably) after each update.  It can be called manually to update the generated project files w/o updating the repository (despite it's confusing name). :)
Comment 11 Dave Barton 2012-09-23 21:01:17 PDT
That all looks to be working great, thanks! (Sorry, I had been looking in Source/WebKit/chromium.)
Comment 12 Tony Chang 2012-09-24 11:17:37 PDT
Sounds like you got things building.

For TestExpectations, I would just mark the lines as [ Failure ]. This will run the tests on the bot and allow anyone to rebaseline them using garden-o-matic.  I don't think using a special font will avoid pixel results since there will still be antialiasing differences across platforms.  It looks like there are only 32 pixel tests.  Adding 32 * 2 (txt and png file) * 3 (win, mac, linux) = 192 files for Chromium pixel results isn't great, but I can't think of anything better.

http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/TestExpectations#L1643
Comment 13 Dave Barton 2012-09-25 09:09:30 PDT
Thanks very much for the expert information and advice.

I indeed have things building, but have been stuck on an assertion failure due to a combination of MathML, FlexBox, and SubPixel layout units. LayoutTests/mathml/presentation/roots.xhtml causes totalWeightedFlexShrink in RenderFlexibleBox::layoutFlexItems to go negative due to roundoff error, at least for chromium mac snowleopard with ENABLE(MATHML). I think the main issue is that MathML uses flexboxes but without any flex, so every child in RenderFlexibleBox::resolveFlexibleLengths becomes a violation [in a case where availableFreeSpace < 0]. Then RenderFlexibleBox::freezeViolations subtracts each preferredChildSize from totalWeightedFlexShrink [child->style()->flexShrink() is always 1], and the roundoff error brings totalWeightedFlexShrink slightly below 0.

I realize I should file a bug for this, but it'd be easier after MathML is enabled in Chrome, since I haven't been able to reproduce this without MathML yet. However, I'm thinking this summary may be enough for Tony & Ojan to say something? Thanks greatly!
Comment 14 Eric Seidel (no email) 2012-09-25 10:54:57 PDT
Calling in the sub-pixel brigade!  (Be warned, all Googlers are distracted this week with annual peer-reviews, due friday.)
Comment 15 Ojan Vafai 2012-09-25 12:07:58 PDT
This sounds like a genuine flexbox bug. It's not clear to me where the bug is at a quick glance at the code. It's hard to look into it further without a flexbox-only test case.
Comment 16 Eric Seidel (no email) 2012-10-09 14:31:13 PDT
I'm happy to help you flip this flag tomorrow if you'd like, Dave.
Comment 17 Dave Barton 2012-10-09 16:03:34 PDT
(In reply to comment #16)
> I'm happy to help you flip this flag tomorrow if you'd like, Dave.

I have it compiling, and could submit a patch to turn MathML on in Chrome, marking the layout tests as failing. I guess I was waiting on bug 98791, and I'm working on bug 97390, and also I'm guessing -webkit-line-box-contain will fail on Windows so all the MathML won't layout right there. There are also some smaller layout issues, but maybe it's better to turn MathML on so we can see these issues? I think I'll discuss this with you (Eric) by e-mail, so I can get clarity, thanks!
Comment 18 Dave Barton 2012-10-10 10:26:32 PDT
Created attachment 168027 [details]
Patch
Comment 19 Eric Seidel (no email) 2012-10-10 10:31:13 PDT
Comment on attachment 168027 [details]
Patch

If the EWS likes it, then I do.  SGTM.  Please let the bubbles roll before landing.
Comment 20 Eric Seidel (no email) 2012-10-10 10:31:29 PDT
CCing todays gardeners.
Comment 21 WebKit Review Bot 2012-10-10 11:57:24 PDT
Comment on attachment 168027 [details]
Patch

Attachment 168027 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14259163

New failing tests:
http/tests/xmlviewer/dumpAsText/mathml.xml
Comment 22 Eric Seidel (no email) 2012-10-10 12:37:30 PDT
abarth says "bring it on".  Feel free to land whenever and we'll be in #webkit to help you clean up any troubles.
Comment 23 Dave Barton 2012-10-10 12:52:55 PDT
Created attachment 168056 [details]
Patch
Comment 24 Adam Barth 2012-10-10 13:11:07 PDT
Comment on attachment 168056 [details]
Patch

Forwarding Eric's R+.
Comment 25 Dave Barton 2012-10-10 13:33:32 PDT
Committed r130950: <http://trac.webkit.org/changeset/130950>