[webkit-reviews] review denied: [Bug 123825] Add "id" attribute to TextTrack : [Attachment 216070] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Brendan Long
<b.long at cablelabs.com>'s request for review:
Bug 123825: Add "id" attribute to TextTrack
https://bugs.webkit.org/show_bug.cgi?id=123825

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

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


> Source/WebCore/html/track/InbandTextTrack.cpp:62
>  InbandTextTrack::InbandTextTrack(ScriptExecutionContext* context,
TextTrackClient* client, PassRefPtr<InbandTextTrackPrivate> tracksPrivate)
> -    : TextTrack(context, client, emptyString(), tracksPrivate->label(),
tracksPrivate->language(), InBand)
> +    : TextTrack(context, client, emptyString(), tracksPrivate->id(),
tracksPrivate->label(), tracksPrivate->language(), InBand)
>      , m_private(tracksPrivate)

Nit: while you are here, can you please change "tracksPrivate" to
"trackPrivate" since there is only one private track?

> Source/WebCore/html/track/LoadableTextTrack.cpp:40
>  LoadableTextTrack::LoadableTextTrack(HTMLTrackElement* track, const String&
kind, const String& label, const String& language)
> -    : TextTrack(&track->document(), track, kind, label, language,
TrackElement)
> +    : TextTrack(&track->document(), track, kind, emptyString(), label,
language, TrackElement)

Doesn't LoadableTextTrack need to override "id()" so it can return the
HTMLTrackElement id attribute? From the spec:

For tracks that correspond to track elements, the track's identifier is the
value of the element's id attribute, if any

> Source/WebCore/html/track/TrackBase.h:50
> +    AtomicString id() const { return m_id; }

This should be virtual, see above.