Bug 70451

Created attachment 111700 [details] Patch

Comment on attachment 111700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111700&action=review > Source/WebCore/ChangeLog:9 > + Covered by existing tests: > + Any test in media/track, especially track-text-track-cue-list.html How do the tests change? Is it a progression? It makes reviewing much easier if the tests are included in the patch. > Source/WebCore/html/TextTrackCueList.cpp:49 > + for (Vector<RefPtr<TextTrackCue> >::const_iterator it = m_list.begin(); it != m_list.end(); it++) { > + TextTrackCue* cue = it->get(); > + if (cue->id() == id) > + return cue; > + } We generally use index style iteration for Vectors instead of iterator style. > Source/WebCore/html/TextTrackCueList.cpp:63 > + for (Vector<RefPtr<TextTrackCue> >::const_iterator it = newCues.begin(); it != newCues.end(); it++) > + add(*it); Again, index style indexing preferred. > Source/WebCore/html/TextTrackCueList.cpp:69 > + RefPtr<TextTrackCue> newCue = cue; > + add(newCue, 0, m_list.size()); There doesn't seem a reason here to put cue in a RefPtr, just pass it along to the other add() function as is. > Source/WebCore/html/TextTrackCueList.cpp:74 > +void TextTrackCueList::add(PassRefPtr<TextTrackCue> cue, size_t start, size_t end) > +{ > + // Maintain text track cue order: It probably makes sense to sanity check start and end here in an ASSERT. > Source/WebCore/html/TextTrackCueList.cpp:96 > + if (m_list.contains(cue)) { > + cue->setIsActive(false); > + m_list.remove(m_list.find(cue)); > + } This looks like it does two searches of m_list. Instead, you should use find(), test for not found, and if found, use the index to remove it. > Source/WebCore/html/TextTrackCueList.h:60 > + Iterator begin(); > + Iterator end(); These don't seem to be used.

Comment on attachment 111700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111700&action=review Thanks Sam! >> Source/WebCore/ChangeLog:9 >> + Any test in media/track, especially track-text-track-cue-list.html > > How do the tests change? Is it a progression? It makes reviewing much easier if the tests are included in the patch. Sorry for not including tests in with the patch. I believe all the functionality in this patch is currently being tested. media/track/track-text-track-cue-list.html thoroughly tests length(), getCueById(), item() as required for previous patches. media/track/track-mutable.html thoroughly tests adding and removing cues from text tracks which will then use the internal add() and remove() to update their TextTrackCueLists. I'll add that test to the changelog. >> Source/WebCore/html/TextTrackCueList.cpp:49 >> + } > > We generally use index style iteration for Vectors instead of iterator style. Done. >> Source/WebCore/html/TextTrackCueList.cpp:63 >> + add(*it); > > Again, index style indexing preferred. Done. >> Source/WebCore/html/TextTrackCueList.cpp:69 >> + add(newCue, 0, m_list.size()); > > There doesn't seem a reason here to put cue in a RefPtr, just pass it along to the other add() function as is. Done. >> Source/WebCore/html/TextTrackCueList.cpp:74 >> + // Maintain text track cue order: > > It probably makes sense to sanity check start and end here in an ASSERT. Done. >> Source/WebCore/html/TextTrackCueList.cpp:96 >> + } > > This looks like it does two searches of m_list. Instead, you should use find(), test for not found, and if found, use the index to remove it. Done. >> Source/WebCore/html/TextTrackCueList.h:60 >> + Iterator end(); > > These don't seem to be used. Gone.

Created attachment 111807 [details] addressing Sam's comments and removing iterators

Created attachment 112048 [details] updating to ToT I think that the remaining bots weren't able to process the patch because of eric's renaming patch: http://trac.webkit.org/changeset/98000. This patch just updates.

Comment on attachment 112048 [details] updating to ToT Attachment 112048 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10203045

Comment on attachment 112048 [details] updating to ToT Attachment 112048 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10204035

Created attachment 112053 [details] fixing typo

(In reply to comment #3) > (From update of attachment 111700 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111700&action=review > > Thanks Sam! > > >> Source/WebCore/ChangeLog:9 > >> + Any test in media/track, especially track-text-track-cue-list.html > > > > How do the tests change? Is it a progression? It makes reviewing much easier if the tests are included in the patch. > > Sorry for not including tests in with the patch. I believe all the functionality in this patch is currently being tested. media/track/track-text-track-cue-list.html thoroughly tests length(), getCueById(), item() as required for previous patches. media/track/track-mutable.html thoroughly tests adding and removing cues from text tracks which will then use the internal add() and remove() to update their TextTrackCueLists. I'll add that test to the changelog. I would really rather you include the actual changes to the test results in the patch as we require of everyone else.

Sam, I didn't include any changes to tests because existing tests already cover this functionality. This feature is a work in progress and I am incrementally adding functionality in small, easy to digest patches. As a result, layout tests have been submitted with prior patches even though not enough functionality existed at that time to actually run the tests successfully. This patch adds some of that functionality, so that existing tests can soon be un-skipped. I am certainly not opposed to adding new test cases, but please take a look at media/track/track-text-track-cue-list.html and media/track/track-mutable.html and let me know if you think there is functionality in this patch that is not covered by those tests. Thanks!

I understand that you implemented a bunch of stuff out of the main tree and are trying to commit it piecemeal, but part of the breaking up of the large change needs to include the test result changes so we can see the progress.

Thanks Sam, I understand. I'm working on some other patches to put the functionality in place first to be able to run tests against this patch and others. After that I'll come back to this patch and include tests. Stay tuned :).

