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

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted 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 189562: Patch
https://bugs.webkit.org/attachment.cgi?id=189562&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=189562&action=review


> Source/WebCore/ChangeLog:22
> +	   (WebCore::HTMLVideoElement::attach):
> +	   * page/Settings.in:
> +	   * testing/InternalSettings.cpp:
> +	   (WebCore::InternalSettings::Backup::Backup):
> +	   (WebCore::InternalSettings::Backup::restoreTo):
> +	   (WebCore::InternalSettings::setDefaultVideoPosterURL):
> +	   (WebCore):
> +	   * testing/InternalSettings.h:
> +	   (Backup):
> +	   (InternalSettings):
> +	   * testing/InternalSettings.idl:

Sorry I didn't notice this before, but it is helpful to have per-method
comments about what changed here.

> LayoutTests/media/video-default-poster.html:11
> +    var default_poster = "about:blank";

Why use an invalid poster?

> LayoutTests/media/video-default-poster.html:17
> +	 if (poster) {
> +	   if (poster.value)
> +	     document.getElementById("result-no-poster").innerText = "PASS";

Shouldn't this test the poster value before declaring success?

> ChangeLog:8
> +	   * Source/autotools/symbols.filter:

Ditto.