Bug 177206

Created attachment 321253 [details] [IMAGE] Default Columns (for now)

Created attachment 321254 [details] [IMAGE] All Columns

Created attachment 321255 [details] [IMAGE] Column Options

Created attachment 321260 [details] [PATCH] Network Table Some potential follow-ups to this: - Improve even/odd row appearance in rubber band scrolling area - Make the main table area horizontally scrollable (not possible without JS scroll events, so I avoided this) - Modify table content to be more useful - Modify flex / resize behavior to prefer where to distribute pixels (e.g. make/keep the Waterfall column generally larger then others in resize)

Comment on attachment 321260 [details] [PATCH] Network Table View in context: https://bugs.webkit.org/attachment.cgi?id=321260&action=review > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:32 > + --icon-margin-end: 4px; You could use `-webkit-margin-end: 4px` instead. Then you wouldn't ned the body[dir=*] rules that follow. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:48 > + color: gray; Use var(--text-color-gray-medium). The colors are basically identical. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:63 > + scopeBarItem.__checker = checker; Since adding properties to objects is fairly common practice, it might be nice to better support this for certain types with a generic `tag` or `dataObject` property. I always liked this feature in C#/WinForms. When objects are being shared, and this becomes impractical, we can always fallback to double-underscore properties or Symbols. Just a thought. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:90 > + WI.timelineManager.persistentNetworkTimeline.addEventListener(WI.Timeline.Event.RecordAdded, this._networkTimelineRecordAdded, this); So that I'm clean on our own best practices, what is our preferred idiom for event listener management in ContentViews? A) Add in constructor, remove in ContentView.closed B) Add in ContentView.shown, remove in ContentView.hidden This is getting a bit into the weeds, but I think `closed` and `hidden` are confusingly similar. Maybe closed should be renamed `disposed`, if we always plan to release the view reference immediately after. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:183 > + this._table.reloadData(); Will a Table always want to reload itself when the sort changes? If so, this should be controlled by Table, not the delegate. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:729 > + for (let checker of this._activeTypeFilters) { return this._activeTypeCheckers.some(checker => checker(entry.resource.type)); > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:745 > + if (!this._activeTypeFilters) Unless there is a benefit to branch prediction or something, testing the value instead of its inverse is more straightforward, IMO: if (this._activeTypeFilters) this._filteredEntries = this._entries.filter(this._passFilter, this); else this._filteredEntries = this._entries.slice(); > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:763 > + if (listA.length !== listB.length) Use Array.shallowEqual. > Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSidebarPanel.css:27 > + color: gray; var(--text-color-gray-medium) > Source/WebInspectorUI/UserInterface/Views/Table.css:55 > + background-color: hsl(0, 0%, 90%); -webkit-padding-end: 18px; This lets you remove the body[dir=*] rules for padding-right/left below. > Source/WebInspectorUI/UserInterface/Views/Table.css:178 > +} -- Pausing review here --

