[webkit-reviews] review granted: [Bug 189361] [macOS] Switching to discrete GPU should be done in the UI process : [Attachment 351268] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 189361: [macOS] Switching to discrete GPU should be done in the UI process
https://bugs.webkit.org/show_bug.cgi?id=189361

Attachment 351268: Patch

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




--- Comment #32 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 351268
  --> https://bugs.webkit.org/attachment.cgi?id=351268
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1518
> +    bool m_hasMuxed { false };

I think this needs a better name. "muxing" to me implies a switch in either
direction, and someone not familiar with the term won't know at all. Maybe
m_hasSwitchedToDiscreteGPU?

> Source/WebCore/platform/graphics/GraphicsContext3DManager.h:47
> +WEBCORE_EXPORT bool hasMuxableGPU();

That sounds like there's one GPU that's muxable. It's the system that's
muxable, not the GPU.

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:336
> -#endif // !PLATFORM(MAC)
> +#endif

We like the ending comments.

> Source/WebKit/UIProcess/mac/HighPerformanceGPUManager.h:32
> +// FIXME: This class is using OpenGL to control the muxing of GPUs.
Ultimately
> +// we want to use Metal, but currently there isn't a way to "release" a
> +// discrete MTLDevice, such that the process muxes back to an integrated
GPU.

This comment belongs in the implementation.

> Source/WebKit/WebProcess/WebCoreSupport/mac/WebGPUClient.cpp:53
> +    LOG(WebGL, "WebGPUClient::releaseHighPerformanceGPU() from WebProcess");
> +   
WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessPro
xy::ReleaseHighPerformanceGPU(), 0);

Doesn't this need to do counting? For now there's just a single client
(GraphicsContext3D()), but when we add more this needs to store a per-process
count of things wanting the high performance GPU.

> Source/WebKit/WebProcess/WebCoreSupport/mac/WebGPUClient.h:32
> +class WebGPUClient : public WebCore::GPUClient {

This is not about WebGPU.

> Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.h:32
> +class WebGPUClient : public WebCore::GPUClient {

This sounds like it's related to WebGPU which it is not.