[webkit-reviews] review denied: [Bug 55611] The memory of FloatingObject should be handled automatically : [Attachment 85839] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 55611: The memory of FloatingObject should be handled automatically
https://bugs.webkit.org/show_bug.cgi?id=55611

Attachment 85839: Patch
https://bugs.webkit.org/attachment.cgi?id=85839&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.