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.)
Argh, hit "save" too soon. Just "create" it, don't "preCreate" it...whatever the heck that means.
Created attachment 321172 [details] Patch
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.
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.
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";
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; }
Created attachment 321389 [details] Patch
Created attachment 321392 [details] Patch
Comment on attachment 321392 [details] Patch Feedback addressed. (Whoops, last patch had a typo.)
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.
Created attachment 321397 [details] Patch
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.
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.
(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.
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.
Created attachment 321442 [details] Patch
Comment on attachment 321442 [details] Patch r=me
Comment on attachment 321442 [details] Patch Clearing flags on attachment: 321442 Committed r222342: <http://trac.webkit.org/changeset/222342>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34693284>