Bug 224917

Summary: Changing the source of a model element with clipping applied does not update the model
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, graouts, kondapallykalyan, sam, sergio, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Tim Horton 2021-04-21 22:36:06 PDT
Changing the source of a model element with clipping applied does not update the model
Comment 1 Tim Horton 2021-04-21 22:38:03 PDT
Created attachment 426775 [details]
Patch
Comment 2 Tim Horton 2021-04-21 22:38:58 PDT
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 3 Tim Horton 2021-04-21 22:46:50 PDT
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.
Comment 4 Tim Horton 2021-04-22 16:06:21 PDT
Created attachment 426859 [details]
Patch
Comment 5 Tim Horton 2021-04-22 16:34:54 PDT
Created attachment 426864 [details]
Patch
Comment 6 Sam Weinig 2021-04-22 16:40:29 PDT
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 7 Tim Horton 2021-04-22 16:47:43 PDT
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 :)
Comment 8 Tim Horton 2021-04-22 17:02:06 PDT
Created attachment 426866 [details]
Patch
Comment 9 Tim Horton 2021-04-22 17:21:56 PDT
Created attachment 426873 [details]
Patch
Comment 10 Sam Weinig 2021-04-22 17:52:57 PDT
(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?
Comment 11 Tim Horton 2021-04-22 17:55:28 PDT
(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).
Comment 12 Sam Weinig 2021-04-22 17:55:44 PDT
> >> 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?
Comment 13 Tim Horton 2021-04-22 18:04:56 PDT
(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).
Comment 14 EWS 2021-04-24 19:12:17 PDT
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].
Comment 15 Radar WebKit Bug Importer 2021-04-24 19:13:15 PDT
<rdar://problem/77112389>