Bug 150165

Created attachment 271769 [details] Proposed patch

Created attachment 271795 [details] Updated patch Fixed debug build issues.

Comment on attachment 271795 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=271795&action=review > Source/WebCore/ChangeLog:11 > + configure the WebRTC backend how to send and receive. It also abstracts Nit: "configure the WebRTC backend how to send and receive" -> something like "configure how the the WebRTC backend sends and receives" > Source/WebCore/platform/mediastream/IceCandidate.h:2 > + * Copyright (C) 2015 Ericsson AB. All rights reserved. Nit: you might want to update this. > Source/WebCore/platform/mediastream/IceCandidate.h:103 > + : m_componentId(0) > + , m_priority(0) > + , m_port(0) > + , m_relatedPort(0) These should be initialized in the class definition: unsigned m_componentId { 0 }; int m_priority { 0 }; etc. > Source/WebCore/platform/mediastream/MediaEndpoint.cpp:2 > + * Copyright (C) 2015 Ericsson AB. All rights reserved. Update? > Source/WebCore/platform/mediastream/MediaEndpoint.cpp:40 > + return nullptr; Should this ASSERT? > Source/WebCore/platform/mediastream/MediaEndpoint.h:2 > + * Copyright (C) 2015 Ericsson AB. All rights reserved. Update? > Source/WebCore/platform/mediastream/MediaEndpointConfiguration.cpp:2 > + * Copyright (C) 2015 Ericsson AB. All rights reserved. Update? > Source/WebCore/platform/mediastream/MediaEndpointConfiguration.h:2 > + * Copyright (C) 2015 Ericsson AB. All rights reserved. Update? > Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:2 > + * Copyright (C) 2015 Ericsson AB. All rights reserved. Update? > Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:72 > + : m_sessionId(cryptographicallyRandomNumber()) // FIXME: should be 64 bits Can you initialize this by bit-shifting two random 32 bit numbers? > Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:73 > + , m_sessionVersion(0) This should be initialized in the class definition. > Source/WebCore/platform/mediastream/MediaPayload.h:2 > + * Copyright (C) 2015 Ericsson AB. All rights reserved. Update? > Source/WebCore/platform/mediastream/MediaPayload.h:102 > + : m_type(0) > + , m_clockRate(0) > + , m_channels(0) > + , m_ccmfir(false) > + , m_nackpli(false) > + , m_nack(false) These should be initialized in the class definition. > Source/WebCore/platform/mediastream/PeerMediaDescription.h:2 > + * Copyright (C) 2015 Ericsson AB. All rights reserved. Update? > Source/WebCore/platform/mediastream/PeerMediaDescription.h:154 > + : m_port(0) > + , m_rtcpMux(false) > + , m_rtcpPort(0) > + , m_source(nullptr) Ditto.

Created attachment 272108 [details] Updated patch for landing

(In reply to comment #3) > Comment on attachment 271795 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271795&action=review > > > Source/WebCore/ChangeLog:11 > > + configure the WebRTC backend how to send and receive. It also abstracts > > Nit: "configure the WebRTC backend how to send and receive" -> something > like "configure how the the WebRTC backend sends and receives" Fixed. > > Source/WebCore/platform/mediastream/IceCandidate.h:2 > > + * Copyright (C) 2015 Ericsson AB. All rights reserved. > > Nit: you might want to update this. This code has been available open source since 2015 and hasn't changed much (<10 lines) since. I can follow up with an update if it's wrong to have it 2015 only. > > Source/WebCore/platform/mediastream/IceCandidate.h:103 > > + : m_componentId(0) > > + , m_priority(0) > > + , m_port(0) > > + , m_relatedPort(0) > > These should be initialized in the class definition: > > unsigned m_componentId { 0 }; > int m_priority { 0 }; > > etc. Fixed. Update style guide lines if this is the new preference? > > > Source/WebCore/platform/mediastream/MediaEndpoint.cpp:2 > > + * Copyright (C) 2015 Ericsson AB. All rights reserved. > > Update? > > > Source/WebCore/platform/mediastream/MediaEndpoint.cpp:40 > > + return nullptr; > > Should this ASSERT? This factory function is used when there are no implementations. The value is asserted in the MediaEndpointPeerConnection implementation. > > > Source/WebCore/platform/mediastream/MediaEndpoint.h:2 > > + * Copyright (C) 2015 Ericsson AB. All rights reserved. > > Update? > > > Source/WebCore/platform/mediastream/MediaEndpointConfiguration.cpp:2 > > + * Copyright (C) 2015 Ericsson AB. All rights reserved. > > Update? > > > Source/WebCore/platform/mediastream/MediaEndpointConfiguration.h:2 > > + * Copyright (C) 2015 Ericsson AB. All rights reserved. > > Update? > > > Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:2 > > + * Copyright (C) 2015 Ericsson AB. All rights reserved. > > Update? > > > Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:72 > > + : m_sessionId(cryptographicallyRandomNumber()) // FIXME: should be 64 bits > > Can you initialize this by bit-shifting two random 32 bit numbers? That should work. Fixed. > > Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:73 > > + , m_sessionVersion(0) > > This should be initialized in the class definition. Fixed. > > Source/WebCore/platform/mediastream/MediaPayload.h:2 > > + * Copyright (C) 2015 Ericsson AB. All rights reserved. > > Update? > > > Source/WebCore/platform/mediastream/MediaPayload.h:102 > > + : m_type(0) > > + , m_clockRate(0) > > + , m_channels(0) > > + , m_ccmfir(false) > > + , m_nackpli(false) > > + , m_nack(false) > > These should be initialized in the class definition. Fixed. > > Source/WebCore/platform/mediastream/PeerMediaDescription.h:2 > > + * Copyright (C) 2015 Ericsson AB. All rights reserved. > > Update? > > > Source/WebCore/platform/mediastream/PeerMediaDescription.h:154 > > + : m_port(0) > > + , m_rtcpMux(false) > > + , m_rtcpPort(0) > > + , m_source(nullptr) > > Ditto. Fixed

Comment on attachment 272108 [details] Updated patch for landing Clearing flags on attachment: 272108 Committed r197053: <http://trac.webkit.org/changeset/197053>