Bug 62883

Created attachment 101679 [details] storage and retrieval for TextTrackCues

Comment on attachment 101679 [details] storage and retrieval for TextTrackCues View in context: https://bugs.webkit.org/attachment.cgi?id=101679&action=review > Source/WebCore/html/TextTrack.h:81 > + Vector<TextTrackCue*> m_newestCues; What keeps the TextTrackCue* alive in this Vector. Are they owned (refed) by something else?

Comment on attachment 101679 [details] storage and retrieval for TextTrackCues Hi Sam, thanks for taking a look! It occurred to me that you might not have the big-picture context that would make the review easier. I'll send you more info in a separate email. -Anna View in context: https://bugs.webkit.org/attachment.cgi?id=101679&action=review >> Source/WebCore/html/TextTrack.h:81 >> + Vector<TextTrackCue*> m_newestCues; > > What keeps the TextTrackCue* alive in this Vector. Are they owned (refed) by something else? TextTrackCues are ultimately owned by TextTrackCueList (and each TextTrack has a TextTrackCueList). In this case, m_newestCues is just a temporary storage used when notifying CueIndex that new cues have been loaded and need to be accounted for.

Comment on attachment 101679 [details] storage and retrieval for TextTrackCues View in context: https://bugs.webkit.org/attachment.cgi?id=101679&action=review > Source/WebCore/ChangeLog:9 > + No new tests because feature is hidden behind VIDEO_TRACK feature > + define, which is turned off. This isn't accurate because you have tests for some features now. This comment should either point to existing tests that cover this functionality, or you should add new tests with this patch. > Source/WebCore/html/CueIndex.cpp:108 > -void CueIndex::fetchNewCuesFromLoader(CueLoader*) > +void CueIndex::fetchNewCuesFromLoader(CueLoader* loader) > { > - // FIXME(62883): Implement. > + Vector<TextTrackCue*> newCues; > + loader->fetchNewestCues(newCues); > + for (size_t i = 0; i < newCues.size(); ++i) > + add(newCues[i]); > } Is the client *required* to pull the cues from the loader at this point? If not, this name is misleading. Maybe newCuesAvailable? > Source/WebCore/html/LoadableTextTrack.cpp:85 > void LoadableTextTrack::newCuesLoaded() > { > - // FIXME(62885): Tell the client to fetch the latest cues. > + m_client->fetchNewCuesFromLoader(this); > } Is there any reason to have a separate function for this? Why not call the client function directly from newCuesParsed? >>> Source/WebCore/html/TextTrack.h:81 >>> + Vector<TextTrackCue*> m_newestCues; >> >> What keeps the TextTrackCue* alive in this Vector. Are they owned (refed) by something else? > > TextTrackCues are ultimately owned by TextTrackCueList (and each TextTrack has a TextTrackCueList). In this case, m_newestCues is just a temporary storage used when notifying CueIndex that new cues have been loaded and need to be accounted for. Here you have a vector of TextTrackCue raw pointers, but in the parser you have a vector of TextTrackCue ref pointers (from "WebVTTParser.h: Vector<RefPtr<TextTrackCue> > m_cuelist"). Why the difference? You get the cues from the parser via fetchParsedCues which returns a vector of ref ptrs, but LoadableTextTrack::fetchNewestCues returns a vector of raw pointers. Why?

Created attachment 103331 [details] addressing comments and adding check for TextTrackCue chrono order

