[webkit-reviews] review denied: [Bug 103663] [Mac] Add support for in-band text tracks : [Attachment 177065] One more update
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Dec 1 18:24:12 PST 2012
Sam Weinig <sam at webkit.org> has denied Eric Carlson <eric.carlson at apple.com>'s
request for review:
Bug 103663: [Mac] Add support for in-band text tracks
https://bugs.webkit.org/show_bug.cgi?id=103663
Attachment 177065: One more update
https://bugs.webkit.org/attachment.cgi?id=177065&action=review
------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=177065&action=review
> Source/WebCore/html/HTMLMediaElement.cpp:2756
> + RefPtr<TextTrack> textTrack =
InbandTextTrack::create(ActiveDOMObject::scriptExecutionContext(), this,
privateTrack);
privateTrack should probably have a .get() here, it is kind of weird to pass a
RefPtr<> to a function taking a PassRefPtr (I wonder why we allow that).
> Source/WebCore/html/HTMLMediaElement.h:673
> + Vector<RefPtr<InbandTextTrackPrivate> > m_privateTextTracks;
Seems like this should be a Vector of RefPtr<InbandTextTrack>s not
RefPtr<InbandTextTrackPrivate>s.
>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:608
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
You should switch these to a more descriptive macro, eg.
HAVE(AV_FOUNDATION_INBAND_TRACK_SUPPORT). You can probably make it more
concise.
>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:861
> + InbandTextTrackPrivateAVF* trackToEnable = 0;
> +
> + // AVFoundation can only emit cues for one track at a time, so enable
the first track that is showing, or the first that
> + // is hidden if none are showing, otherwise disable all tracks.
> + for (unsigned i = 0; i < m_textTracks.size(); ++i) {
> + InbandTextTrackPrivateAVF* track =
static_cast<InbandTextTrackPrivateAVF*>(m_textTracks[i].get());
> + if (track->mode() == InbandTextTrackPrivate::showing) {
We usually like to keep RefCounted objects in RefPtr<>s on the stack.
>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:892
> + String attributedStringValue =
String(CFAttributedStringGetString(attributedString));
I don't think you need to call the String constructor explicitly here.
>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:1011
> + content.append(tagStart);
> +
content.append(attributedStringValue.substring(effectiveRange.location,
effectiveRange.length));
> + content.append(tagEnd);
The fact that we are using strings to build this markup worries me, it seems
ripe for an injection attack. Can we build up the DOM programmatically? I
realize that doing it in this class would be a clear layering violation, as
this is in platform/ but perhaps we can have our own intermediate
representation that and the HTMLMediaElement layer can construct the DOM from
that.
>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:1066
> + m_currentCueId = emptyString();
Why empty string rather than null string, which is what the constructor would
set it to?
>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:
306
> +class InbandTextTrackPrivateAVF : public InbandTextTrackPrivate {
> +public:
Ideally this class would have its own header/implementation files.
>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:
315
> + void processCue(CFArrayRef, Float64);
Can this use double rather than Float64?
>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:
316
> + void processCueAttributes(CFAttributedStringRef, StringBuilder& content,
StringBuilder& settings);
Does anyone outside of this class call this function? Can it be private?
>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.h:199
> +class InbandTextTrackPrivateAVFObjC : public InbandTextTrackPrivateAVF {
> +public:
Ideally this class would have tis own header/implementation file.
>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:881
> + m_legibleOutput.adoptNS([[AVPlayerItemLegibleOutput alloc]
initWithMediaSubtypesForNativeRepresentation:nil]);
The preferred idiom is now m_legibleOutput = adoptNS([...]);
>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1199
> + for (AVMediaSelectionOption *option in [legibleGroup options]) {
> +
> + if ([[option mediaType] isEqualToString:AVMediaTypeSubtitle]) {
Extra newline.
>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1268
> + return emptyString();
If you want to return the empty string here the emptyAtom is preferred as it is
already an AtomicString.
>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1272
> + if ([titles count])
> + {
{ should go on the previous line.
>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1281
> + return emptyString();
If you want to return the empty string here the emptyAtom is preferred as it is
already an AtomicString.
>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1287
> + if (!m_mediaSelectionOption)
> + return emptyString();
If you want to return the empty string here the emptyAtom is preferred as it is
already an AtomicString.
More information about the webkit-reviews
mailing list