Bug 122848

Created attachment 214287 [details] Patch

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!

(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

(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

Created attachment 214471 [details] Refactored patch

(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 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.

(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?

(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?

(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.

Created attachment 214873 [details] Patch

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).

Created attachment 214877 [details] Patch for landing

Comment on attachment 214877 [details] Patch for landing Clearing flags on attachment: 214877 Committed r157808: <http://trac.webkit.org/changeset/157808>

All reviewed patches have been landed. Closing bug.