Comment on attachment 101679 [details] storage and retrieval for TextTrackCues Thanks for the feedback! View in context: https://bugs.webkit.org/attachment.cgi?id=101679&action=review >> Source/WebCore/html/CueIndex.cpp:108 >> } > > Is the client *required* to pull the cues from the loader at this point? If not, this name is misleading. Maybe newCuesAvailable? Done. >> Source/WebCore/html/LoadableTextTrack.cpp:85 >> } > > Is there any reason to have a separate function for this? Why not call the client function directly from newCuesParsed? Done. Good call. >>>> Source/WebCore/html/TextTrack.h:81 >>>> + Vector<TextTrackCue*> m_newestCues; >>> >>> What keeps the TextTrackCue* alive in this Vector. Are they owned (refed) by something else? >> >> TextTrackCues are ultimately owned by TextTrackCueList (and each TextTrack has a TextTrackCueList). In this case, m_newestCues is just a temporary storage used when notifying CueIndex that new cues have been loaded and need to be accounted for. > > Here you have a vector of TextTrackCue raw pointers, but in the parser you have a vector of TextTrackCue ref pointers (from "WebVTTParser.h: Vector<RefPtr<TextTrackCue> > m_cuelist"). Why the difference? You get the cues from the parser via fetchParsedCues which returns a vector of ref ptrs, but LoadableTextTrack::fetchNewestCues returns a vector of raw pointers. Why? Uh, yeah. That was a mistake. Changed to Vector<RefPtr<TextTrackCue> > m_newestCues;

Comment on attachment 103331 [details] addressing comments and adding check for TextTrackCue chrono order View in context: https://bugs.webkit.org/attachment.cgi?id=103331&action=review > Source/WebCore/ChangeLog:26 > + * html/CueIndex.cpp: > + * html/CueIndex.h: > + * html/HTMLMediaElement.cpp: > + * html/HTMLMediaElement.h: > + * html/HTMLTrackElement.cpp: > + * html/HTMLTrackElement.h: > + * html/LoadableTextTrack.cpp: > + * html/LoadableTextTrack.h: > + * html/MutableTextTrack.cpp: > + * html/MutableTextTrack.h: > + * html/TextTrack.h: > + * html/TextTrackCueList.cpp: > + * html/TextTrackCueList.h: > + * loader/CueLoader.h: > + * platform/track/CueParser.cpp: > + * platform/track/CueParser.h: It would be great to have more comments in the change log about the changes you are making. > Source/WebCore/html/CueIndex.h:57 > HashSet<TextTrackCue*> m_set; It would be good to add a comment about the lifetime of the objects in this set as well. > Source/WebCore/html/CueIndex.h:74 > + Vector<TextTrackCue*> m_cuesByStartTime; Either you should be using a smart pointer for these TextTrackCues, or you should have a comment explaining the ownership/lifetime of the objects contained by the Vector. > Source/WebCore/html/HTMLTrackElement.cpp:138 > + if (hasAttribute(srcAttr)) { > + m_track = LoadableTextTrack::create(kind(), label(), srclang(), isDefault()); > + m_track->load(getNonEmptyURLAttribute(srcAttr), context, cueClient); > + } It seems a shame to lookup the attribute twice here (once for hasAttribute, once for getNonEmptyURLAttribute). It might also read better with an early return. > Source/WebCore/html/TextTrackCueList.h:57 > + void add(const PassRefPtr<TextTrackCue>); There is no reason to pass a const PassRefPtr. > Source/WebCore/html/TextTrackCueList.h:59 > + void remove(const PassRefPtr<TextTrackCue>); It seems unlikely that remove would hand over ownership of the TextTrackCue, so this should probably be a raw pointer. > Source/WebCore/html/TextTrackCueList.h:67 > + Vector <RefPtr<TextTrackCue> > m_list; There should not be a space after the Vector and before the <.

Comment on attachment 103331 [details] addressing comments and adding check for TextTrackCue chrono order View in context: https://bugs.webkit.org/attachment.cgi?id=103331&action=review >> Source/WebCore/ChangeLog:26 >> + * platform/track/CueParser.h: > > It would be great to have more comments in the change log about the changes you are making. Done. >> Source/WebCore/html/CueIndex.h:57 > > It would be good to add a comment about the lifetime of the objects in this set as well. Done. Added a comment describing CueSet and the lifetime of TextTrackCue objects. >> Source/WebCore/html/CueIndex.h:74 > > Either you should be using a smart pointer for these TextTrackCues, or you should have a comment explaining the ownership/lifetime of the objects contained by the Vector. Done. Added a comment describing CueIndex and the lifetime responsibilities of TextTrackCues. I hope it is clear. >> Source/WebCore/html/HTMLTrackElement.cpp:138 >> + } > > It seems a shame to lookup the attribute twice here (once for hasAttribute, once for getNonEmptyURLAttribute). It might also read better with an early return. Done. >> Source/WebCore/html/TextTrackCueList.h:57 >> + void add(const PassRefPtr<TextTrackCue>); > > There is no reason to pass a const PassRefPtr. Done. >> Source/WebCore/html/TextTrackCueList.h:59 >> + void remove(const PassRefPtr<TextTrackCue>); > > It seems unlikely that remove would hand over ownership of the TextTrackCue, so this should probably be a raw pointer. Done. Thanks. >> Source/WebCore/html/TextTrackCueList.h:67 >> + Vector <RefPtr<TextTrackCue> > m_list; > > There should not be a space after the Vector and before the <. Done.

