[webkit-reviews] review granted: [Bug 123322] [MSE] Add mock MediaSource classes for testing. : [Attachment 215524] Patch

Fri Nov 1 14:53:18 PDT 2013

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 123322: [MSE] Add mock MediaSource classes for testing.
https://bugs.webkit.org/show_bug.cgi?id=123322

Attachment 215524: Patch
https://bugs.webkit.org/attachment.cgi?id=215524&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215524&action=review

r=me with a few nits.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1213
> +void MediaPlayerFactorySupport::callRegisterMediaEngine(MediaEngineRegister
registerMediaEngine)

"callRegisterMediaEngine" is an odd name for an externally visible method (even
though that is what it does).

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:206
> +#if ENABLE(MEDIA_SOURCE)
> +void MediaPlayerPrivateAVFoundation::load(const String&,
PassRefPtr<HTMLMediaSource>)
> +{
> +    m_networkState = MediaPlayer::FormatError;
> +    m_player->networkStateChanged();
> +}
> +#endif

This was part of bug 123593.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:
133
> +    virtual void load(const String&, PassRefPtr<HTMLMediaSource>);

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:910
> +#if ENABLE(MEDIA_SOURCE)
> +    if (parameters.isMediaSource)
> +	   return MediaPlayer::IsNotSupported;
> +#endif

Ditto.

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.h:98
> +    virtual void load(const String&, PassRefPtr<HTMLMediaSource>);

Ditto.

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:678
> +#if ENABLE(MEDIA_SOURCE)
> +void MediaPlayerPrivateQTKit::load(const String&,
PassRefPtr<HTMLMediaSource>)
> +{
> +    m_networkState = MediaPlayer::FormatError;
> +    m_player->networkStateChanged();
> +}
> +#endif

Ditto.

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1481
> +#if ENABLE(MEDIA_SOURCE)
> +    if (parameters.isMediaSource)
> +	   return MediaPlayer::IsNotSupported;
> +#endif

Ditto.

> Source/WebCore/platform/mock/mediasource/MockBox.cpp:42
> +MockBox::MockBox(ArrayBuffer* data)
> +{
> +    m_type = peekType(data);
> +    m_length = peekLength(data);
> +}

Should you check that data is big enough for a box?

> Source/WebCore/platform/mock/mediasource/MockBox.cpp:63
> +    RefPtr<JSC::DataView> view = JSC::DataView::create(data, 0,
data->byteLength());
> +    m_trackID = view->get<int32_t>(8, true);

Ditto.

> Source/WebCore/platform/mock/mediasource/MockBox.cpp:83
> +    RefPtr<JSC::DataView> view = JSC::DataView::create(data, 0,
data->byteLength());

Ditto.

> Source/WebCore/platform/mock/mediasource/MockBox.cpp:110
> +    RefPtr<JSC::DataView> view = JSC::DataView::create(data, 0,
data->byteLength());

Ditto.

