[webkit-reviews] review denied: [Bug 119537] The muted attribute in media element is not set before loading resource. : [Attachment 208247] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 7 09:37:55 PDT 2013


Eric Carlson <eric.carlson at apple.com> has denied Sanghyun Park
<sh919.park at samsung.com>'s request for review:
Bug 119537: The muted attribute in media element is not set before loading
resource.
https://bugs.webkit.org/show_bug.cgi?id=119537

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

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


Thank you for fixing this!

The patch is close, but not quite there yet.

> Source/WebCore/ChangeLog:14
> +	   (WebCore::HTMLMediaElement::parseAttribute):
> +	   (WebCore::HTMLMediaElement::loadResource):

Please add comments describing what changed in each method.

> LayoutTests/media/video-defaultmuted-expected.txt:11
>  *** Test before setting src, IDL attribute should default to false

This comment is incorrect, maybe quote the spec text instead - "When a media
element is created, if it has a muted attribute specified, the user agent must
mute the media element's audio output, overriding any user preference."

> LayoutTests/media/video-defaultmuted-expected.txt:68
>  *** Add 'muted' content attribute, it should have no effect on IDL attribute

>  RUN(video.setAttribute('muted', 'muted'))
> -EXPECTED (video.muted == 'false') OK
> +EXPECTED (video.muted == 'true') OK

This no longer tests anything, because video.muted is already true before
setting the content attribute. Set video.muted to false before setting the
content attribute to make sure it is not changed.


More information about the webkit-reviews mailing list