Add comments and improve code styles for RenderMathMLUnderOver::stretchHorizontalOper...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 23 Dec 2017 16:05:41 +0000 (16:05 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 23 Dec 2017 16:05:41 +0000 (16:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180923

Patch by Minsheng Liu <lambda@liu.ms> on 2017-12-23
Reviewed by Frédéric Wang.

Source/WebCore:

The patch improves the code for RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren()
and related function, incorporating the following suggestions from bug 179682:

- Remove several lines of trailing spaces.
- Change several pointers to references.
- Rewrite horizontalStretchyOperator() (formerly toHorizontalStretchyOperator())
  to make it more conforming to WebKit's coding style.
- Make unembellishedOperator() a const method.
- Add comments for stretchHorizontalOperatorsAndLayoutChildren().
- Eliminate an unnecessary call to fixLayoutAfterStretch() in stretchHorizontalOperatorsAndLayoutChildren().

Add one more case for the test "mathml/opentype/munderover-stretch-width.html"
to handle the corner case where all components of <munderover>/<munder>/<mover> are stretchy.

Since there is no behavior change, no new test is required.

* rendering/mathml/RenderMathMLBlock.h:
(WebCore::RenderMathMLBlock::unembellishedOperator const):
(WebCore::RenderMathMLBlock::unembellishedOperator): Deleted.
* rendering/mathml/RenderMathMLFraction.cpp:
(WebCore::RenderMathMLFraction::unembellishedOperator const):
(WebCore::RenderMathMLFraction::unembellishedOperator): Deleted.
* rendering/mathml/RenderMathMLFraction.h:
* rendering/mathml/RenderMathMLOperator.cpp:
(WebCore::RenderMathMLOperator::resetStretchSize):
* rendering/mathml/RenderMathMLOperator.h:
* rendering/mathml/RenderMathMLScripts.cpp:
(WebCore::RenderMathMLScripts::unembellishedOperator const):
(WebCore::RenderMathMLScripts::unembellishedOperator): Deleted.
* rendering/mathml/RenderMathMLScripts.h:
* rendering/mathml/RenderMathMLUnderOver.cpp:
(WebCore::horizontalStretchyOperator):
(WebCore::fixLayoutAfterStretch):
(WebCore::RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren):
(WebCore::RenderMathMLUnderOver::layoutBlock):
(WebCore::toHorizontalStretchyOperator): Deleted.

LayoutTests:

Add one more case for the test "mathml/opentype/munderover-stretch-width.html"
to handle the corner case where all components of <munderover>/<munder>/<mover> are stretchy.

Since there is no behavior change, no new test is required.

* mathml/opentype/munderover-stretch-width-expected.txt:
* mathml/opentype/munderover-stretch-width.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226287 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/ChangeLog
LayoutTests/mathml/opentype/munderover-stretch-width-expected.txt
LayoutTests/mathml/opentype/munderover-stretch-width.html
Source/WebCore/ChangeLog
Source/WebCore/rendering/mathml/RenderMathMLBlock.h
Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp
Source/WebCore/rendering/mathml/RenderMathMLFraction.h
Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp
Source/WebCore/rendering/mathml/RenderMathMLOperator.h
Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp
Source/WebCore/rendering/mathml/RenderMathMLScripts.h
Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp

index 6aaf0c5..c3f4108 100644 (file)
@@ -1,3 +1,18 @@
+2017-12-23  Minsheng Liu  <lambda@liu.ms>
+
+        Add comments and improve code styles for RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren() and related functions
+        https://bugs.webkit.org/show_bug.cgi?id=180923
+
+        Reviewed by Frédéric Wang.
+
+        Add one more case for the test "mathml/opentype/munderover-stretch-width.html" 
+        to handle the corner case where all components of <munderover>/<munder>/<mover> are stretchy.
+
+        Since there is no behavior change, no new test is required.
+
+        * mathml/opentype/munderover-stretch-width-expected.txt:
+        * mathml/opentype/munderover-stretch-width.html:
+
 2017-12-22  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         Unreviewed GTK layout test gardening
index 1fa51da..7169253 100644 (file)
@@ -23,6 +23,10 @@ This test passes if you see the black thing has the same width as the red bar.
 This test passes if you see the black thing has the same width as the red bar.
 
 ↜
+This test passes if you see both the red and blue things have the same width as the black bar.
+
+↜
+↜
 
 PASS simple stretchy over 
 PASS simple stretchy under 
@@ -32,4 +36,5 @@ PASS unembellished op under with plain op over
 PASS nested embellished op 
 PASS nested non-munderover embellished op 
 PASS simple stretchy under should equal over 
+PASS all stretchy embellished op 
 
index eaee11e..cdb8f01 100644 (file)
@@ -50,6 +50,7 @@
         runCase(6, 'nested embellished op', 'red', 'op');
         runCase(7, 'nested non-munderover embellished op', 'red', 'op');
         runCase(8, 'simple stretchy under should equal over', 'red', 'op');
+        runCase(9, 'all stretchy embellished op', 'red', 'blue', 'bar');
         done();
       }
     </script>
         <mspace id="8-red" width="200px" height="10px" depth="10px" mathbackground="red"></mspace>
       </munderover>
     </math>
+
+    <p>This test passes if you see both the red and blue things have the same width as the black bar.</p>
+    <math display="block">
+      <mover>
+        <mo id="9-blue" lspace="0px" rspace="0px" stretchy="true" mathbackground="blue">&#x219C;</mo>
+        <munder>
+          <mo id="9-red" lspace="0px" rspace="0px" stretchy="true" mathbackground="red">&#x219C;</mo>
+          <mspace id="9-bar" width="200px" height="10px" depth="10px" mathbackground="black"></mspace>
+        </munder>
+      </mover>
+    </math>
   </body>
 </html>
index b6fb8d4..c922e34 100644 (file)
@@ -1,3 +1,47 @@
+2017-12-23  Minsheng Liu  <lambda@liu.ms>
+
+        Add comments and improve code styles for RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren() and related functions
+        https://bugs.webkit.org/show_bug.cgi?id=180923
+
+        Reviewed by Frédéric Wang.
+
+        The patch improves the code for RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren()
+        and related function, incorporating the following suggestions from bug 179682:
+
+        - Remove several lines of trailing spaces.
+        - Change several pointers to references.
+        - Rewrite horizontalStretchyOperator() (formerly toHorizontalStretchyOperator()) 
+          to make it more conforming to WebKit's coding style.
+        - Make unembellishedOperator() a const method.
+        - Add comments for stretchHorizontalOperatorsAndLayoutChildren().
+        - Eliminate an unnecessary call to fixLayoutAfterStretch() in stretchHorizontalOperatorsAndLayoutChildren().
+
+        Add one more case for the test "mathml/opentype/munderover-stretch-width.html" 
+        to handle the corner case where all components of <munderover>/<munder>/<mover> are stretchy.
+
+        Since there is no behavior change, no new test is required.
+
+        * rendering/mathml/RenderMathMLBlock.h:
+        (WebCore::RenderMathMLBlock::unembellishedOperator const):
+        (WebCore::RenderMathMLBlock::unembellishedOperator): Deleted.
+        * rendering/mathml/RenderMathMLFraction.cpp:
+        (WebCore::RenderMathMLFraction::unembellishedOperator const):
+        (WebCore::RenderMathMLFraction::unembellishedOperator): Deleted.
+        * rendering/mathml/RenderMathMLFraction.h:
+        * rendering/mathml/RenderMathMLOperator.cpp:
+        (WebCore::RenderMathMLOperator::resetStretchSize):
+        * rendering/mathml/RenderMathMLOperator.h:
+        * rendering/mathml/RenderMathMLScripts.cpp:
+        (WebCore::RenderMathMLScripts::unembellishedOperator const):
+        (WebCore::RenderMathMLScripts::unembellishedOperator): Deleted.
+        * rendering/mathml/RenderMathMLScripts.h:
+        * rendering/mathml/RenderMathMLUnderOver.cpp:
+        (WebCore::horizontalStretchyOperator):
+        (WebCore::fixLayoutAfterStretch):
+        (WebCore::RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren):
+        (WebCore::RenderMathMLUnderOver::layoutBlock):
+        (WebCore::toHorizontalStretchyOperator): Deleted.
+
 2017-12-22  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         Unreviewed, try to fix the build on recent SDKs after r226274.
index e9c18a4..93926f6 100644 (file)
@@ -57,7 +57,7 @@ public:
     // embellished operator, and omits any embellishments.
     // FIXME: We don't yet handle all the cases in the MathML spec. See
     // https://bugs.webkit.org/show_bug.cgi?id=78617.
-    virtual RenderMathMLOperator* unembellishedOperator() { return 0; }
+    virtual RenderMathMLOperator* unembellishedOperator() const { return 0; }
 
     int baselinePosition(FontBaseline, bool firstLine, LineDirectionMode, LinePositionMode = PositionOnContainingLine) const override;
 
index 7a147e4..447c160 100644 (file)
@@ -152,7 +152,7 @@ RenderMathMLFraction::StackParameters RenderMathMLFraction::stackParameters() co
     return parameters;
 }
 
