[Webkit-unassigned] [Bug 118053] Parsing of MathML length

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from chris fleizach <cfleizach at apple.com>  2013-06-26 13:59:59 PST ---
(In reply to comment #4)
> (In reply to comment #2)
> > it looks like ParseNamedSpace is only used inside ParseLength… that method should be static inside the class file
> 
> As indicated in bug 115610 comment 5, I've made two separated functions because of mpadded attribute parsing which require calling the two functions plus doing some other stuff with pseudo-units. However, this is not implemented in WebKit yet, so I can make ParseNamedSpace static for the moment.
> 
> > 
> > > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:82
> > > +        ParseLength(thickness, m_lineThickness, false);
> > 
> > you might want to check if the conversion succeeded before setting the value
> 
> Not sure what you mean here... m_lineThickness = gLineMedium sets m_lineThickness to the default value. If ParseLength fails, then the attribute is ignored and the default value is used. If ParseLength succeeds, then m_lineThickness is set to the parsed value (perhaps a multiple of the specified default for % and unitless). So the boolean success result is not really used here, but I think it may be useful in other situations.
> 
> I'd like to have clarification on what you expect exactly for the tests and about this last point before submitting a new version.

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 205475 [details] [details])
> > none of the tests have expected results
> >
> > what is this testing? there is nothing about math in this test
> > 
> > these thickness tests should be in one line thickness test. i don't think we need separate files for each one
> 
> Thanks Chris. I don't know if you've seen my comments on bug 115610, but it seems that we have a misunderstanding about what these tests are. You seem to expect some other particular unit test format but note that I have used reftests:
> 
> http://trac.webkit.org/wiki/Writing%20Reftests
> 
> So basically the two pages
> 
>    mathml/presentation/mfrac-linethickness1.html
>    mathml/presentation/mfrac-linethickness1-expected-mismatch.html
> 
> form one != reftests that is the test passes iff they don't render the same while the pair
> 
>    mathml/presentation/mfrac-linethickness2.html
>    mathml/presentation/mfrac-linethickness2-expected-mismatch.html
> 
> forms one == reftest that is the test passes iff the two pages render exactly the same. Similarly for -3 -4 and -5.
> 
> Also note that by design it is clear that we can not group == and != reftests in the same test. It's perhaps less obvious, but we can not group != reftests in the same test either. However, I can group the == reftests in the same test if you want.

I see. 

I don't know if this is the best way to test these features. For example, the veryverythinspace would pass as long as it's different from the default thickness. 

But if the parsing logic changed and this value became different somehow, it could still pass as long as it was different.

I feel like having expected results for these will result in more strict tests

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