[webkit-reviews] review requested: [Bug 228531] Missing playing events when the ready state becomes HAVE_FUTURE_DATA/HAVE_ENOUGH_DATA from HAVE_METADATA state : [Attachment 434491] patch

[bugzilla-daemon at webkit.org]

Tomoki Imai <tomoki.imai at sony.com> has asked  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 434491: patch

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




--- Comment #3 from Tomoki Imai <tomoki.imai at sony.com> ---
Created attachment 434491

  --> https://bugs.webkit.org/attachment.cgi?id=434491&action=review

patch

(In reply to Eric Carlson from comment #2)

Thanks for your review!
I reflected your comments except the last one to the patch.

Is it acceptable for you?

> > 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;

In my opinion, we should keep the current style even if there is an additional
identing, because the current code reflects the specification structure.
I feel it's a bit confusing to apply the different style if condition from the
other ones.

```
Apply the first applicable set of substeps from the following list:
- If the previous ready state was HAVE_CURRENT_DATA or less, and the new ready
state is HAVE_FUTURE_DATA
  ...
- If the new ready state is HAVE_ENOUGH_DATA
  ...
```
https://html.spec.whatwg.org/multipage/media.html#ready-states



> Comment on attachment 434400 [details]
> 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

Agreed, fixed.

> 
> > 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

Fixed.

> 
> > 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)`

Thanks for your suggestion, applied. 
At first I thought the current code might be better because this if condition
comes from the specification text. 
But rethinked that `if (m_readyState < HAVE_FUTURE_DATA)` is better on the
performance and there is no confusion because we have the comment about the
specification.

> 
> > 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;

Thanks, moving tracksAreReady if condition sounds good. Fixed.