Summary: | Migrate the MathML stretchy code from UChar to Glyph | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||||||
Component: | MathML | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | andersca, bfulgham, buildbot, cfleizach, commit-queue, darin, dbarton, esprehn+autocc, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, mrobinson, rniwa | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Bug Depends on: | 115786 | ||||||||||||||||||||||
Bug Blocks: | 122297 | ||||||||||||||||||||||
Attachments: |
|
Description
Frédéric Wang (:fredw)
2014-02-17 06:38:03 PST
Created attachment 225390 [details]
Patch
Comment on attachment 225390 [details] Patch I'm removing the review request since I'll make this depend on bug 115786. Created attachment 225997 [details] Patch This one now applies on top of bug 115786. Created attachment 226406 [details]
Patch + 115787 + 115786 for testing
Created attachment 226416 [details]
Patch for review
Created attachment 226493 [details]
Patch
Comment on attachment 226493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226493&action=review > Source/WebCore/platform/graphics/GraphicsContext.h:353 > + void drawGlyphs(const Font&, const SimpleFontData*, const GlyphBuffer&, const FloatPoint&, int from = 0, int numGlyphs = 1); seems like the order of arguments should be the same as in Font.h > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1285 > +FloatRect RenderMathMLOperator::boundsForGlyph(GlyphData data) can this be passed as reference > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1290 > +float RenderMathMLOperator::heightForGlyph(GlyphData data) ditt about reference > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1425 > + m_stretchyCharacter = assembly; It's unclear that this method will modify an ivar when you call it. I think either the method name should be updated or you should return by reference assembly, so you can store it in m_stretchyCharacter > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:123 > + GlyphAssembly m_stretchyCharacter; should probably rename this ivar to something like m_stretchyGlyphAssembly (In reply to comment #7) > (From update of attachment 226493 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226493&action=review > > > Source/WebCore/platform/graphics/GraphicsContext.h:353 > > + void drawGlyphs(const Font&, const SimpleFontData*, const GlyphBuffer&, const FloatPoint&, int from = 0, int numGlyphs = 1); > > seems like the order of arguments should be the same as in Font.h > This is to allow default argument values from and numGlyphs. This function was already defined in Font.h, but for some reason the argument were not in the same order as drawText. I wonder if I should update all the calls to drawGlyphs or just give up with trying to have default argument values for GraphicsContext::drawGlyphs... > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1425 > > + m_stretchyCharacter = assembly; > > It's unclear that this method will modify an ivar when you call it. > I think either the method name should be updated or you should return by reference assembly, so you can store it in m_stretchyCharacter > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:123 > > + GlyphAssembly m_stretchyCharacter; > > should probably rename this ivar to something like m_stretchyGlyphAssembly I'll try to see what I can do. Note that in bug 122297 this becomes m_stretchyGlyph = a union of (pointer to) a glyph assembly and a glyph variant. Also, findAcceptable* will decide whether glyph assembly or glyph variant is used and will set m_stretchyGlyph appropriately. Comment on attachment 226493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226493&action=review r=me but I presume you are going to make some refinements based on Chris’s comments. >>> Source/WebCore/platform/graphics/GraphicsContext.h:353 >>> + void drawGlyphs(const Font&, const SimpleFontData*, const GlyphBuffer&, const FloatPoint&, int from = 0, int numGlyphs = 1); >> >> seems like the order of arguments should be the same as in Font.h > > This is to allow default argument values from and numGlyphs. This function was already defined in Font.h, but for some reason the argument were not in the same order as drawText. I wonder if I should update all the calls to drawGlyphs or just give up with trying to have default argument values for GraphicsContext::drawGlyphs... When adding a new function, we should use references when possible unless something can be null. So it should be const SimpleFontData& unless a null value is legal. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:57 > +static StretchyCharacter stretchyCharacters[14] = { Why not const? >> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1285 >> +FloatRect RenderMathMLOperator::boundsForGlyph(GlyphData data) > > can this be passed as reference I am not sure that a const GlyphData& is better here. In the past we almost always passed structures by const& but more recently Anders has said that we can get good performance copying simple structures as long as they are truly simple and GlyphData is one of those super-simple structures. Anders, care to comment? > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:73 > + // FIXME: Ideally (for MATH fonts) we would only need to store the Glyph indices here. Why is MATH all capitals here? Is that a specific technical term that is always all capitals like that? (In reply to comment #9) > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:73 > > + // FIXME: Ideally (for MATH fonts) we would only need to store the Glyph indices here. > > Why is MATH all capitals here? Is that a specific technical term that is always all capitals like that? "MATH fonts" means "OpenType fonts with a MATH table", as used in Microsoft's document "The MATH table and OpenType Features for Math Processing" (not public yet) and in the call for proposal http://mpeg.chiariglione.org/standards/mpeg-4/open-font-format/call-proposals-isoiec-14496-22-open-font-format-color-font. I think M, A, T, H are just the four letters used as the OpenType tag of the corresponding table. Created attachment 226615 [details]
Patch
Comment on attachment 226615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226615&action=review thanks > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:111 > + void setNormal() I would call these method set*Mode() (setNormalMode(), etc) (In reply to comment #9) > I am not sure that a const GlyphData& is better here. In the past we almost always passed structures by const& but more recently Anders has said that we can get good performance copying simple structures as long as they are truly simple and GlyphData is one of those super-simple structures. Anders, care to comment? I'll wait tomorrow, in case Anders has a comment on these. Comment on attachment 226615 [details] Patch Attachment 226615 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5275410250072064 New failing tests: mathml/very-large-stretchy-operators.html mathml/presentation/mo-stretch.html Created attachment 226649 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 226659 [details]
Patch
Committed r165608: <http://trac.webkit.org/changeset/165608> |