[webkit-reviews] review granted: [Bug 129525] [MSE] YouTube videos fail to play : [Attachment 225539] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 129525: [MSE] YouTube videos fail to play
https://bugs.webkit.org/show_bug.cgi?id=129525

Attachment 225539: Patch
https://bugs.webkit.org/attachment.cgi?id=225539&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225539&action=review


> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1185
> +    if (!m_bufferedSinceLastMonitor)
> +	   return;

Why do this after computing the values above? I suggest moving this line of
code up to the top of the function.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1203
> +    double nearest = m_buffered->nearest(m_source->currentTime());
> +    double currentTime = m_source->currentTime();

Lets evaluate this in the reverse order so we can use currentTime to compute
nearest. Seems more logical what way too. In fact, I probably would not put
nearest into a local variable at all.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1204
> +    return abs(nearest - currentTime) <= CurrentTimeFudgeFactor;

This is wrong. Calling abs here converts the result to an integer. We want to
call fabs. This also means this code wasn’t working before; instead of 1/24
second our threshold was more like 1 second. Is there some way we can test to
make sure it is really working?

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1214
> +    return m_buffered->end(found, IGNORE_EXCEPTION) - currentTime >
CurrentTimeFudgeFactor;

Isn’t ASSERT_NO_EXCEPTION what we want, rather than IGNORE_EXCEPTION? And
further, Antti has been encouraging us not to use the public DOM functions in
internal WebKit code in this way, so it would be nice to avoid it entirely.

> Source/WebCore/Modules/mediasource/SourceBuffer.h:88
> +    bool hasCurrentTime();
> +    bool hasFutureTime();
> +    bool canPlayThrough();

Why aren’t any of these functions const? I guess canPlayThrough has some
serious side effects. But the other two don’t.