Bug 152244

Here is a first analysis of how to split the class after bug 152242. At the moment there are some overlap with the member variable (e.g. m_stretchyData) or function (e.g. stretchTo), which makes things unclear. RenderMathMLOperator class: struct StretchyCharacter static const short leftRightPairsCount static const StretchyCharacter stretchyCharacters setOperatorFlagAndScheduleLayoutIfNeeded setOperatorFlagFromAttribute setOperatorFlagFromAttributeValue setOperatorPropertiesFromOpDictEntry setOperatorProperties isChildAllowed stretchTo? resetStretchSize firstLineBaseline computePreferredLogicalWidths computeLogicalHeight (we should probably move that to a new "layout" function, so that we can set the actual width too) rebuildTokenContent updateTokenContent updateFromElement updateOperatorProperties shouldAllowStretching hasOperatorFlag isLargeOperatorInDisplayStyle stretchSize isVertical updateStyle trailingSpaceError (to be removed when we compute the actual width in "layout" correctly and rewrite RenderMathMLRoot) paintChildren StretchyOperator class: paintGlyph boundsForGlyph heightForGlyph advanceForGlyph maxWidth (to be used in computePreferredLogicalWidths) getGlyphAssemblyFallBack getDisplayStyleLargeOperator findStretchyData fillWithVerticalExtensionGlyph fillWithHorizontalExtensionGlyph paint paintVerticalGlyphAssembly paintHorizontalGlyphAssembly stretchTo?

Created attachment 267648 [details] WIP Patch This is an untested WIP patch to add a StretchyOperator class with the selection/measuring of stretchy operator. Painting is not done yet.

Created attachment 267684 [details] WIP Patch

Created attachment 267784 [details] WIP Patch

Created attachment 267795 [details] WIP Patch

Created attachment 267833 [details] WIP Patch I'm now more or less happy with this patch. However, there are still some small differences that I suspect could only be fixed with the new MathML layout model (in particular, handling the difference between preferred width & final width when positioning the horizontal stretchy operators). We also need to run the tests, update the references and build on other platforms. I won't go back to this until we merge the MathML layout branch.

Created attachment 273166 [details] Patch A new version of the patch that applies on top of the patch of bug 153918 and all the other dependencies.

Created attachment 273704 [details] Patch I'm uploading updated version of the MathML refactoring patches to solve conflicts after bug 154388 and bug 155005 ; and to do other minor changes.

Created attachment 275287 [details] Patch

Can you expand on why this changes rendering results. If possible it would be much better to split this code into two patches. One that does the refactoring and one that changes the behavior.

