[model] improve sizing on macOS
<rdar://problem/88569881>
Created attachment 451092 [details] Patch
Comment on attachment 451092 [details] Patch Can you make a test? Otherwise it looks good.
This sets the size in ways that are internal to ARQL, I don't think we can test this.
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.
(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().
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.
Created attachment 451356 [details] Patch
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.
(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?
Created attachment 451402 [details] Patch
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.
(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.
Created attachment 451419 [details] Patch for landing
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].