[webkit-reviews] review granted: [Bug 232988] IOSurface memory attribution is hard to use in constructors : [Attachment 446413] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 232988: IOSurface memory attribution is hard to use in constructors
https://bugs.webkit.org/show_bug.cgi?id=232988

Attachment 446413: Patch

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




--- Comment #29 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 446413
  --> https://bugs.webkit.org/attachment.cgi?id=446413
Patch

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

> Source/WebCore/platform/ProcessIdentity.cpp:49
> +ProcessIdentity::ProcessIdentity() = default;

Do we really still need to go out of our way to un-inline all of these? Can't
we let the compiler generate all those and omit them here and in the header?

> Source/WebCore/platform/ProcessIdentity.cpp:62
> +task_id_token_t ProcessIdentity::taskIdToken() const

We can probably inline this too?

> Source/WebCore/platform/ProcessIdentity.h:49
> +    WEBCORE_EXPORT ProcessIdentity();

Same comment as above.

> Source/WebCore/platform/graphics/RemoteVideoSample.h:53
> +    void setOwnershipIdentity(const ProcessIdentity& resourceOwner);

I think can omit the parameter name.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:210
> +    m_resourceOwner = ProcessIdentity { WTFMove(resourceOwner) };

Doesn't `m_resourceOwner = WTFMove(resourceOwner);` work?

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:163
> +    WEBCORE_EXPORT void setOwnershipIdentity(const ProcessIdentity&
resourceOwner);

We can probably omit the parameter name.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:490
> +    ASSERT(resourceOwner);

Shouldn't the assertion be inside the #if check?

> Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:80
> +	   resourceOwner = gpuConnectionToWebProcess.webProcessIdentity(),

now that there is no longer any #if, we should probably put all the lambda
captures on a single line.

>
Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableMappedIOSurface
Backend.h:53
> +    void setOwnershipIdentity(const WebCore::ProcessIdentity&
resourceOwner);

We can probably omit the parameter name.