[webkit-reviews] review granted: [Bug 60379] Adding initial interfaces and stubs for track : [Attachment 96650] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Anna Cavender
<annacc at chromium.org>'s request for review:
Bug 60379: Adding initial interfaces and stubs for track
https://bugs.webkit.org/show_bug.cgi?id=60379

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

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

r=me with the minor cleanup suggested.

> Source/WebCore/ChangeLog:28
> +	   (WebCore::CueSet::difference):
> +	   (WebCore::CueSet::unionSet):
> +	   (WebCore::CueSet::add):
> +	   (WebCore::CueSet::contains):
> +	   (WebCore::CueSet::remove):
> +	   (WebCore::CueSet::isEmpty):
> +	   (WebCore::CueSet::size):

There is no need to include the names of all of the new methods, just listing
the files is enough (as long as there are no comments).

> Source/WebCore/html/CueIndex.cpp:40
> +    // FIXME(annacc/vrk): Implement.

All FIXMEs should have bug numbers rather than irc nicks, here and throughout
the rest of the patch.


> Source/WebCore/html/CueIndex.cpp:54
> +void CueSet::add(const TextTrackCue& cue)
> +{
> +    // FIXME(annacc/vrk): Implement.
> +    UNUSED_PARAM(cue);
> +}

Because "cue" can never be used, you can leave out the parameter name instead
of using UNUSED_PARAM here and throughout the rest of the patch.


> Source/WebCore/html/CueIndex.h:44
> +    CueSet unionSet(const CueSet& other) const;

You don't have "Set" or "Cue" in the other method names, so this might be
better as just "union"?


> Source/WebCore/html/CueIndex.h:61
> +    // Returns set of cues visible at a time in seconds.
> +    CueSet getVisibleCues(double time) const;

I don't think "get" is necessary, maybe "visibleCuesAtTime()"? This will also
allow you to leave out the parameter name here.


> Source/WebCore/html/MutableTextTrackImpl.cpp:65
> +String MutableTextTrackImpl::kind() const
> +{
> +    return m_kind;
> +}

MutableTextTrackImpl and LoadableTextTrackImpl have almost exactly the same
right now. Is it worth factoring the common code into a shared base class?


> Source/WebCore/html/TextTrack.cpp:50
> +    virtual String kind() const { return ""; }
> +    virtual String label() const { return ""; }
> +    virtual String language() const { return ""; }

You can use emptyString() instead of "".


> Source/WebCore/html/TextTrack.cpp:101
> +void TextTrack::setMode(unsigned short mode)
> +{
> +    m_private->setMode(static_cast<Mode>(mode));
> +}

It doesn't look like the spec says what to do if the mode is set to an invalid
value, but we should at least clamp to SHOWING, and it may be worth logging an
error message as well.

> Source/WebCore/html/TextTrackCue.cpp:85
> +String TextTrackCue::getCueAsSource()
> +{
> +    // FIXME(annacc/vrk): Implement.
> +    return "";
> +}

Use emptyString() instead of "".


> Source/WebCore/html/TextTrackCue.h:74
> +    bool m_pauseOnExit;
> +
> +    bool m_isActive;
> +    TextTrack* m_track;

Putting all of the bools at the end of the class can allow the compiler to make
the structure slightly smaller.


> Source/WebCore/loader/CueLoader.h:55
> +    // Transfers ownership of currently loaded cues into outCues.
> +    virtual void fetchNewestCues(Vector<TextTrackCue*>& cues) = 0;

Your comment now has the wrong parameter name.