Bug 122848

Summary: Adding Mock class to test RTCPeerConnection
Product: WebKit Reporter: Thiago de Barros Lacerda <thiago.lacerda>
Component: WebCore Misc.Assignee: Thiago de Barros Lacerda <thiago.lacerda>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, glenn, gyuyoung.kim, hta, jer.noble, pnormand, rakuco, sergio, tommyw
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 124288    
Attachments:
Description Flags
Patch
none
Refactored patch
none
Patch
none
Patch for landing none

Description Thiago de Barros Lacerda 2013-10-15 11:41:16 PDT
In order to run the RTCPeerConnection Layout tests, a mock class must be provided, so we don't depend on the user's platform.
This patch adds a mock class RTCPeerConnectionHandler. Then the following tests can be run:

    * RTCPeerConnection-createAnswer.html
    * RTCPeerConnection-createOffer.html
    * RTCPeerConnection-ice.html
    * RTCPeerConnection-localDescription.html
    * RTCPeerConnection-remoteDescription.html
    * RTCPeerConnection-state.html

Tests that require a MediaStream object, by invoking getUserMedia, are not ready to run yet.
Comment 1 Thiago de Barros Lacerda 2013-10-15 11:50:03 PDT
Created attachment 214287 [details]
Patch
Comment 2 Eric Carlson 2013-10-16 22:24:20 PDT
Comment on attachment 214287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214287&action=review

> Source/WebCore/ChangeLog:44
> +        (WebCore::RTCPeerConnectionHandlerMock::create):
> +        (WebCore::RTCPeerConnectionHandlerMock::RTCPeerConnectionHandlerMock):
> +        (WebCore::RTCPeerConnectionHandlerMock::initialize):
> +        (WebCore::RTCPeerConnectionHandlerMock::createOffer):
> +        (WebCore::RTCPeerConnectionHandlerMock::createAnswer):
> +        (WebCore::RTCPeerConnectionHandlerMock::setLocalDescription):
> +        (WebCore::RTCPeerConnectionHandlerMock::setRemoteDescription):
> +        (WebCore::RTCPeerConnectionHandlerMock::localDescription):
> +        (WebCore::RTCPeerConnectionHandlerMock::remoteDescription):
> +        (WebCore::RTCPeerConnectionHandlerMock::updateIce):
> +        (WebCore::RTCPeerConnectionHandlerMock::addIceCandidate):
> +        (WebCore::RTCPeerConnectionHandlerMock::addStream):
> +        (WebCore::RTCPeerConnectionHandlerMock::removeStream):
> +        (WebCore::RTCPeerConnectionHandlerMock::getStats):
> +        (WebCore::RTCPeerConnectionHandlerMock::createDataChannel):
> +        (WebCore::RTCPeerConnectionHandlerMock::createDTMFSender):
> +        (WebCore::RTCPeerConnectionHandlerMock::stop):

Nit: these un-commented method names for the newly added file are unnecessary.

> Source/WebCore/ChangeLog:57
> +        (WebCore::RequestNotifier::~RequestNotifier):
> +        (WebCore::SessionRequestNotifier::SessionRequestNotifier):
> +        (WebCore::SessionRequestNotifier::fire):
> +        (WebCore::VoidRequestNotifier::VoidRequestNotifier):
> +        (WebCore::VoidRequestNotifier::fire):
> +        (WebCore::IceConnectionNotifier::IceConnectionNotifier):
> +        (WebCore::IceConnectionNotifier::fire):
> +        (WebCore::RTCPeerConnectionHandlerMock::~RTCPeerConnectionHandlerMock):
> +        (WebCore::RTCPeerConnectionHandlerMock::removeEventFromVector):
> +        (WebCore::TimerEvent::TimerEvent):
> +        (WebCore::TimerEvent::~TimerEvent):
> +        (WebCore::TimerEvent::timerFired):

Ditto.

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:2
> + *  Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).

Is this correct?

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:48
> +bool RTCPeerConnectionHandlerMock::initialize(PassRefPtr<RTCConfiguration>, PassRefPtr<MediaConstraints> constraints)

"constraints" is unused. The parameter name should be omitted and you should have a FIXME with a bug number about adding support.

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:67
> +void RTCPeerConnectionHandlerMock::createAnswer(PassRefPtr<RTCSessionDescriptionRequest> request, PassRefPtr<MediaConstraints> constraints)

Ditto.

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:120
> +bool RTCPeerConnectionHandlerMock::addIceCandidate(PassRefPtr<RTCVoidRequest> request, PassRefPtr<RTCIceCandidateDescriptor> iceDescriptor)

"iceDescriptor" is unused, you should omit the name.

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:127
> +bool RTCPeerConnectionHandlerMock::addStream(PassRefPtr<MediaStreamDescriptor>, PassRefPtr<MediaConstraints>)

