[webkit-reviews] review granted: [Bug 210492] Make use of WeakHashSet for MediaStreamTrackPrivate and RealtimeMediaSource observers : [Attachment 396410] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 210492: Make use of WeakHashSet for MediaStreamTrackPrivate and
RealtimeMediaSource observers
https://bugs.webkit.org/show_bug.cgi?id=210492

Attachment 396410: Patch

https://bugs.webkit.org/attachment.cgi?id=396410&action=review




--- Comment #4 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 396410
  --> https://bugs.webkit.org/attachment.cgi?id=396410
Patch

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

r=me

>>> Source/WTF/wtf/WeakHashSet.h:144
>>> +		 if (item && m_set.contains(*item.m_impl))
>> 
>> This doesn't look right to me.
>> 
>> The lambda shouldn't need to call makeWeakPtr. That's a needless cost. It
can just return the pointer or null, depending on whether the WeakPtrImpl is
truthy. 
>> 
>> Also, we shouldn't need to check contains(). We just need to check whether
the pointer is non-null / the WeakPtrImpl is truthy.
> 
> What is the guarantee that the vector pointers remain valid while looping
over the vector and calling callback(item)?
> 
> WeakHashSet::add/remove could be made knowledgeable about the fact that a
forEach call is ongoing to make sure m_set does not get modified during the
forEach execution. Then we no longer need a vector at all. We would still
probably need to check for any to-be-removed entry.

I see. I guess I was imagining behavior more like other iteration and
WTF::forEach, where modifying the set during iteration just isn't supported.
But I don't object to supporting it.