Bug 70269 - Use the new cached cue loader
: Use the new cached cue loader
Status: RESOLVED FIXED
: WebKit
Media Elements
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
: 43668
  Show dependency treegraph
 
Reported: 2011-10-17 14:13 PST by
Modified: 2011-10-19 14:50 PST (History)


Attachments
Proposed patch (47.03 KB, patch)
2011-10-17 15:21 PST, Eric Carlson
darin: review+
Review Patch | Details | Formatted Diff | Diff
Updated for review comments and missing HTMLTrackElement/HTMLMediaElement changes (53.55 KB, patch)
2011-10-17 21:13 PST, Eric Carlson
no flags Review Patch | 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 PST, Eric Carlson
koivisto: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-10-17 14:13:30 PST
Update the cue loader and parser to use the cached loader added in http://trac.webkit.org/changeset/97637.
------- Comment #1 From 2011-10-17 14:13:44 PST -------
<rdar://problem/10298656>
------- Comment #2 From 2011-10-17 15:21:22 PST -------
Created an attachment (id=111329) [details]
Proposed patch
------- Comment #3 From 2011-10-17 15:23:35 PST -------
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 From 2011-10-17 15:58:23 PST -------
(In reply to comment #3)
> Attachment 111329 [details] [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 From 2011-10-17 15:58:54 PST -------
(From update of attachment 111329 [details])
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 From 2011-10-17 19:36:08 PST -------
(From update of attachment 111329 [details])
Attachment 111329 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10121061
------- Comment #7 From 2011-10-17 21:13:50 PST -------
Created an attachment (id=111375) [details]
Updated for review comments and missing HTMLTrackElement/HTMLMediaElement changes
------- Comment #8 From 2011-10-17 21:17:40 PST -------
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 From 2011-10-17 21:36:51 PST -------
Created an attachment (id=111377) [details]
Yet another patch. This time to appease my friend the style bot.
------- Comment #10 From 2011-10-18 08:09:07 PST -------
(From update of attachment 111377 [details])
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 From 2011-10-18 10:33:56 PST -------
http://trac.webkit.org/changeset/97773
------- Comment #12 From 2011-10-19 14:50:03 PST -------
*** Bug 62893 has been marked as a duplicate of this bug. ***