[webkit-reviews] review granted: [Bug 70269] Use the new cached cue loader : [Attachment 111329] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 70269: Use the new cached cue loader
https://bugs.webkit.org/show_bug.cgi?id=70269

Attachment 111329: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=111329&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=111329&action=review


> Source/WebCore/html/LoadableTextTrack.cpp:61
> +    ASSERT_UNUSED(loader, m_cueLoader.get() == loader);

Should not need the get() here.

> Source/WebCore/html/LoadableTextTrack.cpp:68
> +    ASSERT_UNUSED(loader, m_cueLoader.get() == loader);

Nor here.

> Source/WebCore/html/LoadableTextTrack.h:46
> +    virtual void textTrackLoadingCompleted(LoadableTextTrack*, bool /*
loadingFailed */) { }

Should this be pure virtual instead of having an empty default implementation?

> Source/WebCore/html/LoadableTextTrack.h:49
> +class LoadableTextTrack : public TextTrack, public CueLoaderClient {

Does the CueLoaderClient inheritance need to be public? I’d like to see it be
private.

> Source/WebCore/html/LoadableTextTrack.h:64
> +    // CueLoaderClient
> +    virtual bool shouldLoadCues(CueLoader*) { return true; }
> +    virtual void newCuesAvailable(CueLoader*);
> +    virtual void cueLoadingStarted(CueLoader*);
> +    virtual void cueLoadingCompleted(CueLoader*, bool loadingFailed);

These should be private.

> Source/WebCore/html/TextTrack.h:79
> +    enum TextTrackType { BaseTextTrack, MutableTextTrack, LoadableTextTrack
};

If this is a member of the TextTrack class it could just be named Type instead
of TextTrackType.

> Source/WebCore/html/track/WebVTTParser.cpp:47
> +static const int secondsPerHour = 3600;
> +static const int secondsPerMinute = 60;
> +static const double malformedTime = -1;

Alexey told me we don’t need the static keyword to get internal linkage for
these, at least the int ones.

> Source/WebCore/html/track/WebVTTParser.cpp:49
> +static unsigned bomLength = 3;
> +static unsigned fileIdentiferLength = 6;

I think you forgot the const here.

> Source/WebCore/html/track/WebVTTParser.cpp:76
> +    if (line.length() > fileIdentiferLength && (line.substring(0,
fileIdentiferLength) != "WEBVTT"
> +				 || (line[fileIdentiferLength] != ' ' &&
line[fileIdentiferLength] != '\t')))
>	   return false;

I think this would read better if it was written as a short inline helper
function instead of a single long line.

> Source/WebCore/loader/CueLoader.cpp:73
> +    ASSERT(m_cachedCueData.get() == resource);

Should not need get here.

> Source/WebCore/loader/CueLoader.cpp:126
> +    ASSERT(m_cachedCueData.get() == resource);

Should not need get here.

> Source/WebCore/loader/CueLoader.cpp:136
> +    ASSERT(m_cachedCueData.get() == resource);

Should not need get here.

> Source/WebCore/loader/CueLoader.h:54
> +class CueLoader : public CachedResourceClient, public WebVTTParserClient {

Does these client interfaces have to be public? Can we inherit from one or both
of these base classes with private inheritance instead of public?

> Source/WebCore/loader/CueLoader.h:72
> +    // CachedResourceClient
> +    virtual void notifyFinished(CachedResource*);
> +    virtual void didReceiveData(CachedResource*);
> +    
> +    // WebVTTParserClient
> +    virtual void newCuesParsed();

Can these functions be private?