Bug 62098 – Update layout for msub, msup, msubsup to handle script changes

• Component: MathML
• Status: RESOLVED FIXED

[Alex Milowski]

The renderer for msub, msup, and msubsup have incorrect assumptions about child order that has caused a crash in the past (see bug 57897).  The child order may change if a script changes the MathML element.  A modest attempt should be made to ensure the children are placed into the right part of the layout if the base, etc. are replaced.

[Alex Milowski]

Created attachment 96034 [details]
New layout code that handles child replacement

[WebKit Review Bot]

Attachment 96034 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/math..." exit_code: 1

LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Milowski]

Created attachment 96035 [details]
New layout code that handles child replacement w/ fixed change log

Lovely.

[Kenneth Rohde Christiansen]

Comment on attachment 96035 [details]
New layout code that handles child replacement w/ fixed change log

View in context: https://bugs.webkit.org/attachment.cgi?id=96035&action=review

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:66
> +    int position = 0;

unsigned?

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:68
> +        if (current->nodeType() == Node::ELEMENT_NODE)

Isnt there an isElementNode() ? im pretty sure that exists

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:176
> -            int heightDiff = m_scripts ? (m_scripts->offsetHeight() - maxHeight) / 2 : 0;
> +            int heightDiff = m_scripts ?
> +                (m_scripts->offsetHeight() - maxHeight) / 2 
> +                : 0;

Im not sure what our style guide says, but I havent run into this style before. I would just keep it on one line

[Alex Milowski]