Comment on attachment 321260 [details] [PATCH] Network Table View in context: https://bugs.webkit.org/attachment.cgi?id=321260&action=review > Source/WebInspectorUI/ChangeLog:200 > + The NetworkTableContentView is a Table DataSource and Delegate. The NetworkTableContentView is a Table, data source, and delegate. > Source/WebInspectorUI/UserInterface/Views/Table.js:44 > + // syncronized scrolling between multiple elements, or making `position: sticky` Spelling: synchronized > Source/WebInspectorUI/UserInterface/Views/Table.js:47 > + this.element.className = "table"; Use classList.add, since the base class owns the element. > Source/WebInspectorUI/UserInterface/Views/Table.js:52 > + this._headerElement.className = "header"; Why not use a <header> element? > Source/WebInspectorUI/UserInterface/Views/Table.js:55 > + this._scrollContainerElement = this._element.appendChild(document.createElement("div")); this._element -> this.element > Source/WebInspectorUI/UserInterface/Views/Table.js:76 > + this._generation = 0; This is incremented but never read. Since this is a transitional patch, it's fine for now to leave it in. > Source/WebInspectorUI/UserInterface/Views/Table.js:115 > + get element() { return this._element; } This shadows View.prototype.element and should be removed. > Source/WebInspectorUI/UserInterface/Views/Table.js:161 > + console.assert(column, "Column not found", columnIdentifier); Nit: message is a complete sentence, should end in period. > Source/WebInspectorUI/UserInterface/Views/Table.js:165 > + console.assert(column.sortable, "Column is not sortable", columnIdentifier); Ditto. > Source/WebInspectorUI/UserInterface/Views/Table.js:208 > + this._needsLayout(); Table shouldn't implement its own layout scheduling. Use View.prototype.needsLayout. > Source/WebInspectorUI/UserInterface/Views/Table.js:214 > + this._needsLayout(); Ditto. > Source/WebInspectorUI/UserInterface/Views/Table.js:309 > + visible ? this.showColumn(column) : this.hideColumn(column); I had to read this twice. Using a ternary like this is pretty unusual. > Source/WebInspectorUI/UserInterface/Views/Table.js:317 > + console.assert(this._columnSpecs.get(column.identifier) === column, "Column not in this table"); Nit: message should end in a period. > Source/WebInspectorUI/UserInterface/Views/Table.js:318 > + console.assert(!column.locked, "Locked columns should always be shown"); Ditto. > Source/WebInspectorUI/UserInterface/Views/Table.js:374 > + console.assert(this._columnSpecs.get(column.identifier) === column, "Column not in this table"); Nit: period at end of sentence. I won't belabor the point anymore. > Source/WebInspectorUI/UserInterface/Views/Table.js:414 > + this.layout(); Use View.prototype.needsLayout, that way if multiple columns are hidden you only have a single layout. > Source/WebInspectorUI/UserInterface/Views/Table.js:464 > + positionDelta *= -1; I prefer `positionDelta = -positionDelta`, but it's up to you. > Source/WebInspectorUI/UserInterface/Views/Table.js:506 > + if (column.flexible) { Get rid of nested ifs: if (!column.flexible) continue; > Source/WebInspectorUI/UserInterface/Views/Table.js:575 > + } Remove this, use View layout facilities. > Source/WebInspectorUI/UserInterface/Views/Table.js:739 > + console.assert(!remainder, "we were unable to distribute pixels"); Nit: "Should not have undistributed pixels." > Source/WebInspectorUI/UserInterface/Views/Table.js:783 > + // Repeat with a new flex size now that we have less flexible columns. Nit: "...fewer flexible columns." > Source/WebInspectorUI/UserInterface/Views/Table.js:831 > + this._fillerHeight += this._rowHeight + 100; // Extend past edge. Magic number `100`. Consider creating a const. > Source/WebInspectorUI/UserInterface/Views/Table.js:845 > + let overflowPadding = updateOffsetThreshold * 3; Magic numbers `10` and `3`. Consider creating consts. > Source/WebInspectorUI/UserInterface/Views/Table.js:987 > + this._needsLayout(); Use View.prototype.needsLayout. > Source/WebInspectorUI/UserInterface/Views/Table.js:999 > + Remove empty line. > Source/WebInspectorUI/UserInterface/Views/Table.js:1009 > + if (!isNaN(rowToSelect)) { Reduce indent: if (isNaN(rowToSelect)) return; > Source/WebInspectorUI/UserInterface/Views/Table.js:1013 > + console.assert(row, "Moving up or down one should always find a cached row since it is within the overflow bounds."); Nit: "Moving up or down by one..." > Source/WebInspectorUI/UserInterface/Views/Table.js:1018 > + this._needsLayout(); Use View.prototype.needsLayout. > Source/WebInspectorUI/UserInterface/Views/Table.js:1062 > + Remove empty line. > Source/WebInspectorUI/UserInterface/Views/TableColumn.js:28 > + constructor(identifier, name, {initialWidth, minWidth, maxWidth, hidden, sortable, align, resizeType} = {}) I'm not sure how I feel about destructuring options in the header like this. Here it's not too bad, but in general I think it impedes readability. I'd prefer: constructor(identifier, name, options = {}) { super(); let {initialWidth, minWidth, maxWidth, hidden, sortable, align, resizeType} = options; ... } > Source/WebInspectorUI/UserInterface/Views/TableColumn.js:70 > + // Private I know the intent is something like friend in C++, but let's just drop the "// Private" comment and make these setters.

