Bug 177115 – Web Inspector: Add details sidebar to Layers tab.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Ross Kirsling]

Created attachment 321161 [details]
Screenshot

preCreate a new details sidebar for the Layers tab based on the Elements tab's Layers sidebar.
(Eventually we can remove the old one, but that day is not today.)

[Ross Kirsling]

Argh, hit "save" too soon.
Just "create" it, don't "preCreate" it...whatever the heck that means.

[Ross Kirsling]

Created attachment 321172 [details]
Patch

[Ross Kirsling]

Comment on attachment 321172 [details]
Patch

This patch introduces LayerDetailsSidebarPanel, which is largely the same as LayerTreeDetailsSidebarPanel except for a couple of key differences, which all follow from the fact that we're inspecting "all layers on the current document" and not "layers arising from the children of the selected DOM node":

1. There are no DetailsSections, the DataGrid fills the content area of the sidebar.

2. Dimension information for each layer has been moved to the row popover, and the difference between composited and visible dimensions has been made explicit.

3. Layer data is not retrieved by the sidebar itself, but is passed in from the main ContentView via supplementalRepresentedObjects.

Now, since DetailsSidebarPanels generally instanceof-check all incoming objects (so as to be unassuming of the tab on which they reside), this patch also adds a model class for layer objects. To avoid having it be 100% boilerplate, I've moved the "composited bounds don't have a position" workaround inside that class.

[Ross Kirsling]

Comment on attachment 321172 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:269
> +        compositedRow.appendChild(document.createElement("td")).textContent = layer.compositedBounds.width + "px à " + layer.compositedBounds.height + "px";

I suppose this should be rewritten to use `WI.UIString("%d \xd7 %d pixels")` instead of a raw string.

[Matt Baker]

Comment on attachment 321172 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:269
>> +        compositedRow.appendChild(document.createElement("td")).textContent = layer.compositedBounds.width + "px à " + layer.compositedBounds.height + "px";
> 
> I suppose this should be rewritten to use `WI.UIString("%d \xd7 %d pixels")` instead of a raw string.

We use raw unicode characters in CSS and JS (such as dashes). I think that's okay here, although the multiplication sign is visually similar to an "x".

Utilities.js defines some commonly used characters at the top:

var emDash = "\u2014";
var enDash = "\u2013";
var figureDash = "\u2012";
var ellipsis = "\u2026";
var zeroWidthSpace = "\u200b";

You could add:
var multiplicatationSign = "\u00d7";

[Devin Rousso]

Comment on attachment 321172 [details]
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;
    }

[Ross Kirsling]

Created attachment 321389 [details]
Patch

[Ross Kirsling]

Created attachment 321392 [details]
Patch

[Ross Kirsling]

Comment on attachment 321392 [details]
Patch

Feedback addressed.
(Whoops, last patch had a typo.)

[Matt Baker]

Comment on attachment 321389 [details]
Patch

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

Mostly style/nits.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.css:49
> +    color: hsl(0, 0%, 50%);

color: var(--text-color-gray-medium);

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.css:53
> +    color: hsla(0, 0%, 100%, 0.75);

color: var(--selected-secondary-text-color);

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.css:68
> +

Remove extra newline.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.css:70
> +

Remove extra newline.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.css:76
> +    flex: 1;

display and flex properties are typically at the top.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.css:107
> +    --layer-popover-ul-padding-start: 1em;

-webkit-margin-start: 1em;
-webkit-padding-start: 1em;

Then you can remove the direction-specific rules that follow.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:52
> +        let layers = objects.filter((object) => object instanceof WI.Layer);

Parentheses can be omitted: (object) -> object.

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

Helper functions defined within methods should have opening parenthesis on same line as function header.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:115
> +            let item2 = b.layer[sortColumnIdentifier] || 0;

Style: we typically write `itemA` `itemB` in comparators.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:266
> +        let dimensionsTitle = content.appendChild(document.createElement("p"));

Prefer <div> over <p>. Nothing here is semantically a paragraph.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:283
> +        let reasonsTitle = content.appendChild(document.createElement("p"));

Ditto.

[Ross Kirsling]

Created attachment 321397 [details]
Patch

[Devin Rousso]

Comment on attachment 321389 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.css:107
>> +    --layer-popover-ul-padding-start: 1em;
> 
> -webkit-margin-start: 1em;
> -webkit-padding-start: 1em;
> 
> Then you can remove the direction-specific rules that follow.

We have never used this property in WebInspector.  I wasn't even aware that it existed.  Should we move all of our [dir=rtl] rules to using something like this?

>> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:52
>> +        let layers = objects.filter((object) => object instanceof WI.Layer);
> 
> Parentheses can be omitted: (object) -> object.

I thought our style was to always have the () around arrow functions?

Searching for /\w+ =>/ in WebInspectorUI gives results in 6 files.
Searching for /\w+\) =>/ in WebInspectorUI gives results in 146 files.

I think our style is to always keep the parenthesis, as it makes it clearer that it is an arrow function and that that value is a parameter.

[Matt Baker]

Comment on attachment 321389 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.css:107
>>> +    --layer-popover-ul-padding-start: 1em;
>> 
>> -webkit-margin-start: 1em;
>> -webkit-padding-start: 1em;
>> 
>> Then you can remove the direction-specific rules that follow.
> 
> We have never used this property in WebInspector.  I wasn't even aware that it existed.  Should we move all of our [dir=rtl] rules to using something like this?

We use -webkit-padding-start in 6 files. It's true that we don't use -webkit-margin-*, and I didn't even know it existed until just now. Since these are available, and provide the functionality we replicate with body[dir=*], why not use them?

>>> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:52
>>> +        let layers = objects.filter((object) => object instanceof WI.Layer);
>> 
>> Parentheses can be omitted: (object) -> object.
> 
> I thought our style was to always have the () around arrow functions?
> 
> Searching for /\w+ =>/ in WebInspectorUI gives results in 6 files.
> Searching for /\w+\) =>/ in WebInspectorUI gives results in 146 files.
> 
> I think our style is to always keep the parenthesis, as it makes it clearer that it is an arrow function and that that value is a parameter.

Hmm.

[Ross Kirsling]

(In reply to Matt Baker from comment #13)
> >>> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:52
> >>> +        let layers = objects.filter((object) => object instanceof WI.Layer);
> >> 
> >> Parentheses can be omitted: (object) -> object.
> > 
> > I thought our style was to always have the () around arrow functions?
> > 
> > Searching for /\w+ =>/ in WebInspectorUI gives results in 6 files.
> > Searching for /\w+\) =>/ in WebInspectorUI gives results in 146 files.
> > 
> > I think our style is to always keep the parenthesis, as it makes it clearer that it is an arrow function and that that value is a parameter.
> 
> Hmm.

I asked Matt about this in the channel since it appeared to be in conflict, but he clarified that his suggestion was specific to one-line, single-parameter, braceless arrow functions. I'm happy to do whichever.

[Ross Kirsling]

Also, file counts were discussed, but I think occurrence counts are more relevant:

\(\w+ => [^{] occurs 7 times
\(\(\w+\) => [^{] occurs 125 times

So unless we're coming up with a new policy, I think Devin's right.

[Ross Kirsling]

Created attachment 321442 [details]
Patch

[Devin Rousso]

Comment on attachment 321442 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 321442 [details]
Patch

Clearing flags on attachment: 321442

Committed r222342: <http://trac.webkit.org/changeset/222342>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693284>