[Webkit-unassigned] [Bug 122848] Adding Mock class to test RTCPeerConnection
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 17 06:53:50 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=122848
--- Comment #4 from Thiago de Barros Lacerda <thiago.lacerda at openbossa.org> 2013-10-17 06:52:35 PST ---
(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
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list