(In reply to comment #10) > Can you expand on why this changes rendering results. If possible it would > be much better to split this code into two patches. One that does the > refactoring and one that changes the behavior. The changes that I see in the screenshots are better stretch size and baseline alignment and maybe small shift differences. Unfortunately, I'm not sure I can explain all the rendering changes. Rather than just moving code, I really rewrote a new StretchyOperator class from scratch and import some of the logic. I was never able to get perfect compatibility with the old behavior and there were always some subtle adjustments to avoid completely breaking tests or compilation ; even if I succeed I doubt that will make review easier. The one intentional change that I did is for drawing horizontal glyph assembly where we now align all the pieces on the same baseline. IRC, the old code align them with respect to their top edge. This did not work for horizontal braces with some of the math fonts (but not the one used for the test).

Comment on attachment 275287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275287&action=review LGTM, I like the approach and the patch, it fixes the problem of having to use a render operator just to paint an operator glyph. I have doubts about the name of the class because we are always creating a StretchyOperator even when the operator does not have to be stretchy, maybe OperatorGlyph is a better name? > Source/WebCore/ChangeLog:8 > + We introduce a class to select, measure and draw stretchy operators that is independent from RenderMathMLOperator. That way, we will be able use stretchy operator without having to introduce & manage anonymous RenderMathMLOperator's (e.g for <mroot>, <msqrt> and <mfenced>). Probably it is a good idea to wrap the line. > Source/WebCore/mathml/MathMLInlineContainerElement.cpp:67 > if (is<RenderMathMLRow>(childRenderer)) > downcast<RenderMathMLRow>(*childRenderer).updateOperatorProperties(); Should we remove then these lines now that the math/row is handled in the other part of the if? > Source/WebCore/mathml/MathMLTextElement.cpp:71 > if (is<RenderMathMLOperator>(renderer())) > - downcast<RenderMathMLOperator>(*renderer()).setOperatorFlagAndScheduleLayoutIfNeeded(MathMLOperatorDictionary::Stretchy, value); > + downcast<RenderMathMLBlock>(*renderer()).updateFromElement(); The if checks if the renderer is an operator but we later downcast to block? > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:54 > + , m_stretchyOperator(&RenderMathMLOperator::style()) I see the StretchyOperator has a reference to the operator RenderStyle, considering it is more a helper than a class of the render tree could we avoid that reference and just update the information when we initialize using the style or passing directly the required values. That way we avoid references. > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:129 > + if (renderOperator && renderOperator->hasOperatorFlag(MathMLOperatorDictionary::Stretchy) && renderOperator->isVertical()) Can we create a isVerticalStretchy function for the Operator if we have this condition in other places? > Source/WebCore/rendering/mathml/StretchyOperator.cpp:26 > +#define _USE_MATH_DEFINES 1 Where is this define used here? > Source/WebCore/rendering/mathml/StretchyOperator.cpp:479 > + while (lastPaintedGlyphRect.maxY() < to.y()) { > + lastPaintedGlyphRect = paintGlyph(info, m_assembly.extension, glyphOrigin, TrimTopAndBottom); We are painting the exact same glyph multiple times, probably this is more costly than paint once and just copy the pattern many times? Maybe we can add a FIXME if we do not address this now. > Source/WebCore/rendering/mathml/StretchyOperator.h:98 > + bool getBaseGlyph(GlyphData& baseGlyph) const; The name of the parameter is not required. > Source/WebCore/rendering/mathml/StretchyOperator.h:113 > + RenderStyle* m_style; As I said before maybe we need a RefPtr or even avoid having the style here and just pass all the information in the init.

Comment on attachment 275287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275287&action=review >> Source/WebCore/ChangeLog:8 >> + We introduce a class to select, measure and draw stretchy operators that is independent from RenderMathMLOperator. That way, we will be able use stretchy operator without having to introduce & manage anonymous RenderMathMLOperator's (e.g for <mroot>, <msqrt> and <mfenced>). > > Probably it is a good idea to wrap the line. Oops, I forgot that one. >> Source/WebCore/mathml/MathMLInlineContainerElement.cpp:67 >> downcast<RenderMathMLRow>(*childRenderer).updateOperatorProperties(); > > Should we remove then these lines now that the math/row is handled in the other part of the if? Here is the explanation in the changelog. "The base wrapper of the <msqrt> element is actually the last child in the render tree. Selecting the correct wrapper makes presentation/mo-form-dynamic.html pass. The <math> element does not have anonymous child but is already a RenderMathMLRow, so we remove the special case for it." In bug 275997, we remove the anonymous wrapper of msqrt so we indeed will be able to remove these lines. >> Source/WebCore/mathml/MathMLTextElement.cpp:71 >> + downcast<RenderMathMLBlock>(*renderer()).updateFromElement(); > > The if checks if the renderer is an operator but we later downcast to block? I guess that's enough to cast to block since we now call updateFromElement but we can just do RenderMathMLOperator everywhere. >> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:54 >> + , m_stretchyOperator(&RenderMathMLOperator::style()) > > I see the StretchyOperator has a reference to the operator RenderStyle, considering it is more a helper than a class of the render tree could we avoid that reference and just update the information when we initialize using the style or passing directly the required values. That way we avoid references. I initially tried to store RefPtr but that did not work well. I should probably try again or pass style parameters everywhere. >> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:129 >> + if (renderOperator && renderOperator->hasOperatorFlag(MathMLOperatorDictionary::Stretchy) && renderOperator->isVertical()) > > Can we create a isVerticalStretchy function for the Operator if we have this condition in other places? I'm not sure it is or will be used elsewhere... >> Source/WebCore/rendering/mathml/StretchyOperator.cpp:26 >> +#define _USE_MATH_DEFINES 1 > > Where is this define used here? That was in the original RenderMathMLOperator file. IIRC, it's necessary for the Windows port to use some math constants. >> Source/WebCore/rendering/mathml/StretchyOperator.cpp:479 >> + lastPaintedGlyphRect = paintGlyph(info, m_assembly.extension, glyphOrigin, TrimTopAndBottom); > > We are painting the exact same glyph multiple times, probably this is more costly than paint once and just copy the pattern many times? Maybe we can add a FIXME if we do not address this now. I suspect in practice, there are only a few glyphs repeated. In theory one could ask arbitrary size to try to hang the browser, I opened bug 155434 for that edge case. Also, I suspect in the future we want to move again most of the logic to the font backend (at least Google devs suggested to work directly on MATH support in Harfbuzz which could be used for Gecko/Blink and WebKit GTK) so that we don't need to perform all these low-level stuff in rendering/.

Created attachment 276087 [details] Patch Here is and updated patch that addresses the feedback. I haven't renamed the class yet but I agree that a better name should be used.

Created attachment 276110 [details] Patch (In reply to comment #12) > LGTM, I like the approach and the patch, it fixes the problem of having to > use a render operator just to paint an operator glyph. I have doubts about > the name of the class because we are always creating a StretchyOperator even > when the operator does not have to be stretchy, maybe OperatorGlyph is a > better name? This is exactly the same patch with s/StretchyOperator/MathOperator/ Currently the class is only used for stretchy operators, but this changes in bug 155018 where we use it for the minus sign or for anonymous RenderMathMLOperator's generated by the <mfenced> element ; so it's probably best to have a more general name.

Comment on attachment 276110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276110&action=review Wow, it's a huge patch, I understand that most of the code is being "just" moved to the new class, however I don't have the knowledge to review this patch sorry. I've added some small comments, and also suggestions to move some parts of this patch to different patches, if possible. > Source/WebCore/ChangeLog:140 > + (WebCore::MathOperator::GlyphAssemblyData::GlyphAssemblyData): Structure to hold pieces > + for glyph assembly. > + (WebCore::MathOperator::stretchSize): stretch size of the operator. This is a super-nit, so feel free to ignore it, but you're using uppercase or lowercase after the ":" in different methods (for example these 2 last ones). Probably we should always use uppercase after ":" to be consistent.

Comment on attachment 276110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276110&action=review > Source/WebCore/ChangeLog:140 > + (WebCore::MathOperator::GlyphAssemblyData::GlyphAssemblyData): Structure to hold pieces > + for glyph assembly. > + (WebCore::MathOperator::stretchSize): stretch size of the operator. This is a super-nit, so feel free to ignore it, but you're using uppercase or lowercase after the ":" in different methods (for example these 2 last ones). Probably we should always use uppercase after ":" to be consistent. > Source/WebCore/mathml/MathMLInlineContainerElement.cpp:65 > + else if (hasTagName(msqrtTag)) { > + // Update operator properties for the base wrapper. > + auto* childRenderer = renderer()->lastChild(); I guess this change could be part of a separated patch. As it seems to be fixing an issue but itself, dunno if it needs the rest of the patch. > Source/WebCore/mathml/MathMLTextElement.cpp:71 > + downcast<RenderMathMLOperator>(*renderer()).updateFromElement(); Again is this needed for the patch? Can it be split to a different patch (maybe it's a follow-up patch after this one)? > Source/WebCore/rendering/mathml/MathOperator.cpp:26 > +#define _USE_MATH_DEFINES 1 This was removed in r194416 so probably it's not needed anymore. > Source/WebCore/rendering/mathml/MathOperator.cpp:57 > +static float advanceForGlyph(const GlyphData& data) Nit: Maybe call this advanceWidthForGlyph() to be more explicit. > Source/WebCore/rendering/mathml/MathOperator.cpp:132 > +bool MathOperator::getBaseGlyph(const RenderStyle& style, GlyphData& baseGlyph) const Remove the "get" prefix. > Source/WebCore/rendering/mathml/MathOperator.cpp:199 > +bool MathOperator::getGlyphAssemblyFallBack(const RenderStyle& style, const Vector<OpenTypeMathData::AssemblyPart>& assemblyParts, GlyphAssemblyData& assemblyData) const Remove "get" prefix. > Source/WebCore/rendering/mathml/MathOperator.cpp:220 > + unsigned state = 1; Wouldn't be better to use an enum with descriptive names for each state, instead of a number with a definition in the previous comment. I think it'd make easier to follow the code bellow. > Source/WebCore/rendering/mathml/MathOperator.cpp:397 > + // than full coverage. These edge pixels can introduce small seams between connected glyphs Nit: Missing dot at the end of the last sentence. > Source/WebCore/rendering/mathml/MathOperator.cpp:414 > + case TrimTopAndBottom: { > + LayoutUnit temp = glyphPaintRect.y() + 1; > + glyphPaintRect.shiftYEdgeTo(temp.ceil()); > + glyphPaintRect.shiftMaxYEdgeTo(glyphPaintRect.maxY().floor() - 1); > + clipBounds.shiftYEdgeTo(glyphPaintRect.y()); > + clipBounds.shiftMaxYEdgeTo(glyphPaintRect.maxY()); > + } Do we really need the curly brackets here? > Source/WebCore/rendering/mathml/MathOperator.cpp:430 > + case TrimLeftAndRight: { > + LayoutUnit temp = glyphPaintRect.x() + 1; > + glyphPaintRect.shiftXEdgeTo(temp.ceil()); > + glyphPaintRect.shiftMaxXEdgeTo(glyphPaintRect.maxX().floor() - 1); > + clipBounds.shiftXEdgeTo(glyphPaintRect.x()); > + clipBounds.shiftMaxXEdgeTo(glyphPaintRect.maxX()); > + } Ditto > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:124 > + auto renderOperator = downcast<RenderMathMLBlock>(child)->unembellishedOperator(); > + if (renderOperator && renderOperator->hasOperatorFlag(MathMLOperatorDictionary::Stretchy) && renderOperator->isVertical()) Could this be done in a separated patch?

Comment on attachment 276110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276110&action=review >> Source/WebCore/rendering/mathml/MathOperator.cpp:132 >> +bool MathOperator::getBaseGlyph(const RenderStyle& style, GlyphData& baseGlyph) const > > Remove the "get" prefix. I'm not sure it's a good idea to remove the get* when the output is passed as a reference variable. So I'll keep it for now. >> Source/WebCore/rendering/mathml/MathOperator.cpp:414 >> + } > > Do we really need the curly brackets here? Otherwise it's complaining about the definition of the temp variable. But I'm going to define it before the switch.

Created attachment 276319 [details] Patch Updated patch addressing Manuel's comments. I'm now going to see if I can move small changes into preliminary patches (although I'm not sure that will help much to review the big move).

Created attachment 276412 [details] Patch

Comment on attachment 276412 [details] Patch Attachment 276412 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1157553 New failing tests: mathml/opentype/horizontal-munderover.html

Created attachment 276415 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5

Created attachment 276425 [details] Patch

Created attachment 276472 [details] Patch Just updating the ChangeLog...

Comment on attachment 276472 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276472&action=review The patch looks good to me but we need to try to separate the changes the modify the tests in a different patch before refactoring, which in theory it is just moving the code. > Source/WebCore/rendering/mathml/MathOperator.cpp:61 > +// FIXME: This hardcoded data can be removed when OpenType MATH font are widely available. Add reference to the bug describing the issue.

(In reply to comment #25) > The patch looks good to me but we need to try to separate the changes the > modify the tests in a different patch before refactoring, which in theory it > is just moving the code. OK, I've put the biggest code move (essentially all without the metrics improvements) into bug 156906, bug 156910, bug 156913, bug 156921 and this does not change the test results.

Created attachment 277254 [details] Patch

Created attachment 277478 [details] Patch > The patch looks good to me but we need to try to separate the changes the modify the tests in a different patch before refactoring, which in theory it is just moving the code. So I think this is the best split I can do. Is are my explanations of the test changes (although the MathML code does weird things and we can not be 100% sure): 1) For largeop, I removed the STIX hack so that m_maxPreferredWidth is set to the same value as m_width (in MathOperator::findDisplayStyleLargeOperator). 2) The new code tries to be more accurate by using the real m_ascent / m_descent / m_width of the MathOperator instead of just using the request stretch sizes. By design of the TeX / MATH table rules, the resulting stretched operator can be slightly larger than the target size. 3) For the painting of horizontal assembly, I fixed the bad vertical alignment of pieces and align them all on the same baseline. I'm not sure that affects the tests though, but I found it in some existing fonts that have pieces of different sizes. 4) stretchTo changes the width / ascent / descent of the operator. It's incorrect to do that and the old code cheated by not calling setPreferredLogicalWidthsDirty(false) after RenderMathMLOperator::computePreferredLogicalWidths so that this preferred width changes after a stretchTo call (note that the function uses stretchSize()). This makes the result unpredictable... Now I'm just using the width of the base size for horizontal operators (in the future, we could change RenderUnderOver::computePreferredLogicalWidths to do a "stretchTo" and estimate the stretch size, but I guess it is fine to just keep the maximum width of non-stretchy under/over/base).

