Bug 97228

Summary: [MathML] Increase visual space around fraction parts, italic variables, and operators
Product: WebKit Reporter: Dave Barton <dbarton>
Component: MathMLAssignee: Dave Barton <dbarton>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, davidc, eric, fred.wang, jchaffraix, macpherson, menard, mitz, peter, spartha80, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 97293    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch eric: review+

Description Dave Barton 2012-09-20 09:59:37 PDT 
[MathML] Increase visual space around fraction parts, italic variables, and operators
Comment 1 Dave Barton 2012-09-20 10:12:35 PDT 
Created attachment 164937 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-09-20 10:16:57 PDT 
Comment on attachment 164937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164937&action=review

Otherwise looks fine to me.  I don't know if you want your MathML peeps to sign off or not.  I'm not much help in evaluating the beauty of the Math.

I'm ready to r+ with teh enum.

> Source/WebCore/rendering/mathml/RenderMathMLFenced.h:44
> +    RenderMathMLOperator* createMathMLOperator(UChar, bool isFence);

Much better to use an enum here.  Makes the callsites clearer.

enum OperatorType {
    FenceOperator,
    NormalOperator?
}

createMathMLOperator can even have OperatorType = NormalOperator so most callsites don't have to pass anything if you like.
Comment 3 Dave Barton 2012-09-20 11:07:56 PDT 
Created attachment 164949 [details]
Patch
Comment 4 Dave Barton 2012-09-20 11:12:57 PDT 
I added the enum.

I cc'd David C. & Fred on the bug, but they're in Europe & may not see this soon. I don't think it needs their expert MathML review before landing. I'm just adding some space, making our layout look more similar to Firefox and TeX. I think we can't ask them to look at every tweak I make. We'll get plenty of reactions to layout issues from real users soon. :)
Comment 5 Eric Seidel (no email) 2012-09-20 11:17:01 PDT 
Comment on attachment 164949 [details]
Patch

LGTM.  I agree, we don't need to consult the mathml council for every tweak. :)
Comment 6 Dave Barton 2012-09-20 11:26:44 PDT 
Committed r129146: <http://trac.webkit.org/changeset/129146>
Comment 7 Chris Dumez 2012-09-20 22:56:20 PDT 
I think it would it would be nice in the future to move the tests to TestExpectations with a comment like "Needs rebaseline after rxxxx" for ports you don't rebaseline. That would avoid making our bots red until we do the rebaseline. Thanks in advance.
Comment 8 Dave Barton 2012-09-20 23:49:59 PDT 
(In reply to comment #7)
> I think it would it would be nice in the future to move the tests to TestExpectations with a comment like "Needs rebaseline after rxxxx" for ports you don't rebaseline. That would avoid making our bots red until we do the rebaseline. Thanks in advance.

Good idea, I will do this.
Comment 9 spartha80 2012-10-16 03:37:36 PDT 
From what I have checked, Firefox refers to an operator dictionary for Operator spacing. Are there any plans for Webkit to use something similar?
Comment 10 Dave Barton 2012-10-17 12:09:11 PDT 
(In reply to comment #9)
> From what I have checked, Firefox refers to an operator dictionary for Operator spacing. Are there any plans for Webkit to use something similar?

Yes! See bug 99620 (or bug 99623).