FIXME about adding support for constraints?

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:142
> +void RTCPeerConnectionHandlerMock::getStats(PassRefPtr<RTCStatsRequest>)
> +{
> +}

FIXME?

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:152
> +PassOwnPtr<RTCDTMFSenderHandler> RTCPeerConnectionHandlerMock::createDTMFSender(PassRefPtr<MediaStreamSource>)
> +{
> +    return 0;
> +}

Ditto.

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:2
> + *  Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).

Is this correct?

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:106
> +class RequestNotifier : public RefCounted<RequestNotifier> {
> +public:
> +    virtual ~RequestNotifier() { }
> +    virtual void fire() = 0;
> +};
> +
> +class SessionRequestNotifier : public RequestNotifier {
> +public:
> +    SessionRequestNotifier(PassRefPtr<RTCSessionDescriptionRequest> request, PassRefPtr<RTCSessionDescriptionDescriptor> descriptor)
> +        : m_request(request)
> +        , m_descriptor(descriptor)
> +    { }
> +
> +    void fire()
> +    {
> +        if (m_descriptor)
> +            m_request->requestSucceeded(m_descriptor);
> +        else
> +            m_request->requestFailed("TEST_ERROR");
> +    }
> +
> +private:
> +    RefPtr<RTCSessionDescriptionRequest> m_request;
> +    RefPtr<RTCSessionDescriptionDescriptor> m_descriptor;
> +};
> +
> +class VoidRequestNotifier : public RequestNotifier {
> +public:
> +    VoidRequestNotifier(PassRefPtr<RTCVoidRequest> request, bool success)
> +        : m_request(request)
> +        , m_success(success)
> +    { }
> +
> +    void fire()
> +    {
> +        if (m_success)
> +            m_request->requestSucceeded();
> +        else
> +            m_request->requestFailed("TEST_ERROR");
> +    }
> +
> +private:
> +    RefPtr<RTCVoidRequest> m_request;
> +    bool m_success;
> +};
> +
> +class IceConnectionNotifier : public RequestNotifier {
> +public:
> +    IceConnectionNotifier(RTCPeerConnectionHandlerClient* client, RTCPeerConnectionHandlerClient::IceConnectionState connectionState, RTCPeerConnectionHandlerClient::IceGatheringState gatheringState)
> +        : m_client(client)
> +        , m_connectionState(connectionState)
> +        , m_gatheringState(gatheringState)
> +    { }
> +
> +    void fire()
> +    {
> +        m_client->didChangeIceGatheringState(m_gatheringState);
> +        m_client->didChangeIceConnectionState(m_connectionState);
> +    }
> +
> +private:
> +    RTCPeerConnectionHandlerClient* m_client;
> +    RTCPeerConnectionHandlerClient::IceConnectionState m_connectionState;
> +    RTCPeerConnectionHandlerClient::IceGatheringState m_gatheringState;
> +};

Do these need to be in the header file?

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:168
> +struct TimerEvent : public RefCounted<TimerEvent> {
> +    TimerEvent(RTCPeerConnectionHandlerMock* mock, PassRefPtr<RequestNotifier> notifier)
> +        : m_mock(mock)
> +        , m_timer(this, &TimerEvent::timerFired)
> +        , m_notifier(notifier)
> +    {
> +        m_timer.startOneShot(0.5);
> +    }
> +
> +    virtual ~TimerEvent() { }
> +
> +    void timerFired(Timer<TimerEvent>*)
> +    {
> +        m_notifier->fire();
> +        m_mock->removeEventFromVector(this);
> +    }
> +
> +    RTCPeerConnectionHandlerMock* m_mock;
> +    Timer<TimerEvent> m_timer;
> +    RefPtr<RequestNotifier> m_notifier;
> +};

Ditto.

> Source/WebCore/testing/Internals.cpp:654
> +    RTCPeerConnectionHandler::create = RTCPeerConnectionHandlerMock::create;

