[webkit-reviews] review granted: [Bug 228531] Missing playing events when the ready state becomes HAVE_FUTURE_DATA/HAVE_ENOUGH_DATA from HAVE_METADATA state : [Attachment 434400] patch to fix the issue

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Tomoki Imai
<tomoki.imai at sony.com>'s request for review:
Bug 228531: Missing playing events when the ready state becomes
HAVE_FUTURE_DATA/HAVE_ENOUGH_DATA from HAVE_METADATA state
https://bugs.webkit.org/show_bug.cgi?id=228531

Attachment 434400: patch to fix the issue

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




--- Comment #2 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 434400
  --> https://bugs.webkit.org/attachment.cgi?id=434400
patch to fix the issue

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

> Source/WebCore/ChangeLog:16
> +	   - According to the specification, scheduleNotifyAboutPlaying should
be in  "internal play steps" and check the ready state. Checking ready state
fixes the issue where playing event is fired twice.

Nit: it would be nice to wrap this line to make it possible to read without
scrolling on a small screen

> Source/WebCore/ChangeLog:22
> +	   (WebCore::HTMLMediaElement::setReadyState): Added missing
scheduleNotifyAboutPlaying calls. Added do-while to make the implementation
similar to the specification text
> +	   (WebCore::HTMLMediaElement::playInternal): Added
scheduleNotifyAboutPlaying call. According to the specification, "internal play
steps" should "notify about playing" when the ready state is HAVE_FUTURE_DATA
or HAVE_ENOUGH_DATA.

Ditto

> Source/WebCore/html/HTMLMediaElement.cpp:2442
> +	       if (!(m_readyState == HAVE_FUTURE_DATA || m_readyState ==
HAVE_ENOUGH_DATA))

This could be simplified to `if (m_readyState < HAVE_FUTURE_DATA)`

> Source/WebCore/html/HTMLMediaElement.cpp:2446
> +	   if (m_readyState == HAVE_FUTURE_DATA && oldState <=
HAVE_CURRENT_DATA && tracksAreReady) {

Can't you out here break if tracks aren't ready:

    if (!tracksAreReady)
	break;

> Source/WebCore/html/HTMLMediaElement.cpp:2455
> +	   if (m_readyState == HAVE_ENOUGH_DATA && oldState < HAVE_ENOUGH_DATA
&& tracksAreReady) {

I think this would be easier to read if you break if the condition isn't true
instead of indenting:

    if (m_readyState != HAVE_ENOUGH_DATA || oldState >= HAVE_ENOUGH_DATA)
	break;