Bug 70269 – Use the new cached cue loader

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Update the cue loader and parser to use the cached loader added in http://trac.webkit.org/changeset/97637.

[Radar WebKit Bug Importer]

<rdar://problem/10298656>

[Eric Carlson]

Created attachment 111329 [details]
Proposed patch

[WebKit Review Bot]

Attachment 111329 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   1bcef7b..c0d4f37  master     -> origin/master
	M	LayoutTests/platform/chromium-gpu-win/fast/canvas/canvas-composite-transformclip-expected.png
	M	LayoutTests/platform/chromium-gpu-mac/fast/canvas/canvas-composite-transformclip-expected.png
	M	LayoutTests/ChangeLog
r97657 = c0d4f372e21680c78a2d8e16c2a90b5968f63fb3 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

(In reply to comment #3)
> Attachment 111329 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2
> 
> Updating OpenSource
> From git://git.webkit.org/WebKit
>    1bcef7b..c0d4f37  master     -> origin/master
>     M    LayoutTests/platform/chromium-gpu-win/fast/canvas/canvas-composite-transformclip-expected.png
>     M    LayoutTests/platform/chromium-gpu-mac/fast/canvas/canvas-composite-transformclip-expected.png
>     M    LayoutTests/ChangeLog
> r97657 = c0d4f372e21680c78a2d8e16c2a90b5968f63fb3 (refs/remotes/trunk)
> First, rewinding head to replay your work on top of it...
> Fast-forwarded master to refs/remotes/trunk.
> Updating chromium port dependencies using gclient...
> Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
> Re-trying 'depot_tools/gclient sync'
> Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
> Re-trying 'depot_tools/gclient sync'
> Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
> Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
> Re-trying 'depot_tools/gclient sync'
> No such file or directory at Tools/Scripts/update-webkit line 104.
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Huh, none of these files are changed by this patch.

[Darin Adler]

Comment on attachment 111329 [details]
Proposed patch

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?

[WebKit Review Bot]

Comment on attachment 111329 [details]
Proposed patch

Attachment 111329 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10121061

[Eric Carlson]

Created attachment 111375 [details]
Updated for review comments and missing HTMLTrackElement/HTMLMediaElement changes

[WebKit Review Bot]

Attachment 111375 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/html/track/WebVTTParser.h:40:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/html/HTMLMediaElement.cpp:880:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/HTMLMediaElement.cpp:885:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/TextTrack.h:50:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/TextTrack.h:51:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/HTMLMediaElement.h:357:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/HTMLMediaElement.h:358:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 7 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 111377 [details]
Yet another patch. This time to appease my friend the style bot.

[Antti Koivisto]

Comment on attachment 111377 [details]
Yet another patch. This time to appease my friend the style bot.

The patch looks fine, the comments are really about the existing code.

Seems like lots of stuff that is currently called "Cue" should really be called "TextTrack". CueLoader -> TextTrackLoader, CachedCues -> CachedTextTrack, etc. Probably better to do those as a separate step. 

I'm confused by two level loader client interface. We have CueLoader which is a CachedResourceClient. That exposed client interface of its own, CueLoaderClient. I wonder if CueLoader and CachedCues (CachedTextTrack!)  could merge and have just one level of clients.

[Eric Carlson]

http://trac.webkit.org/changeset/97773

[Anna Cavender]

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