Bug 199546 – Layer visualization needs to show more data about the layers

• Status: NEW
• Component: Web Inspector

[Simon Fraser (smfr)]

The layer 3d view needs to show layers with backing store (i.e. those that use memory), those that are solid color or have image contents, those that are clipping etc.

[Radar WebKit Bug Importer]

<rdar://problem/52729708>

[Simon Fraser (smfr)]

Created attachment 373606 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 373607 [details]
Screenshot

[EWS Watchlist]

Attachment 373606 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/agents/InspectorLayerTreeAgent.cpp:246:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 373606 [details]
Patch

Attachment 373606 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12684861

New failing tests:
inspector/layers/layers-for-node.html

[EWS Watchlist]

Created attachment 373611 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 373606 [details]
Patch

Attachment 373606 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12684908

New failing tests:
inspector/layers/layers-for-node.html

[EWS Watchlist]

Created attachment 373612 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

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?

[Simon Fraser (smfr)]

Created attachment 373698 [details]
Patch

[Blaze Burg]

Comment on attachment 373698 [details]
Patch

r=me

[Joseph Pecoraro]

This was reviewed, will it land?

[Simon Fraser (smfr)]

There was a desire for a border layer color key.