Changing the source of a model element with clipping applied does not update the model
Created attachment 426775 [details] Patch
Not for review yet, I'm not sure this is right at all (and haven't even written an explanation). Mostly just want EWS to run.
Comment on attachment 426775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426775&action=review > LayoutTests/model-element/model-element-graphics-layers-change-when-clipped-expected.txt:30 > + (bounds 0.00 0.00) This seems bad. I think Antoine noticed this, need to check what his fix was.
Created attachment 426859 [details] Patch
Created attachment 426864 [details] Patch
Comment on attachment 426864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426864&action=review > Source/WebCore/platform/graphics/GraphicsLayerClient.h:94 > IncludeOpacity = 1 << 2, > + IncludeModels = 1 << 3, I regret the IncludeOpacity. Let's remove it and this new one and always include them if they are not the default. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1247 > + noteLayerPropertyChanged(ContentsRectsChanged); > noteLayerPropertyChanged(OpacityChanged); These can be noteLayerPropertyChanged(ContentsRectsChanged | OpacityChanged). Side question. Why is tsetContentsToModel() is so different from setContentsToImage() which uses a noteLayerPropertyChanged(ContentsImageChanged) and then creates the platform layer in GraphicsLayerCA::updateContentsImage()? > LayoutTests/model-element/model-element-contents-layer-updates-with-clipping.html:7 > +<pre id="layers"></pre> Nice </pre>.
Comment on attachment 426864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426864&action=review >> Source/WebCore/platform/graphics/GraphicsLayerClient.h:94 >> + IncludeModels = 1 << 3, > > I regret the IncludeOpacity. Let's remove it and this new one and always include them if they are not the default. I agree for IncludeOpacity. IncludeModels is more useful because otherwise any test that has models *at all* requires separate results for RemoteLayerTree vs. not, even if it doesn't care about the model size. I'll remove IncludeOpacity. >> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1247 >> noteLayerPropertyChanged(OpacityChanged); > > These can be noteLayerPropertyChanged(ContentsRectsChanged | OpacityChanged). > > Side question. Why is tsetContentsToModel() is so different from setContentsToImage() which uses a noteLayerPropertyChanged(ContentsImageChanged) and then creates the platform layer in GraphicsLayerCA::updateContentsImage()? We don't currently have a way to set a model on a PlatformCALayer, only create a PlatformCALayer with a model, so we re-create the platform layer every time. That's why they look so different. This was done in order to keep knowledge of model off of PlatformCALayer proper, which might have been a mistake, but we don't need to change it right now. I also don't think it's a big cost. >> LayoutTests/model-element/model-element-contents-layer-updates-with-clipping.html:7 >> +<pre id="layers"></pre> > > Nice </pre>. You joke, but it was wrong in this patch until yesterday :)
Created attachment 426866 [details] Patch
Created attachment 426873 [details] Patch
(In reply to Tim Horton from comment #7) > Comment on attachment 426864 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426864&action=review > > >> Source/WebCore/platform/graphics/GraphicsLayerClient.h:94 > >> + IncludeModels = 1 << 3, > > > > I regret the IncludeOpacity. Let's remove it and this new one and always include them if they are not the default. > > I agree for IncludeOpacity. IncludeModels is more useful because otherwise > any test that has models *at all* requires separate results for > RemoteLayerTree vs. not, even if it doesn't care about the model size. > Why is that? Can't we dump the size in both cases?
(In reply to Sam Weinig from comment #10) > (In reply to Tim Horton from comment #7) > > Comment on attachment 426864 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=426864&action=review > > > > >> Source/WebCore/platform/graphics/GraphicsLayerClient.h:94 > > >> + IncludeModels = 1 << 3, > > > > > > I regret the IncludeOpacity. Let's remove it and this new one and always include them if they are not the default. > > > > I agree for IncludeOpacity. IncludeModels is more useful because otherwise > > any test that has models *at all* requires separate results for > > RemoteLayerTree vs. not, even if it doesn't care about the model size. > > > > Why is that? Can't we dump the size in both cases? No, because only PlatformCALayerRemoteModelHosting even hangs on to the model; currently in the non-UI-side-compositing case we just end up with a plain-old PlatformCALayer (which doesn't know about models, again, because of review comments in the original patch).
> >> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1247 > >> noteLayerPropertyChanged(OpacityChanged); > > > > These can be noteLayerPropertyChanged(ContentsRectsChanged | OpacityChanged). > > > > Side question. Why is tsetContentsToModel() is so different from setContentsToImage() which uses a noteLayerPropertyChanged(ContentsImageChanged) and then creates the platform layer in GraphicsLayerCA::updateContentsImage()? > > We don't currently have a way to set a model on a PlatformCALayer, only > create a PlatformCALayer with a model, so we re-create the platform layer > every time. That's why they look so different. > > This was done in order to keep knowledge of model off of PlatformCALayer > proper, which might have been a mistake, but we don't need to change it > right now. I also don't think it's a big cost. > Sorry, I am not following this. updateContentsImage() creates the m_contentsLayer in some cases? Couldn't we do the same with a updateContentsModel() but just do it unconditionally if the model had changed?
(In reply to Sam Weinig from comment #12) > > >> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1247 > > >> noteLayerPropertyChanged(OpacityChanged); > > > > > > These can be noteLayerPropertyChanged(ContentsRectsChanged | OpacityChanged). > > > > > > Side question. Why is tsetContentsToModel() is so different from setContentsToImage() which uses a noteLayerPropertyChanged(ContentsImageChanged) and then creates the platform layer in GraphicsLayerCA::updateContentsImage()? > > > > We don't currently have a way to set a model on a PlatformCALayer, only > > create a PlatformCALayer with a model, so we re-create the platform layer > > every time. That's why they look so different. > > > > This was done in order to keep knowledge of model off of PlatformCALayer > > proper, which might have been a mistake, but we don't need to change it > > right now. I also don't think it's a big cost. > > > > Sorry, I am not following this. updateContentsImage() creates the > m_contentsLayer in some cases? Couldn't we do the same with a > updateContentsModel() but just do it unconditionally if the model had > changed? Oh, sure, we could mimic the flow of the others /except/ for the in-place update, if we wanted. But deferring the change to flush time like updateContentsImage() isn't necessary if you're never going to touch the live layer (setContentsToModel never touches the live layer, just makes changes that will get applied at flush time).
Committed r276562 (236998@main): <https://commits.webkit.org/236998@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426873 [details].
<rdar://problem/77112389>