Comment on attachment 321260 [details] [PATCH] Network Table View in context: https://bugs.webkit.org/attachment.cgi?id=321260&action=review >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:63 >> + scopeBarItem.__checker = checker; > > Since adding properties to objects is fairly common practice, it might be nice to better support this for certain types with a generic `tag` or `dataObject` property. I always liked this feature in C#/WinForms. When objects are being shared, and this becomes impractical, we can always fallback to double-underscore properties or Symbols. Just a thought. I've settled on really liking the double underscore. It has a unique name and is easy to search for, so it is easy and clean. I like symbol too but the Symbol syntax ends up being super verbose and hard to use in the console when debugging. Having just a `tag` may make it hard to search for (1) others trying to access the tag because its "tag" everywhere its cluttered, and (2) to detect if someone else is using the tag to know if you or someone else is stomping over each other's values. >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:90 >> + WI.Resource.addEventListener(WI.Resource.Event.LoadingDidFinish, this._resourceLoadingDidFinish, this); >> + WI.Resource.addEventListener(WI.Resource.Event.LoadingDidFail, this._resourceLoadingDidFail, this); >> + WI.Resource.addEventListener(WI.Resource.Event.TransferSizeDidChange, this._resourceTransferSizeDidChange, this); >> + WI.timelineManager.persistentNetworkTimeline.addEventListener(WI.Timeline.Event.RecordAdded, this._networkTimelineRecordAdded, this); > > So that I'm clean on our own best practices, what is our preferred idiom for event listener management in ContentViews? > > A) Add in constructor, remove in ContentView.closed > B) Add in ContentView.shown, remove in ContentView.hidden > > This is getting a bit into the weeds, but I think `closed` and `hidden` are confusingly similar. Maybe closed should be renamed `disposed`, if we always plan to release the view reference immediately after. • We usually prefer (A). The intent is "as long as this ContentView is alive, keep it up to date so when it is displayed it is accurate and ready". • However sometimes we may do (B) for cases we don't need to always update things. We would do this to reduce unnecessary work by only updating it while it is visible. I agree that `closed()` should probably be renamed `displosed()`. I don't think we ever intend to use a ContentView after it is closed. I think that would be a worthwhile change. >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:183 >> + this._table.reloadData(); > > Will a Table always want to reload itself when the sort changes? If so, this should be controlled by Table, not the delegate. I think in practice you are right (any sort change we will expect a reload), but I still think we should leave it up to the client to keep table simple. Clients are likely to have a pattern (when they change their entries, they will reload). > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:540 > + this._updateCellsForResource(resource); I'm going to rename _updateCellsForResource to _updateEntryForResource. Update cells was an old name, I've since changed this and things happen in batches. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:554 > + let index = this._entries.findIndex((x) => x.resource === resource); This could be a binary search given we have the _entriesSortComparator. >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:763 >> + if (listA.length !== listB.length) > > Use Array.shallowEqual. See comment below on line 780. I couldn't use shallowEqual in this case.

