[webkit-reviews] review denied: [Bug 103663] [Mac] Add support for in-band text tracks : [Attachment 178017] Rebased patch.

[bugzilla-daemon at webkit.org]

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 178017: Rebased patch.
https://bugs.webkit.org/attachment.cgi?id=178017&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=178017&action=review


> Source/WebCore/html/HTMLMediaElement.cpp:2733
> +    RefPtr<TextTrack> textTrack =
InbandTextTrack::create(ActiveDOMObject::scriptExecutionContext(), this,
prpTrack);

The type of textTract should be RefPtr<InbandTextTrack>, there is no reason to
use a less specific type here I don't think.

> Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:40
> +class InbandTextTrackPrivate : public RefCounted<InbandTextTrackPrivate> {

Not crazy about this name, but don't have something immediately better.

> Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:67
> +    void setTrack(TextTrack* track) { m_track = track; }
> +    TextTrack* textTrack() { return m_track; }

This is a layering violation.  TextTrack is in html/.  All interaction with the
TextTrack should probably be through the client.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:46
>  #if ENABLE(VIDEO_TRACK)
> +#include "InbandTextTrack.h"
>  #include "InbandTextTrackPrivate.h"
>  #endif

This is a layering violation.  platform/graphics cannot reference
InbandTextTrack.h as it is in html/.  You can probably fix this by moving
InbandTextTrackClient to platform, as I think that is the only thing you are
using this #include for.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1123
> +    m_mediaPlayerClient->mediaPlayerDidAddTrack(track.get());

No reason to call .get()

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1131
> +    m_mediaPlayerClient->mediaPlayerDidRemoveTrack(track.get());

No reason to call .get().

>
Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:283

> +}
> +
> +
> +} // namespace WebCore

Extra newlines.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:36
> +#include "InbandTextTrack.h"

Again, this a layering violation.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:
122
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090

Can we use a more specific macro?

>
Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateAVFObj
C.h:39
> +#ifndef __OBJC__
> +typedef struct objc_object *id;
> +#endif

I don't see id being used in this header.

>
Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateAVFObj
C.mm:39
> +#import "TextTrack.h"

Including TextTrack here is a layering violation.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:43
> +#import "TextTrack.h"

Layering violation.