[webkit-reviews] review denied: [Bug 110263] Add the default video poster if it doesn't exist in video tag : [Attachment 192318] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied michaelbai at chromium.org's request for
review:
Bug 110263: Add the default video poster if it doesn't exist in video tag
https://bugs.webkit.org/show_bug.cgi?id=110263

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

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


> Source/WebCore/html/HTMLVideoElement.cpp:189
> +    const AtomicString& url = getAttribute(posterAttr);
> +    if (!url.isEmpty())
> +	   return url;

On reflection, it’s a little strange here to special case only an empty URL
without considering whitespace stripping. I could imagine special casing only a
null URL, which means we’d only do this when the poster attribute is entirely
missing. Or special casing any URL that ends up empty, and therefore is
something we won't even try to load.

> Source/WebCore/html/HTMLVideoElement.cpp:316
> +    const AtomicString& url = imageSourceURL();
> +    if (url.isEmpty())
> +	   return KURL();
> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(url));

This is still wrong. The stripping of needs to be done before the isEmpty
check. Just look at the code in getNonEmptyURLAttribute and match its behavior
exactly. That’s the problem with copied and pasted code, I guess. Hard to get
it right!

> Source/WebCore/html/HTMLVideoElement.h:94
> +    AtomicString m_defaultPosterURL;

It’s a little strange to store a copy of this in every video element. There’s
no reason we couldn’t fetch it from the document settings when we need it
instead of when we create the video element.