Bug 160396

Created attachment 285234 [details] Patch

Comment on attachment 285234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285234&action=review Why is this change safe? Why is it OK to just delete one of these instead of calling destroy() on it? > Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:89 > + std::unique_ptr<RenderMathMLFencedOperator> newOperator = std::make_unique<RenderMathMLFencedOperator>(document(), RenderStyle::createAnonymousStyleWithDisplay(style(), BLOCK), operatorString, form, flag); auto is preferred over writing out the long type here > Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:96 > + std::unique_ptr<RenderMathMLFencedOperator> openFence = createMathMLOperator(m_open, MathMLOperatorDictionary::Prefix, MathMLOperatorDictionary::Fence); auto is preferred over writing out the long type here > Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:99 > + std::unique_ptr<RenderMathMLFencedOperator> closeFence = createMathMLOperator(m_close, MathMLOperatorDictionary::Postfix, MathMLOperatorDictionary::Fence); auto is preferred over writing out the long type here

@jfernandez: Since you suggested that change, can you please take a look at this patch?

Comment on attachment 285234 [details] Patch I think that the change is correct, after applying the changes suggested by darin.

After thinking a bit more on previous comments, I wonder we we want RenderMathMLFenced to be an unique_ptr, instead of a regular RefPtr like most of render objects are. I guess this change comes from the fact that this class is currently implemented using RenderPtr, which as far as I understand it, it's like the old OwnPtr smart pointer class. In that sense, using unique_ptr is the natural change, but I alos like to know why this class was implemented using RenderPTr in the first place.

(In reply to comment #5) > After thinking a bit more on previous comments, I wonder we we want > RenderMathMLFenced to be an unique_ptr, instead of a regular RefPtr like > most of render objects are. > > I guess this change comes from the fact that this class is currently > implemented using RenderPtr, which as far as I understand it, it's like the > old OwnPtr smart pointer class. In that sense, using unique_ptr is the > natural change, but I alos like to know why this class was implemented using > RenderPTr in the first place. I'm not sure about these. There is clearly an issue with the design of the mfenced element since its text is not in text node and one has to do some hack display it in some way: either by painting the text by hand (which is what Gecko does) or by creating an anonymous operators (which is what WebKit does and what is assumed by the accessibility code). r161491 seems to be where the anonymous operators became a RenderPtr, but there is no explanation on bug 126622.

Created attachment 286598 [details] Patch I'm attaching the updated patch. I don't think it's urgent and in general my preference for bug 160509 would just be to drop support for the mfenced element. So I'm unassigning myself from this bug.