Summary: | Update layout for msub, msup, msubsup to handle script changes | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Milowski <alex> | ||||||||||||
Component: | MathML | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bdakin, darin, jkjiang, kenneth, webkit.review.bot, yong.li.webkit | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Alex Milowski
2011-06-04 14:03:45 PDT
Created attachment 96034 [details]
New layout code that handles child replacement
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.
Created attachment 96035 [details]
New layout code that handles child replacement w/ fixed change log
Lovely.
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 (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. 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.
(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. (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. (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. Created attachment 96050 [details]
Updated patch to address review comments and removes hack for style check
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()). (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. (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. (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. Created attachment 96054 [details]
unsigned update
Comment on attachment 96035 [details]
New layout code that handles child replacement w/ fixed change log
Cleared review on old patch.
(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. (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. 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 on attachment 97808 [details] A version with the requested code change. Clearing flags on attachment: 97808 Committed r89273: <http://trac.webkit.org/changeset/89273> All reviewed patches have been landed. Closing bug. 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); 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()) { ..... } |