Bug 62098 - Update layout for msub, msup, msubsup to handle script changes
Summary: Update layout for msub, msup, msubsup to handle script changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-04 14:03 PDT by Alex Milowski
Modified: 2012-03-09 14:37 PST (History)
6 users (show)

See Also:


Attachments
New layout code that handles child replacement (135.03 KB, patch)
2011-06-04 14:09 PDT, Alex Milowski
no flags Details | Formatted Diff | Diff
New layout code that handles child replacement w/ fixed change log (135.08 KB, patch)
2011-06-04 14:15 PDT, Alex Milowski
kenneth: commit-queue-
Details | Formatted Diff | Diff
Updated patch to address review comments and removes hack for style check (134.57 KB, patch)
2011-06-05 09:08 PDT, Alex Milowski
no flags Details | Formatted Diff | Diff
unsigned update (134.56 KB, patch)
2011-06-05 12:34 PDT, Alex Milowski
no flags Details | Formatted Diff | Diff
A version with the requested code change. (134.29 KB, patch)
2011-06-20 09:13 PDT, Alex Milowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Milowski 2011-06-04 14:03:45 PDT
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.
Comment 1 Alex Milowski 2011-06-04 14:09:33 PDT
Created attachment 96034 [details]
New layout code that handles child replacement
Comment 2 WebKit Review Bot 2011-06-04 14:13:04 PDT
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.
Comment 3 Alex Milowski 2011-06-04 14:15:47 PDT
Created attachment 96035 [details]
New layout code that handles child replacement w/ fixed change log

Lovely.
Comment 4 Kenneth Rohde Christiansen 2011-06-05 08:43:12 PDT
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
Comment 5 Alex Milowski 2011-06-05 08:53:49 PDT
(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.
Comment 6 Kenneth Rohde Christiansen 2011-06-05 08:56:43 PDT
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.
Comment 7 Alex Milowski 2011-06-05 09:01:52 PDT
(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.
Comment 8 Kenneth Rohde Christiansen 2011-06-05 09:04:57 PDT
(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.
Comment 9 Alex Milowski 2011-06-05 09:06:14 PDT
(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.
Comment 10 Alex Milowski 2011-06-05 09:08:21 PDT
Created attachment 96050 [details]
Updated patch to address review comments and removes hack for style check
Comment 11 Darin Adler 2011-06-05 09:37:21 PDT
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()).
Comment 12 Alex Milowski 2011-06-05 10:36:17 PDT
(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.
Comment 13 Beth Dakin 2011-06-05 11:44:46 PDT
(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.
Comment 14 Alex Milowski 2011-06-05 12:22:40 PDT
(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.
Comment 15 Alex Milowski 2011-06-05 12:34:15 PDT
Created attachment 96054 [details]
unsigned update
Comment 16 Darin Adler 2011-06-18 12:20:11 PDT
Comment on attachment 96035 [details]
New layout code that handles child replacement w/ fixed change log

Cleared review on old patch.
Comment 17 Darin Adler 2011-06-18 12:21:56 PDT
(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.
Comment 18 Alex Milowski 2011-06-20 05:09:53 PDT
(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.
Comment 19 Alex Milowski 2011-06-20 09:13:21 PDT
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.
Comment 20 WebKit Review Bot 2011-06-20 12:36:03 PDT
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>
Comment 21 WebKit Review Bot 2011-06-20 12:36:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Yong Li 2012-03-09 14:25:12 PST
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);
Comment 23 Jacky Jiang 2012-03-09 14:37:30 PST
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()) {

  .....
}