[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