[Webkit-unassigned] [Bug 205460] Update TrackBase to store m_mediaElement as a WeakPtr

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=205460

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com

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

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

Copying a WeakPtr is expensive so it’s better if we transfer ownership when we can rather than making copies.

> Source/WebCore/html/track/AudioTrack.h:68
> +    void setMediaElement(WeakPtr<HTMLMediaElement>) override;

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/AudioTrackList.h:38
> +    static Ref<AudioTrackList> create(WeakPtr<HTMLMediaElement> owner, ScriptExecutionContext* context)

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/AudioTrackList.h:54
> +    AudioTrackList(WeakPtr<HTMLMediaElement>, ScriptExecutionContext*);

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/InbandTextTrack.h:52
> +    void setMediaElement(WeakPtr<HTMLMediaElement>) override;

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/TextTrackList.h:39
> +    static Ref<TextTrackList> create(WeakPtr<HTMLMediaElement> element, ScriptExecutionContext* context)

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/TextTrackList.h:63
> +    TextTrackList(WeakPtr<HTMLMediaElement>, ScriptExecutionContext*);

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/TrackBase.h:52
> +    virtual void setMediaElement(WeakPtr<HTMLMediaElement>);

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/TrackBase.h:53
> +    WeakPtr<HTMLMediaElement> mediaElement() { return m_mediaElement; }

Might be better to use const WeakPtr& for the return type. Or even leave the return type a raw pointer. Not sure which idiom is best. Returning a copy of the WeakPtr is costly.

> Source/WebCore/html/track/TrackBase.h:86
> +    WeakPtr<HTMLMediaElement> m_mediaElement { nullptr };

No need for the "{ nullptr }" when it’s a smart pointer. Only raw pointers need that initialization.

> Source/WebCore/html/track/TrackListBase.h:60
> +    WeakPtr<HTMLMediaElement> mediaElement() const { return m_element; }

Might be better to use const WeakPtr& for the return type. Or even leave the return type a raw pointer. Not sure which idiom is best. Returning a copy of the WeakPtr is costly.

> Source/WebCore/html/track/TrackListBase.h:69
> +    TrackListBase(WeakPtr<HTMLMediaElement>, ScriptExecutionContext*);

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/VideoTrack.h:76
> +    void setMediaElement(WeakPtr<HTMLMediaElement>) override;

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/VideoTrackList.h:38
> +    static Ref<VideoTrackList> create(WeakPtr<HTMLMediaElement> owner, ScriptExecutionContext* context)

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/VideoTrackList.h:55
> +    VideoTrackList(WeakPtr<HTMLMediaElement>, ScriptExecutionContext*);

This should be WeakPtr&& so we can transfer ownership.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20191220/20a407c2/attachment.htm>