[webkit-reviews] review granted: [Bug 53556] Adding Feature Defines for <track> : [Attachment 88361] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Anna Cavender
<annacc at chromium.org>'s request for review:
Bug 53556: Adding Feature Defines for <track>
https://bugs.webkit.org/show_bug.cgi?id=53556

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
r+ with the changes suggested below.

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

> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

Shouldn't leave an OOPS in the ChangeLog. It would be helpful to say why no new
tests were added with this patch.


> Source/WebCore/html/HTMLTrackElement.cpp:66
> +	   // TODO(annacc):
> +	   //
static_cast<HTMLMediaElement*>(parentNode())->trackWasAdded(this);

This isn't right, maybe you mean something like "trackWillBeRemoved(this)"?


> Source/WebCore/html/HTMLTrackElement.cpp:114
> +bool HTMLTrackElement::isDefault() const
> +{
> +    return equalIgnoringCase(getAttribute(defaultAttr), "true");
> +}

"default" is a boolean attribute, so I think you should use "return
hasAttribute(defaultAttr)"


> Source/WebCore/html/HTMLTrackElement.cpp:119
> +void HTMLTrackElement::setIsDefault(bool isDefault)
> +{
> +    setAttribute(defaultAttr, isDefault ? "true" : "false");
> +}

And here you can use "setBooleanAttribute(defaultAttr, isDefault);"