[webkit-reviews] review granted: [Bug 224279] OfflineAudioContext objects are leaking : [Attachment 425467] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 224279: OfflineAudioContext objects are leaking
https://bugs.webkit.org/show_bug.cgi?id=224279

Attachment 425467: Patch

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




--- Comment #16 from Darin Adler <darin at apple.com> ---
Comment on attachment 425467
  --> https://bugs.webkit.org/attachment.cgi?id=425467
Patch

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

Surprised there is no less-intrusive solution, but this seems to be implemented
well.

> Source/WTF/wtf/WeakPtr.h:126
> +    explicit WeakPtr(Ref<WeakPtrImpl<Counter>>&& ref,
EnableWeakPtrThreadingAssertions enableWeakPtrThreadingAssertions)

I would use a shorter argument name. Like shouldEnableAssertions.

> Source/WTF/wtf/WeakPtr.h:171
> +    template<typename U> WeakPtr<U, Counter> createWeakPtr(U& object,
EnableWeakPtrThreadingAssertions enableWeakPtrThreadingAssertions =
EnableWeakPtrThreadingAssertions::Yes) const

Ditto.

> Source/WTF/wtf/WeakPtr.h:249
> +#if ASSERT_ENABLED
> +    m_areThreadingAssertionsEnabled = o.m_areThreadingAssertionsEnabled;
> +#endif

Seems like this is not needed. The WeakPtr we are assigning can keep its
m_areThreadingAssertionsEnabled, doesn’t need to inherit it from the thing it’s
getting the pointer value from.

> Source/WTF/wtf/WeakPtr.h:258
> +#if ASSERT_ENABLED
> +    m_areThreadingAssertionsEnabled = o.m_areThreadingAssertionsEnabled;
> +#endif

Ditto.

> Source/WebCore/Modules/webaudio/BaseAudioContext.h:110
> +    WEBCORE_EXPORT static unsigned numberOfInstances();

Seems like we should have a comment somewhere, not necessarily here, to make it
clear that this is specifically for lifetime testing and not otherwise part of
the interface of this class.

> Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.h:36
> +    DefaultAudioDestinationNode(BaseAudioContext&, Optional<float> =
WTF::nullopt);

I think we should use explicit here.

> Source/WebCore/rendering/FloatingObjects.cpp:37
> +    WeakPtr<RenderBox> renderer;

Do we need a check somewhere that WeakPtr is same size as a pointer when
assertions are disabled?