Bug 62881 - Loading out-of-band text track files from <track>
Summary: Loading out-of-band text track files from <track>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anna Cavender
URL:
Keywords:
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2011-06-17 10:53 PDT by Anna Cavender
Modified: 2011-06-24 16:06 PDT (History)
8 users (show)

See Also:


Attachments
Patch (17.78 KB, patch)
2011-06-20 21:40 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Patch (17.48 KB, patch)
2011-06-21 09:58 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Patch (20.92 KB, patch)
2011-06-21 16:45 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Adding files to GNUmakefile.list.am (20.89 KB, patch)
2011-06-21 22:22 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Adding tabs to GNUmakefile.list.am (20.37 KB, patch)
2011-06-21 22:29 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Adding "Source/WebCore/platform/track \" to GNUmakefile.am (20.95 KB, patch)
2011-06-21 22:46 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Removing TextTrackPrivateInterface, LoadableTextTrackImpl, and MutableTextTrackImpl. (47.87 KB, patch)
2011-06-24 13:39 PDT, Anna Cavender
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Cavender 2011-06-17 10:53:16 PDT
This change will load a file from the src (or <source>) of a <track>.
Comment 1 Anna Cavender 2011-06-20 21:40:10 PDT
Created attachment 97928 [details]
Patch
Comment 2 WebKit Review Bot 2011-06-20 21:42:07 PDT
Attachment 97928 [details] did not pass style-queue:

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

Source/WebCore/html/HTMLTrackElement.cpp:141:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Collabora GTK+ EWS bot 2011-06-20 21:45:56 PDT
Comment on attachment 97928 [details]
Patch

Attachment 97928 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8909848
Comment 4 Sam Weinig 2011-06-20 21:53:27 PDT
Comment on attachment 97928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97928&action=review

> Source/WebCore/html/HTMLTrackElement.cpp:45
> +    , m_track(0)

RefPtr's are initialized to null, so this is not necessary.

> Source/WebCore/html/HTMLTrackElement.cpp:54
> +    document()->unregisterForDocumentActivationCallbacks(this);

You probably need to add calls to this and registerForDocumentActivationCallbacks when moving to a different document.  See HTMLMediaElement::willMoveToNewOwnerDocument and HTMLMediaElement::didMoveToNewOwnerDocument.