-RenderMathMLOperator* RenderMathMLFraction::unembellishedOperator()
+RenderMathMLOperator* RenderMathMLFraction::unembellishedOperator() const
 {
     if (!isValid() || !is<RenderMathMLBlock>(numerator()))
         return nullptr;
index 147470b..3145cd3 100644 (file)
@@ -53,7 +53,7 @@ private:
     void layoutBlock(bool relayoutChildren, LayoutUnit pageLogicalHeight = 0) final;
     std::optional<int> firstLineBaseline() const final;
     void paint(PaintInfo&, const LayoutPoint&) final;
-    RenderMathMLOperator* unembellishedOperator() final;
+    RenderMathMLOperator* unembellishedOperator() const final;
 
     MathMLFractionElement& element() const { return static_cast<MathMLFractionElement&>(nodeForNonAnonymous()); }
 
index 7b06b07..17fa994 100644 (file)
@@ -178,7 +178,7 @@ void RenderMathMLOperator::stretchTo(LayoutUnit width)
 void RenderMathMLOperator::resetStretchSize()
 {
     ASSERT(!isStretchWidthLocked());
-    
+
     if (isVertical()) {
         m_stretchHeightAboveBaseline = 0;
         m_stretchDepthBelowBaseline = 0;
index b2478c9..2380dd5 100644 (file)
@@ -80,7 +80,7 @@ private:
     bool isInvisibleOperator() const;
 
     std::optional<int> firstLineBaseline() const final;
-    RenderMathMLOperator* unembellishedOperator() final { return this; }
+    RenderMathMLOperator* unembellishedOperator() const final { return const_cast<RenderMathMLOperator*>(this); }
 
     LayoutUnit verticalStretchedOperatorShift() const;
 
index 3e2a559..0251423 100644 (file)
@@ -59,7 +59,7 @@ ScriptType RenderMathMLScripts::scriptType() const
     return element().scriptType();
 }
 
-RenderMathMLOperator* RenderMathMLScripts::unembellishedOperator()
+RenderMathMLOperator* RenderMathMLScripts::unembellishedOperator() const
 {
     auto base = firstChildBox();
     if (!is<RenderMathMLBlock>(base))
index 404d7f4..7eb3f08 100644 (file)
@@ -42,7 +42,7 @@ class RenderMathMLScripts : public RenderMathMLBlock {
     WTF_MAKE_ISO_ALLOCATED(RenderMathMLScripts);
 public:
     RenderMathMLScripts(MathMLScriptsElement&, RenderStyle&&);
-    RenderMathMLOperator* unembellishedOperator() final;
+    RenderMathMLOperator* unembellishedOperator() const final;
 
 protected:
     bool isRenderMathMLScripts() const override { return true; }
index 853c4cd..4a7969a 100644 (file)
@@ -50,56 +50,74 @@ MathMLUnderOverElement& RenderMathMLUnderOver::element() const
     return static_cast<MathMLUnderOverElement&>(nodeForNonAnonymous());
 }
 
-static RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox* box)
+static RenderMathMLOperator* horizontalStretchyOperator(const RenderBox& box)
 {
-    if (is<RenderMathMLBlock>(box)) {
-        if (auto renderOperator = downcast<RenderMathMLBlock>(*box).unembellishedOperator()) {
-            if (renderOperator->isStretchy() && !renderOperator->isVertical() && !renderOperator->isStretchWidthLocked())
-                return renderOperator;
-        }
-    }
-    return nullptr;
+    if (!is<RenderMathMLBlock>(box))
+        return nullptr;
+
+    auto* renderOperator = downcast<RenderMathMLBlock>(box).unembellishedOperator();
+    if (!renderOperator)
+        return nullptr;
+
+    if (!renderOperator->isStretchy() || renderOperator->isVertical() || renderOperator->isStretchWidthLocked())
+        return nullptr;
+
+    return renderOperator;
 }
-    
-static void fixLayoutAfterStretch(RenderBox* ancestor, RenderMathMLOperator* stretchyOperator)
+
+static void fixLayoutAfterStretch(RenderBox& ancestor, RenderMathMLOperator& stretchyOperator)
 {
-    stretchyOperator->setStretchWidthLocked(true);
-    stretchyOperator->setNeedsLayout();
-    ancestor->layoutIfNeeded();
-    stretchyOperator->setStretchWidthLocked(false);
+    stretchyOperator.setStretchWidthLocked(true);
+    stretchyOperator.setNeedsLayout();
+    ancestor.layoutIfNeeded();
+    stretchyOperator.setStretchWidthLocked(false);
 }
 
 void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren()
 {
     ASSERT(isValid());
     ASSERT(needsLayout());
+
+    // 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 opersators 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.
     
     Vector<RenderBox*, 3> embellishedOperators;
     Vector<RenderMathMLOperator*, 3> stretchyOperators;
     bool isAllStretchyOperators = true;
     LayoutUnit stretchWidth = 0;
-    
+
     for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
-        if (auto* stretchyOperator = toHorizontalStretchyOperator(child)) {
+        if (auto* stretchyOperator = horizontalStretchyOperator(*child)) {
             embellishedOperators.append(child);
             stretchyOperators.append(stretchyOperator);
-            stretchyOperator->resetStretchSize();
-            fixLayoutAfterStretch(child, stretchyOperator);
         } else {
             isAllStretchyOperators = false;
             child->layoutIfNeeded();
             stretchWidth = std::max(stretchWidth, child->logicalWidth());
         }
     }
-    
+
     if (isAllStretchyOperators) {
-        for (auto* embellishedOperator : embellishedOperators)
-            stretchWidth = std::max(stretchWidth, embellishedOperator->logicalWidth());
+        for (size_t i = 0; i < embellishedOperators.size(); i++) {
+            stretchyOperators[i]->resetStretchSize();
+            fixLayoutAfterStretch(*embellishedOperators[i], *stretchyOperators[i]);
+            stretchWidth = std::max(stretchWidth, embellishedOperators[i]->logicalWidth());
+        }
     }
-    
+
     for (size_t i = 0; i < embellishedOperators.size(); i++) {
         stretchyOperators[i]->stretchTo(stretchWidth);
-        fixLayoutAfterStretch(embellishedOperators[i], stretchyOperators[i]);
+        fixLayoutAfterStretch(*embellishedOperators[i], *stretchyOperators[i]);
     }
 }
 
@@ -290,7 +308,7 @@ void RenderMathMLUnderOver::layoutBlock(bool relayoutChildren, LayoutUnit pageLo
     ASSERT(!base().needsLayout());
     ASSERT(scriptType() == ScriptType::Over || !under().needsLayout());
     ASSERT(scriptType() == ScriptType::Under || !over().needsLayout());
-    
+
     LayoutUnit logicalWidth = base().logicalWidth();
     if (scriptType() == ScriptType::Under || scriptType() == ScriptType::UnderOver)
         logicalWidth = std::max(logicalWidth, under().logicalWidth());