Clever!
Comment 3 Thiago de Barros Lacerda 2013-10-17 06:47:12 PDT
(In reply to comment #2)
> (From update of attachment 214287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214287&action=review
> 
> > Source/WebCore/ChangeLog:44
> > +        (WebCore::RTCPeerConnectionHandlerMock::create):
> > +        (WebCore::RTCPeerConnectionHandlerMock::RTCPeerConnectionHandlerMock):
> > +        (WebCore::RTCPeerConnectionHandlerMock::initialize):
> > +        (WebCore::RTCPeerConnectionHandlerMock::createOffer):
> > +        (WebCore::RTCPeerConnectionHandlerMock::createAnswer):
> > +        (WebCore::RTCPeerConnectionHandlerMock::setLocalDescription):
> > +        (WebCore::RTCPeerConnectionHandlerMock::setRemoteDescription):
> > +        (WebCore::RTCPeerConnectionHandlerMock::localDescription):
> > +        (WebCore::RTCPeerConnectionHandlerMock::remoteDescription):
> > +        (WebCore::RTCPeerConnectionHandlerMock::updateIce):
> > +        (WebCore::RTCPeerConnectionHandlerMock::addIceCandidate):
> > +        (WebCore::RTCPeerConnectionHandlerMock::addStream):
> > +        (WebCore::RTCPeerConnectionHandlerMock::removeStream):
> > +        (WebCore::RTCPeerConnectionHandlerMock::getStats):
> > +        (WebCore::RTCPeerConnectionHandlerMock::createDataChannel):
> > +        (WebCore::RTCPeerConnectionHandlerMock::createDTMFSender):
> > +        (WebCore::RTCPeerConnectionHandlerMock::stop):
> 
> Nit: these un-commented method names for the newly added file are unnecessary.

OK, I will remove them

> 
> > Source/WebCore/ChangeLog:57
> > +        (WebCore::RequestNotifier::~RequestNotifier):
> > +        (WebCore::SessionRequestNotifier::SessionRequestNotifier):
> > +        (WebCore::SessionRequestNotifier::fire):
> > +        (WebCore::VoidRequestNotifier::VoidRequestNotifier):
> > +        (WebCore::VoidRequestNotifier::fire):
> > +        (WebCore::IceConnectionNotifier::IceConnectionNotifier):
> > +        (WebCore::IceConnectionNotifier::fire):
> > +        (WebCore::RTCPeerConnectionHandlerMock::~RTCPeerConnectionHandlerMock):
> > +        (WebCore::RTCPeerConnectionHandlerMock::removeEventFromVector):
> > +        (WebCore::TimerEvent::TimerEvent):
> > +        (WebCore::TimerEvent::~TimerEvent):
> > +        (WebCore::TimerEvent::timerFired):
> 
> Ditto.
> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:2
> > + *  Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).
> 
> Is this correct?

Yes :). We are a Nokia Research Center

> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:48
> > +bool RTCPeerConnectionHandlerMock::initialize(PassRefPtr<RTCConfiguration>, PassRefPtr<MediaConstraints> constraints)
> 
> "constraints" is unused. The parameter name should be omitted and you should have a FIXME with a bug number about adding support.

The constraints support depends on the RTCPeerConnectionHandler implementation (he is the one who will take care of that). By now there is no default RTCPeerConnectionHandler implementation (each platform is supposed to implement its own). So I think we don't need a bug to fix that, now.
Additionally, there is no LayouTests testing the RTCPeerConnection + constraints. If, in the future, is added a test like this, then this should be taken into account.
What do you think?
> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:67
> > +void RTCPeerConnectionHandlerMock::createAnswer(PassRefPtr<RTCSessionDescriptionRequest> request, PassRefPtr<MediaConstraints> constraints)
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:120
> > +bool RTCPeerConnectionHandlerMock::addIceCandidate(PassRefPtr<RTCVoidRequest> request, PassRefPtr<RTCIceCandidateDescriptor> iceDescriptor)
> 
> "iceDescriptor" is unused, you should omit the name.

OK

> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:127
> > +bool RTCPeerConnectionHandlerMock::addStream(PassRefPtr<MediaStreamDescriptor>, PassRefPtr<MediaConstraints>)
> 
> FIXME about adding support for constraints?
> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:142
> > +void RTCPeerConnectionHandlerMock::getStats(PassRefPtr<RTCStatsRequest>)
> > +{
> > +}
> 
> FIXME?

This test requires getUserMedia support, which is not possible in the current trunk. I will put a "notImplemented()" and a comment telling that when getUserMedia is ready this method should be fixed. Is that OK for you?

> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:152
> > +PassOwnPtr<RTCDTMFSenderHandler> RTCPeerConnectionHandlerMock::createDTMFSender(PassRefPtr<MediaStreamSource>)
> > +{
> > +    return 0;
> > +}
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:2
> > + *  Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).
> 
> Is this correct?
> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:106
> > +class RequestNotifier : public RefCounted<RequestNotifier> {
> > +public:
> > +    virtual ~RequestNotifier() { }
> > +    virtual void fire() = 0;
> > +};
> > +
> > +class SessionRequestNotifier : public RequestNotifier {
> > +public:
> > +    SessionRequestNotifier(PassRefPtr<RTCSessionDescriptionRequest> request, PassRefPtr<RTCSessionDescriptionDescriptor> descriptor)
> > +        : m_request(request)
> > +        , m_descriptor(descriptor)
> > +    { }
> > +
> > +    void fire()
> > +    {
> > +        if (m_descriptor)
> > +            m_request->requestSucceeded(m_descriptor);
> > +        else
> > +            m_request->requestFailed("TEST_ERROR");
> > +    }
> > +
> > +private:
> > +    RefPtr<RTCSessionDescriptionRequest> m_request;
> > +    RefPtr<RTCSessionDescriptionDescriptor> m_descriptor;
> > +};
> > +
> > +class VoidRequestNotifier : public RequestNotifier {
> > +public:
> > +    VoidRequestNotifier(PassRefPtr<RTCVoidRequest> request, bool success)
> > +        : m_request(request)
> > +        , m_success(success)
> > +    { }
> > +
> > +    void fire()
> > +    {
> > +        if (m_success)
> > +            m_request->requestSucceeded();
> > +        else
> > +            m_request->requestFailed("TEST_ERROR");
> > +    }
> > +
> > +private:
> > +    RefPtr<RTCVoidRequest> m_request;
> > +    bool m_success;
> > +};
> > +
> > +class IceConnectionNotifier : public RequestNotifier {
> > +public:
> > +    IceConnectionNotifier(RTCPeerConnectionHandlerClient* client, RTCPeerConnectionHandlerClient::IceConnectionState connectionState, RTCPeerConnectionHandlerClient::IceGatheringState gatheringState)
> > +        : m_client(client)
> > +        , m_connectionState(connectionState)
> > +        , m_gatheringState(gatheringState)
> > +    { }
> > +
> > +    void fire()
> > +    {
> > +        m_client->didChangeIceGatheringState(m_gatheringState);
> > +        m_client->didChangeIceConnectionState(m_connectionState);
> > +    }
> > +
> > +private:
> > +    RTCPeerConnectionHandlerClient* m_client;
> > +    RTCPeerConnectionHandlerClient::IceConnectionState m_connectionState;
> > +    RTCPeerConnectionHandlerClient::IceGatheringState m_gatheringState;
> > +};
> 
> Do these need to be in the header file?

No, they do not need to be here. I just put them inside this header because they are small classes with tiny implementations. But I can put them in another header (maybe named Notifiers.h), what do you think? Do you think it is better to separate the methods and constructors implementations in a cpp file as well?

> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:168
> > +struct TimerEvent : public RefCounted<TimerEvent> {
> > +    TimerEvent(RTCPeerConnectionHandlerMock* mock, PassRefPtr<RequestNotifier> notifier)
> > +        : m_mock(mock)
> > +        , m_timer(this, &TimerEvent::timerFired)
> > +        , m_notifier(notifier)
> > +    {
> > +        m_timer.startOneShot(0.5);
> > +    }
> > +
> > +    virtual ~TimerEvent() { }
> > +
> > +    void timerFired(Timer<TimerEvent>*)
> > +    {
> > +        m_notifier->fire();
> > +        m_mock->removeEventFromVector(this);
> > +    }
> > +
> > +    RTCPeerConnectionHandlerMock* m_mock;
> > +    Timer<TimerEvent> m_timer;
> > +    RefPtr<RequestNotifier> m_notifier;
> > +};
> 
> Ditto.
> 
> > Source/WebCore/testing/Internals.cpp:654
> > +    RTCPeerConnectionHandler::create = RTCPeerConnectionHandlerMock::create;
> 
> Clever!
Thanks :). hugopl also deserves this compliment
Comment 4 Thiago de Barros Lacerda 2013-10-17 06:52:35 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 214287 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=214287&action=review
> > 
> > > Source/WebCore/ChangeLog:44
> > > +        (WebCore::RTCPeerConnectionHandlerMock::create):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::RTCPeerConnectionHandlerMock):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::initialize):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::createOffer):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::createAnswer):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::setLocalDescription):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::setRemoteDescription):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::localDescription):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::remoteDescription):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::updateIce):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::addIceCandidate):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::addStream):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::removeStream):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::getStats):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::createDataChannel):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::createDTMFSender):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::stop):
> > 
> > Nit: these un-commented method names for the newly added file are unnecessary.
> 
> OK, I will remove them
> 
> > 
> > > Source/WebCore/ChangeLog:57
> > > +        (WebCore::RequestNotifier::~RequestNotifier):
> > > +        (WebCore::SessionRequestNotifier::SessionRequestNotifier):
> > > +        (WebCore::SessionRequestNotifier::fire):
> > > +        (WebCore::VoidRequestNotifier::VoidRequestNotifier):
> > > +        (WebCore::VoidRequestNotifier::fire):
> > > +        (WebCore::IceConnectionNotifier::IceConnectionNotifier):
> > > +        (WebCore::IceConnectionNotifier::fire):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::~RTCPeerConnectionHandlerMock):
> > > +        (WebCore::RTCPeerConnectionHandlerMock::removeEventFromVector):
> > > +        (WebCore::TimerEvent::TimerEvent):
> > > +        (WebCore::TimerEvent::~TimerEvent):
> > > +        (WebCore::TimerEvent::timerFired):
> > 
> > Ditto.
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:2
> > > + *  Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).
> > 
> > Is this correct?
> 
> Yes :). We are a Nokia Research Center
> 
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:48
> > > +bool RTCPeerConnectionHandlerMock::initialize(PassRefPtr<RTCConfiguration>, PassRefPtr<MediaConstraints> constraints)
> > 
> > "constraints" is unused. The parameter name should be omitted and you should have a FIXME with a bug number about adding support.
> 
> The constraints support depends on the RTCPeerConnectionHandler implementation (he is the one who will take care of that). By now there is no default RTCPeerConnectionHandler implementation (each platform is supposed to implement its own). So I think we don't need a bug to fix that, now.
> Additionally, there is no LayouTests testing the RTCPeerConnection + constraints. If, in the future, is added a test like this, then this should be taken into account.
> What do you think?
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:67
> > > +void RTCPeerConnectionHandlerMock::createAnswer(PassRefPtr<RTCSessionDescriptionRequest> request, PassRefPtr<MediaConstraints> constraints)
> > 
> > Ditto.
> 
> Ditto.

