Bug 224917 – Changing the source of a model element with clipping applied does not update the model

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

[Tim Horton]

Changing the source of a model element with clipping applied does not update the model

[Tim Horton]

Created attachment 426775 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

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.

[Tim Horton]

Created attachment 426859 [details]
Patch

[Tim Horton]

Created attachment 426864 [details]
Patch

[Sam Weinig]

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

[Tim Horton]

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

[Tim Horton]

Created attachment 426866 [details]
Patch

[Tim Horton]

Created attachment 426873 [details]
Patch

[Sam Weinig]

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

[Tim Horton]

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

[Sam Weinig]

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

[Tim Horton]

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

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/77112389>