Created attachment 113504 [details] now with tests A functional TextTrackCueList with passing tests.

Comment on attachment 113504 [details] now with tests View in context: https://bugs.webkit.org/attachment.cgi?id=113504&action=review > Source/WebCore/html/TextTrack.cpp:49 > + m_cues = TextTrackCueList::create(); Can this be done when newCuesAvailable is called for the first time? > Source/WebCore/html/TextTrack.cpp:103 > PassRefPtr<TextTrackCueList> TextTrack::cues() const Returning a PassRefPtr? > Source/WebCore/html/TextTrack.cpp:107 > - // FIXME(62885): Implement. > + if (m_mode != TextTrack::DISABLED) > + return m_cues; > return 0; I am a big fan of quoting the spec text and noting the spec section number to make it easier to find the relevant section in the spec, and to make it easier to figure out when the logic is out of sync with spec changes. > Source/WebCore/html/TextTrackCueList.cpp:50 > +TextTrackCue* TextTrackCueList::getCueById(const String& id) const This would be more efficient if the cue id and parameters were both AtomicString so the == is a pointer comparison. This change could be made as a follow up optimization. > Source/WebCore/html/TextTrackCueList.cpp:86 > + || (newCue->startTime() == m_list[index]->startTime() > + && newCue->endTime() > m_list[index]->endTime())) Having these on the same line will make it easier to follow the logic. > Source/WebCore/html/TextTrackCueList.cpp:89 > + add(newCue, start, index); > + else > + add(newCue, index + 1, end); newCue.release() will prevent reference count churn. > Source/WebCore/html/TextTrackCueList.h:50 > PassRefPtr<TextTrackCueList> activeCues(); PassRefPtr? > LayoutTests/media/track/track-text-track-cue-list.html:12 > + cues = testTrack.track.cues; Using "testTrack" as a global without looking it up works, but it is an old crusty compat feature so please add a getElementById().

Comment on attachment 113504 [details] now with tests View in context: https://bugs.webkit.org/attachment.cgi?id=113504&action=review Thanks Eric! >> Source/WebCore/html/TextTrack.cpp:49 >> + m_cues = TextTrackCueList::create(); > > Can this be done when newCuesAvailable is called for the first time? Sure. It would also have to be checked and possibly created in addCue, but that's fine. >> Source/WebCore/html/TextTrack.cpp:103 >> PassRefPtr<TextTrackCueList> TextTrack::cues() const > > Returning a PassRefPtr? Huh, good call. Can't remember why it got changed to PassRefPtr, but it should definitely be a raw pointer (ownership is not being transfered). >> Source/WebCore/html/TextTrack.cpp:107 >> return 0; > > I am a big fan of quoting the spec text and noting the spec section number to make it easier to find the relevant section in the spec, and to make it easier to figure out when the logic is out of sync with spec changes. Done. >> Source/WebCore/html/TextTrackCueList.cpp:50 >> +TextTrackCue* TextTrackCueList::getCueById(const String& id) const > > This would be more efficient if the cue id and parameters were both AtomicString so the == is a pointer comparison. This change could be made as a follow up optimization. Good idea. Noted for follow up patch. >> Source/WebCore/html/TextTrackCueList.cpp:86 >> + && newCue->endTime() > m_list[index]->endTime())) > > Having these on the same line will make it easier to follow the logic. Done. >> Source/WebCore/html/TextTrackCueList.cpp:89 >> + add(newCue, index + 1, end); > > newCue.release() will prevent reference count churn. Done. >> Source/WebCore/html/TextTrackCueList.h:50 >> PassRefPtr<TextTrackCueList> activeCues(); > > PassRefPtr? Done. Raw pointer now. Thanks. >> LayoutTests/media/track/track-text-track-cue-list.html:12 >> + cues = testTrack.track.cues; > > Using "testTrack" as a global without looking it up works, but it is an old crusty compat feature so please add a getElementById(). Done.

Created attachment 113543 [details] Patch for landing

Comment on attachment 113543 [details] Patch for landing Clearing flags on attachment: 113543 Committed r99234: <http://trac.webkit.org/changeset/99234>

All reviewed patches have been landed. Closing bug.