Comment on attachment 321260 [details] [PATCH] Network Table View in context: https://bugs.webkit.org/attachment.cgi?id=321260&action=review >> Source/WebInspectorUI/UserInterface/Views/Table.js:76 >> + this._generation = 0; > > This is incremented but never read. Since this is a transitional patch, it's fine for now to leave it in. I'll drop it. It was partially for debugging. Really its no different from _cachedRows.clear(). >> Source/WebInspectorUI/UserInterface/Views/Table.js:208 >> + this._needsLayout(); > > Table shouldn't implement its own layout scheduling. Use View.prototype.needsLayout. Oh wow, I can't believe I left this in. This is from when I was prototyping outside of Web Inspector. Thanks for catching this! >> Source/WebInspectorUI/UserInterface/Views/Table.js:464 >> + positionDelta *= -1; > > I prefer `positionDelta = -positionDelta`, but it's up to you. I like that too! >> Source/WebInspectorUI/UserInterface/Views/Table.js:1009 >> + if (!isNaN(rowToSelect)) { > > Reduce indent: > > if (isNaN(rowToSelect)) > return; Sometimes I don't think we want to early return. In this case in particular I think its likely someone in the future might add more code to this method (other key handling), and so they if there were early returns they might need to rewrite logic to remove them. >> Source/WebInspectorUI/UserInterface/Views/TableColumn.js:28 >> + constructor(identifier, name, {initialWidth, minWidth, maxWidth, hidden, sortable, align, resizeType} = {}) > > I'm not sure how I feel about destructuring options in the header like this. Here it's not too bad, but in general I think it impedes readability. I'd prefer: > > constructor(identifier, name, options = {}) > { > super(); > > let {initialWidth, minWidth, maxWidth, hidden, sortable, align, resizeType} = options; > ... > } Heh I actually think it helps readability. Someone searching for the TableColumn constructor can immediately see what the options are.

Created attachment 321611 [details] [PATCH] Proposed Fix This addresses first round of comments.

Comment on attachment 321611 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=321611&action=review r=me > Source/WebInspectorUI/ChangeLog:30 > + the sub-content browser. Forwarding shown and hidden to subviews will no longer be necessary once <https://webkit.org/b/150741> is completed. Some ContentViews that perform forwarding have FIXMEs, but these spots are easy enough to find so it's up to you. > Source/WebInspectorUI/ChangeLog:183 > + (WI.NetworkTableContentView.prototype._updateCellsForResource): Changelog needs to be re-generated. `_updateCellsForResource` was renamed `_updateEntryForResource`. > Source/WebInspectorUI/ChangeLog:200 > + The NetworkTableContentView has a Table and it is data source and The NetworkTableContentView has a Table and is a data source and delegate. > Source/WebInspectorUI/UserInterface/Views/Table.js:55 > + this._scrollContainerElement = this._element.appendChild(document.createElement("div")); Should be `this.element`, otherwise we're reading the private variable from the base class.

Comment on attachment 321611 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=321611&action=review r=me as well > Source/WebInspectorUI/ChangeLog:113 > + (resorting, adding rows, modifying rows, etc the visible rows are Nit: missing ) > Source/WebInspectorUI/ChangeLog:114 > + simply recreated from scratch. Clients should therefore make generating We might run into problems if the delegate puts an <input> into a table, the user starts editing the input value, then the data source changes (i.e., if we had live previews for cookies). I think in this case, it would be the delegate's job to freeze/restore editing state? I can't think of a way that we would avoid this without going down a painful road of incremental cell updates. > Source/WebInspectorUI/ChangeLog:117 > + Unlike DataGrid rows are just an <li> with a bunch of cells. Since the Nit: DataGrid, > Source/WebInspectorUI/UserInterface/Views/Table.js:59 > + if (this._delegate.tableCellClicked) I would prefer to add the listener unconditionally and early exit if no delegate. Unlike scroll listeners, these two have no performance implications. > Source/WebInspectorUI/UserInterface/Views/Table.js:74 > + this._fillerRow.classList.add("filler"); Why use classList here but not for "spacer"? I always prefer classList version. > Source/WebInspectorUI/UserInterface/Views/Table.js:120 > + get sortOrder() I had to look around to figure out the type of the property is WI.Table.SortOrder. Maybe add a comment, or validation code for the setter? > Source/WebInspectorUI/UserInterface/Views/Table.js:369 > + > + // Now populate only the new cells for this column. > + for (let cell of cellsToPopulate) > + this._delegate.tablePopulateCell(this, cell, column, cell.parentElement.__index); Oh, so apparently this delegate method is required. Can you add dummy class definitions for the data source and delegate with appropriate NotImplementedError / comments to help understand what each does and if it's required? This will be helpful when we start migrating other clients off DataGrid.

