Bug 179682

Created attachment 327598 [details] Patch

As you can see from the patch, one test case is updated: mathml/opentype/opentype-stretchy-horizontal.html I have updated it for platform "mac". How could I update test results for other platforms as well?

Created attachment 327599 [details] Patch

Attachment 327599 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 327600 [details] Patch

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

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

The iOS simulator has the old expectation, so the failure is expected. Am I expected to simply delete the expectation file? Also, gtk also contains an outdated expectation file, but it is not included in EWS

Comment on attachment 327600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327600&action=review Thanks Minsheng. I think we will really have to rewrite the function (see below). For the tests, you can edit TestExpectations files. There are platform-specific expectations in Layout/platform: fred@igalia-fwang:~/src-obj/webkit/WebKit/LayoutTests$ find . -type f -name TestExpectations ./platform/ios-device-wk2/TestExpectations ./platform/gtk-wk2/TestExpectations ./platform/mac/TestExpectations ./platform/wk2/TestExpectations ./platform/ios-wk2/TestExpectations ./platform/ios-simulator/TestExpectations ./platform/ios-wk1/TestExpectations ./platform/ios-simulator-wk1/TestExpectations ./platform/ios-simulator-wk2/TestExpectations ./platform/ios-device/TestExpectations ./platform/gtk/TestExpectations ./platform/mac-wk2/TestExpectations ./platform/ios-device-wk1/TestExpectations ./platform/win/TestExpectations ./platform/gtk-wayland/TestExpectations ./platform/wpe/TestExpectations ./platform/ios/TestExpectations ./platform/mac-wk1/TestExpectations ./platform/mac-elcapitan/TestExpectations ./TestExpectations And there are corresponding test expectations (e.g. platform/ios/mathml/radical-fallback-expected.txt) that you can update too. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:54 > { Can you add an ASSERT(isValid()) here? Then we are sure that there are exactly 2 or 3 children. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:57 > + Vector<RenderBox*, 2> stretchingBlocks; Maybe, we should use 3 as the initial size here, since that's the maximum we can get. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:58 > So I think we should maybe try to understand the algorithm we want, check with the Math WG and explain with comments here. See https://www.w3.org/TR/MathML3/chapter3.html#id.3.2.5.8.2. For vertical operators they say "cover ... non-stretchy direct sub-expressions" but for horizontal operators, they say "cover ... other direct sub-expressions" which seems inconsistent, we should ask them clarification. Later they indicate that "If a stretchy operator is required to stretch, but all other expressions in the containing element (as described above) are also stretchy, all elements that can stretch should grow to the maximum of the normal unstretched sizes of all elements in the containing object, if they can grow that large". The "Skipping the embellished op" comment suggests that it won't work if we skip non-stretchy (as we do for the vertical rule in RenderMathMLRow::computeLineVerticalStretch) and indeed we should really consider the non-strechy parts of an embellished operators for the size computation. Note that the Math WG is "on hold" ( https://lists.w3.org/Archives/Public/www-math/2016Jul/0001.html ) so we should probably not wait for them to reply soon. In any case, there is an issue here: The target stretch width calculated here might change after a second layout now that RenderMathMLOperator::stretchTo changes the logical width to give the correct stretched size of the operator instead of the unstretched size. I think we will have no other choice than calculating the target size again with unstretched operators. Performance will probably not be good when we have many nested embellished operators, but that's not common in practice and I think Gecko does something similar. So for now in this bug, I think we should just rewrite things that way: 1) If none of the child is a horizontal stretchy operators, then just call layoutIfNeeded() on children and exit. 2) If there is at least one horizontal stretchy operator, then * reset the stretch size for all of them and force them to relayout. * call layoutIfNeeded on other children. * calculate the target width as the maximum of all child->logicalWidth() [note that at this point, the operators are unstretched] * call stretchTo on horizontal stretchy operators. * force them to relayout again with the correct size [probably you can skip that for direct <mo> children, as you did here].

Created attachment 327641 [details] a corner case I am still trying to processing your comment and the standard, but I just realize some other corner case. Consider \over{some short equation}{\over{long horizontal brace}{some long text}} The brace shall not be stretched wider than the equation, but our algorithm does! > calculate the target width as the maximum of all child->logicalWidth() So is the existing implementation in WebKit. By the way, Firefox handles it correct.

Created attachment 327642 [details] a corner case sorry the previous example contains inappropriate language

The standard covers this case: > should stretch to cover the width of the **other** direct sub-expressions in the given element (or in the same table column), given the constraints mentioned above.

(In reply to Minsheng Liu from comment #11) > Created attachment 327642 [details] > a corner case > > sorry the previous example contains inappropriate language Good point. Note that the brace in <mover> <mo>⏞</mo> <mrow><mi>k</mi><mspace width="thinmathspace"></mspace><mtext>some long text goes here, really long</mtext></mrow> </mover> renders the same width as the long expression in Gecko and WebKit. With, <mover> <mrow> <mi>x</mi> <mo>+</mo> <mo>...</mo> <mo>+</mo> <mi>x</mi> </mrow> <mover> <mo>⏞</mo> <mrow><mi>k</mi><mspace width="thinmathspace"></mspace><mtext>some long text goes here, really long</mtext></mrow> </mover> </mover> the brace is embellished and should stretch to cover the maximum width of the "other direct sub-expressions" per the MathML spec. So you are right, we should exclude the width of the inner mover for the stretching of the outer mover. (I think we should actually only include non-stretchy operators but the spec only says that for vertical stretching and the "Skipping the embellished op" in our code says we shouldn't, so I'm not sure...) With the algorithm I proposed and taking into account "other", the brace will initially have the wide size when laying out the inner mover but then will recover its final size when laying out the outer mover. IIRC, Gecko does something more clever and delay the stretching of the operator until we arrive at the highest level of the embellished op hierarchy for the operator. But maybe that's a bit more difficult to do.

Created attachment 327649 [details] <munderover> with narrow base Consider this example of <munderover> with a narrow base. I think you would agree that the stretchy operator should not stretch any more than the base, as adopted in the math typesetting convention. So, it is not really “cover the width of the other direct sub-expressions in the given element” but the base sub-expression. Firefox gives the desired result. If we stretch to base width when necessary, I would say just throw away the for loop and do things manually. How do you think?

My proposed algorithm would then be: 1. layout base() and gets its width, which I will refer to as stretchWidth 2. check if under() exists, if so, stretch its embellished operator (if exists) to stretchWidth, and then layout under() 3. check if over() exists, if so, stretch its embellished operator (if exists) to stretchWidth, and then layout over() The nested case is also handled, consider \over{base}{\over{brace}{comment}} By the time we are layouting the inner over, the brace’s logical width has already been set, and we layout brace, which does nothing (Does it change anything, like reverting the logical size to the default? I am unsure of the code base and unfamiliar with the rendering process. Maybe I should take a look at it.), and then we layout the comment part, which might or might not stretch to the brace’s width depending on whether it is an embellished operator for the brace.

Using the base as the stretch width won't work for simple case like <mrow> <mi> x </mi> <munder> <mo> &#x2192;<!--RIGHTWARDS ARROW--> </mo> <mtext> maps to </mtext> </munder> <mi> y </mi> </mrow> taken from https://www.w3.org/TR/MathML/chapter3.html#presm.horiz.stretch.rules I think in order to make you use case work, we really need to consider only the width of non-stretchy operators (as the spec says for vertical stretching). But again, I wonder about the "Skipping embellished op" comment in the WebKit source. I'm not sure I understand your comment, but note that the layout is performed bottom-to-top. So we first layout inner expression and then outer expression. Also, if you modify some parts of the rendering tree (via JS, CSS etc) the whole layout is not necessarily performed again, for optimization purpose. So you have to think that operators might be already laid out / stretched and their logical widths set and hence you might need to unstretch them and force a relayout if necessary.

Here is my latest solution: 1. layout all sub-expressions and set the stretchWidth to be the maximal logical width 2. stretch base() or the embellished op at base()’s core to stretchWidth 3. stretch under()/over() or the embellished ops at their cores to base()’s logical width It satisfies and violates the standard in the following way: * This algorithm’s output only stretches stretchy operators to non-stretchy parts’ maximal width. This can be proven inductively. After step 1, each sub-expression satisfies the invariant so those sub-expressions’ logical width is the same as the maximal width of non-stretchy parts. Subsequent stretching operations only stretch stretchy operators to the maximal width of the logical width of those sub-expressions, i.e. non-stretch parts’ maximal width. * This algorithm violates the "other direct sub-expressions" condition we discuss earlier. More specifically, it stretches an embellished operator inside base() to the maximal width of not only over()/under()—which are “other direct sub-expressions”—but also base() itself. The motivation is for the following test case: <mrow> <mi> x </mi> <mover> <munder> <mo> &#x2192;<!--RIGHTWARDS ARROW--> </mo> <mtext> some long long text </mtext> </munder> <mtext> maps to </mtext> </mover> <mi> y </mi> </mrow> If we look at the outer <mover>, then the operator should stretch to as wide as the shorter text “maps to”, which I found quite strange. How do you think? Note that Firefox does not follow my algorithm’s behavior and sticks with the standard in this situation. In any case, my algorithm could be easily changed to follow Firefox’s practice: in step 1, just set the stretchWidth to be max(over()->logicalWidth, under()->logicalWidth()). <mover> <mrow> <mi>x</mi> <mo>+</mo> <mo>...</mo> <mo>+</mo> <mi>x</mi> </mrow> <mover> <mo>⏞</mo> <mtext>some long over text goes here, really long</mtext> </mover> </mover> Namely, the brace is as wide as the equation but shorter than the text comment. * For under()/over(), this algorithm violates the "other direct sub-expressions" condition as it really just stretches the embellished operators to the width of the base, the motivation behind which is justified by the attachment “<munderover> with narrow base”.

Created attachment 327719 [details] the algorithm in action (just for visual comparison, not a patch to review) I add a helper variable to RenderMathMLOperator to lock the stretch width of those operators. The motivation should hopefully be lucid: after stretching the operator and re-layout its ancestors, the recursive call will automatically revert the stretched width, an undesired behavior. The other trick is that I set stretchWidth to base()’s width in the second loop. It is a bit tricky (I am not sure if such code is welcome here for WebKit) but it reduces code reduplication.

Comment on attachment 327719 [details] the algorithm in action (just for visual comparison, not a patch to review) View in context: https://bugs.webkit.org/attachment.cgi?id=327719&action=review Thanks for working on this. Good point about the lock/unlock. I've added comments to make it closer to the MathML spec / Firefox and less hacky. Can you please try it, test with the various testcases and run the MathML tests? > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:194 > + This should be setStretchWidthLocked(bool stretchWidthLocked) const { m_isStretchedWidthLocked = stretchWidthLocked; }. Also setStretchWidthLocked and isStretchWidthLocked are small getter/setter so they can just be moved into the *.h file. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:55 > + ASSERT(isValid()); Can you also repeat the ASSERT(needsLayout()) here? So that we are sure that the extra work in this function only happens when layout is needed. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:56 > + You might also consider introducing a helper function: RenderOperator* toHorizontalStretchyOperator(RenderBox* box) { if (is<RenderMathMLBlock>(box)) { if (auto renderOperator = downcast<RenderMathMLBlock>(*box).unembellishedOperator()) { if (renderOperator->isStretchy() && !renderOperator->isVertical()) return renderOperator; } } return nullptr; } > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:-65 > - } Can you skip non-stretchy operators with? if (toHorizontalStretchyOperator(child)) continue; This is copying the vertical stretchy rule "of the *non-stretchy* direct sub-expressions" of 3.2.5.8.2, but for horizontal stretching. That will cover the case of stretchy under/over and a non-stretchy base. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:62 > + I think you should first count the number of children satisfying toHorizontalStretchyOperator(child). If it is < number of children then you run the loop above and can even exit just after if it is == 0. It is is == number of children then you can instead do: for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { auto renderOperator = toHorizontalStretchyOperator(child); ASSERT(renderOperator); renderOperator->resetStretchSize(); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); renderOperator->setStretchWidthLocked(false); stretchWidth = std::max(stretchWidth, child->logicalWidth()); } This will address the case "Skipping the embellished op does not work for nested structures like <munder><mover><mo>_</mo>...</mover> <mo>_</mo></munder>" where elements should stretch to the max unstretched size I guess. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:66 > + if (renderOperator->isStretchy() && !renderOperator->isVertical() && !renderOperator->isStretchWidthLocked()) { You can use if (auto renderOperator == toHorizontalStretchyOperator()) { if (!renderOperator->isStretchWidthLocked()) { > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:-74 > - stretchWidth = std::max(stretchWidth, child->logicalWidth()); Maybe move the comment in the loop I mentioned above. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:80 > + stretchWidth = firstChildBox()->logicalWidth(); Then you don't need this hack anymore, as it is again addressed above.

While I am working on the rest of your review, could I ask a simple stupid question: for your suggested helper function toHorizontalStretchyOperator, where should I put it? As a static member of some class or just a normal function? I plan to just put inside the cpp file, but WebKit’s compiling options force me to declare a prototype. Currently I just have something like RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox* box); RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox* box) { if (is<RenderMathMLBlock>(box)) { if (auto renderOperator = downcast<RenderMathMLBlock>(*box).unembellishedOperator()) { if (renderOperator->isStretchy() && !renderOperator->isVertical()) return renderOperator; } } return nullptr; } But that seems stupid…What is the common practice regarding those types of helper functions?

(In reply to Minsheng Liu from comment #20) > But that seems stupid…What is the common practice regarding those types of > helper functions? mmh, you should not need do declare the prototype. See for example static bool shouldScaleColumnsForParent(const RenderTable& table) in Source/WebCore/rendering/AutoTableLayout.cpp. Otherwise, you can also declare it as a private function member, but that does not seem necessary here.

I am unsure about your stance on the following case: <math display="block"> <mrow> <munder><mover><mo>→</mo><mtext>hello world</mtext></mover> <mo>⏞</mo></munder> </mrow> </math> \under{\over{\arrow}{hello world}}{\topbrace} According to the standard, the \arrow will stretch to the maximal width of *other* sub-expressions. And since the only other sub-expression is the \topbrace, we stretch \arrow to the un-stretchy size of \topbrace. Firefox obeys the standard on this particular case (you can check it out yourself). I do not find that result intuitive/appealing though. What do you think?

(In reply to Minsheng Liu from comment #22) > I am unsure about your stance on the following case: > > <math display="block"> > <mrow> > <munder><mover><mo>→</mo><mtext>hello world</mtext></mover> > <mo>⏞</mo></munder> > </mrow> > </math> > > \under{\over{\arrow}{hello world}}{\topbrace} > > According to the standard, the \arrow will stretch to the maximal width of > *other* sub-expressions. And since the only other sub-expression is the > \topbrace, we stretch \arrow to the un-stretchy size of \topbrace. Firefox > obeys the standard on this particular case (you can check it out yourself). > I do not find that result intuitive/appealing though. What do you think? I'm not exactly sure, but I think this falls into "maximum of the normal unstretched sizes of all elements in the containing object" (3.2.5.8.4). What's the unstretched size of the embellished op \over{\arrow}{hello world} is not clear (it may or may not include the width of "hello world"). Let's first try with the changes I proposed and see if that break regression tests or concrete use cases. For edge cases, we could ask on the Math WG mailing list.

I have adopted the first part of your change of layout-ing sub-expressions with their stretchy operators’ widths set to zero. However, due to the <munderover> with narrow base case, the hacky last line cannot be removed. My code now looks like this: void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren() { // The function will be called either "freshly" // or as a fix after an embellished operator is stretched. ASSERT(isValid()); ASSERT(needsLayout()); LayoutUnit stretchWidth = 0; // The loop layouts each sub-expression's width with // embellished stretchy operators' width set to 0, // unless the stretchy operators' stretch widths have been locked. for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { auto renderOperator = toHorizontalStretchyOperator(child); if (renderOperator) { if (!renderOperator->isStretchWidthLocked()) renderOperator->resetStretchSize(); renderOperator->setStretchWidthLocked(true); // This step is necessary if the function is called as a fix, // since the embellished operator could be nested down deeply. // Each direct ancestor of the operator shall be re-layout-ed. child->setNeedsLayout(); } child->layoutIfNeeded(); if (renderOperator) { // Note that during the fix the embellished operator might be locked // and unlocked multiple times, so I change the implementation of the lock // to a counter. renderOperator->setStretchWidthLocked(false); } // Only base() will stretch to the maximum of all other elements, // but the spec requires base() to stretch to the maximum width of elements other than the base(). // A decision is needed. // The behavior is not consistent with Firefox. stretchWidth = std::max(stretchWidth, child->logicalWidth()); } for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { if (auto renderOperator = toHorizontalStretchyOperator(child)) { if (!renderOperator->isStretchWidthLocked()) { renderOperator->resetStretchSize(); renderOperator->stretchTo(stretchWidth); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); renderOperator->setStretchWidthLocked(false); } } // For children other than base(), the stretchWidth is the width of base(). // This is for <munderover> in which we have braces on the top and the bottom. // The braces should be stretched as wide as the base(), not lonoger or shorter. // The behavior is consistent with Firefox. stretchWidth = firstChildBox()->logicalWidth(); } } And it causes no regression other than the expected one. I will provide a review-ready patch (without tests for now) soon.

Created attachment 327767 [details] a new algorithm

(In reply to Minsheng Liu from comment #24) > I have adopted the first part of your change of layout-ing sub-expressions > with their stretchy operators’ widths set to zero. However, due to the > <munderover> with narrow base case, the hacky last line cannot be removed. It does not very look like what I proposed and in particular without addressing the "Can you skip non-stretchy operators" comment you won't be able to remove your hack. Anyway, I'll only go back to this tomorrow.

I must admit that I am confused with your "skip non-stretchy operators". The skipping code you gave was: if (toHorizontalStretchyOperator(child)) continue; which seems more like "skip stretchy operators" than non-stretchy ones. I try to summarize your points (based on my understanding) to make sure that we are on the same page. I checked your original algorithm: > 1) If none of the child is a horizontal stretchy operators, then just call layoutIfNeeded() on children and exit. > 2) If there is at least one horizontal stretchy operator, then > * reset the stretch size for all of them and force them to relayout. > * call layoutIfNeeded on other children. > * calculate the target width as the maximum of all child->logicalWidth() [note that at this point, the operators are unstretched] > * call stretchTo on horizontal stretchy operators. > * force them to relayout again with the correct size [probably you can skip that for direct <mo> children, as you did here]. Compare that with your review comment, I think you are suggesting this algorithm. I would then take your "skip non-stretchy operators" as skipping those operators in step 2.1. However, in my latest patch, I merely combined all step 1, 2.1, 2.2, 2.3 into one loop. In my code, the first loop traverses all children. By default, layoutIfNeeded is called on each child. In addition to that, if a child has a stretchy operator (actually, our code really says that if a child is an embellished horizontal stretchy operator, more on this later.), then the stretch size is reset and the child is forced to relayout. I add some protections so that the code work as expected in recursive cases. Step 2.4 and 2.5 is just the second loop. However, I have to point out that this could not really cover the case of stretchy under/over and a non-stretchy base. You see, fundamentllay speaking, the issue is that stretch width is different for each child. If we take the standard literally, the stretch width for some child x is the maximal width of all children other than x. As a result, I have to set stretch width to something different for each child in the second loop. Now, I realize that the vertical stretching case is actually simpler than our situation. There, the only thing we need to worry about is to let stretchy operators to cover the whole row. Here, things get complicated. Operators in the base have to cover the whole element, while in the case of non-stretchy base and stretchy braces the stretchy brace must cover the base only. Therefore, I made the following three rules: 1. stretch width for op in base = max(width of base, width of over, widht of under) standard says: max(width of over, width of under) 2. stretch width for op in under = width of base standard says: max(width of over, width of base) 2. stretch width for op in over = width of base standard says: max(width of under, width of base) New code with other parts removed and no hack: void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren() { // omitted code LayoutUnit stretchWidthForBase = 0; // The loop layouts each sub-expression's width with // embellished stretchy operators' width set to 0, // unless the stretchy operators' stretch widths have been locked. for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { // omitted code stretchWidthForBase = std::max(stretchWidthForBase, child->logicalWidth()); } LayoutUnit stretchWidthForOthers = firstChildBox()->logicalWidth(); for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { LayoutUnit stretchWidth = child == firstChildBox() ? stretchWidthForBase : stretchWidthForOthers; if (auto renderOperator = toHorizontalStretchyOperator(child)) { if (!renderOperator->isStretchWidthLocked()) { renderOperator->resetStretchSize(); renderOperator->stretchTo(stretchWidth); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); renderOperator->setStretchWidthLocked(false); } } } } I do not quite understand the comment "Skipping the embellished op does not work for nested structures like <munder><mover><mo>_</mo>...</mover> <mo>_</mo></munder>" but my alogrithm does handle that case well. I hope I have understood you correctly and explain myself clearly. If not, could you explain your view, especially those parts I misunderstood you? Thanks. And, our code so far, when detecting the strechy operators, merely detects those embellished. That might be an issue. Consider the following strcutures: <munder><mover><mtext>...</mtext><mo>_</mo></mover><mo>_</mo></munder> The inner <mo> is not an embellished operator, so the code will layout it as if it is not stretchy. When <mo> contains leading/trailing space, this becomes an issue. If the inner <mo> is not stretched, the outer <mo> is stretched to the same width of <mtext>. However, since <mo> is not an embellished operator, the outer <mo> will stretch to <mtext> + leading spaces for <mo>. Without significant change to the code I cannot handle this case.

@Minsheng: I believe things are becoming complicated but we have made some progress. We should keep in mind that the code must be readable, so mixing too many special cases and stretch width computation in the same loop or departing too much from the official spec is not a good idea. Sorry if I added more confusion with my suggestion (BTW, I really meant to skip *stretchy* children). Let's forget what was discussed above for now and focused on a simpler approach, which I believe is closer to Gecko & the spec. What do you think of this: 1) First add isStretchWidthLocked() into the helper function. That way, the algorithm will just treat such operators as non-stretchy and will never reset their stretch size or lock them again (hence you don't need a counter for it). static RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox* box) { ... if (renderOperator->isStretchy() && !renderOperator->isVertical() && !renderOperator->isStretchWidthLocked()) return renderOperator; ... } 2) Let's calculate the stretch width in one loop. Instead of calculating the size of the whole embellished operator as I suggested, let's take the unstretched size of the <mo> at the core. I think this is what Gecko does and will follow what you like in edge cases like in comment 22. Also, that will render attachment 327649 [details] as we want. Finally, we will not "Skip the embellished op" although I'm still not sure what this comment was about so we may just remove it. Last, but not least this avoids to do the costly lock & relayout. // The stretch size is calculated by taking the maximum widths of children. // For stretchy (possibly embellished) operators, the unstretched size of the <mo> operator at the core is considered. // This allows to satisfy the following rules from the MathML specification: // a) "cover the width of the other direct sub-expressions in the given element" // b) "If a stretchy operator is required to stretch, but all other expressions in the containing element [...] are also stretchy, all elements that can stretch should grow to the maximum of the normal unstretched sizes of all elements in the containing object" LayoutUnit stretchWidth = 0; Vector<RenderBox*, 3> embellishedOperators; for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { if (auto renderOperator = toHorizontalStretchyOperator(child)) { renderOperators.append(stretchyOperators); renderOperator->resetStretchSize(); renderOperator->setNeedsLayout(); renderOperaror->layout(); stretchWidth = std::max(stretchWidth, renderOperator->logicalWidth()); } else { child->layoutIfNeeded(); stretchWidth = std::max(stretchWidth, child->logicalWidth()); } } 3) Finally, let's stretch the embellished operators in one loop. // Now that the stretch width has been calculated, let's stretch the (possibly embellished) operators and relayout them. // Note that we lock the stretch width of the <mo> elements at the core of embellished operators so that the stretch size is not modified during the relayout e.g. in // // <munder> // <mtext>Base</mtext> // <munder> // <mtext>LoooongBaaaaase</mtext> // <mo>_</mo> // </munder> // </munder> // for (auto& child : embellishedOperators) { auto renderOperator = toHorizontalStretchyOperator(child); ASSERT(renderOperator); renderOperator->stretchTo(stretchWidth); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); renderOperator->setStretchWidthLocked(false); } Can you please test the code above and tell me the result? I haven't tried it, so I may have made mistakes...

Created attachment 327947 [details] Test file The test file covers all cases I am concerned with. I put it in a single file with comments to facilitate our communication. I hate this bug tracker which lacks code highlight, bold text, in-line image, etc. Therefore, I post everything on an public GitHub issue (https://github.com/notcome/free-markdown-previewer/issues/1), so that you would not miss any details. In case that GitHub bankrupts before Apple does, I copy the whole text below, which should also help you if you wish to do e-mail style citation. -- BELOW IS MAIN TEXT -- I have two versions ready to present, one is very faithful to your suggestion (but you did make many typos) and the other is closer to my intention. Here is the visual comparison:  On the left is Firefox’s result. On the right is your proposal (with typos fixed). In the middle is the combination of my previous work and your proposal. Below is my reproduce of your code: ```C++ void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren() { ASSERT(isValid()); ASSERT(needsLayout()); LayoutUnit stretchWidth = 0; Vector<RenderBox*, 3> embellishedOperators; for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { if (auto renderOperator = toHorizontalStretchyOperator(child)) { embellishedOperators.append(child); renderOperator->resetStretchSize(); child->setNeedsLayout(); child->layout(); stretchWidth = std::max(stretchWidth, renderOperator->logicalWidth()); } else { child->layoutIfNeeded(); stretchWidth = std::max(stretchWidth, child->logicalWidth()); } } for (auto& child : embellishedOperators) { auto renderOperator = toHorizontalStretchyOperator(child); ASSERT(renderOperator); renderOperator->stretchTo(stretchWidth); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); renderOperator->setStretchWidthLocked(false); } } ``` Several problems: First, I am unsure if you mean `renderOperator->logicalWidth()` or `child->logicalWidth()`. According to the standard and your comment I believe it is the latter. However, it does not affect the output—I cannot make sense of that immediately. Second, the over-stretched brace issue persists: <img width="75" alt="2017-11-29 19 20 47" src="https://user-images.githubusercontent.com/2232964/33411423-706aac52-d53a-11e7-9844-329fcb37a7e2.png"> I do not see how “cover the width of the other direct sub-expressions in the given element” could be satisfied with this loop. I am a bit concerned of language here. By “direct sub-expression”, which ways of the follows should it be interpreted? * Other siblings of the embellished operators. * Siblings of `<mo>`. I prefer the former, and here is the original text: > [...] then it, or the mo element at its core, should stretch to cover the width of the other direct sub-expressions in the given element [...] I believe “it” refers to the (embellished) operator. Your second point on “[...] should grow to the maximum of the normal unstretched sizes of all elements in the containing object” is well-acknowledged and reflected already in yesterday’s code. Third, look at the bottom of the screenshot, namely those nested cases: <img width="148" alt="2017-11-29 19 27 51" src="https://user-images.githubusercontent.com/2232964/33411611-759ac2f6-d53b-11e7-9e70-ec05b60d5984.png"> You could see that the inner arrow is a bit off-center. The reason is that your proposal does not lock `stretchWidth` in the first loop: ```C++ for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { if (auto renderOperator = toHorizontalStretchyOperator(child)) { embellishedOperators.append(child); renderOperator->resetStretchSize(); child->setNeedsLayout(); child->layout(); stretchWidth = std::max(stretchWidth, renderOperator->logicalWidth()); } else { child->layoutIfNeeded(); stretchWidth = std::max(stretchWidth, child->logicalWidth()); } } ``` Therefore, when the recursive call (on the inner structure) enters the second loop, the `<mo>` is actually stretched. How does that make the arrow off-center is hard to explain, but I believe we want `<mo>` to be unstretched the whole time, so I take the liberty to add the following code: ```C++ for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { if (auto renderOperator = toHorizontalStretchyOperator(child)) { embellishedOperators.append(child); renderOperator->resetStretchSize(); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); stretchWidth = std::max(stretchWidth, renderOperator->logicalWidth()); renderOperator->setStretchWidthLocked(false); } else { child->layoutIfNeeded(); stretchWidth = std::max(stretchWidth, child->logicalWidth()); } } ``` This way, the lock has to be a counter. There is still an issue. In the second loop, during the fix process, the embellished operator will be called again to re-layout after stretching. You should agree that it is the first loop in the recursive call that do the re-layout. However, since the embellished operator has its core locked, in the loop it will actually call `layoutIfNeeded()`, which does nothing. Therefore, it needs to be changed: ```C++ for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { if (auto renderOperator = toHorizontalStretchyOperator(child)) { embellishedOperators.append(child); renderOperator->resetStretchSize(); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); renderOperator->setStretchWidthLocked(false); } else { child->setNeedsLayout(); child->layout(); } stretchWidth = std::max(stretchWidth, child->logicalWidth()); } ``` Now we return to the over-stretched brace: <img width="75" alt="2017-11-29 19 20 47" src="https://user-images.githubusercontent.com/2232964/33411423-706aac52-d53a-11e7-9844-329fcb37a7e2.png"> As I said, I do not think by manipulating the for-loop could get us anywhere. The key is that the stretch width is different for base, under, and over. As I said before, I think the reasonable way to set stretch width is: 1. `base.stretchWidth = max(over.width, under.width, base.width)` 2. `under.stretchWidth = base.width` 3. `over.stretchWidth = base.width` Recall that it is different from the standard: 1. `base.stretchWidth = max(over.width, under.width)` 2. `under.stretchWidth = max(base.width, over.width)` 3. `over.stretchWidth = max(base.width, under.width)` And Firefox adopts: 1. `base.stretchWidth = max(over.width, under.width, base.width)` 2. `under.stretchWidth = max(base.width, over.width)` 3. `over.stretchWidth = max(base.width, under.width)` Check out my test file (as attachment) and previous comments (which should be unnecessary as my test file should be complete) for my motivation. Combined together gives the final code: ```C++ void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren() { ASSERT(isValid()); ASSERT(needsLayout()); LayoutUnit stretchWidthForBase = 0; Vector<RenderBox*, 3> embellishedOperators; for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { if (auto renderOperator = toHorizontalStretchyOperator(child)) { embellishedOperators.append(child); renderOperator->resetStretchSize(); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); renderOperator->setStretchWidthLocked(false); } else { child->setNeedsLayout(); child->layout(); } stretchWidthForBase = std::max(stretchWidthForBase, child->logicalWidth()); } LayoutUnit stretchWidthForOthers = firstChildBox()->logicalWidth(); for (auto& child : embellishedOperators) { auto stretchWidth = child == firstChildBox() ? stretchWidthForBase : stretchWidthForOthers; auto renderOperator = toHorizontalStretchyOperator(child); ASSERT(renderOperator); renderOperator->stretchTo(stretchWidth); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); renderOperator->setStretchWidthLocked(false); } } ``` Two things to point out. First, in the first loop, else branch, the force re-layout is EXPENSIVE. The only way to be more efficient, as I can think of, is to handle the following three cases differently: * unlocked embellished stretchy operator, lock and force layout (top-level call, initial layout) * locked embellished stretchy operator, lock and force layout (we are fixing the layout!) * other cases, layout if necessary That is exactly what my yesterday’s code does, though that code is shitty. We could discuss how to get it done more elegantly later. The other pressing matter, as I mentioned in the test file, is the following case: <img width="100" alt="2017-11-29 19 45 35" src="https://user-images.githubusercontent.com/2232964/33412040-0715d944-d53e-11e7-85cc-10ddeb7d97a7.png"> > In all our patches, when the arrow is stretched to the width of the text, its surrounding space makes the base acutally wider than text. So we would observe the brace to be slightly wider. The issue is not present (thus a regression) because in the past the logical width of that arrow is unstretched, and the logical width of the base would be that of the text. Firefox is not affected. (Copied from my test file.) That is all I have to say.

(In reply to Minsheng Liu from comment #29) > renderOperator->resetStretchSize(); > child->setNeedsLayout(); > child->layout(); > stretchWidth = std::max(stretchWidth, > renderOperator->logicalWidth()); Note that I said "Instead of calculating the size of the whole embellished operator as I suggested, let's take the unstretched size of the <mo> at the core". So I really meant renderOperator->setNeedsLayout(), renderOperator->layout() and renderOperator->logicalWidth(). No "child" should appear here. > First, I am unsure if you mean `renderOperator->logicalWidth()` or > `child->logicalWidth()`. According to the standard and your comment I > believe it is the latter. However, it does not affect the output—I cannot > make sense of that immediately. I really meant renderOperator. > Second, the over-stretched brace issue persists: Indeed because you layout the whole child instead of just the <mo> at the core, so the renderOperator will be stretched in the first loop. > I do not see how “cover the width of the other direct sub-expressions in the > given element” could be satisfied with this loop. I am a bit concerned of > language here. By “direct sub-expression”, which ways of the follows should > it be interpreted? > > * Other siblings of the embellished operators. > * Siblings of `<mo>`. I think "direct sub-expression" mean children of the <munderover>. However, the trick here is to consider that the width of (possibly embellished) operators is the unstretched size of the <mo> at the core as Gecko does. > You could see that the inner arrow is a bit off-center. The reason is that > your proposal does not lock `stretchWidth` in the first loop: You must not lock the stretchWidth in the first loop. Again, I'm only proposing to layout the renderOperator at the core, not the whole child.

How should I get a RenderBox out of a RenderBlock? It told me that layout is a protected member of RenderBlock (and hence RenderMathMLOperator).

Created attachment 327975 [details] your algorithm’s result So I just re-interpret the pointer. There should be some other safer way to do it. void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren() { ASSERT(isValid()); ASSERT(needsLayout()); LayoutUnit stretchWidth = 0; Vector<RenderBox*, 3> embellishedOperators; for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) { if (auto renderOperator = toHorizontalStretchyOperator(child)) { embellishedOperators.append(child); renderOperator->resetStretchSize(); auto* operatorBox = (RenderBox*)renderOperator; operatorBox->setNeedsLayout(); operatorBox->layout(); stretchWidth = std::max(stretchWidth, operatorBox->logicalWidth()); } else { child->layoutIfNeeded(); stretchWidth = std::max(stretchWidth, child->logicalWidth()); } } for (auto& child : embellishedOperators) { auto renderOperator = toHorizontalStretchyOperator(child); ASSERT(renderOperator); renderOperator->stretchTo(stretchWidth); renderOperator->setStretchWidthLocked(true); child->setNeedsLayout(); child->layout(); renderOperator->setStretchWidthLocked(false); } } In any case, why this should fail about complicated case is not hard to see. You cannot get around the variable stretchWidth issue.

(In reply to Minsheng Liu from comment #31) > How should I get a RenderBox out of a RenderBlock? It told me that layout is > a protected member of RenderBlock (and hence RenderMathMLOperator). Mmmh, can't you just do RenderBox* renderBox = renderOperator; renderBox->... renderBox->... or static_cast<RenderBox*>(renderOperator)->... ?

(In reply to Minsheng Liu from comment #32) > In any case, why this should fail about complicated case is not hard to see. > You cannot get around the variable stretchWidth issue. What about the more costly option to force a relayout of the children in the first loop?

(In reply to Frédéric Wang (:fredw) from comment #33) > (In reply to Minsheng Liu from comment #31) > > How should I get a RenderBox out of a RenderBlock? It told me that layout is > > a protected member of RenderBlock (and hence RenderMathMLOperator). > > Mmmh, can't you just do > > RenderBox* renderBox = renderOperator; > renderBox->... > renderBox->... > > or > > static_cast<RenderBox*>(renderOperator)->... > > ? layoutIfNeeded() (optionally layoutBlock())

> What about the more costly option to force a relayout of the children in the first loop? I have detailed of that in the GitHub issue and lengthy reply above (https://github.com/notcome/free-markdown-previewer/issues/1). Plus I gave you a step-by-step derivation of my algorithm which does handle all cases correctly. > layoutIfNeeded() (optionally layoutBlock()) Thanks. Anyway, I want to reiterate the fact that the more pressing matter is the following case: <math display="block"> <mrow> <mover> <munder> <mtext>long text</mtext> <mo>&#x2192;</mo> </munder> <mo>⏞</mo> </mover> </mrow> </math> The arrow (&#x2192;) is not an embellished operator. Its surrounding lspace/rspace will make the brace a bit wider than it should be. The correct response to this would be nontrivial (involving multiple methods and potentially a new virtual one finding all operators and underovers), and we really should discuss this case now.

To handle the latest edge case I provided, I think we really could benefit a lot from a logicalWidthWithoutStretchyOperators() virtual method. With that, we could have something like: for (child : children) layout child if needed this->m_logicalWidthWithoutStretchyOperator = max(children’s) for (child : children that are embellished operators) let operator = ... stretch operator fixLayout(parent node = child, bad node = operator) layout children set logical width et cetera This way we can combine the method we are working on with layoutBlock. Also I suspect our existing fixLayout (setNeedsLayout() then layout()) is problematic for cases like an operator embellished with multiple subscript/superscript. — Sent from my iPhone.

On the one hand, it is much harder than I thought to keep a logicalWidthWithoutStretchyOperators. On the other hand, it seems that space surrounding an operator shall only be considered when it is in a <mrow>, as the spec says: > The amount of horizontal space added around an operator (or embellished operator), when it occurs in an mrow, can be directly specified by the lspace and rspace attributes. Therefore, the edge case is really beyond the scope of this patch, and I shall open a new bug handling it. If that is the case, then my code has properly handled all cases, with the additional flexibility to adapt to different interpretation of stretch widths fairly easily. Once we make up decision on that front, I suggest we freeze the discussion on the layout algorithm and enters the next step. In the mean time, I will convert the manual test file into a JavaScript based test file that compares stretchy operators’ widths. I will also try to add a performance test, but I might need some help from you guys. So again, @fred, out of the following three definitions, which one should we put into this patch: // my proposal, it is the middle column of this picture: https://camo.githubusercontent.com/606494a142955188301373ca36780293faa669d7/68747470733a2f2f692e696d6775722e636f6d2f583631524b594e2e706e67 base.stretchWidth = max(over.width, under.width, base.width) under.stretchWidth = base.width over.stretchWidth = base.width // the standard: base.stretchWidth = max(over.width, under.width) under.stretchWidth = max(base.width, over.width) over.stretchWidth = max(base.width, under.width) // Firefox: it is the left column of this picture: https://camo.githubusercontent.com/606494a142955188301373ca36780293faa669d7/68747470733a2f2f692e696d6775722e636f6d2f583631524b594e2e706e67 base.stretchWidth = max(over.width, under.width, base.width) under.stretchWidth = max(base.width, over.width) over.stretchWidth = max(base.width, under.width) I do not think we should wait for the committee, as again in the future we could change our decision fairly easily, but future patches should crucially rely on a sensible logical bounds of <munderover>. (In reply to Minsheng Liu from comment #37) > To handle the latest edge case I provided, I think we really could benefit a > lot from a logicalWidthWithoutStretchyOperators() virtual method. With that, > we could have something like: > > for (child : children) > layout child if needed > > this->m_logicalWidthWithoutStretchyOperator = max(children’s) > > for (child : children that are embellished operators) > let operator = ... > stretch operator > fixLayout(parent node = child, bad node = operator) > > layout children > set logical width > et cetera > > This way we can combine the method we are working on with layoutBlock. > > Also I suspect our existing fixLayout (setNeedsLayout() then layout()) is > problematic for cases like an operator embellished with multiple > subscript/superscript. > > — Sent from my iPhone.

Created attachment 328081 [details] Tetscase with different widths Thanks for the perseverance on this bug. For your information, I'm working on other projects right now so obviously I don't have time to read and test things in details or to reply to each C++ build error, sorry. I'm really willing to help to provide guideline and review MathML patches though, so please continue your effort! As indicated on the WebKit wiki, the rule of thumb to get changes accepted more easily is to try and split the work in small tasks so that it's easier for people to understand and to avoid breaking things. Also as I said you can also start with smaller bugs like bug 158937, bug 180029 or bug 161306 to get more familiar with C++ and WebKit code/process (BTW, check Tools/Script/webkit-patch --help to upload a patch without asking for review). Regarding this bug, the initial goal was to fix the logical width of stretched operators in munderover. You are right that things are getting more complicated that I thought with embellished operators (they are actually not completely implemented yet, see bug 124831 ; Gecko has bugs too) so maybe you should not spend too much time thinking on it. By far, the most important use cases is when there are no embellished operators (row 1 in the attached test case) and in that case the behavior is clear: They should stretch to the maximum width of base/under/over and this is how the spec & Gecko behave and how WebKit must remain. In practice, there is one non-stretchy element giving the width to use so the things in the spec ("other", "unstretched size") or even skipping stretchy operators (as for vertical operators) does not really affect the value of the stretch width. When we have embellished operators, one has to include them in the width calculation (this is the comment "Skipping the embellished op..." in WebKit code) and then the spec is not really clear about how to calculate their unstretched width (do we include unstretched <mo> width? embellishement width?) and their actual stretch width (e.g. Gecko ignores embellishments and just stretch the <mo> at the core to the target size as you can see in the testcase). Again, I think these use cases are rare and in general embellishments are small (row 2 and 3 in the testcase). So I think we have some freedom on what to do here, for example if we want to cover the use cases of horizontal braces with some long text labels. However, we should try to keep the code 1) Simple and readable 2) Close to the spec, Gecko and to the base case with no embellishments 3) Efficient, especially for the base case. That's all I can say for now, I hope this is helpful.

Thanks for your patience. I plan the following two things: 1. I will summarize a patch with test cases so that when we look back in the future we have something to work/test against. Note that some part of my code is complicated (like the nested lock structure) and expensive is due to the way we fix layout after re-stretching. I have a new method in my mind that should make everyone satisfied in terms of code efficiency and clarity. 2. If you have not done so, I will send a summary of some edge cases that the MathML 3 standard fails to clarify to their mailing list. Both will not be done before next week though, as some schoolwork is piling up. Thanks.

Created attachment 328250 [details] Patch

Comment on attachment 328250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328250&action=review I’d love to say r=me, great that this includes a test and it seems logical, but I am concerned about the setNeedsLayout/layout thing done here. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:73 > + auto* fixingBox = static_cast<RenderBox*>(stretchyOperator); > + while (fixingBox != ancestor) { > + fixingBox->setNeedsLayout(); > + fixingBox->layout(); > + fixingBox = fixingBox->parentBox(); > + } I would write: RenderBox* fixingBox = stretchyOperator; Instead of writing the static_cast. I also would write this as a for loop: for (RenderBox* box = stretchyOperator; box != ancestor; box = box->parentBox()) { box->setNeedsLayout(); box->layout(); } But also, it is really strange to do synchronous calls to layout after setNeedsLayout. We need a layout expert to weigh in on this. It seems wrong. Antti? Zalan? > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:75 > + ancestor->setNeedsLayout(); > + ancestor->layout(); Ditto. It seems extremely unlikely that it is OK to do this. Despite the fact that the old code was doing something like this before. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:106 > + child->setNeedsLayout(); > child->layout(); That same pattern again.

Thanks Darin for commenting! I am completely new to WebKit. I really hope there is some detailed documentation on layout stuff. The only thing I could find is the 2007 blog article (https://webkit.org/blog/116/webcore-rendering-iii-layout-basics/) but the series feels more like a tutorial on how to write a simple browser… In any case, if some layout expert could take a look at it, I have a question. Since the stretch width of a stretch operator depends on the width of other elements in a fairly complicated way, I am unsure how the code behaves when a stretch-width-affecting element is changed (e.g. by JavaScript). In WebKit, which part of code determines which elements become dirty after DOM change? I have not yet tested the issue as there are other layouting/painting problems regarding DOM change, page size scaling, and scrolling, but I think we should be cautious here.

(In reply to Minsheng Liu from comment #43) > I really hope there is some detailed documentation on layout stuff. Sorry,no.

Comment on attachment 328250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328250&action=review >> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:73 >> + } > > I would write: > > RenderBox* fixingBox = stretchyOperator; > > Instead of writing the static_cast. > > I also would write this as a for loop: > > for (RenderBox* box = stretchyOperator; box != ancestor; box = box->parentBox()) { > box->setNeedsLayout(); > box->layout(); > } > > But also, it is really strange to do synchronous calls to layout after setNeedsLayout. We need a layout expert to weigh in on this. It seems wrong. Antti? Zalan? Normally you set the dirty bit on the renderer soon after you figure its position/size are invalid (usually result of a style change), schedule an async layout and here you would just call layoutIfNeeded(). However since in this function we are already have a valid layout context (through RenderMathMLUnderOver::layoutBlock), this looks more like a multi-pass layout where the descendants need to be readjusted after some ancestors/siblings etc layouts are done. I don't know much about MathML so if that's not the case, then the setNeedsLayout calls are wrong here and they need to be moved somewhere else (see above). Even if it was a type of multi-pass layout, the stretch operation should probably dirty the descendants (since that's where the children get invalid) and you'd just call layoutIfNeeded here.

(In reply to zalan from comment #45) > Comment on attachment 328250 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328250&action=review > > >> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:73 > >> + } > > > > I would write: > > > > RenderBox* fixingBox = stretchyOperator; > > > > Instead of writing the static_cast. > > > > I also would write this as a for loop: > > > > for (RenderBox* box = stretchyOperator; box != ancestor; box = box->parentBox()) { > > box->setNeedsLayout(); > > box->layout(); > > } > > > > But also, it is really strange to do synchronous calls to layout after setNeedsLayout. We need a layout expert to weigh in on this. It seems wrong. Antti? Zalan? > > Normally you set the dirty bit on the renderer soon after you figure its > position/size are invalid (usually result of a style change), schedule an > async layout and here you would just call layoutIfNeeded(). > However since in this function we are already have a valid layout context > (through RenderMathMLUnderOver::layoutBlock), this looks more like a > multi-pass layout where the descendants need to be readjusted after some > ancestors/siblings etc layouts are done. I don't know much about MathML so > if that's not the case, then the setNeedsLayout calls are wrong here and > they need to be moved somewhere else (see above). > Even if it was a type of multi-pass layout, the stretch operation should > probably dirty the descendants (since that's where the children get invalid) > and you'd just call layoutIfNeeded here. So what is the difference between layoutIfNeeded() and layout()? Is it that when layout() is called then a layout is carried out immediately, while when layoutIfNeeded() is called then a layout is not executed until we are doing some actual layout? Second, this is indeed a multi-pass layout. The case I try to handle is that I am stretching a child deeply nested so after the stretching I need to layout not only the child but also all its ancestors. It is like: <munder> // we are executing layout here <msub> <mover> <msup> <mo>...</mo> // the <mo> being stretched </msup> </mover> </msub> ... </munder> So if I understand you correctly, I could just set <msub>, <mover>, <msup>, all the way to <mo> dirty, and then call layoutIfNeeded() on <musb>? That does seem better! Thanks!

Created attachment 328323 [details] Patch

Comment on attachment 328323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328323&action=review We are getting closer, but I'm doing r- at least because the patch disables an important pixel test for horizontal stretching and causes a regression with the base case (no embellishments). It seems some things are wrong/unclear in the relayout, let's see if we can improve that a bit too in the next iteration. > Source/WebCore/ChangeLog:24 > + what it tests is covered in the new test case. mathml/opentype/opentype-stretchy-horizontal.html is a pixel test to check the rendering of stretchy operators with a base size, variant size and glyph assembly. The rendering is *not* tested by the new test so you should not disable it. Operator spacing is irrelevant for this mathml/opentype/opentype-stretchy-horizontal.html, though so please open a preliminary bug that sets lspace="0px" rspace="0px" on the <mo> operators of this mathml/opentype/opentype-stretchy-horizontal.html and updates the PNG/txt expectations, so that we can really see what this patch is changing (hopefully it should not change anything). > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:49 > + void setStretchWidthLocked(bool stretchWidthLock) { m_isStretchWidthLocked = stretchWidthLock; } nit: The argument name should be "stretchWidthLocked" > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:73 > + stretchyOperator->setStretchWidthLocked(true); This leaves the stretchyOperator locked, I guess you mean setStretchWidthLocked(false) instead? > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:102 > child->layout(); I wonder whether that should be stretchyOperator->setNeedsLayout(); child->layoutIfNeeded(); > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:113 > + stretchyOperators[i]->stretchTo(stretchWidthForNonBase()); This is wrong as it does not work the base case (no embellishments, first row of attachment 328081 [details]). I think what you want is two LayoutUnit variables stretchWidthForScripts and stretchWidthForBase both are set to the maximum of the width of non-stretchy children in the first loop as this is what is clear from the spec. Then you can adjust stretchWidthForScripts and stretchWidthForBase to include other widths of stretchy children in the calculation. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.h:52 > + LayoutUnit stretchWidthForNonBase() const { return firstChildBox()->logicalWidth(); }; I'm not sure you really need these helper functions. The variables and extra comments should be enough. > LayoutTests/TestExpectations:568 > +mathml/opentype/opentype-stretchy-horizontal.html [ Skip ] This should not be disabled. > LayoutTests/mathml/opentype/munderover-stretch-width.html:46 > + runCase(2, 'simple stretchy under', 'red', 'op'); The base case (no embellishments) is the most important one and is clear from the spec, so I propose you move it into a separate test and performs exhaustive testing of the combinations: - munder/mover/munderover - one, two or three stretchy operators. - base/under/over as the widest non-stretchy operator (is there is one) OR comparing with the max width of unstretched operators (if all operators are stretchy). You can put the operator in a <mo stretchy="false"> or an <mtext> to get the unstretched size. Currently, your test is only checking munder/mover, one stretchy operator as the script and one non-stretchy as the base so you can not see that your code is wrong.

Comment on attachment 328323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328323&action=review > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:71 > + ancestor->layoutIfNeeded(); (this comment was lost, so adding it again) This seems complicated. Isn't it enough to call setNeedsLayout() on the stretchyOperator and layoutIfNeeded() on the direct children of <munderover>?

(In reply to Frédéric Wang (:fredw) from comment #49) > Comment on attachment 328323 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328323&action=review > > > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:71 > > + ancestor->layoutIfNeeded(); > > (this comment was lost, so adding it again) > > This seems complicated. Isn't it enough to call setNeedsLayout() on the > stretchyOperator and layoutIfNeeded() on the direct children of <munderover>? Oh that is the main point of this new patch! Consider the following case: <munder> // we are executing layout here <msub> <mover> <msup> <mo>...</mo> // the <mo> being stretched </msup> </mover> </msub> ... </munder> When layoutIfNeeded() is called on <msub>, it will find its immediate child <mover> clean, so the recursion will not go down. I have tested and making sure the whole chain from the immediate child to the operator at its core dirty is crucial for correct layout. Note that this approach only requires minimal re-layout. Essentially, only logicalWidth() and location() are updated. Therefore, I could get rid of my initially complicated first loop and counter-based lock and all other crazy stuff. I think we are optimal here.

(In reply to Minsheng Liu from comment #50) > When layoutIfNeeded() is called on <msub>, it will find its immediate child > <mover> clean, so the recursion will not go down. I have tested and making > sure the whole chain from the immediate child to the operator at its core > dirty is crucial for correct layout. > > Note that this approach only requires minimal re-layout. Essentially, only > logicalWidth() and location() are updated. Therefore, I could get rid of my > initially complicated first loop and counter-based lock and all other crazy > stuff. I think we are optimal here. Sorry, I was not clear. I mean (quickly looking to the implementation of layoutIfNeeded) it seems marking the <mo> at the core dirty will also already mark the whole ancestor chain and then you just need to layout the direct child.

(In reply to Frédéric Wang (:fredw) from comment #51) > (In reply to Minsheng Liu from comment #50) > > When layoutIfNeeded() is called on <msub>, it will find its immediate child > > <mover> clean, so the recursion will not go down. I have tested and making > > sure the whole chain from the immediate child to the operator at its core > > dirty is crucial for correct layout. > > > > Note that this approach only requires minimal re-layout. Essentially, only > > logicalWidth() and location() are updated. Therefore, I could get rid of my > > initially complicated first loop and counter-based lock and all other crazy > > stuff. I think we are optimal here. > > Sorry, I was not clear. I mean (quickly looking to the implementation of > layoutIfNeeded) it seems marking the <mo> at the core dirty will also > already mark the whole ancestor chain and then you just need to layout the > direct child. That will be great (but unintuitive…I clearly need more understanding of those little layout functions). I will double check that. Other replies: > mathml/opentype/opentype-stretchy-horizontal.html is a pixel test to check the rendering of stretchy operators with a base size, variant size and glyph assembly. The rendering is *not* tested by the new test so you should not disable it. Operator spacing is irrelevant for this mathml/opentype/opentype-stretchy-horizontal.html, though so please open a preliminary bug that sets lspace="0px" rspace="0px" on the <mo> operators of this mathml/opentype/opentype-stretchy-horizontal.html and updates the PNG/txt expectations, so that we can really see what this patch is changing (hopefully it should not change anything). Yeah that is a better idea. I will get it done soon. Note that the RenderTree will be different since the width of the stretchy operator will change. The pixel test should be fine, but let’s figure it out. > This is wrong as it does not work the base case (no embellishments, first row of attachment 328081 [details]). I think what you want is two LayoutUnit variables stretchWidthForScripts and stretchWidthForBase both are set to the maximum of the width of non-stretchy children in the first loop as this is what is clear from the spec. Then you can adjust stretchWidthForScripts and stretchWidthForBase to include other widths of stretchy children in the calculation. I will look into that when I get home. > I'm not sure you really need these helper functions. The variables and extra comments should be enough. I will conform to whatever WebKit chooses. It does make the structure more clear and easier to modify, at least in the current iterative process.

(In reply to Minsheng Liu from comment #52) > Yeah that is a better idea. I will get it done soon. Note that the > RenderTree will be different since the width of the stretchy operator will > change. The pixel test should be fine, but let’s figure it out. > Right, the change in the render tree should not be a problem, that's a fix :-) That's why I think it's better to do that step in a preliminary patch so we clearly see the diff. But now that you mention it, I believe we can't do bug 180351 without fixing the logical width first.

Created attachment 328436 [details] Patch

I have made some changes to the layout algorithm and the resulting one is more standard conforming and should behave the same as Firefox does. As for the fix layout algorithm, @fred you are right that we just need to dirty the op at the core and layoutIfNeeded the outermost wrapper for the embellished op. As for the tests, I add one simple case that the previous version fails in the script test. The pixel test is also updated, and I believe the visual results are correct for both macOS and iOS. As for expected results for GTK and Win, I cannot generate them so I delete them. Could you help me get the desired results? Or could we let the bot do that for us?

Comment on attachment 328436 [details] Patch Attachment 328436 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5496501 New failing tests: mathml/opentype/munderover-stretch-width.html

Created attachment 328439 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Stupid…I added a test case but forgot to update the expected file.

Created attachment 328440 [details] Patch

*** Bug 180351 has been marked as a duplicate of this bug. ***

Comment on attachment 328440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328440&action=review @Minsheng Liu: OK, I think this looks much better now! However you did not address/reply to some of my previous comments and especially the fact that the stretch operator remains locked in fixLayoutAfterStretch. We are close I guess I can r+ in the next iteration once all the comments are addressed. Regarding the GTK / Windows, you don't have to worry about it for now, we can land the patch as it and let the port maintainers update the expectations (or ask them to do it). You can also retrieve them from https://build.webkit.org/console after the patch lands and submit a follow-up patch. > Source/WebCore/ChangeLog:20 > + mathml/opentype/opentype-stretchy-horizonta.html nit: horizontal > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:49 > + void setStretchWidthLocked(bool stretchWidthLock) { m_isStretchWidthLocked = stretchWidthLock; } nit: argument name should be "stretchWidthLocked" > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:69 > + stretchyOperator->setStretchWidthLocked(true); I'm still not sure why this is not stretchyOperator->setStretchWidthLocked(false) ? > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:90 > + stretchyOperator->setStretchWidthLocked(false); Does it work if you rewrite this stretchyOperator->resetStretchSize(); fixLayoutAfterStretch(child, stretchyOperator); ? > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:101 > + } Ah OK, this was the trick! That makes the code much more readable and understandable indeed. I think it would be great to add a C++ comment at the top of the function to explain the algorithm and rationale (comparing with the spec & Gecko maybe) since as you see we took some time to figure it out. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:103 > + for (std::size_t i = 0; i < embellishedOperators.size(); i ++) { nit 1: do you really need the namespace prefix for size_t? nit 2: there should be no space between i and ++. > LayoutTests/mathml/opentype/munderover-stretch-width.html:46 > + runCase(2, 'simple stretchy under', 'red', 'op'); I really think it would be nice to have more tests for the base case. But I guess it's ok to do that in a separate patch. > LayoutTests/mathml/opentype/munderover-stretch-width.html:52 > + runCase(8, 'simpe stretchy under should equal over', 'red', 'op'); s/simpe/simple/

Created attachment 328775 [details] Patch

Comment on attachment 328775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328775&action=review Thanks, I have some minor comments below but otherwise LGTM. It looks like wincairo failed, you should probably consider rebasing your patch against the latest upstream revision. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:181 > + nit: please remove the trailing whitespace. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:63 > + nit: please remove the trailing whitespace. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:75 > + ASSERT(needsLayout()); Can you please add the following comment: // We apply horizontal stretchy rules from the MathML spec (sections 3.2.5.8.3 and 3.2.5.8.4), which // can be roughly summarized as "stretching operators to the maximum widths of all children" and // minor variations of that algorithm do not affect the result. However, the spec is a bit ambiguous // for embellished operators (section 3.2.5.7.3) and different approaches can lead to significant // stretch size differences. We made the following decisions: // - The unstretched size is the embellished operator width with the <mo> at the core unstretched. // - In general, the target size is just the maximum widths of non-stretchy children because the // embellishments could make widths significantly larger. // - In the edge case when all operators of stretchy, we follow the specification and take the // maximum of all unstretched sizes. // - The <mo> at the core is stretched to cover the target size, even if the embellished operator // might become much wider. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:76 > + nit: trailing whitespace. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:81 > + nit: trailing whitespace > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:87 > + fixLayoutAfterStretch(child, stretchyOperator); It is a bit sad that we have to do this relayout when we actually only measure the unstretched size for the edge case isAllStretchyOperators == true. Are you able to move that fixLayoutAfterStretch call into the isAllStretchyOperators conditional? > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:94 > + nit: trailing whitespace > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:99 > + nit: trailing whitespace

Comment on attachment 328775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328775&action=review > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:53 > +static RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox* box) const RenderBox& box (unembellishedOperator should really be a const function) > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:55 > + if (is<RenderMathMLBlock>(box)) { in WebKit, we tend to use early returns instead if (!is<RenderMathMLBlock>(box)) return nullptr; > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:56 > + if (auto renderOperator = downcast<RenderMathMLBlock>(*box).unembellishedOperator()) { if (auto* renderOperator > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:64 > +static void fixLayoutAfterStretch(RenderBox* ancestor, RenderMathMLOperator* stretchyOperator) RenderBox& ancestor, RenderMathMLOperator& stretchyOperator

First a disclaimer, I am not quite familiar with VCS. I notice that wincairo’s result says: > Original content of LayoutTests/platform/ios/mathml/opentype/opentype-stretchy-horizontal-expected.png mismatches at C:/WebKit-EWS/WebKit\Tools\Scripts\svn-apply line 272. And in fact that all previous patches did not apply to wincairo for the exact same reason. I am quite confused because I think all those platforms share the same branch, right? How could the patch apply to one platform but fail on the other? Also, I am unsure if I am doing the patch creating workflow correctly. Every time I submit a new patch, I have to first revert my change to the ChangeLog, do a `git stash` to save my current work, update through the script, `git stash apply` to restore my work, and then manually fixing ChangeLog. Is there a way to (partially) automate that? Thanks! (In reply to Frédéric Wang (:fredw) from comment #63) > Comment on attachment 328775 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328775&action=review > > Thanks, I have some minor comments below but otherwise LGTM. It looks like > wincairo failed, you should probably consider rebasing your patch against > the latest upstream revision. > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:181 > > + > > nit: please remove the trailing whitespace. > > > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:63 > > + > > nit: please remove the trailing whitespace. > > > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:75 > > + ASSERT(needsLayout()); > > Can you please add the following comment: > > // We apply horizontal stretchy rules from the MathML spec (sections > 3.2.5.8.3 and 3.2.5.8.4), which > // can be roughly summarized as "stretching operators to the maximum widths > of all children" and > // minor variations of that algorithm do not affect the result. However, the > spec is a bit ambiguous > // for embellished operators (section 3.2.5.7.3) and different approaches > can lead to significant > // stretch size differences. We made the following decisions: > // - The unstretched size is the embellished operator width with the <mo> at > the core unstretched. > // - In general, the target size is just the maximum widths of non-stretchy > children because the > // embellishments could make widths significantly larger. > // - In the edge case when all operators of stretchy, we follow the > specification and take the > // maximum of all unstretched sizes. > // - The <mo> at the core is stretched to cover the target size, even if the > embellished operator > // might become much wider. > > > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:76 > > + > > nit: trailing whitespace. > > > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:81 > > + > > nit: trailing whitespace > > > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:87 > > + fixLayoutAfterStretch(child, stretchyOperator); > > It is a bit sad that we have to do this relayout when we actually only > measure the unstretched size for the edge case isAllStretchyOperators == > true. Are you able to move that fixLayoutAfterStretch call into the > isAllStretchyOperators conditional? > > > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:94 > > + > > nit: trailing whitespace > > > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:99 > > + > > nit: trailing whitespace

(In reply to Minsheng Liu from comment #65) > First a disclaimer, I am not quite familiar with VCS. I notice that > wincairo’s result says: > > > Original content of LayoutTests/platform/ios/mathml/opentype/opentype-stretchy-horizontal-expected.png mismatches at C:/WebKit-EWS/WebKit\Tools\Scripts\svn-apply line 272. > > And in fact that all previous patches did not apply to wincairo for the > exact same reason. I am quite confused because I think all those platforms > share the same branch, right? How could the patch apply to one platform but > fail on the other? Sounds like there might be problem with "eol" handling. It’s possible that the MIME type isn’t set for this PNG file in Subversion so it is treating it as a text file and trying to do CRLF conversion in some Windows. If so, the fix is for someone to land a patch that sets the property on the PNG file. > Also, I am unsure if I am doing the patch creating workflow correctly. Every > time I submit a new patch, I have to first revert my change to the > ChangeLog, do a `git stash` to save my current work, update through the > script, `git stash apply` to restore my work, and then manually fixing > ChangeLog. Is there a way to (partially) automate that? There is a script called resolve-ChangeLogs that you can set up in your git configuration to make this easier. I can’t find the instructions about how to use it, but someone else on the project probably can point you to the right place.

(In reply to Darin Adler from comment #66) > > Also, I am unsure if I am doing the patch creating workflow correctly. Every > > time I submit a new patch, I have to first revert my change to the > > ChangeLog, do a `git stash` to save my current work, update through the > > script, `git stash apply` to restore my work, and then manually fixing > > ChangeLog. Is there a way to (partially) automate that? > > There is a script called resolve-ChangeLogs that you can set up in your git > configuration to make this easier. I can’t find the instructions about how > to use it, but someone else on the project probably can point you to the > right place. It's https://trac.webkit.org/wiki/UsingGitWithWebKit#WebKitScriptsupportforGit

(In reply to Frédéric Wang (:fredw) from comment #67) > (In reply to Darin Adler from comment #66) > > > Also, I am unsure if I am doing the patch creating workflow correctly. Every > > > time I submit a new patch, I have to first revert my change to the > > > ChangeLog, do a `git stash` to save my current work, update through the > > > script, `git stash apply` to restore my work, and then manually fixing > > > ChangeLog. Is there a way to (partially) automate that? > > > > There is a script called resolve-ChangeLogs that you can set up in your git > > configuration to make this easier. I can’t find the instructions about how > > to use it, but someone else on the project probably can point you to the > > right place. > > It's > https://trac.webkit.org/wiki/UsingGitWithWebKit#WebKitScriptsupportforGit Thanks! So how should we deal with that PNG? I will not be able to touch my iMac for the next few hours, so could you help me resolve that? I will prepare a new patch tonight (ten hours later) that adapt your and zalan’s latest comments.

(In reply to Darin Adler from comment #66) > Sounds like there might be problem with "eol" handling. It’s possible that > the MIME type isn’t set for this PNG file in Subversion so it is treating it > as a text file and trying to do CRLF conversion in some Windows. If so, the > fix is for someone to land a patch that sets the property on the PNG file. Hmm, no that theory seems to be wrong. It says "Property svn:mime-type set to image/png" and so I don’t know why. Maybe involved in maintaining the WinCairo EWS bot can investigate?

(In reply to Darin Adler from comment #69) > (In reply to Darin Adler from comment #66) > > Sounds like there might be problem with "eol" handling. It’s possible that > > the MIME type isn’t set for this PNG file in Subversion so it is treating it > > as a text file and trying to do CRLF conversion in some Windows. If so, the > > fix is for someone to land a patch that sets the property on the PNG file. > > Hmm, no that theory seems to be wrong. It says "Property svn:mime-type set > to image/png" and so I don’t know why. Maybe involved in maintaining the > WinCairo EWS bot can investigate? @Don: Can you please help?

I’ll ask someone to apply the patch locally to see what happens and if it works we can just cq it.

Comment on attachment 328775 [details] Patch Fujii suspects that on the bots core.autocrlf is `true` but it should be `false`. I’ll confirm this tomorrow. He also verified that the patch compiled on wincairo so I’m going to cq+ this to not block anyone.

Comment on attachment 328775 [details] Patch Clearing flags on attachment: 328775 Committed r225736: <https://trac.webkit.org/changeset/225736>

All reviewed patches have been landed. Closing bug.

<rdar://problem/35959544>

(In reply to Don Olmstead from comment #72) > Comment on attachment 328775 [details] > Patch > > Fujii suspects that on the bots core.autocrlf is `true` but it should be > `false`. I’ll confirm this tomorrow. > > He also verified that the patch compiled on wincairo so I’m going to cq+ > this to not block anyone. Thanks for checking! Note that I set cq- because there were some remaining review feedback to fix before landing it. @Minsheng: Can you please open a follow-up bug with a patch addressing the concerns raised by Zalan and I?

Comment on attachment 328775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328775&action=review > LayoutTests/ChangeLog:17 > + * platform/gtk/mathml/opentype/opentype-stretchy-horizontal-expected.txt: Removed. Oops, this is bad! The GTK/windows tests should either have been updated or untouched so that someone can rebase them after the patch lands. You should not just remove them.

(In reply to Frédéric Wang (:fredw) from comment #78) > https://trac.webkit.org/changeset/226119/ Thanks! I have a question: you mentioned before that test outputs can be fetched from build.webkit.org, but I cannot find any relevant test outputs on that site. Where could I find them?

(In reply to Minsheng Liu from comment #79) > (In reply to Frédéric Wang (:fredw) from comment #78) > > https://trac.webkit.org/changeset/226119/ > > Thanks! > > I have a question: you mentioned before that test outputs can be fetched > from build.webkit.org, but I cannot find any relevant test outputs on that > site. Where could I find them? You'll find them in https://build.webkit.org/results ; for example https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r226096%20(3524)/results.html for Apple Windows port.