> Source/WebCore/html/HTMLTrackElement.cpp:143
> +        Node* node;
> +        for (node = firstChild(); node; node = node->nextSibling()) {

There is no reason to declare the Node* out side of the for-loop syntax.

> Source/WebCore/platform/track/CueParser.h:70
>      void fetchParsedCues(Vector<PassRefPtr<TextTrackCue> >&);

Not new code, but this is a highly unusual thing to do.  This Vector should probably be a Vector<RefPtr<foo> >.
Comment 5 Anna Cavender 2011-06-21 09:57:58 PDT
Comment on attachment 97928 [details]
Patch

Thanks for the fast review response!

View in context: https://bugs.webkit.org/attachment.cgi?id=97928&action=review

>> Source/WebCore/html/HTMLTrackElement.cpp:45
>> +    , m_track(0)
> 
> RefPtr's are initialized to null, so this is not necessary.

Done.

>> Source/WebCore/html/HTMLTrackElement.cpp:54
>> +    document()->unregisterForDocumentActivationCallbacks(this);
> 
> You probably need to add calls to this and registerForDocumentActivationCallbacks when moving to a different document.  See HTMLMediaElement::willMoveToNewOwnerDocument and HTMLMediaElement::didMoveToNewOwnerDocument.

Ah, ok, I misunderstood the meaning of this call.  Will remove.  Plus, I'm not entirely sure that HTMLTrackElement needs to be an ActiveDOMObject, so I've eliminated that altogether and instead pass along the ScriptExecutionContext from HTMLMediaElement during a load.  Please let me know if this is not the right approach.

>> Source/WebCore/html/HTMLTrackElement.cpp:143
>> +        for (node = firstChild(); node; node = node->nextSibling()) {
> 
> There is no reason to declare the Node* out side of the for-loop syntax.

Agreed.  Done.

>> Source/WebCore/platform/track/CueParser.h:70
>>      void fetchParsedCues(Vector<PassRefPtr<TextTrackCue> >&);
> 
> Not new code, but this is a highly unusual thing to do.  This Vector should probably be a Vector<RefPtr<foo> >.

Thanks, I'll change it to RefPtr instead of PassRefPtr, but I need to leave it as a reference (&) because the parameter in an output parameter to be filled by this function.
Comment 6 Anna Cavender 2011-06-21 09:58:25 PDT
Created attachment 98003 [details]
Patch
Comment 7 WebKit Review Bot 2011-06-21 10:00:30 PDT
Attachment 98003 [details] did not pass style-queue:

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

Source/WebCore/html/HTMLTrackElement.cpp:137:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Collabora GTK+ EWS bot 2011-06-21 10:12:53 PDT
Comment on attachment 98003 [details]
Patch

Attachment 98003 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8913977
Comment 9 Anna Cavender 2011-06-21 16:45:45 PDT
Created attachment 98080 [details]
Patch
Comment 10 Martin Robinson 2011-06-21 17:28:20 PDT
Comment on attachment 98080 [details]
Patch

Attachment 98080 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8925094
Comment 11 Anna Cavender 2011-06-21 22:22:04 PDT
Created attachment 98117 [details]
Adding files to GNUmakefile.list.am
Comment 12 Anna Cavender 2011-06-21 22:29:24 PDT
Created attachment 98118 [details]
Adding tabs to GNUmakefile.list.am
Comment 13 Martin Robinson 2011-06-21 22:34:16 PDT
Comment on attachment 98118 [details]
Adding tabs to GNUmakefile.list.am

Attachment 98118 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8923223
Comment 14 Anna Cavender 2011-06-21 22:46:06 PDT
Created attachment 98121 [details]
Adding "Source/WebCore/platform/track \" to GNUmakefile.am
Comment 15 Eric Carlson 2011-06-23 08:46:50 PDT
(In reply to comment #9)
> Created an attachment (id=98121) [details]
> Patch

(In reply to comment #11)
> Created an attachment (id=98121) [details]
> Patch

(In reply to comment #12)
> Created an attachment (id=98121) [details]
> Patch

(In reply to comment #14)
> Created an attachment (id=98121) [details]
> Patch

I find it is often helpful to have a comment about what changed when a patch is updated.
Comment 16 Eric Carlson 2011-06-23 10:52:34 PDT
Comment on attachment 98121 [details]
Adding "Source/WebCore/platform/track \" to GNUmakefile.am

View in context: https://bugs.webkit.org/attachment.cgi?id=98121&action=review

Clearing r? so you can respond to comments.


> Source/WebCore/html/HTMLMediaElement.h:306
> +#if ENABLE(VIDEO_TRACK)
> +    // loading tracks
> +    void loadTextTracks();
> +#endif

Comments should be complete sentences (first word capitalized, followed by a period, etc). However in this case I don't think this comment adds anything because the function name says what it does.

> Source/WebCore/html/HTMLTrackElement.cpp:146
> +    if (hasAttribute(srcAttr))
> +        m_track->load(getNonEmptyURLAttribute(srcAttr), context);
> +    else
> +        for (Node* node = firstChild(); node; node = node->nextSibling()) {
> +            if (node->hasTagName(sourceTag)) {
> +                HTMLSourceElement* source = static_cast<HTMLSourceElement*>(node);
> +                KURL url = source->getNonEmptyURLAttribute(srcAttr);
> +                if (!url.isEmpty() && m_track->supportsType(url))
> +                    m_track->load(url, context);
> +            }
> +        }
> +

While I agree that <track> should support <source>, it is a void element in both specs at the moment so I think you shouldn't land support for it yet.

> Source/WebCore/html/LoadableTextTrack.cpp:41
>  LoadableTextTrack::~LoadableTextTrack()
>  {
> -    // FIXME(62881): Implement.
>  }

Aren't you leaking "m_private" here? Is there any reason not to use an OwnPtr?

As discussed in IRC, please consider just getting rid of LoadableTextTrackImpl by combining it with this class.
Comment 17 Anna Cavender 2011-06-24 13:39:02 PDT
Created attachment 98531 [details]
Removing TextTrackPrivateInterface, LoadableTextTrackImpl, and MutableTextTrackImpl.
Comment 18 Anna Cavender 2011-06-24 13:44:10 PDT
Comment on attachment 98121 [details]
Adding "Source/WebCore/platform/track \" to GNUmakefile.am

View in context: https://bugs.webkit.org/attachment.cgi?id=98121&action=review

>> Source/WebCore/html/HTMLMediaElement.h:306
>> +#endif
> 
> Comments should be complete sentences (first word capitalized, followed by a period, etc). However in this case I don't think this comment adds anything because the function name says what it does.

Done.

>> Source/WebCore/html/HTMLTrackElement.cpp:146
>> +
> 
> While I agree that <track> should support <source>, it is a void element in both specs at the moment so I think you shouldn't land support for it yet.

OK.  Too bad :(  I guess I got a little excited.

>> Source/WebCore/html/LoadableTextTrack.cpp:41
>>  }
> 
> Aren't you leaking "m_private" here? Is there any reason not to use an OwnPtr?
> 
> As discussed in IRC, please consider just getting rid of LoadableTextTrackImpl by combining it with this class.

I thought about it and I agree.  Can't remember why the extra abstraction was there in the first place.  It is gone in the latest patch
Comment 19 Eric Carlson 2011-06-24 14:19:15 PDT
Comment on attachment 98531 [details]
Removing TextTrackPrivateInterface, LoadableTextTrackImpl, and MutableTextTrackImpl.

Nice simplification!
r=me
Comment 20 Hin-Chung Lam 2011-06-24 16:06:16 PDT
Committed r89711: <http://trac.webkit.org/changeset/89711>