Bug 167289

Created attachment 299459 [details] Patch

Attachment 299459 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSource.h:55: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSource.h:56: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:58: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:117: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:144: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:104: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:117: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:130: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 8 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

This patch is not functional as is as it is missing WK1/WK2 pieces.

Created attachment 299464 [details] Patch

Attachment 299464 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSource.h:55: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSource.h:56: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:117: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:144: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:104: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:117: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:130: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 8 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 299469 [details] Patch

Created attachment 299859 [details] Patch

Attachment 299859 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:107: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:121: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:135: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 299859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299859&action=review > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:284 > + RefPtr<LibWebRTCMediaEndpoint> protectedThis(this); > + callOnMainThread([protectedThis = WTFMove(protectedThis), stream = WTFMove(stream)] { Here's the cool C++14/WTF way of doing things like this. protectedThis = makeRef(*this) > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:107 > + explicit CreateSessionDescriptionObserver(LibWebRTCMediaEndpoint &endpoint) : m_endpoint(endpoint) { } needs to be on more than one line according to stylebot.

Comment on attachment 299859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299859&action=review > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:13 > + * 3. Neither the name of Apple Inc. nor the names of I think we use a 2-clause copyright header by default for new files now. See Source/WebCore/LICENSE-APPLE > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:64 > + m_backend = client.createPeerConnection(*this); This can be in the initializers. > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:316 > + // ASSERT_NOT_REACHED(); Commented out? Maybe this and the previous one should be notImplemented(); > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:336 > + default: Could we not do a default?

Comment on attachment 299859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299859&action=review >> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:13 >> + * 3. Neither the name of Apple Inc. nor the names of > > I think we use a 2-clause copyright header by default for new files now. See Source/WebCore/LICENSE-APPLE I'll check that >> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:64 >> + m_backend = client.createPeerConnection(*this); > > This can be in the initializers. ok >> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:316 >> + // ASSERT_NOT_REACHED(); > > Commented out? Maybe this and the previous one should be notImplemented(); Sure >> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:336 >> + default: > > Could we not do a default? There is a kMax value that we could use instead of default. >> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:107 >> + explicit CreateSessionDescriptionObserver(LibWebRTCMediaEndpoint &endpoint) : m_endpoint(endpoint) { } > > needs to be on more than one line according to stylebot. I prefer this style when there is only one initialiser.

Created attachment 299890 [details] Patch

Attachment 299890 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:103: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:117: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:131: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #13) > Attachment 299890 [details] did not pass style-queue: > > > ERROR: > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:103: > Should be indented on a separate line, with the colon or comma first on that > line. [whitespace/indent] [4] > ERROR: > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:117: > Should be indented on a separate line, with the colon or comma first on that > line. [whitespace/indent] [4] > ERROR: > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:131: > Should be indented on a separate line, with the colon or comma first on that > line. [whitespace/indent] [4] > Total errors found: 3 in 6 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. Fixing style and license comments

Comment on attachment 299890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299890&action=review > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:56 > +class LibWebRTCMediaEndpoint final : public RefCounted<LibWebRTCMediaEndpoint>, private webrtc::PeerConnectionObserver { This should probably be ThreadSafeRefCounted<LibWebRTCMediaEndpoint> since we are ref'ing and deref'ing on different threads. It hurts performance a bit if we ref and deref a lot because it has to clear the instruction pipeline, but it is necessary if we are ref'ing and deref'ing from different threads. > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:47 > +static std::unique_ptr<PeerConnectionBackend> createLibWebRTCPeerConnectionBackend(RTCPeerConnection& peerConnection) > +{ > + return std::unique_ptr<PeerConnectionBackend>(new LibWebRTCPeerConnectionBackend(peerConnection)); > +} This whole function shouldn't be necessary. Just make the constructor public and call std::make_unique > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:53 > + ASSERT(peerConnection.scriptExecutionContext()->isDocument()); It would be nice if we could make every scriptExecutionContext have a LibWebRTCProvider. We would need to make LibWebRTCProvider ThreadSafeRefCounted and pass Ref's around. I might do it later. This is fine for now, though.

(In reply to comment #15) > Comment on attachment 299890 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299890&action=review > > > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:56 > > +class LibWebRTCMediaEndpoint final : public RefCounted<LibWebRTCMediaEndpoint>, private webrtc::PeerConnectionObserver { > > This should probably be ThreadSafeRefCounted<LibWebRTCMediaEndpoint> since > we are ref'ing and deref'ing on different threads. It hurts performance a > bit if we ref and deref a lot because it has to clear the instruction > pipeline, but it is necessary if we are ref'ing and deref'ing from different > threads. Good point, it is probably the simplest solution. > > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:47 > > +static std::unique_ptr<PeerConnectionBackend> createLibWebRTCPeerConnectionBackend(RTCPeerConnection& peerConnection) > > +{ > > + return std::unique_ptr<PeerConnectionBackend>(new LibWebRTCPeerConnectionBackend(peerConnection)); > > +} > > This whole function shouldn't be necessary. Just make the constructor > public and call std::make_unique Will use make_unique. > > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:53 > > + ASSERT(peerConnection.scriptExecutionContext()->isDocument()); > > It would be nice if we could make every scriptExecutionContext have a > LibWebRTCProvider. We would need to make LibWebRTCProvider > ThreadSafeRefCounted and pass Ref's around. I might do it later. This is > fine for now, though. There is no planned support for WebRTC in workers so is it really worth it. Having a RefCounted LibWebRTCProvider would be useful for the mock implementation too though.

Created attachment 299910 [details] Patch for landing

Comment on attachment 299910 [details] Patch for landing Clearing flags on attachment: 299910 Committed r211255: <http://trac.webkit.org/changeset/211255>

All reviewed patches have been landed. Closing bug.