Comment on attachment 321611 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=321611&action=review >> Source/WebInspectorUI/ChangeLog:114 >> + simply recreated from scratch. Clients should therefore make generating > > We might run into problems if the delegate puts an <input> into a table, the user starts editing the input value, then the data source changes (i.e., if we had live previews for cookies). I think in this case, it would be the delegate's job to freeze/restore editing state? I can't think of a way that we would avoid this without going down a painful road of incremental cell updates. Yes, I think that would be the appropriate solution for editing. The Delegate should prevent any table reloads during editing. >> Source/WebInspectorUI/UserInterface/Views/Table.js:55 >> + this._scrollContainerElement = this._element.appendChild(document.createElement("div")); > > Should be `this.element`, otherwise we're reading the private variable from the base class. Bah, thanks again. I fixed the ones here and in _positionResizerElements. >> Source/WebInspectorUI/UserInterface/Views/Table.js:59 >> + if (this._delegate.tableCellClicked) > > I would prefer to add the listener unconditionally and early exit if no delegate. Unlike scroll listeners, these two have no performance implications. This is just here to avoid putting a check inside the event listeners for the delegate method. If the delegate isn't implemented we'd have to bail on every click. >> Source/WebInspectorUI/UserInterface/Views/Table.js:74 >> + this._fillerRow.classList.add("filler"); > > Why use classList here but not for "spacer"? I always prefer classList version. I'll keep these all consistent. I wish we had some guidance. For a while I was using classList because it could make searches better /classList\.(add|remove)\(/. But over time I've migrated back to the simplicity of className = "...". I'm fine to go either way, but I'd like to a have a justification. >> Source/WebInspectorUI/UserInterface/Views/Table.js:369 >> + this._delegate.tablePopulateCell(this, cell, column, cell.parentElement.__index); > > Oh, so apparently this delegate method is required. Can you add dummy class definitions for the data source and delegate with appropriate NotImplementedError / comments to help understand what each does and if it's required? This will be helpful when we start migrating other clients off DataGrid. I'll assert this in the constructor.

Comment on attachment 321611 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=321611&action=review >>> Source/WebInspectorUI/UserInterface/Views/Table.js:59 >>> + if (this._delegate.tableCellClicked) >> >> I would prefer to add the listener unconditionally and early exit if no delegate. Unlike scroll listeners, these two have no performance implications. > > This is just here to avoid putting a check inside the event listeners for the delegate method. If the delegate isn't implemented we'd have to bail on every click. Oh, you mentioned you would prefer the early exit in handlers. I don't agree. Avoiding registering a handler just lets us do less work in every way. However if it turns out that we always register click handlers, lets go that route.

Comment on attachment 321611 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=321611&action=review >>> Source/WebInspectorUI/UserInterface/Views/Table.js:74 >>> + this._fillerRow.classList.add("filler"); >> >> Why use classList here but not for "spacer"? I always prefer classList version. > > I'll keep these all consistent. I wish we had some guidance. For a while I was using classList because it could make searches better /classList\.(add|remove)\(/. But over time I've migrated back to the simplicity of className = "...". I'm fine to go either way, but I'd like to a have a justification. My guidance is based solely on safety, rather than benefits to searching (but I have no problem favoring something that helps with that): 1) For isolated elements and code that creates and returns an element, className is fine. 2) For all other code, which is probably consuming elements from elsewhere, always use classList to prevent wiping out existing classes.

<https://trac.webkit.org/changeset/222470/webkit>

<rdar://problem/34693187>