Bug 70269 - Use the new cached cue loader
: Use the new cached cue loader
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Media Elements
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Eric Carlson
: InRadar
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2011-10-17 14:13 PDT by Eric Carlson
Modified: 2011-10-19 14:50 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (47.03 KB, patch)
2011-10-17 15:21 PDT, Eric Carlson
darin: review+
Details | Formatted Diff | Diff
Updated for review comments and missing HTMLTrackElement/HTMLMediaElement changes (53.55 KB, patch)
2011-10-17 21:13 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Yet another patch. This time to appease my friend the style bot. (53.57 KB, patch)
2011-10-17 21:36 PDT, Eric Carlson
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2011-10-17 14:13:30 PDT
Update the cue loader and parser to use the cached loader added in http://trac.webkit.org/changeset/97637.
Comment 1 Radar WebKit Bug Importer 2011-10-17 14:13:44 PDT
<rdar://problem/10298656>
Comment 2 Eric Carlson 2011-10-17 15:21:22 PDT
Created attachment 111329 [details]
Proposed patch
Comment 3 WebKit Review Bot 2011-10-17 15:23:35 PDT
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.
Comment 4 Eric Carlson 2011-10-17 15:58:23 PDT
(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.
Comment 5 Darin Adler 2011-10-17 15:58:54 PDT
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?
Comment 6 WebKit Review Bot 2011-10-17 19:36:08 PDT
Comment on attachment 111329 [details]
Proposed patch

Attachment 111329 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10121061
Comment 7 Eric Carlson 2011-10-17 21:13:50 PDT
Created attachment 111375 [details]
Updated for review comments and missing HTMLTrackElement/HTMLMediaElement changes
Comment 8 WebKit Review Bot 2011-10-17 21:17:40 PDT
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.
Comment 9 Eric Carlson 2011-10-17 21:36:51 PDT
Created attachment 111377 [details]
Yet another patch. This time to appease my friend the style bot.
Comment 10 Antti Koivisto 2011-10-18 08:09:07 PDT
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.
Comment 11 Eric Carlson 2011-10-18 10:33:56 PDT
http://trac.webkit.org/changeset/97773
Comment 12 Anna Cavender 2011-10-19 14:50:03 PDT
*** Bug 62893 has been marked as a duplicate of this bug. ***