[webkit-reviews] review granted: [Bug 223121] Simulated WebGL context screen change events should work with GPU process : [Attachment 423067] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 223121: Simulated WebGL context screen change events should work with GPU
process
https://bugs.webkit.org/show_bug.cgi?id=223121

Attachment 423067: Patch

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




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

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

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:118
> +#if ENABLE(WEBGL)
> +    void dispatchDisplayWasReconfiguredForTesting() {
dispatchDisplayWasReconfigured(); };
> +#endif

Style-wise, a while back we found that using boolean expressions and separate
#if was clearer, and easier to keep correct when refactoring than nested #if.
So this would be nice as:

#endif
#if PLATFORM(MAC) && ENABLE(WEBGL)
    // ...
#endif

Please consider that form. I’d suggest that above in the .cpp file too for the
same reason.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:281
> +	   callOnMainRunLoop([gpuConnectionToWebProcess =
m_gpuConnectionToWebProcess]() {
> +	       if (auto connectionToWeb = gpuConnectionToWebProcess.get())
> +		   connectionToWeb->dispatchDisplayWasReconfiguredForTesting();
> +	   });

You don’t have to take my suggestions, but I think the best names for these two
would be: weakConnection (the capture) and connection (the local).