[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
bugzilla-daemon at webkit.org
Wed Jul 28 19:56:45 PDT 2021
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.
More information about the webkit-reviews
mailing list