[webkit-reviews] review granted: [Bug 211464] Poster set after playback begins should be ignored : [Attachment 399133] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 211464: Poster set after playback begins should be ignored
https://bugs.webkit.org/show_bug.cgi?id=211464

Attachment 399133: Patch

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




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

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

Is there code that stops the poster loading once we load the first frame?

> Source/WebCore/ChangeLog:22
> +020-05-11  Antoine Quint  <graouts at apple.com>

Oops, deleted a "2" here.

> Source/WebCore/html/HTMLVideoElement.cpp:136
> +	   if (hasAvailableVideoFrame())
> +	       return;

Not really about this patch:

I wish we had a more consistent rule about when to call through to the base
class in parseAttribute functions. If it was up to me, all parseAttribute
functions would call through to the base class always, even when they handle
the attribute, except when they are trying to disable some specific work the
base class does. But it’s more efficient to *not* call through to the base
class if you know it has no work to do, so some might prefer our current status
quo where we often don’t call through.

> Source/WebCore/html/HTMLVideoElement.cpp:286
> +    else if (displayMode() < Poster && !hasAvailableVideoFrame())
>	   setDisplayMode(Poster);

Is the idea here that if hasAvailableVideoFrame() is true, the display mode
will be changing to Video very soon? Why not change it to Video right now?