<rdar://problem/10419196>

Created attachment 115292 [details] Proposed patch

Comment on attachment 115292 [details] Proposed patch Attachment 115292 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10494001

Created attachment 115311 [details] Patch

Comment on attachment 115311 [details] Patch Nice patch, thanks Eric!

Comment on attachment 115292 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=115292&action=review > Source/WebCore/html/HTMLMediaElement.cpp:2099 > -TextTrackList* HTMLMediaElement::textTracks() const > +TextTrackList* HTMLMediaElement::textTracks() Left a trailing space on there. > Source/WebCore/html/HTMLTrackElement.cpp:106 > + } else if (attrName == kindAttr) > + track()->setKind(attr->value()); > + else if (attrName == labelAttr) > + track()->setLabel(attr->value()); > + else if (attrName == srclangAttr) > + track()->setLanguage(attr->value()); I’m not sure why this is in attributeChanged rather than parseMappedAttribute. > Source/WebCore/html/HTMLTrackElement.cpp:109 > KURL HTMLTrackElement::src() const I don’t understand why this doesn’t just use [Reflect]. Why do we need a custom function for this? And for any of these other attributes. > Source/WebCore/html/HTMLTrackElement.cpp:122 > String HTMLTrackElement::kind() const > { > - return getAttribute(kindAttr); > + return track()->kind(); > } I don’t understand this change. How can these two get out of sync? > Source/WebCore/html/HTMLTrackElement.cpp:166 > + String kind = getAttribute(kindAttr); Should be fastGetAttribute. > Source/WebCore/html/HTMLTrackElement.h:-51 > KURL src() const; > + void setSrc(const String&); > + > String kind() const; > + void setKind(const String&); > + > String srclang() const; > + void setSrclang(const String&); > + > String label() const; > + void setLabel(const String&); > > bool isDefault() const; > - void setKind(const String&); > - void setSrc(const String&); > - void setSrclang(const String&); > - void setLabel(const String&); This seems like an unneeded change. > Source/WebCore/html/TextTrack.h:70 > + static const AtomicString& subtitlesKeyword(); > + static const AtomicString& captionsKeyword(); > + static const AtomicString& descriptionsKeyword(); > + static const AtomicString& chaptersKeyword(); > + static const AtomicString& metadataKeyword(); Do these need to be member functions?

(In reply to comment #7) > (From update of attachment 115292 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115292&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:2099 > > -TextTrackList* HTMLMediaElement::textTracks() const > > +TextTrackList* HTMLMediaElement::textTracks() > > Left a trailing space on there. > I will fix that. > > Source/WebCore/html/HTMLTrackElement.cpp:106 > > + } else if (attrName == kindAttr) > > + track()->setKind(attr->value()); > > + else if (attrName == labelAttr) > > + track()->setLabel(attr->value()); > > + else if (attrName == srclangAttr) > > + track()->setLanguage(attr->value()); > > I’m not sure why this is in attributeChanged rather than parseMappedAttribute. > No good reason, I will fix this. > > Source/WebCore/html/HTMLTrackElement.cpp:109 > > KURL HTMLTrackElement::src() const > > I don’t understand why this doesn’t just use [Reflect]. Why do we need a custom function for this? And for any of these other attributes. > Good point. > > Source/WebCore/html/HTMLTrackElement.cpp:122 > > String HTMLTrackElement::kind() const > > { > > - return getAttribute(kindAttr); > > + return track()->kind(); > > } > > I don’t understand this change. How can these two get out of sync? > The kind IDL attribute must "reflect the content attribute of the same name, limited to only known values". I decided to put the knowledge about what kind values are allowed in TextTrack because it also applies to tracks created with HTMLMediaElement::addTrack(). I will add a comment to this function. > > Source/WebCore/html/HTMLTrackElement.cpp:166 > > + String kind = getAttribute(kindAttr); > > Should be fastGetAttribute. > Good point, I will fix this. > > Source/WebCore/html/HTMLTrackElement.h:-51 > > KURL src() const; > > + void setSrc(const String&); > > + > > String kind() const; > > + void setKind(const String&); > > + > > String srclang() const; > > + void setSrclang(const String&); > > + > > String label() const; > > + void setLabel(const String&); > > > > bool isDefault() const; > > - void setKind(const String&); > > - void setSrc(const String&); > > - void setSrclang(const String&); > > - void setLabel(const String&); > > This seems like an unneeded change. > It was just drive-by cleanup. > > Source/WebCore/html/TextTrack.h:70 > > + static const AtomicString& subtitlesKeyword(); > > + static const AtomicString& captionsKeyword(); > > + static const AtomicString& descriptionsKeyword(); > > + static const AtomicString& chaptersKeyword(); > > + static const AtomicString& metadataKeyword(); > > Do these need to be member functions? > Not necessarily. They are also used by HTMLMediaElement and HTMLTrackElement so I thought it would be helpful to give them class scope. Thanks for the feedback!