Bug 72828

Created attachment 229368 [details] Another testcase for STIX Math

Created attachment 229369 [details] Patch

Created attachment 229370 [details] Screenshot of the testcase (WebKitGTK+) Screenshot of the testcase with STIX Math fonts and the patch applied. Note: the patch applies on top of bug 130322.

(In reply to comment #2) > Created an attachment (id=229369) [details] > Patch Did you want this to be reviewed?

(In reply to comment #4) > (In reply to comment #2) > > Created an attachment (id=229369) [details] [details] > > Patch > > Did you want this to be reviewed? This is blocked by the MATH font support, so I'd prefer to get feedback on bug 130322 first.

Created attachment 232034 [details] Patch

Created attachment 232035 [details] Patch

Created attachment 232036 [details] Patch

Comment on attachment 232036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232036&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Can you add some ChangeLog comments as to what needed to happen in general to fix this > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1129 > +static const UChar horizontalOperators[] = { can you add a comment as to where this list originated from > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1346 > + m_maxPreferredLogicalWidth = m_minPreferredLogicalWidth = m_leadingSpace + maximumGlyphWidth + m_trailingSpace; i've been told WK style is to not put multiple assignments on the same line (but i couldn't find it in style guidelines) > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1655 > + float baseSize = m_isVertical ? heightForGlyph(baseGlyph) : advanceForGlyph(baseGlyph); should advanceForGlyph be renamed to widthForGlyph to match (heightForGlyph)? > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1692 > + glyphBounds = boundsForGlyph(m_stretchyData.right()); can you put some newlines between sections here, or refactor so you can use a method to re-use this code. It looks like there's some duplication here that could be collapsed > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:114 > + GlyphData left() const { return m_data[2]; } left and bottom are the same data?

Created attachment 232483 [details] Patch

(In reply to comment #9) > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1655 > > + float baseSize = m_isVertical ? heightForGlyph(baseGlyph) : advanceForGlyph(baseGlyph); > > should advanceForGlyph be renamed to widthForGlyph to match (heightForGlyph)? > I prefer to keep "advance" since that's really what widthForGlyph is returning. "width" would correspond to boundsForGlyph(data).width(). Concretely, we also take into account the space around the ink of the glyph, so a U+0020 SPACE has non-zero width. I don't know what's the best for math operators but that's what computePreferredLogicalWidths would return for normal text. > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1692 > > + glyphBounds = boundsForGlyph(m_stretchyData.right()); > > can you put some newlines between sections here, or refactor so you can use a method to re-use this code. It looks like there's some duplication here that could be collapsed > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:114 > > + GlyphData left() const { return m_data[2]; } > > left and bottom are the same data? Yes, that's on purpose. The operator is either horizontal or vertical, so there is no overlap.

(In reply to comment #11) > (In reply to comment #9) > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1655 > > > + float baseSize = m_isVertical ? heightForGlyph(baseGlyph) : advanceForGlyph(baseGlyph); > > > > should advanceForGlyph be renamed to widthForGlyph to match (heightForGlyph)? > > > > I prefer to keep "advance" since that's really what widthForGlyph is returning. "width" would correspond to boundsForGlyph(data).width(). Concretely, we also take into account the space around the ink of the glyph, so a U+0020 SPACE has non-zero width. I don't know what's the best for math operators but that's what computePreferredLogicalWidths would return for normal text. > > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1692 > > > + glyphBounds = boundsForGlyph(m_stretchyData.right()); > > > > can you put some newlines between sections here, or refactor so you can use a method to re-use this code. It looks like there's some duplication here that could be collapsed > > > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:114 > > > + GlyphData left() const { return m_data[2]; } > > > > left and bottom are the same data? > > Yes, that's on purpose. The operator is either horizontal or vertical, so there is no overlap. Looks like patch doesn't apply, otherwise seems ok

Committed r169607: <http://trac.webkit.org/changeset/169607>

Tests added here have missing results. Can you please land the results?

(In reply to comment #14) > Tests added here have missing results. Can you please land the results? I only had the references for GTK. I tried to mark the other platforms as failing, but it seems that the bots still want the results. Can someone generate the references for other platforms? Or should I just copy those from GTK?

(In reply to comment #15) > (In reply to comment #14) > > Tests added here have missing results. Can you please land the results? > > I only had the references for GTK. I tried to mark the other platforms as failing, but it seems that the bots still want the results. Can someone generate the references for other platforms? Or should I just copy those from GTK? You should be able to get them from the bots. I'm not sure if webkit-patch rebaseline can get new results or not.

Committed r169643: <http://trac.webkit.org/changeset/169643>

Committed r169644: <http://trac.webkit.org/changeset/169644>

What are the expected results for opentype-stretchy-horizontal? How can I tell whether the test fails of passes? I'm getting different results on some OS X versions, and I don't know if that's an improvement or a regression.

(In reply to comment #19) > What are the expected results for opentype-stretchy-horizontal? How can I tell whether the test fails of passes? > > I'm getting different results on some OS X versions, and I don't know if that's an improvement or a regression. The rendering should look like something like LayoutTests/platform/gtk/mathml/opentype/opentype-stretchy-horizontal-expected.png: - one rectangle glyph (actually perhaps square). - one rectangle glyph of larger width. - a wider construction with left/middle/right pieces and connecting rectangle glyphs. All the glyphs should be taken from stretchy.woff font. This corresponds to stretching horizontally an operator using the size variants (first two rectangles) or construction provided in the OpenType MATH table.