Sorry, this one we can do the same approach as in createOffer

> 
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:120
> > > +bool RTCPeerConnectionHandlerMock::addIceCandidate(PassRefPtr<RTCVoidRequest> request, PassRefPtr<RTCIceCandidateDescriptor> iceDescriptor)
> > 
> > "iceDescriptor" is unused, you should omit the name.
> 
> OK
> 
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:127
> > > +bool RTCPeerConnectionHandlerMock::addStream(PassRefPtr<MediaStreamDescriptor>, PassRefPtr<MediaConstraints>)
> > 
> > FIXME about adding support for constraints?
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:142
> > > +void RTCPeerConnectionHandlerMock::getStats(PassRefPtr<RTCStatsRequest>)
> > > +{
> > > +}
> > 
> > FIXME?
> 
> This test requires getUserMedia support, which is not possible in the current trunk. I will put a "notImplemented()" and a comment telling that when getUserMedia is ready this method should be fixed. Is that OK for you?
> 
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:152
> > > +PassOwnPtr<RTCDTMFSenderHandler> RTCPeerConnectionHandlerMock::createDTMFSender(PassRefPtr<MediaStreamSource>)
> > > +{
> > > +    return 0;
> > > +}
> > 
> > Ditto.
> 
> Ditto.

Ditto == Requires RTCDTMFSender support (handler as well)

> 
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:2
> > > + *  Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).
> > 
> > Is this correct?
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:106
> > > +class RequestNotifier : public RefCounted<RequestNotifier> {
> > > +public:
> > > +    virtual ~RequestNotifier() { }
> > > +    virtual void fire() = 0;
> > > +};
> > > +
> > > +class SessionRequestNotifier : public RequestNotifier {
> > > +public:
> > > +    SessionRequestNotifier(PassRefPtr<RTCSessionDescriptionRequest> request, PassRefPtr<RTCSessionDescriptionDescriptor> descriptor)
> > > +        : m_request(request)
> > > +        , m_descriptor(descriptor)
> > > +    { }
> > > +
> > > +    void fire()
> > > +    {
> > > +        if (m_descriptor)
> > > +            m_request->requestSucceeded(m_descriptor);
> > > +        else
> > > +            m_request->requestFailed("TEST_ERROR");
> > > +    }
> > > +
> > > +private:
> > > +    RefPtr<RTCSessionDescriptionRequest> m_request;
> > > +    RefPtr<RTCSessionDescriptionDescriptor> m_descriptor;
> > > +};
> > > +
> > > +class VoidRequestNotifier : public RequestNotifier {
> > > +public:
> > > +    VoidRequestNotifier(PassRefPtr<RTCVoidRequest> request, bool success)
> > > +        : m_request(request)
> > > +        , m_success(success)
> > > +    { }
> > > +
> > > +    void fire()
> > > +    {
> > > +        if (m_success)
> > > +            m_request->requestSucceeded();
> > > +        else
> > > +            m_request->requestFailed("TEST_ERROR");
> > > +    }
> > > +
> > > +private:
> > > +    RefPtr<RTCVoidRequest> m_request;
> > > +    bool m_success;
> > > +};
> > > +
> > > +class IceConnectionNotifier : public RequestNotifier {
> > > +public:
> > > +    IceConnectionNotifier(RTCPeerConnectionHandlerClient* client, RTCPeerConnectionHandlerClient::IceConnectionState connectionState, RTCPeerConnectionHandlerClient::IceGatheringState gatheringState)
> > > +        : m_client(client)
> > > +        , m_connectionState(connectionState)
> > > +        , m_gatheringState(gatheringState)
> > > +    { }
> > > +
> > > +    void fire()
> > > +    {
> > > +        m_client->didChangeIceGatheringState(m_gatheringState);
> > > +        m_client->didChangeIceConnectionState(m_connectionState);
> > > +    }
> > > +
> > > +private:
> > > +    RTCPeerConnectionHandlerClient* m_client;
> > > +    RTCPeerConnectionHandlerClient::IceConnectionState m_connectionState;
> > > +    RTCPeerConnectionHandlerClient::IceGatheringState m_gatheringState;
> > > +};
> > 
> > Do these need to be in the header file?
> 
> No, they do not need to be here. I just put them inside this header because they are small classes with tiny implementations. But I can put them in another header (maybe named Notifiers.h), what do you think? Do you think it is better to separate the methods and constructors implementations in a cpp file as well?
> 
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:168
> > > +struct TimerEvent : public RefCounted<TimerEvent> {
> > > +    TimerEvent(RTCPeerConnectionHandlerMock* mock, PassRefPtr<RequestNotifier> notifier)
> > > +        : m_mock(mock)
> > > +        , m_timer(this, &TimerEvent::timerFired)
> > > +        , m_notifier(notifier)
> > > +    {
> > > +        m_timer.startOneShot(0.5);
> > > +    }
> > > +
> > > +    virtual ~TimerEvent() { }
> > > +
> > > +    void timerFired(Timer<TimerEvent>*)
> > > +    {
> > > +        m_notifier->fire();
> > > +        m_mock->removeEventFromVector(this);
> > > +    }
> > > +
> > > +    RTCPeerConnectionHandlerMock* m_mock;
> > > +    Timer<TimerEvent> m_timer;
> > > +    RefPtr<RequestNotifier> m_notifier;
> > > +};
> > 
> > Ditto.
> > 
> > > Source/WebCore/testing/Internals.cpp:654
> > > +    RTCPeerConnectionHandler::create = RTCPeerConnectionHandlerMock::create;
> > 
> > Clever!
> Thanks :). hugopl also deserves this compliment
Comment 5 Thiago de Barros Lacerda 2013-10-17 10:38:21 PDT
Created attachment 214471 [details]
Refactored patch
Comment 6 Eric Carlson 2013-10-18 21:58:41 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:2
> > > + *  Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).
> > 
> > Is this correct?
> 
> Yes :). We are a Nokia Research Center
> 
  Fair enough :-)

> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:48
> > > +bool RTCPeerConnectionHandlerMock::initialize(PassRefPtr<RTCConfiguration>, PassRefPtr<MediaConstraints> constraints)
> > 
> > "constraints" is unused. The parameter name should be omitted and you should have a FIXME with a bug number about adding support.
> 
> The constraints support depends on the RTCPeerConnectionHandler implementation (he is the one who will take care of that). By now there is no default RTCPeerConnectionHandler implementation (each platform is supposed to implement its own). So I think we don't need a bug to fix that, now.
> Additionally, there is no LayouTests testing the RTCPeerConnection + constraints. If, in the future, is added a test like this, then this should be taken into account.
> What do you think?
> > 

  We need test it at some point, can you please file a bug and add a FIXME to the code?

> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:67

> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:142
> > > +void RTCPeerConnectionHandlerMock::getStats(PassRefPtr<RTCStatsRequest>)
> > > +{
> > > +}
> > 
> > FIXME?
> 
> This test requires getUserMedia support, which is not possible in the current trunk. I will put a "notImplemented()" and a comment telling that when getUserMedia is ready this method should be fixed. Is that OK for you?
> 

  Sure, that sounds fine.


> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:106
> > > +class RequestNotifier : public RefCounted<RequestNotifier> {
> > > +public:
> > > +    virtual ~RequestNotifier() { }
> > > +    virtual void fire() = 0;
> > > +};
> > > +
> > > +class SessionRequestNotifier : public RequestNotifier {
> > > +public:
> > > +    SessionRequestNotifier(PassRefPtr<RTCSessionDescriptionRequest> request, PassRefPtr<RTCSessionDescriptionDescriptor> descriptor)
> > > +        : m_request(request)
> > > +        , m_descriptor(descriptor)
> > > +    { }
> > > +
> > > +    void fire()
> > > +    {
> > > +        if (m_descriptor)
> > > +            m_request->requestSucceeded(m_descriptor);
> > > +        else
> > > +            m_request->requestFailed("TEST_ERROR");
> > > +    }
> > > +
> > > +private:
> > > +    RefPtr<RTCSessionDescriptionRequest> m_request;
> > > +    RefPtr<RTCSessionDescriptionDescriptor> m_descriptor;
> > > +};
> > > +
> > > +class VoidRequestNotifier : public RequestNotifier {
> > > +public:
> > > +    VoidRequestNotifier(PassRefPtr<RTCVoidRequest> request, bool success)
> > > +        : m_request(request)
> > > +        , m_success(success)
> > > +    { }
> > > +
> > > +    void fire()
> > > +    {
> > > +        if (m_success)
> > > +            m_request->requestSucceeded();
> > > +        else
> > > +            m_request->requestFailed("TEST_ERROR");
> > > +    }
> > > +
> > > +private:
> > > +    RefPtr<RTCVoidRequest> m_request;
> > > +    bool m_success;
> > > +};
> > > +
> > > +class IceConnectionNotifier : public RequestNotifier {
> > > +public:
> > > +    IceConnectionNotifier(RTCPeerConnectionHandlerClient* client, RTCPeerConnectionHandlerClient::IceConnectionState connectionState, RTCPeerConnectionHandlerClient::IceGatheringState gatheringState)
> > > +        : m_client(client)
> > > +        , m_connectionState(connectionState)
> > > +        , m_gatheringState(gatheringState)
> > > +    { }
> > > +
> > > +    void fire()
> > > +    {
> > > +        m_client->didChangeIceGatheringState(m_gatheringState);
> > > +        m_client->didChangeIceConnectionState(m_connectionState);
> > > +    }
> > > +
> > > +private:
> > > +    RTCPeerConnectionHandlerClient* m_client;
> > > +    RTCPeerConnectionHandlerClient::IceConnectionState m_connectionState;
> > > +    RTCPeerConnectionHandlerClient::IceGatheringState m_gatheringState;
> > > +};
> > 
> > Do these need to be in the header file?
> 
> No, they do not need to be here. I just put them inside this header because they are small classes with tiny implementations. But I can put them in another header (maybe named Notifiers.h), what do you think? Do you think it is better to separate the methods and constructors implementations in a cpp file as well?
> 

  I see that you split each out into a new file, which is fine but you could also put them into RTCPeerConnectionHandlerMock.cpp because they are completely private to that file.
