Created attachment 97928 [details] Patch

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 on attachment 97928 [details] Patch Attachment 97928 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8909848

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 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.

Created attachment 98003 [details] Patch

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 on attachment 98003 [details] Patch Attachment 98003 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8913977

Created attachment 98080 [details] Patch

Comment on attachment 98080 [details] Patch Attachment 98080 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8925094

Created attachment 98117 [details] Adding files to GNUmakefile.list.am

Created attachment 98118 [details] Adding tabs to GNUmakefile.list.am

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

Created attachment 98121 [details] Adding "Source/WebCore/platform/track \" to GNUmakefile.am

(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 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.

Created attachment 98531 [details] Removing TextTrackPrivateInterface, LoadableTextTrackImpl, and MutableTextTrackImpl.

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 on attachment 98531 [details] Removing TextTrackPrivateInterface, LoadableTextTrackImpl, and MutableTextTrackImpl. Nice simplification! r=me

Committed r89711: <http://trac.webkit.org/changeset/89711>