[webkit-reviews] review granted: [Bug 236233] [model] improve sizing on macOS : [Attachment 451402] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 236233: [model] improve sizing on macOS
https://bugs.webkit.org/show_bug.cgi?id=236233

Attachment 451402: Patch

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




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

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:647
> +LayoutSize HTMLModelElement::platformLayerSize() const

Don't use the term "platform layer" here. Maybe contentsSize.

> Source/WebCore/Modules/model-element/HTMLModelElement.h:108
> +    void parentLayerSizeMayHaveChanged();

Why does DOM code care about "parent layer" stuff?

> Source/WebCore/Modules/model-element/HTMLModelElement.h:151
> +    LayoutSize platformLayerSize() const;

Rename

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:273
> +	       auto fenceSendRight = MachSendRight::adopt([strongFenceHandle
copyPort]);
> +	       [strongFenceHandle invalidate];

This looks weird but I don't understand how CAFenceHandle and MachSendRights
interact

> Source/WebKit/UIProcess/ModelElementController.h:74
> +    void modelElementSizeDidChange(const String& uuid, WebCore::FloatSize,
CompletionHandler<void(Expected<MachSendRight, WebCore::ResourceError>)>&&);

You should name the MachSendRight so reading the code I know it's about
fencing.

> Source/WebKit/WebProcess/Model/mac/ARKitInlinePreviewModelPlayerMac.mm:180
> +void ARKitInlinePreviewModelPlayerMac::sizeDidChange(WebCore::LayoutSize
size)

I feel like size should be a FloatSize, and we should have pixel-snapped it
somewhere.