Bug 199546

<rdar://problem/52729708>

Created attachment 373606 [details] Patch

Created attachment 373607 [details] Screenshot

Comment on attachment 373606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373606&action=review > Source/JavaScriptCore/inspector/protocol/LayerTree.json:27 > + "id": "LayerAttributes", All of these properties could just be attached to `Layer`. It would make compatibility checks easier, and simplify the overall structure of the payload. > Source/JavaScriptCore/inspector/protocol/LayerTree.json:31 > + { "name": "isRoot", "type": "boolean" }, I'd make this optional, and only include it for objects that are the root (e.g. if provided, we effectively expect it to be `true`). > Source/JavaScriptCore/inspector/protocol/LayerTree.json:46 > + { "name": "attributes", "$ref": "LayerAttributes", "description": "Layer attributes." }, Ditto (>31). > Source/WebCore/inspector/agents/InspectorLayerTreeAgent.cpp:249 > + if (compositingType == MediaCompositingLayer) Should these be `else if`s? > Source/WebInspectorUI/UserInterface/Models/Layer.js:27 > + constructor(layerId, nodeId, bounds, attributes, paintCount, memory, compositedBounds, isInShadowTree, isReflection, isGeneratedContent, isAnonymous, pseudoElementId, pseudoElement) I'd rather change this object to use a more "modern" approach to its constructor (and adjust `WI.Layer.fromPayload` accordingly). ``` constructor(layerId, nodeId, bounds, compositedBounds, paintCount, memory, {isRoot, isBackingStoreDetached, hasMediaContents, hasTiledBacking, isContainerLayer, isInShadowTree, isReflection, isGeneratedContent, isAnonymous, pseudoElementId, pseudoElement} = {}) { console.assert(typeof layerId === "string"); console.assert(typeof nodeId === "number"); console.assert(typeof bounds === "object"); console.assert(typeof compositedBounds === "object"); console.assert(typeof paintCount === "number"); console.assert(typeof memory === "number"); console.assert(isBackingStoreDetached === undefined || typeof isBackingStoreDetached === "boolean"); console.assert(hasMediaContents === undefined || typeof hasMediaContents === "boolean"); console.assert(hasTiledBacking === undefined || typeof hasTiledBacking === "boolean"); console.assert(isContainerLayer === undefined || typeof isContainerLayer === "boolean"); console.assert(isInShadowTree === undefined || typeof isInShadowTree === "boolean"); console.assert(isReflection === undefined || typeof isReflection === "boolean"); console.assert(isGeneratedContent === undefined || typeof isGeneratedContent === "boolean"); console.assert(isAnonymous === undefined || typeof isAnonymous === "boolean"); console.assert(pseudoElementId === undefined || typeof pseudoElementId === "number"); console.assert(pseudoElement === undefined || typeof pseudoElement === "string"); this._layerId = layerId; this._nodeId = nodeId; this._bounds = bounds; this._compositedBounds = compositedBounds; this._paintCount = paintCount; this._memory = memory; this._isRoot = !!isRoot; this._isBackingStoreDetached = !!isBackingStoreDetached; this._hasMediaContents = !!hasMediaContents; this._hasTiledBacking = !!hasTiledBacking; this._isContainerLayer = !!isContainerLayer; this._isInShadowTree = !!isInShadowTree; this._isReflection = !!isReflection; this._isGeneratedContent = !!isGeneratedContent; this._isAnonymous = !!isAnonymous; this._pseudoElementId = pseudoElementId || null; this._pseudoElement = pseudoElement || null; } ``` This way, it's immediately clear what arguments are required and what arguments are optional. It would also require additional `get`ers for the newly added properties. > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:365 > + outline.material.color.set(colors.stroke); > + outline.material.opacity = colors.strokeOpacity; This won't be very good for people who are colorblind. Also, there's no "legend" for the different fill/stroke combinations, so it doesn't seem like there's a way for users to know what they mean. Perhaps we should also show this information in the "popover" when a layer is selected? Should we have a legend in the <canvas> that the user can then reference? > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:516 > + if ("backingStoreIsAttached" in layer.attributes && !layer.attributes.backingStoreIsAttached) { Rather than check that it's provided and falsy, perhaps we could "invert" the property to be `isBackingStoreDetached` and check that it's truthy?

Created attachment 373698 [details] Patch

Comment on attachment 373698 [details] Patch r=me

This was reviewed, will it land?

There was a desire for a border layer color key.