Bug 222798 – <model> should create a model-owning compositing layer

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

<model> should create a model-owning compositing layer

[Tim Horton]

Created attachment 422362 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Dean Jackson]

Comment on attachment 422362 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1856
> +    if (m_uncommittedChanges & ContentsModelChanged) // Needs to happen before ChildrenChanged

Nit: .

[Sam Weinig]

Comment on attachment 422362 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: model-element/model-element-graphics-layers.html

(heart emoji)

> Source/WebCore/Modules/model-element/Model.h:41
> +    WEBCORE_EXPORT static Ref<Model> create(RefPtr<SharedBuffer>);

Should the RefPtr<SharedBuffer> ever be null? Perhaps making this a Ref<SharedBuffer> and storing the SharedBuffer as a Ref<SharedBuffer> would be preferable.

[Simon Fraser (smfr)]

Comment on attachment 422362 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:39
> +#include "Model.h"

This is a layering violation. Platform code can't see Modules code.

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.h:186
> +    RefPtr<Model> model() const override;
> +    void setModel(RefPtr<Model>) override;

It's really weird that our CALayer wrapper knows anything about Model. Why doesn't Model just use a CALayer subclass?

[Devin Rousso]

Comment on attachment 422362 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/LayerTree.json:56
> +                { "name": "model", "type": "boolean", "optional": true, "description": "Composition due to association with a <model> element." },

You'll also need to add this to the Web Inspector frontend with a corresponding `WI.UIString` (which you'll want to add to `Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js`):
 - `WI.Layers3DContentView.prototype._updateReasonsList`
 - `WI.LayerTreeDetailsSidebarPanel.prototype._populateListOfCompositingReasons`

[Tim Horton]

(In reply to Dean Jackson from comment #3)
> Comment on attachment 422362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422362&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1856
> > +    if (m_uncommittedChanges & ContentsModelChanged) // Needs to happen before ChildrenChanged
> 
> Nit: .

I will fix the others :)

(In reply to Sam Weinig from comment #4)
> Comment on attachment 422362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422362&action=review
> 
> > Source/WebCore/Modules/model-element/Model.h:41
> > +    WEBCORE_EXPORT static Ref<Model> create(RefPtr<SharedBuffer>);
> 
> Should the RefPtr<SharedBuffer> ever be null? Perhaps making this a
> Ref<SharedBuffer> and storing the SharedBuffer as a Ref<SharedBuffer> would
> be preferable.

I'll try it! Likely fine.

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 422362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422362&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:39
> > +#include "Model.h"
> 
> This is a layering violation. Platform code can't see Modules code.

Of course! Good point. Also, if it's the <model> equivalent of Image, it doesn't belong here anyway.

> > Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.h:186
> > +    RefPtr<Model> model() const override;
> > +    void setModel(RefPtr<Model>) override;
> 
> It's really weird that our CALayer wrapper knows anything about Model. Why
> doesn't Model just use a CALayer subclass?

Also a good point, let me see if I can make this dance a little cleaner.

[Tim Horton]

(In reply to Devin Rousso from comment #6)
> Comment on attachment 422362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422362&action=review
> 
> > Source/JavaScriptCore/inspector/protocol/LayerTree.json:56
> > +                { "name": "model", "type": "boolean", "optional": true, "description": "Composition due to association with a <model> element." },
> 
> You'll also need to add this to the Web Inspector frontend with a
> corresponding `WI.UIString` (which you'll want to add to
> `Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js`):
>  - `WI.Layers3DContentView.prototype._updateReasonsList`
>  -
> `WI.LayerTreeDetailsSidebarPanel.prototype._populateListOfCompositingReasons`

Ooh! I forgot to search for the string once I found this code. Thanks, Devin!

[Tim Horton]

Created attachment 422453 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 422453 [details]
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:77
> +    if (auto modelData = this->modelData())
> +        return Model::create(*modelData);

Seems weird to create one every time, but I guess for now it's just wrapping the data.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:153
> +    WEBCORE_EXPORT void setContentsToModel(RefPtr<Model>) override;

RefPtr<Model>&&?

[Tim Horton]

Created attachment 422479 [details]
Patch

[EWS]

Committed r274033: <https://commits.webkit.org/r274033>

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

[Radar WebKit Bug Importer]

<rdar://problem/75130174>

[Lauro Moura]

(In reply to EWS from comment #12)
> Committed r274033: <https://commits.webkit.org/r274033>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 422479 [details].

This revision is making the GTK/WPE layout tests to exit early with lots of failures. See b222878.