[webkit-reviews] review granted: [Bug 73964] Use HashMap<Node*, OwnPtr<...>> in ChildListMutationScope : [Attachment 118423] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adam Klein <adamk at chromium.org>'s
request for review:
Bug 73964: Use HashMap<Node*, OwnPtr<...>> in ChildListMutationScope
https://bugs.webkit.org/show_bug.cgi?id=73964

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118423&action=review


> Source/JavaScriptCore/ChangeLog:8
> +	   * wtf/HashTraits.h: Add passOut(std::nullptr_t) to allow callers to
use HashMap::take on an entry whose value is null.

This is an incorrect comment. The code is used when take is called and there is
no entry, not when the entry’s value is null. But even if we never do that, the
reason we have to add passOut is to make HashMake::take compile the code for
that case.

> Source/WebCore/dom/ChildListMutationScope.cpp:245
> -	   m_accumulations.set(target, 0);
> +	   m_accumulations.set(target, nullptr);

Maybe we should do a remove here instead of a set.

> Source/WebCore/dom/ChildListMutationScope.cpp:261
> -    RefPtr<ChildListMutationAccumulator> record =
m_accumulations.take(target);
> +    OwnPtr<ChildListMutationAccumulator>
record(m_accumulations.take(target));

Why not use "="? I find the other style harder to read.