Comment 7 Eric Carlson 2013-10-18 22:02:32 PDT
Comment on attachment 214471 [details]
Refactored patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214471&action=review

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:49
> +    virtual PassRefPtr<RTCSessionDescriptionDescriptor> localDescription() OVERRIDE;
> +    virtual PassRefPtr<RTCSessionDescriptionDescriptor> remoteDescription() OVERRIDE;

PassRefPtrs are no longer needed for return values because Anders added move-semantics to the RefPtr class. You can return RefPtr here.
Comment 8 Thiago de Barros Lacerda 2013-10-19 08:00:18 PDT
(In reply to comment #6)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > 
> > > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:2
> > > > + *  Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).
> > > 
> > > Is this correct?
> > 
> > Yes :). We are a Nokia Research Center
> > 
>   Fair enough :-)
> 
> > > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:48
> > > > +bool RTCPeerConnectionHandlerMock::initialize(PassRefPtr<RTCConfiguration>, PassRefPtr<MediaConstraints> constraints)
> > > 
> > > "constraints" is unused. The parameter name should be omitted and you should have a FIXME with a bug number about adding support.
> > 
> > The constraints support depends on the RTCPeerConnectionHandler implementation (he is the one who will take care of that). By now there is no default RTCPeerConnectionHandler implementation (each platform is supposed to implement its own). So I think we don't need a bug to fix that, now.
> > Additionally, there is no LayouTests testing the RTCPeerConnection + constraints. If, in the future, is added a test like this, then this should be taken into account.
> > What do you think?
> > > 
> 
>   We need test it at some point, can you please file a bug and add a FIXME to the code?

Sure.