Created attachment 103446 [details] addressing Sam's comments

Created attachment 105263 [details] updating to ToT I believe I've addressed all concerns with this patch and it should be ready to go. It just got a bit stale while I was on vacation, sorry about that.

Comment on attachment 105263 [details] updating to ToT View in context: https://bugs.webkit.org/attachment.cgi?id=105263&action=review > Source/WebCore/ChangeLog:11 > + Test: any tests in media/track, but especially > + media/track/track-webvtt-tc012-outoforder.html I don't get how this is being tested by these. Do their results change? > Source/WebCore/ChangeLog:28 > + * html/CueIndex.cpp: > + * html/CueIndex.h: > + * html/HTMLMediaElement.cpp: > + * html/HTMLMediaElement.h: > + * html/HTMLTrackElement.cpp: > + * html/HTMLTrackElement.h: > + * html/LoadableTextTrack.cpp: > + * html/LoadableTextTrack.h: > + * html/MutableTextTrack.cpp: > + * html/MutableTextTrack.h: > + * html/TextTrack.h: > + * html/TextTrackCueList.cpp: > + * html/TextTrackCueList.h: > + * loader/CueLoader.h: > + * platform/track/CueParser.cpp: > + * platform/track/CueParser.h: It would be helpful to have function by function explanation in your changelog. > Source/WebCore/html/TextTrackCueList.cpp:56 > + return m_list[index].get(); This probably needs to handle the case that the index is out of range.

Created attachment 105658 [details] addressing Sam's comments

Thanks Sam for the review! (In reply to comment #11) > (From update of attachment 105263 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105263&action=review > > > Source/WebCore/ChangeLog:11 > > + Test: any tests in media/track, but especially > > + media/track/track-webvtt-tc012-outoforder.html > > I don't get how this is being tested by these. Do their results change? You're right it's not clear since not all the code for the <track> feature is up. All the tests will store and retrieve TextTrackCues to/from the TextTrackCueList, so that part is being tested. The CueIndex is not being tested because the code that will use it in HTMLMediaElement is not submitted yet. I think it would be best to include tests with that patch (coming soon). Sound ok? > > > Source/WebCore/ChangeLog:28 > > + * html/CueIndex.cpp: > > + * html/CueIndex.h: > > + * html/HTMLMediaElement.cpp: > > + * html/HTMLMediaElement.h: > > + * html/HTMLTrackElement.cpp: > > + * html/HTMLTrackElement.h: > > + * html/LoadableTextTrack.cpp: > > + * html/LoadableTextTrack.h: > > + * html/MutableTextTrack.cpp: > > + * html/MutableTextTrack.h: > > + * html/TextTrack.h: > > + * html/TextTrackCueList.cpp: > > + * html/TextTrackCueList.h: > > + * loader/CueLoader.h: > > + * platform/track/CueParser.cpp: > > + * platform/track/CueParser.h: > > It would be helpful to have function by function explanation in your changelog. Done. > > > Source/WebCore/html/TextTrackCueList.cpp:56 > > + return m_list[index].get(); > > This probably needs to handle the case that the index is out of range. Good catch, thanks.

Loading and caching of TextTrackCue has been revamped - thanks eric.carlson! http://trac.webkit.org/changeset/97773 http://trac.webkit.org/changeset/97637 I'm going to split this bug up in to two patches: one for CueIndex and one for TextTrackCueList.

*** Bug 62885 has been marked as a duplicate of this bug. ***