Created attachment 278393 [details] Patch Updating after https://trac.webkit.org/changeset/200569

Created attachment 278607 [details] Patch for EWS testing

Comment on attachment 278393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278393&action=review The patch seems ok, but I don't think I got it 100% yet. Anyway I've some minor comments inline. > Source/WebCore/ChangeLog:21 > + (WebCore::MathOperator::calculateDisplayStyleLargeOperator): Remove the STIX Word hack and change m_maxPreferredWidth to use the actual width instead. Nit: Wrap this line or not? I'm not sure how many chars are you using. > Source/WebCore/ChangeLog:24 > + (WebCore::MathOperator::fillWithHorizontalExtensionGlyph): Align all the glyph baselines on the same axis, given by m_ascent. Nit: 2 spaces after ":". > Source/WebCore/rendering/mathml/MathOperator.cpp:386 > +void MathOperator::stretchTo(const RenderStyle& style, LayoutUnit ascent, LayoutUnit descent) Wouldn't be better to rename this 2 methods to something more specific like "stretchVerticalOperatorTo()" or something like that. That would make simpler to differentiate between 2 calls like: stretchTo(style(), 20, 40); stretchTo(style(), 20); vs stretchVerticalOperatorTo(style(), 20, 40); stretchHorizontalOperatorTo(style(), 20); > Source/WebCore/rendering/mathml/MathOperator.cpp:483 > + // FIXME: In practice, only small stretch size are requested but we may need Nit: s/size/sizes > Source/WebCore/rendering/mathml/MathOperator.cpp:523 > + // FIXME: In practice, only small stretch size are requested but we may need Ditto.

