[webkit-reviews] review granted: [Bug 224704] LibWebRTCCodecs eagerly launches the GPUProcess and always relaunches it on exit : [Attachment 426314] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 224704: LibWebRTCCodecs eagerly launches the GPUProcess and always
relaunches it on exit
https://bugs.webkit.org/show_bug.cgi?id=224704

Attachment 426314: Patch

https://bugs.webkit.org/attachment.cgi?id=426314&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 426314
  --> https://bugs.webkit.org/attachment.cgi?id=426314
Patch

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

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:172
> +void LibWebRTCCodecs::ensureGPUProcessConnectionOnMainThread(Locker<Lock>&)

Just idly wondering: Would you want to add something like this?

    ASSERT_UNUSED(locker, locker.lockable() == &m_connectionLock);

Adding that would require adding a lockable() member function to the Locker
class template.

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:496
> +    std::exchange(m_connection,
nullptr)->removeThreadMessageReceiver(Messages::LibWebRTCCodecs::messageReceive
rName());

Is there a firm guarantee that m_connection is always non-null here? No race or
edge case where it could be null?

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.h:68
> +    static Ref<LibWebRTCCodecs> create()
>      {
> -	   auto instance = std::unique_ptr<LibWebRTCCodecs>(new
LibWebRTCCodecs);
> -	   instance->startListeningForIPC();
> -	   return instance;
> +	   return adoptRef(*new LibWebRTCCodecs);
>      }

This shouldn’t be inlined. I suggest moving it out of the header file. Normally
it’s better that we inline the call to the constructor, and have the call to
this create function be a normal function call. And it also reduces churn in
the header file a little.