Bug 55611 – The memory of FloatingObject should be handled automatically

• Status: NEW
• Component: Layout and Rendering

[Benjamin Poulain]

Currently, the FloatingObjects of RenderBlock have to be delete manually. The ownership of those objects is quite simple, that should be handled automatically.

OwnPtr won't do it, but maybe PassOwnPtr in the structure could do the trick. If not, a simple wrapper to deal with the memory could be a solution.

[Darin Adler]

(In reply to comment #0)
> OwnPtr won't do it

Why? Maybe we can specialize the template to do something simple with OwnPtr.

> maybe PassOwnPtr in the structure could do the trick

Unlikely.

[Benjamin Poulain]

Created attachment 85811 [details]
Patch

First step, make ListHashSet more flexible to be able to use Type and PassType with the structure.

[Benjamin Poulain]

Created attachment 85839 [details]
Patch

First step, make ListHashSet more flexible to be able to use Type and PassType with the structure. Same patch without French-inspired words in the changlog :)

[Darin Adler]

Comment on attachment 85839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85839&action=review

review- because the change to EventQueue is incorrect

> Source/JavaScriptCore/ChangeLog:27
> +        (WTF::ListHashSetNode::ListHashSetNode):
> +        (WTF::ListHashSetTranslator::hash):
> +        (WTF::ListHashSetTranslator::equal):
> +        (WTF::ListHashSetTranslator::translate):
> +        (WTF::::find):
> +        (WTF::::contains):
> +        (WTF::::add):
> +        (WTF::::insertBefore):
> +        (WTF::::remove):

It would be better to add comments explaining what’s changing in these functions. Also, the script generates “WTF::::find”, which is nonsense. Please fix that rather than checking it in.

> Source/JavaScriptCore/wtf/ListHashSet.h:103
> +        template<typename ArgType> iterator find(const ArgType&);
> +        template<typename ArgType> const_iterator find(const ArgType&) const;
> +        template<typename ArgType> bool contains(const ArgType&) const;

This is stylistically different than the approach used in HashSet; we don’t provide a translator so this works only if the type is assignment-compatible. I don’t think it’s good for HashSet and ListHashSet to take different approaches to the same problem.

HashSet also uses T for the type name and here you used the name ArgType.

Lets do this in a way that’s consistent if possible.

> Source/WebCore/ChangeLog:6
> +        The memory of FloatingObject should be handled automatically
> +        https://bugs.webkit.org/show_bug.cgi?id=55611

I don’t think it makes sense to have the bug mention FloatingObject when this patch is just about extending ListHashSet. You should make a new bugs.webkit.org entry for just adding the new feature and then you can use a blocking relationship. It’s best not to use one bug to track two changes.

> Source/WebCore/dom/EventQueue.cpp:112
> +            if (event == lastPendingEvent)
> +                break;

This won’t work if cancelEvent is called, passing in lastPendingEvent, during the execution of the event dispatch.