Bug 34278

Created attachment 47635 [details] Correct patch with missing rendering object

Attachment 47635 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/mathml/RenderMathMLSubSup.cpp:222: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 47637 [details] Updated patch to fix style errors

Created attachment 47735 [details] Updated patch to include updates from the change in CSS from bug 34275

Created attachment 48390 [details] Updated patch to latest trunk with anonymous blocks removed.

Comment on attachment 48390 [details] Updated patch to latest trunk with anonymous blocks removed. > + > + RenderObject* object = 0; Newline at beginning of method, please remove. > +static const int topAdjustDivisor = 3; > +static const int subsupScriptMargin = 1; Mark as global. > + > + > +RenderMathMLSubSup::RenderMathMLSubSup(Node *subsup) Two new lines are not needed. * aligned to the wrong side > + m_kind = SubSup; > + if (element->hasLocalName(MathMLNames::msubTag)) > + m_kind = Sub; > + else if (element->hasLocalName(MathMLNames::msupTag)) > + m_kind = Sup; > +} Same comment as in last patch > + // FIXME: This should be in ems Is this hard to fix? > +void RenderMathMLSubSup::stretchToHeight(int height) > +{ > + RenderObject* base = firstChild(); > + if (!base) > + return; > + > + if (base->isRenderMathMLBlock()) { > + RenderMathMLBlock* block = toRenderMathMLBlock(base); > + block->stretchToHeight(static_cast<int>(1.2 * height)); where does the constant come from > + } > + if (height > 0 && m_kind == SubSup && m_scripts) { > + > + RenderObject* script = m_scripts->firstChild(); No need for the newline above > + if (script) { > + // calculate the script height without the container margins > + int topHeight = 0; > + RenderObject* top = script; > + RenderObject* child = top->firstChild(); > + if (child && child->isBoxModelObject()) { > + RenderBoxModelObject* box = toRenderBoxModelObject(child); > + topHeight = box->offsetHeight(); > + } > + int bottomHeight = 0; > + RenderObject* bottom = script; > + child = bottom->firstChild(); > + if (child && child->isBoxModelObject()) { > + RenderBoxModelObject* box = toRenderBoxModelObject(child); > + bottomHeight = box->offsetHeight(); > + } There is some duplicated code, would it make sense to use an inline method instead? > + > +int RenderMathMLSubSup::nonOperatorHeight() const > +{ > + return 0; > +} You have this code in other classes as well, would it make sense using a parent implementation? or is that one different from 0? > + > +void RenderMathMLSubSup::layout() > +{ > + > + RenderBlock::layout(); Non-needed newline > + if (m_kind == SubSup) { > + int width = 0; > + RenderObject* current = firstChild(); > + while (current) { > + if (current->isBoxModelObject()) { > + > + RenderBoxModelObject* box = toRenderBoxModelObject(current); again. > +int RenderMathMLSubSup::baselinePosition(bool , bool) const remove space before , > + return topAdjust + (base ? base->baselinePosition(true) : 0) + 4; What doest the true stand for here? maybe add a /* comment */ in front of it? like baselinePosition( /* recalc */ true)...

(In reply to comment #6) > > Newline at beginning of method, please remove. > > > +static const int topAdjustDivisor = 3; > > +static const int subsupScriptMargin = 1; > > Mark as global. > > > + > > + > > +RenderMathMLSubSup::RenderMathMLSubSup(Node *subsup) > > Two new lines are not needed. * aligned to the wrong side Odd that the style checker doesn't find these things. > > > + // FIXME: This should be in ems > > Is this hard to fix? That's just incorrect. I removed the comment for now. It may need to be made proportional to the size of the parts but that will be worked on later as we work on "squeezing" the mathematics to make it more readable. > > > +void RenderMathMLSubSup::stretchToHeight(int height) > > +{ > > + RenderObject* base = firstChild(); > > + if (!base) > > + return; > > + > > + if (base->isRenderMathMLBlock()) { > > + RenderMathMLBlock* block = toRenderMathMLBlock(base); > > + block->stretchToHeight(static_cast<int>(1.2 * height)); > > where does the constant come from It is just a subjective expansion of the msubsup since it needs to extend above and below the row height. I added a descriptive global for that. > > + if (script) { > > + // calculate the script height without the container margins > > + int topHeight = 0; > > + RenderObject* top = script; > > + RenderObject* child = top->firstChild(); > > + if (child && child->isBoxModelObject()) { > > + RenderBoxModelObject* box = toRenderBoxModelObject(child); > > + topHeight = box->offsetHeight(); > > + } > > + int bottomHeight = 0; > > + RenderObject* bottom = script; > > + child = bottom->firstChild(); > > + if (child && child->isBoxModelObject()) { > > + RenderBoxModelObject* box = toRenderBoxModelObject(child); > > + bottomHeight = box->offsetHeight(); > > + } > > There is some duplicated code, would it make sense to use an inline method > instead? I made inline functions for this in the RenderMathMLBlock header so I can use them elsewhere too. > > > > + > > +int RenderMathMLSubSup::nonOperatorHeight() const > > +{ > > + return 0; > > +} > > You have this code in other classes as well, would it make sense using a parent > implementation? or is that one different from 0? There is a parent implementation but it doesn't return zero for this object and that is why I need this method here. There are similar requirements for other MathML rendering objects. > > > + if (m_kind == SubSup) { > > + int width = 0; > > + RenderObject* current = firstChild(); > > + while (current) { > > + if (current->isBoxModelObject()) { > > + > > + RenderBoxModelObject* box = toRenderBoxModelObject(current); > > again. > > > +int RenderMathMLSubSup::baselinePosition(bool , bool) const > > remove space before , > > + return topAdjust + (base ? base->baselinePosition(true) : 0) + 4; > > What doest the true stand for here? maybe add a /* comment */ in front of it? > > like baselinePosition( /* recalc */ true)... I fixed that to use the method parameters rather than fixing it. That should be correct and much more clear.

It sounds like you fixed the issues; did you forget to upload a new patch?

(In reply to comment #8) > It sounds like you fixed the issues; did you forget to upload a new patch? I'm waiting for the other patch to commit because I expect merge problems with this patch on MathMLInlineContainer.cpp

Created attachment 49656 [details] Updated patch to address reviewer comments and merged with latest trunk

Attachment 49656 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/mathml/RenderMathMLBlock.h:55: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/mathml/RenderMathMLBlock.h:63: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/mathml/RenderMathMLBlock.h:71: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/mathml/RenderMathMLBlock.h:79: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/mathml/RenderMathMLSubSup.cpp:184: Missing space after , [whitespace/comma] [3] WebCore/mathml/RenderMathMLSubSup.cpp:189: Missing space after , [whitespace/comma] [3] Total errors found: 6 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 49658 [details] Updated patch to address style errors

Comment on attachment 49658 [details] Updated patch to address style errors Clearing flags on attachment: 49658 Committed r55386: <http://trac.webkit.org/changeset/55386>

All reviewed patches have been landed. Closing bug.