Created attachment 276830 [details] WIP Patch This is bug 156536 for the <mspace/> element.
Comment on attachment 276830 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276830&action=review > Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:66 > + spaceElement.depthLength().toUserUnits(style(), m_depth); Note: my plan (not tested yet) was to directly introduce helper function mspaceWidth() mspaceHeight() and mspaceDepth() that would directly call toUserUnits. That way we can just remove m_width, m_height and m_depth members.
Created attachment 279108 [details] WIP Patch
Created attachment 279113 [details] Patch for EWS testing
Created attachment 279114 [details] Patch for EWS
Created attachment 279115 [details] Patch
Created attachment 279241 [details] Patch
Comment on attachment 279241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279241&action=review This looks good except: (1) I think we could reduce some #include churn if you moved those implementations into the CPP file, and (2) this doesn't apply to the tree at present. Could you please revise and I'll approve it? > Source/WebCore/mathml/MathMLElement.h:66 > + bool dirty = true; These should be in our standard C++11 style: LengthType type { LengthType::ParsingFailed; } float value { 0 }; bool dirty { true }; > Source/WebCore/mathml/MathMLSpaceElement.h:40 > + const Length& depth() { return cachedMathMLLength(MathMLNames::depthAttr, m_depth); } If you moved these into the implementation file, you could probably avoid #including MathMLNames.h in the header file. This would be beneficial to build time. > Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:-78 > - setNeedsLayoutAndPrefWidthsRecalc(); Yay!
Created attachment 283153 [details] Patch Review comments addressed, but this is still blocked by bug 156792.
Created attachment 283308 [details] Patch bug 156792 + 156795 (for EWS testing)
Comment on attachment 283153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283153&action=review Looks good. r=me after you land the patch this depends on. > Source/WebCore/mathml/MathMLSpaceElement.cpp:39 > + : MathMLElement(tagName, document) It's a little unusual for us to inline constructors like this. Did you measure this as a bottleneck of some kind?
(In reply to comment #10) > Comment on attachment 283153 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283153&action=review > > Looks good. r=me after you land the patch this depends on. > > > Source/WebCore/mathml/MathMLSpaceElement.cpp:39 > > + : MathMLElement(tagName, document) > > It's a little unusual for us to inline constructors like this. Did you > measure this as a bottleneck of some kind? I don't think I did that on purpose, I'll remove the inline keyword (it seems that I copied it from MathMLMathElement which has had it for a long time).
Created attachment 283387 [details] Patch
Committed r203108: <http://trac.webkit.org/changeset/203108>