Bug 55611 - The memory of FloatingObject should be handled automatically
Summary: The memory of FloatingObject should be handled automatically
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on: 55602
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-02 13:38 PST by Benjamin Poulain
Modified: 2011-03-15 15:48 PDT (History)
3 users (show)

See Also:


Attachments
Patch (12.02 KB, patch)
2011-03-15 08:42 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (12.02 KB, patch)
2011-03-15 12:07 PDT, Benjamin Poulain
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2011-03-02 13:38:08 PST
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.
Comment 1 Darin Adler 2011-03-02 17:13:55 PST
(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.
Comment 2 Benjamin Poulain 2011-03-15 08:42:04 PDT
Created attachment 85811 [details]
Patch

First step, make ListHashSet more flexible to be able to use Type and PassType with the structure.
Comment 3 Benjamin Poulain 2011-03-15 12:07:46 PDT
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 :)
Comment 4 Darin Adler 2011-03-15 15:48:54 PDT
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.