Use std::unique_ptr for mfenced anonymous operators

• Component: MathML
• Status: NEW ---

[Frédéric Wang (:fredw)]

We do not plan to get rid of anonymous mfenced operators in the short term. Currently, these anonymous operators are stored as RenderPtr<RenderMathMLFencedOperator>. We should consider using std::unique_ptr<RenderMathMLFencedOperator> instead.

[Frédéric Wang (:fredw)]

Created attachment 285234 [details]
Patch

[Darin Adler]

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

[Frédéric Wang (:fredw)]

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

[Javier Fernandez]

Comment on attachment 285234 [details]
Patch

I think that the change is correct, after applying the changes suggested by darin.

[Javier Fernandez]

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.

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

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.