> Source/WebCore/platform/mock/mediasource/MockBox.h:70
> +class MockTrackBox : public MockBox {

Can this be marked FINAL?

> Source/WebCore/platform/mock/mediasource/MockBox.h:97
> +class MockInitializationBox : public MockBox {

Ditto.

> Source/WebCore/platform/mock/mediasource/MockBox.h:121
> +class MockSampleBox : public MockBox {

Ditto.

> Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.h:38
> +class MockMediaPlayerMediaSource : public MediaPlayerPrivateInterface {

Can this be marked FINAL?

> Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.h:72
> +    // MediaPlayerPrivate Overrides
> +    virtual void load(const String& url) OVERRIDE;
> +    virtual void load(const String& url, PassRefPtr<HTMLMediaSource>)
OVERRIDE;
> +    virtual void cancelLoad() OVERRIDE;
> +    virtual void play() OVERRIDE;
> +    virtual void pause() OVERRIDE;
> +    virtual IntSize naturalSize() const OVERRIDE;
> +    virtual bool hasVideo() const OVERRIDE;
> +    virtual bool hasAudio() const OVERRIDE;
> +    virtual void setVisible(bool) OVERRIDE;
> +    virtual bool seeking() const OVERRIDE;
> +    virtual bool paused() const OVERRIDE;
> +    virtual MediaPlayer::NetworkState networkState() const OVERRIDE;
> +    virtual MediaPlayer::ReadyState readyState() const OVERRIDE;
> +    virtual PassRefPtr<TimeRanges> buffered() const OVERRIDE;
> +    virtual bool didLoadingProgress() const OVERRIDE;
> +    virtual void setSize(const IntSize&) OVERRIDE;
> +    virtual void paint(GraphicsContext*, const IntRect&) OVERRIDE;
> +    virtual double currentTimeDouble() const OVERRIDE;
> +    virtual double durationDouble() const OVERRIDE;
> +    virtual void seekDouble(double time) OVERRIDE;
> +
> +    void advanceCurrentTime();
> +    void updateDuration(double);
> +    void setReadyState(MediaPlayer::ReadyState);

Can these be made private?

> Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.h:81
> +    bool m_playing;

Nit: packing.

> Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp:131
> +    RefPtr<MockSourceBufferPrivate> sourceBuffer = prpSourceBuffer;
> +    return sourceBuffer->hasAudio();

Nit: the local isn't necessary.

> Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp:141
> +    RefPtr<MockSourceBufferPrivate> sourceBuffer = prpSourceBuffer;

Ditto.

> Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.h:39
> +class MockMediaSourcePrivate : public MediaSourcePrivate {

Can this be marked FINAL?

> Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.h:53
> +    // MediaSourcePrivate Overrides
> +    virtual AddStatus addSourceBuffer(const ContentType&,
RefPtr<SourceBufferPrivate>&) OVERRIDE;
> +    virtual double duration() OVERRIDE;
> +    virtual void setDuration(double) OVERRIDE;
> +    virtual void markEndOfStream(EndOfStreamStatus) OVERRIDE;
> +    virtual void unmarkEndOfStream() OVERRIDE;
> +    virtual MediaPlayer::ReadyState readyState() const OVERRIDE;
> +    virtual void setReadyState(MediaPlayer::ReadyState) OVERRIDE;
> +
> +    const Vector<MockSourceBufferPrivate*>& activeSourceBuffers() const {
return m_activeSourceBuffers; }

Can these be made private?

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:44
> +class MockMediaSample : public MediaSample {

FINAL?

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:46
> +    static RefPtr<MockMediaSample> create(MockSampleBox box) { return
adoptRef(new MockMediaSample(box)); }

const MockSampleBox&

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:58
> +    MockMediaSample(MockSampleBox box)

Ditto.

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:74
> +class MockMediaDescription : public MediaDescription {

FINAL?

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:76
> +    static RefPtr<MockMediaDescription> create(MockTrackBox box) { return
adoptRef(new MockMediaDescription(box)); }

const MockTrackBox&

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:85
> +    MockMediaDescription(MockTrackBox box) : m_box(box) { }

Ditto.

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:143
> +	   MockTrackBox trackBox = *it;

MockTrackBox&

> Source/WebCore/platform/mock/mediasource/MockTracks.h:40
> +    static RefPtr<MockAudioTrackPrivate> create(MockTrackBox box) { return
adoptRef(new MockAudioTrackPrivate(box)); }

const MockTrackBox&

> Source/WebCore/platform/mock/mediasource/MockTracks.h:46
> +    MockAudioTrackPrivate(MockTrackBox box)

Ditto.

> Source/WebCore/platform/mock/mediasource/MockTracks.h:57
> +    static RefPtr<MockTextTrackPrivate> create(MockTrackBox box) { return
adoptRef(new MockTextTrackPrivate(box)); }

Ditto.

> Source/WebCore/platform/mock/mediasource/MockTracks.h:63
> +    MockTextTrackPrivate(MockTrackBox box)

Ditto.

> Source/WebCore/platform/mock/mediasource/MockTracks.h:76
> +    static RefPtr<MockVideoTrackPrivate> create(MockTrackBox box) { return
adoptRef(new MockVideoTrackPrivate(box)); }

Ditto.

> Source/WebCore/platform/mock/mediasource/MockTracks.h:82
> +    MockVideoTrackPrivate(MockTrackBox box)

Ditto.

> LayoutTests/ChangeLog:71
> +

Four ChangeLog entries?