[webkit-reviews] review denied: [Bug 62883] Implement storage and retrieval for text track cues. : [Attachment 103331] addressing comments and adding check for TextTrackCue chrono order

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Anna Cavender <annacc at chromium.org>'s
request for review:
Bug 62883: Implement storage and retrieval for text track cues.
https://bugs.webkit.org/show_bug.cgi?id=62883

Attachment 103331: addressing comments and adding check for TextTrackCue chrono
order
https://bugs.webkit.org/attachment.cgi?id=103331&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
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 <.