[webkit-reviews] review denied: [Bug 110263] Add the default video poster if it doesn't exist in video tag : [Attachment 192766] 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 192766: Patch
https://bugs.webkit.org/attachment.cgi?id=192766&action=review

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


review- because the whitespace stripping is still wrong in this patch; I’m
guessing you uploaded the wrong one

> Source/WebCore/ChangeLog:37
> +	   * WebCore.order: Replace imageSourceAttributeName() with
imageSourceURL()

This file does not need to be updated. You can just remove this change from
your patch.

> Source/WebCore/ChangeLog:39
> +	   * dom/Element.cpp: Replace imageSourceAttributeName() with
imageSourceURL()
> +	   (WebCore::Element::imageSourceURL):

Comments should go on the functions, not the entire file, unless there is no
function name.

> Source/WebCore/ChangeLog:41
> +	   (Element):

Bogus lines like this should just be deleted from the ChangeLog. The script
generates them.

> Source/WebCore/myChangeLog:1
> +2013-03-06  Tao Bai	<michaelbai at chromium.org>

Please don’t land this second change log file!

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

As I mentioned in my last patch, it’s not right to check isEmpty here. It would
be OK to check isNull, or to check isEmpty after calling
stripLeadingAndTrailingHTMLSpaces.

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

The stripLeadingAndTrailingHTMLSpaces call needs to be *before* calling
isEmpty. Isn’t that why I said review- last time? Did you upload the wrong
version of the patch?

> Source/WebCore/html/HTMLVideoElement.h:68
> +    KURL posterImageURL();

Should be const.