[webkit-reviews] review granted: [Bug 209684] ASSERTION FAILED: m_wrapper on imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/ready-states/autoplay.html : [Attachment 395185] 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 209684: ASSERTION FAILED: m_wrapper on
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/
ready-states/autoplay.html
https://bugs.webkit.org/show_bug.cgi?id=209684

Attachment 395185: Patch

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




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

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

> Source/WebCore/html/HTMLMediaElement.cpp:5776
> +    if (m_creatingControls)
> +	   return true;
> +
> +    if (m_asyncEventQueue->hasPendingActivity() ||
m_playbackTargetIsWirelessQueue.hasPendingTasks())
> +	   return true;
> +
> +    // If there is playing audio, we need to make sure the media element we
don't get destroyed so that
> +    // it does not get interrupted.
> +    if (hasAudio() && isPlaying())
> +	   return true;
> +
> +    // We need to keep the wrapper alive as long as we may fire events and
there are event listeners.
> +    return m_player && (!ended() || seeking() || m_networkState >=
NETWORK_IDLE) && hasEventListeners();

I guess it’s nice that the multiple return statements give us a place to put
comments. But I really like the "||" form to clarify there is no tricky logic,
just multiple independent reasons:

    return m_creatingControls
	|| m_asyncEventQueue->hasPendingActivity()
	|| m_playbackTargetIsWirelessQueue.hasPendingTasks()
	|| (hasAudio() && isPlaying())
	|| (m_player && (!ended() || seeking() || m_networkState >=
NETWORK_IDLE) && hasEventListeners());

There’s something so appealing to that style for me.