Created attachment 278844 [details] Patch New patch addressing Manuel's comments. > Wouldn't be better to rename this 2 methods to something more specific like "stretchVerticalOperatorTo()" or something like that. Actually I realized that the stretching of symmetric operators is not quite correct... I believe instead we should only have one function MathOperator::stretchTo to stretch to a target size (width or height) and it will be up to RenderMathOperator to do the proper vertical alignment with respect to the math axis (and not the baseline). But let's keep the current behavior and change that after bug 133567.

Created attachment 280920 [details] Patch

Created attachment 280943 [details] Patch

Can someone please review this patch?

Comment on attachment 280943 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280943&action=review I think this looks good, and the tests all look correct. However, I think Zalan, Simon, or Hyatt should give it a quick look to confirm the rendering code is done properly. > Source/WebCore/rendering/mathml/MathOperator.cpp:54 > +} It might be more efficient to return both ascent and descent at the same time, since "boundsForGlyph" returns a newly-created FloatRect. And while 'boundsForGlyph' is always inlined, it looks complicated enough that calling it back-to-back seems wasteful. > Source/WebCore/rendering/mathml/MathOperator.cpp:110 > + m_descent = descentForGlyph(baseGlyph); ascent-followed-by-descend. > Source/WebCore/rendering/mathml/MathOperator.cpp:138 > + m_descent = descentForGlyph(sizeVariant); ascent-followed-by-descent

