<model> should create a model-owning compositing layer
Created attachment 422362 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
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: .
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.
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?
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`
(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.
(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!
Created attachment 422453 [details] Patch
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>&&?
Created attachment 422479 [details] Patch
Committed r274033: <https://commits.webkit.org/r274033> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422479 [details].
<rdar://problem/75130174>
(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.