(In reply to comment #4)
> (From update of attachment 96035 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96035&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:66
> > +    int position = 0;
> 
> unsigned?

OK.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:68
> > +        if (current->nodeType() == Node::ELEMENT_NODE)
> 
> Isnt there an isElementNode() ? im pretty sure that exists

Yes, there is.  I'll switch to that.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:176
> > -            int heightDiff = m_scripts ? (m_scripts->offsetHeight() - maxHeight) / 2 : 0;
> > +            int heightDiff = m_scripts ?
> > +                (m_scripts->offsetHeight() - maxHeight) / 2 
> > +                : 0;
> 
> Im not sure what our style guide says, but I havent run into this style before. I would just keep it on one line

The current style check script incorrectly identifies this as a bit field.  I filed a bug 62097 on this.  Meanwhile, this is the only way to get around the style check script.  I'd rather not make this change but I certainly don't want to wait for someone to fix that script.

I'd love another way to commit this patch.

[Kenneth Rohde Christiansen]

Our style guide is educational and should not be enforces. Are you sure the commitqueue will reject it because of this ? That should not be the case

> 
> The current style check script incorrectly identifies this as a bit field.  I filed a bug 62097 on this.  Meanwhile, this is the only way to get around the style check script.  I'd rather not make this change but I certainly don't want to wait for someone to fix that script.
> 
> I'd love another way to commit this patch.

[Alex Milowski]

(In reply to comment #6)
> Our style guide is educational and should not be enforces. Are you sure the commitqueue will reject it because of this ? That should not be the case
> 

It will fail the style check.  I don't think the commit queue will reject it but a reviewer will be confused about the comment from the style check.

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> (In reply to comment #6)
> > Our style guide is educational and should not be enforces. Are you sure the commitqueue will reject it because of this ? That should not be the case
> > 
> 
> It will fail the style check.  I don't think the commit queue will reject it but a reviewer will be confused about the comment from the style check.

We often allow code that doesnt 100% follow the style check... no problem :-) in this case the  style checker is wrong. This will just make the code wrong when we fix the style checker.

[Alex Milowski]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Our style guide is educational and should not be enforces. Are you sure the commitqueue will reject it because of this ? That should not be the case
> > > 
> > 
> > It will fail the style check.  I don't think the commit queue will reject it but a reviewer will be confused about the comment from the style check.
> 
> We often allow code that doesnt 100% follow the style check... no problem :-) in this case the  style checker is wrong. This will just make the code wrong when we fix the style checker.

Very true.  I'm always trying to get through this process with the minimum amount of pain.

[Alex Milowski]

Created attachment 96050 [details]
Updated patch to address review comments and removes hack for style check

[Darin Adler]

Comment on attachment 96050 [details]
Updated patch to address review comments and removes hack for style check

View in context: https://bugs.webkit.org/attachment.cgi?id=96050&action=review

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:66
> +    unsigned int position = 0;

unsigned int is wrong for WebKit style. We use just unsigned.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:70
> +    for (Node* current = child->node(); current; current = current->previousSibling()) {
> +        if (current->isElementNode())
> +            position++;
> +    }

Looping through all the child elements is unnecessary. A cleaner way to do it would be to call previousElementSibling a small number of times.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:72
> +    if (position == 1) {

For example, the check here could be if !previousElement.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:105
> +            if (position == 2) 

And the check here could just be if (previousElement && !previousElement->previousElementSibling()).

[Alex Milowski]

(In reply to comment #11)
> (From update of attachment 96050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96050&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:66
> > +    unsigned int position = 0;
> 
> unsigned int is wrong for WebKit style. We use just unsigned.

OK.  That passed the style check script.  I don't see that on the style guide.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:70
> > +    for (Node* current = child->node(); current; current = current->previousSibling()) {
> > +        if (current->isElementNode())
> > +            position++;
> > +    }
> 
> Looping through all the child elements is unnecessary. A cleaner way to do it would be to call previousElementSibling a small number of times.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:72
> > +    if (position == 1) {
> 
> For example, the check here could be if !previousElement.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:105
> > +            if (position == 2) 
> 
> And the check here could just be if (previousElement && !previousElement->previousElementSibling()).


Actually, no.  The number of child element is very small (2 or 3).  While someone could construct a pathological MathML element, that is probably unlikely.  

Meanwhile,  previousElementSibling() loops through the previous siblings just as I've done.  Adding two calls to previousElementSibling would require looping twice.  Also, that is only available on the Element class and that would require a static cast.

I think the code is much cleaner as it stands.

[Beth Dakin]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 96050 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=96050&action=review
> > 
> > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:66
> > > +    unsigned int position = 0;
> > 
> > unsigned int is wrong for WebKit style. We use just unsigned.
> 
> OK.  That passed the style check script.  I don't see that on the style guide.
> 

Neither the style guide nor the style-checker is 100% comprehensive, and we could and should fix that over time. In the meantime, maintaining a stylistically consistent codebase is the best way to have many people hacking on it at once without it turning into a messy jumble of hard-to-read and inconsistent code.

[Alex Milowski]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 96050 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=96050&action=review
> > > 
> > > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:66
> > > > +    unsigned int position = 0;
> > > 
> > > unsigned int is wrong for WebKit style. We use just unsigned.
> > 
> > OK.  That passed the style check script.  I don't see that on the style guide.
> > 
> 
> Neither the style guide nor the style-checker is 100% comprehensive, and we could and should fix that over time. In the meantime, maintaining a stylistically consistent codebase is the best way to have many people hacking on it at once without it turning into a messy jumble of hard-to-read and inconsistent code.

Be that as it may, without either one being up-to-date, people like myself can't know that.  As a result, you'll get inconsistent code.  This is a very frustrating process for people like myself who work on many different projects in different programming languages where such stylistic preferences feel subjective.  I would prefer that the coding style guidelines are always up to date so there is always a canonical reference.  Since I obviously don't know those preferences, I can't contribute to fixing the guidelines.

I'll update the patch, yet again.

[Alex Milowski]

Created attachment 96054 [details]
unsigned update

[Darin Adler]

Comment on attachment 96035 [details]
New layout code that handles child replacement w/ fixed change log

Cleared review on old patch.

[Darin Adler]

(In reply to comment #12)
> Meanwhile,  previousElementSibling() loops through the previous siblings just as I've done.  Adding two calls to previousElementSibling would require looping twice.  Also, that is only available on the Element class and that would require a static cast.
> 
> I think the code is much cleaner as it stands.

I disagree with you here.

[Alex Milowski]

(In reply to comment #17)
> (In reply to comment #12)
> > Meanwhile,  previousElementSibling() loops through the previous siblings just as I've done.  Adding two calls to previousElementSibling would require looping twice.  Also, that is only available on the Element class and that would require a static cast.
> > 
> > I think the code is much cleaner as it stands.
> 
> I disagree with you here.

OK.  Element position is a common criteria upon which rendering and manipulation of markup is a conditionalized.  Rather than having a previousElementSibling(), a useful method would be something like "elementPosition()".  In XPath, this would translate to count(preceding-sibling::*) or count(preceding-sibling::*)+1 depending on whether you want it one or zero based.

[Alex Milowski]

Created attachment 97808 [details]
A version with the requested code change.

I've made the change.  I would just like to see this patch landed so I can move on.

[WebKit Review Bot]

Comment on attachment 97808 [details]
A version with the requested code change.

Clearing flags on attachment: 97808

Committed r89273: <http://trac.webkit.org/changeset/89273>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yong Li]

Alex, what if the new child is an anonymous one and its node() is null? The actual child with node() will later be added to the anonymous one.

See RenderObject::addChild

            table = new (renderArena()) RenderTable(document() /* is anonymous */);
            RefPtr<RenderStyle> newStyle = RenderStyle::create();
            newStyle->inheritFrom(style());
            newStyle->setDisplay(TABLE);
            table->setStyle(newStyle.release());
            addChild(table, beforeChild);
        }
        table->addChild(newChild);

[Jacky Jiang]

Alex, the anonymous block can make childElement null and cause crash, is there a code path for the anonymous block?
See:

void RenderMathMLSubSup::addChild(RenderObject* child, RenderObject* beforeChild)
{
    
    // Note: The RenderMathMLBlock only allows element children to be added.
    Element* childElement = toElement(child->node());

    if (!childElement->previousElementSibling()) {

  .....
}