(In reply to comment #36) > Comment on attachment 280943 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280943&action=review > > I think this looks good, and the tests all look correct. However, I think > Zalan, Simon, or Hyatt should give it a quick look to confirm the rendering > code is done properly. > Thank for the review. Yes, I think that will definitely help if they have time. However, I'd like to highlight that this is just one small step of the current MathML refactoring. The idea here is to move the stretchy operator code out of the RenderMathMLOperator class so that it can be used by the renderer for roots without the need of creating anonymous... but it does change too much the current rendering code. So I'm not sure the MathML rendering code is done "properly" yet but hopefully that will be better after phase 1 of the refactoring (remaining bugs: ​bug 156836, ​bug 153987, bug 157519, ​bug 157521, ​bug 157568 and ​bug 155018). > > Source/WebCore/rendering/mathml/MathOperator.cpp:54 > > +} > > It might be more efficient to return both ascent and descent at the same > time, since "boundsForGlyph" returns a newly-created FloatRect. And while > 'boundsForGlyph' is always inlined, it looks complicated enough that calling > it back-to-back seems wasteful. > OK, I'll try it, thanks.

Created attachment 281233 [details] Patch

Comment on attachment 281233 [details] Patch This looks like a nice simplification. r=me.

Committed r202156: <http://trac.webkit.org/changeset/202156>