> 
> > > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:67
> 
> > > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:142
> > > > +void RTCPeerConnectionHandlerMock::getStats(PassRefPtr<RTCStatsRequest>)
> > > > +{
> > > > +}
> > > 
> > > FIXME?
> > 
> > This test requires getUserMedia support, which is not possible in the current trunk. I will put a "notImplemented()" and a comment telling that when getUserMedia is ready this method should be fixed. Is that OK for you?
> > 
> 
>   Sure, that sounds fine.
> 
> 
> > > 
> > > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:106
> > > > +class RequestNotifier : public RefCounted<RequestNotifier> {
> > > > +public:
> > > > +    virtual ~RequestNotifier() { }
> > > > +    virtual void fire() = 0;
> > > > +};
> > > > +
> > > > +class SessionRequestNotifier : public RequestNotifier {
> > > > +public:
> > > > +    SessionRequestNotifier(PassRefPtr<RTCSessionDescriptionRequest> request, PassRefPtr<RTCSessionDescriptionDescriptor> descriptor)
> > > > +        : m_request(request)
> > > > +        , m_descriptor(descriptor)
> > > > +    { }
> > > > +
> > > > +    void fire()
> > > > +    {
> > > > +        if (m_descriptor)
> > > > +            m_request->requestSucceeded(m_descriptor);
> > > > +        else
> > > > +            m_request->requestFailed("TEST_ERROR");
> > > > +    }
> > > > +
> > > > +private:
> > > > +    RefPtr<RTCSessionDescriptionRequest> m_request;
> > > > +    RefPtr<RTCSessionDescriptionDescriptor> m_descriptor;
> > > > +};
> > > > +
> > > > +class VoidRequestNotifier : public RequestNotifier {
> > > > +public:
> > > > +    VoidRequestNotifier(PassRefPtr<RTCVoidRequest> request, bool success)
> > > > +        : m_request(request)
> > > > +        , m_success(success)
> > > > +    { }
> > > > +
> > > > +    void fire()
> > > > +    {
> > > > +        if (m_success)
> > > > +            m_request->requestSucceeded();
> > > > +        else
> > > > +            m_request->requestFailed("TEST_ERROR");
> > > > +    }
> > > > +
> > > > +private:
> > > > +    RefPtr<RTCVoidRequest> m_request;
> > > > +    bool m_success;
> > > > +};
> > > > +
> > > > +class IceConnectionNotifier : public RequestNotifier {
> > > > +public:
> > > > +    IceConnectionNotifier(RTCPeerConnectionHandlerClient* client, RTCPeerConnectionHandlerClient::IceConnectionState connectionState, RTCPeerConnectionHandlerClient::IceGatheringState gatheringState)
> > > > +        : m_client(client)
> > > > +        , m_connectionState(connectionState)
> > > > +        , m_gatheringState(gatheringState)
> > > > +    { }
> > > > +
> > > > +    void fire()
> > > > +    {
> > > > +        m_client->didChangeIceGatheringState(m_gatheringState);
> > > > +        m_client->didChangeIceConnectionState(m_connectionState);
> > > > +    }
> > > > +
> > > > +private:
> > > > +    RTCPeerConnectionHandlerClient* m_client;
> > > > +    RTCPeerConnectionHandlerClient::IceConnectionState m_connectionState;
> > > > +    RTCPeerConnectionHandlerClient::IceGatheringState m_gatheringState;
> > > > +};
> > > 
> > > Do these need to be in the header file?
> > 
> > No, they do not need to be here. I just put them inside this header because they are small classes with tiny implementations. But I can put them in another header (maybe named Notifiers.h), what do you think? Do you think it is better to separate the methods and constructors implementations in a cpp file as well?
> > 
> 
>   I see that you split each out into a new file, which is fine but you could also put them into RTCPeerConnectionHandlerMock.cpp because they are completely private to that file.

I would prefer to put them in their own cpp file, because there will be other RTC related mock classes that will need notifiers as well (e.g. RTCDataChannelHandler). Sounds good to you?
Comment 9 Thiago de Barros Lacerda 2013-10-19 08:02:01 PDT
(In reply to comment #7)
> (From update of attachment 214471 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214471&action=review
> 
> > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:49
> > +    virtual PassRefPtr<RTCSessionDescriptionDescriptor> localDescription() OVERRIDE;
> > +    virtual PassRefPtr<RTCSessionDescriptionDescriptor> remoteDescription() OVERRIDE;
> 
> PassRefPtrs are no longer needed for return values because Anders added move-semantics to the RefPtr class. You can return RefPtr here.

Those methods overrides others in RTCPeerConnectionHandler class. So, in order to change their return value to RefPtr I would also have to change RTCPeerConnectionHandler class implementation. I think it is better to do that in the future, in a separated patch. What do you think?
Comment 10 Eric Carlson 2013-10-22 10:45:48 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 214471 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=214471&action=review
> > 
> > > Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:49
> > > +    virtual PassRefPtr<RTCSessionDescriptionDescriptor> localDescription() OVERRIDE;
> > > +    virtual PassRefPtr<RTCSessionDescriptionDescriptor> remoteDescription() OVERRIDE;
> > 
> > PassRefPtrs are no longer needed for return values because Anders added move-semantics to the RefPtr class. You can return RefPtr here.
> 
> Those methods overrides others in RTCPeerConnectionHandler class. So, in order to change their return value to RefPtr I would also have to change RTCPeerConnectionHandler class implementation. I think it is better to do that in the future, in a separated patch. What do you think?

That sounds fine.
Comment 11 Thiago de Barros Lacerda 2013-10-22 11:46:18 PDT
Created attachment 214873 [details]
Patch
Comment 12 Eric Carlson 2013-10-22 12:18:31 PDT
Comment on attachment 214873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214873&action=review

> Source/WebCore/ChangeLog:28
> +        * platform/mock/RTCMockNotifiers.cpp: Added.
> +        * platform/mock/RTCMockNotifiers.h: Added.

Sorry for just noticing this, but "RTCNotifiersMock" would probably be a better name than "RTCMockNotifiers".

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.h:37
> +class RTCPeerConnectionHandlerMock : public RTCPeerConnectionHandler, public TimerEventBasedMock {

This can be FINAL (I should have also noticed this as well).
Comment 13 Thiago de Barros Lacerda 2013-10-22 12:38:27 PDT
Created attachment 214877 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2013-10-22 13:16:56 PDT
Comment on attachment 214877 [details]
Patch for landing

Clearing flags on attachment: 214877

Committed r157808: <http://trac.webkit.org/changeset/157808>
Comment 15 WebKit Commit Bot 2013-10-22 13:16:58 PDT
All reviewed patches have been landed.  Closing bug.