[Webkit-unassigned] [Bug 177115] Web Inspector: Add details sidebar to Layers tab.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=177115

--- Comment #6 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 321172
  --> https://bugs.webkit.org/attachment.cgi?id=321172
Patch

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

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:55
> +        return true;

This should only return true if the sidebar is able to inspect one of the objects provided.

    return !!layers.length;

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:75
> +        let columns = {name: {}, paintCount: {}, memory: {}};
> +
> +        columns.name.title = WI.UIString("Node");
> +        columns.name.sortable = false;
> +
> +        columns.paintCount.title = WI.UIString("Paints");
> +        columns.paintCount.sortable = true;
> +        columns.paintCount.aligned = "right";
> +        columns.paintCount.width = "50px";
> +
> +        columns.memory.title = WI.UIString("Memory");
> +        columns.memory.sortable = true;
> +        columns.memory.aligned = "right";
> +        columns.memory.width = "70px";

You could probably make this a single const declaration:

    const columns = {
        name: {
            sortable: false,
            title: WI.UIString("Node"),
        },
        paintCount: {
            aligned: "right",
            sortable: true,
            title: WI.UIString("Paints"),
            width: "50px",
        },
        memory: {
            aligned: "right",
            sortable: true,
            title: WI.UIString("Memory"),
            width: "70px",
        },
    };

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:119
> +    _selectedDataGridNodeChanged()

Style: _dataGridSelectedNodeChanged

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:130
> +    _dataGridGainedFocus(event)

Style: _dataGridFocused

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:136
> +    _dataGridLostFocus(event)

Style: _dataGridBlurred

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:142
> +    _dataGridWasClicked(event)

Style: _dataGridClicked

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:155
> +            WI.domTreeManager.highlightRect(layer.bounds, true);

NIT: for constant arguments, we typically create const variables right before the call:

    const usePageCoordinates = true;
    WI.domTreeManager.highlightRect(layer.bounds, usePageCoordinates);

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:163
> +        this._updateDataGrid(layers);
> +        this._updateBottomBar(layers);

NIT: instead of calling these functions with `layers` as a parameter, why not just set `this._layers` and use that in the functions?

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:175
> +        mutations.removals.forEach(layer => {

Style: we add () around arrow-function parameters, even if there's only one:

    mutations.removals.forEach((layer) => {

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:177
> +            if (node) {

Style: instead of wrapping the logic inside an `if`, we typically early-return:

    let node = this._dataGridNodesByLayerId.get(layer.layerId);
    if (!node)
        return;

    this._dataGrid.removeChild(node);
    this._dataGridNodesByLayerId.delete(layer.layerId);

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:183
> +        mutations.additions.forEach(layer => {

Ditto (175).

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:185
> +            if (node)

This will never be falsy, so this check is unnecessary.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:189
> +        mutations.preserved.forEach(layer => {

Ditto (185).

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:191
> +            if (node)

Ditto (177).

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:220
> +        let layerCount = 0;
> +        let totalMemory = 0;
> +
> +        layers.forEach(layer => {
> +            layerCount++;
> +            totalMemory += layer.memory || 0;
> +        });
> +
> +        this._layersCountLabel.textContent = WI.UIString("Layer Count: %d").format(layerCount);
> +        this._layersMemoryLabel.textContent = WI.UIString("Memory: %s").format(Number.bytesToString(totalMemory));

This can be rewritten using Array.prototype.reduce:

    this._layersCountLabel.textContent = WI.UIString("Layer Count: %d").format(layers.length);

    let totalMemory = layers.reduce((accumulator, layer) => accumulator + (layer.memory || 0), 0);
    this._layersMemoryLabel.textContent = WI.UIString("Memory: %s").format(Number.bytesToString(totalMemory));

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:264
> +        content.appendChild(document.createElement("p")).textContent = WI.UIString("Dimensions");

Style: save this <p> to a variable before setting the `textContent`

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:274
> +        content.appendChild(document.createElement("p")).textContent = WI.UIString("Reasons for compositing:");

Ditto (264).

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:295
> +        {

Style: brace on same line

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:296
> +            list.appendChild(document.createElement("li")).textContent = reason;

Ditto (264).

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:49
> +    get supplementalRepresentedObjects() { return this._layers; }

Style: put this on a separate line

    get supplementalRepresentedObjects()
    {
        return this._layers;
    }

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170920/8e660c64/attachment.html>