Bug 152244 - Add separate MathOperator module for selection/measuring/drawing of stretchy operators
Summary: Add separate MathOperator module for selection/measuring/drawing of stretchy ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 152242 156538 156542 156571 156572 156574 156906 156910 156913 156921 156950 157071
Blocks: 99614 153987 155018 156836 157519 157568 158866
  Show dependency treegraph
 
Reported: 2015-12-14 01:45 PST by Frédéric Wang (:fredw)
Modified: 2016-07-03 14:07 PDT (History)
16 users (show)

See Also:


Attachments
WIP Patch (20.06 KB, patch)
2015-12-18 13:48 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (69.81 KB, patch)
2015-12-19 02:27 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (69.68 KB, patch)
2015-12-22 03:08 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (74.09 KB, patch)
2015-12-22 12:28 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (74.86 KB, patch)
2015-12-23 00:06 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (403.23 KB, patch)
2016-03-07 00:53 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (404.56 KB, patch)
2016-03-11 01:25 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (321.81 KB, patch)
2016-03-31 08:18 PDT, Frédéric Wang (:fredw)
alex: review-
Details | Formatted Diff | Diff
Patch (322.75 KB, patch)
2016-04-09 11:53 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (320.07 KB, patch)
2016-04-10 14:46 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (322.72 KB, patch)
2016-04-13 07:14 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (316.37 KB, patch)
2016-04-14 12:25 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (632.30 KB, application/zip)
2016-04-14 13:18 PDT, Build Bot
no flags Details
Patch (323.43 KB, patch)
2016-04-14 14:02 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (322.12 KB, patch)
2016-04-15 03:35 PDT, Frédéric Wang (:fredw)
alex: review-
Details | Formatted Diff | Diff
Patch (272.19 KB, patch)
2016-04-25 09:15 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (265.07 KB, patch)
2016-04-27 08:11 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (265.70 KB, patch)
2016-05-09 00:52 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS testing (322.10 KB, patch)
2016-05-11 02:20 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (265.58 KB, patch)
2016-05-13 10:06 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (265.53 KB, patch)
2016-06-09 08:03 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (265.55 KB, patch)
2016-06-09 14:01 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (265.59 KB, patch)
2016-06-13 21:59 PDT, Frédéric Wang (:fredw)
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2015-12-14 01:45:25 PST
The RenderMathMLOperator class contains three independent features:
1) The management of the renderer for operator
2) The search in the MathML operator dictionary
3) The selection/measuring/drawing of size variants & glyph assembly for stretchy operators

It is cleaner and more easily readable/manageable for future improvements to do as in Gecko and split these features into separate modules.

This bug is to move 3) into a separate class.
Comment 1 Frédéric Wang (:fredw) 2015-12-14 07:01:53 PST
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?
Comment 2 Frédéric Wang (:fredw) 2015-12-18 13:48:33 PST
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.
Comment 3 Frédéric Wang (:fredw) 2015-12-19 02:27:48 PST
Created attachment 267684 [details]
WIP Patch
Comment 4 Frédéric Wang (:fredw) 2015-12-22 03:08:19 PST
Created attachment 267784 [details]
WIP Patch
Comment 5 Frédéric Wang (:fredw) 2015-12-22 12:28:50 PST
Created attachment 267795 [details]
WIP Patch
Comment 6 Frédéric Wang (:fredw) 2015-12-23 00:06:42 PST
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.
Comment 7 Frédéric Wang (:fredw) 2016-03-07 00:53:24 PST
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.
Comment 8 Frédéric Wang (:fredw) 2016-03-11 01:25:48 PST
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.
Comment 9 Frédéric Wang (:fredw) 2016-03-31 08:18:54 PDT
Created attachment 275287 [details]
Patch
Comment 10 Martin Robinson 2016-04-04 11:59:38 PDT
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.
Comment 11 Frédéric Wang (:fredw) 2016-04-05 08:39:21 PDT
(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 12 Alejandro G. Castro 2016-04-08 09:12:26 PDT
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 13 Frédéric Wang (:fredw) 2016-04-09 09:40:14 PDT
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/.
Comment 14 Frédéric Wang (:fredw) 2016-04-09 11:53:17 PDT
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.
Comment 15 Frédéric Wang (:fredw) 2016-04-10 14:46:55 PDT
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 16 Manuel Rego Casasnovas - OOO (back 1st Sep) 2016-04-12 09:31:21 PDT
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 17 Manuel Rego Casasnovas - OOO (back 1st Sep) 2016-04-12 09:31:51 PDT
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 18 Frédéric Wang (:fredw) 2016-04-13 07:12:49 PDT
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.
Comment 19 Frédéric Wang (:fredw) 2016-04-13 07:14:40 PDT
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).
Comment 20 Frédéric Wang (:fredw) 2016-04-14 12:25:46 PDT
Created attachment 276412 [details]
Patch
Comment 21 Build Bot 2016-04-14 13:17:57 PDT
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
Comment 22 Build Bot 2016-04-14 13:18:00 PDT
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
Comment 23 Frédéric Wang (:fredw) 2016-04-14 14:02:47 PDT
Created attachment 276425 [details]
Patch
Comment 24 Frédéric Wang (:fredw) 2016-04-15 03:35:49 PDT
Created attachment 276472 [details]
Patch

Just updating the ChangeLog...
Comment 25 Alejandro G. Castro 2016-04-22 09:25:32 PDT
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.
Comment 26 Frédéric Wang (:fredw) 2016-04-22 13:10:30 PDT
(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.
Comment 27 Frédéric Wang (:fredw) 2016-04-25 09:15:49 PDT
Created attachment 277254 [details]
Patch
Comment 28 Frédéric Wang (:fredw) 2016-04-27 08:11:38 PDT
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).
Comment 29 Frédéric Wang (:fredw) 2016-05-09 00:52:25 PDT
Created attachment 278393 [details]
Patch

Updating after https://trac.webkit.org/changeset/200569
Comment 30 Frédéric Wang (:fredw) 2016-05-11 02:20:30 PDT
Created attachment 278607 [details]
Patch for EWS testing
Comment 31 Manuel Rego Casasnovas - OOO (back 1st Sep) 2016-05-13 09:44:12 PDT
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.
Comment 32 Frédéric Wang (:fredw) 2016-05-13 10:06:14 PDT
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.
Comment 33 Frédéric Wang (:fredw) 2016-06-09 08:03:49 PDT
Created attachment 280920 [details]
Patch
Comment 34 Frédéric Wang (:fredw) 2016-06-09 14:01:02 PDT
Created attachment 280943 [details]
Patch
Comment 35 Frédéric Wang (:fredw) 2016-06-13 14:21:01 PDT
Can someone please review this patch?
Comment 36 Brent Fulgham 2016-06-13 15:54:00 PDT
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
Comment 37 Frédéric Wang (:fredw) 2016-06-13 20:53:51 PDT
(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.
Comment 38 Frédéric Wang (:fredw) 2016-06-13 21:59:08 PDT
Created attachment 281233 [details]
Patch
Comment 39 Brent Fulgham 2016-06-16 16:26:53 PDT
Comment on attachment 281233 [details]
Patch

This looks like a nice simplification. r=me.
Comment 40 Frédéric Wang (:fredw) 2016-06-16 21:34:35 PDT
Committed r202156: <http://trac.webkit.org/changeset/202156>