Bug 236233 – [model] improve sizing on macOS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

[model] improve sizing on macOS

[Radar WebKit Bug Importer]

<rdar://problem/88569881>

[Antoine Quint]

Created attachment 451092 [details]
Patch

[Antoine Quint]

<rdar://problem/88569881>

[Dean Jackson]

Comment on attachment 451092 [details]
Patch

Can you make a test? Otherwise it looks good.

[Dean Jackson]

Comment on attachment 451092 [details]
Patch

Can you make a test? Otherwise it looks good.

[Antoine Quint]

This sets the size in ways that are internal to ARQL, I don't think we can test this.

[Tim Horton]

Comment on attachment 451092 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        In that new method, we call -[ASVInlinePreview updateFrame:completionHandler:] which provies us with a

`provies`

> Source/WebCore/rendering/RenderLayerBacking.cpp:1520
> +    else if (is<RenderModel>(renderer())) {
> +        auto* element = downcast<HTMLModelElement>(renderer().element());
> +        if (element->usesPlatformLayer())
> +            element->parentLayerSizeMayHaveChanged(m_graphicsLayer->size());
> +    }

This one-off model-only communication about layer size (and in an `else if` about something entirely different?) seems unfortunate. Maybe see if Simon has a better way.

[Antoine Quint]

(In reply to Tim Horton from comment #7)
> Comment on attachment 451092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451092&action=review
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:1520
> > +    else if (is<RenderModel>(renderer())) {
> > +        auto* element = downcast<HTMLModelElement>(renderer().element());
> > +        if (element->usesPlatformLayer())
> > +            element->parentLayerSizeMayHaveChanged(m_graphicsLayer->size());
> > +    }
> 
> This one-off model-only communication about layer size (and in an `else if`
> about something entirely different?) seems unfortunate. Maybe see if Simon
> has a better way.

Sadly, we already do something similar in RenderLayerBacking::updateConfiguration().

[Simon Fraser (smfr)]

Comment on attachment 451092 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:1516
> +    else if (is<RenderModel>(renderer())) {

Do you really want this to be the `else` clause of `subpixelOffsetFromRendererChanged()`? Doesn't make much sense.

>>> Source/WebCore/rendering/RenderLayerBacking.cpp:1520
>>> +    }
>> 
>> This one-off model-only communication about layer size (and in an `else if` about something entirely different?) seems unfortunate. Maybe see if Simon has a better way.
> 
> Sadly, we already do something similar in RenderLayerBacking::updateConfiguration().

It's weird to read m_graphicsLayer->size() here. RenderLayerBacking controls the size fo the GraphicsLayer, so it makes more sense to get it from RenderBox here.

[Antoine Quint]

Created attachment 451356 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 451356 [details]
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.h:161
> +    FloatSize m_platformLayerSize;

Very weird to store this presentation-related thing on a DOM element class. Also weird that it refers to platform layers; sizing is controlled by CSS, so this should just talk about "contentBoxSize" or something.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1539
> +            element->parentLayerSizeMayHaveChanged(primaryGraphicsLayerRect.size());

The primary graphics layer for the model element has a size that's affected by borders, padding, box-shadow etc. It doesn't seem right to use this size. Make a test with a big box-shadow to test.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 451356 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451356&action=review
> 
> > Source/WebCore/Modules/model-element/HTMLModelElement.h:161
> > +    FloatSize m_platformLayerSize;
> 
> Very weird to store this presentation-related thing on a DOM element class.
> Also weird that it refers to platform layers; sizing is controlled by CSS,
> so this should just talk about "contentBoxSize" or something.

I think we can store this on ARKitInlinePreviewModelPlayerMac.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:1539
> > +            element->parentLayerSizeMayHaveChanged(primaryGraphicsLayerRect.size());
> 
> The primary graphics layer for the model element has a size that's affected
> by borders, padding, box-shadow etc. It doesn't seem right to use this size.
> Make a test with a big box-shadow to test.

OK. How would I go about getting the size for the content to be presented?

[Antoine Quint]

Created attachment 451402 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 451402 [details]
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.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #14)
> Comment on attachment 451402 [details]
> 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.

Will rename to contentSize.

> > Source/WebCore/Modules/model-element/HTMLModelElement.h:108
> > +    void parentLayerSizeMayHaveChanged();
> 
> Why does DOM code care about "parent layer" stuff?

Doesn't, I'll rename to "sizeMayHaveChanged()".

> > 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

Tim Horton helped me with this, so I trust that it's correct.

> > 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.

I tried and failed: "error: type-id cannot have a name".

> > 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.

This is something I'd like to look at as a follow-up, filed bug 236388.

[Antoine Quint]

Created attachment 451419 [details]
Patch for landing

[EWS]

Committed r289495 (247033@main): <https://commits.webkit.org/247033@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451419 [details].