[webkit-reviews] review denied: [Bug 193841] REGRESSION (r238563): Web Inspector: Selection is erratic when holding Up/Down on Network Table : [Attachment 373280] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 1 17:16:00 PDT 2019

Devin Rousso <drousso at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 193841: REGRESSION (r238563): Web Inspector: Selection is erratic when
holding Up/Down on Network Table

Attachment 373280: Patch


--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 373280
  --> https://bugs.webkit.org/attachment.cgi?id=373280

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

r-, only because I think this would no longer ensure that a `layout()` call
happens.  The logic looks good :)

> Source/WebInspectorUI/UserInterface/Views/Table.js:369
> +	   if (isNaN(this._cachedScrollTop))
> +	       this._cachedScrollTop = this._scrollContainerElement.scrollTop;

NIT: I'd love it if we had some sort of `_ensureScrollTop()` that did this
logic for us, instead of having this same code in multiple places.

> Source/WebInspectorUI/UserInterface/Views/Table.js:-376
> -	   if (this._isRowVisible(rowIndex)) {
> -	       let row = this._cachedRows.get(rowIndex);
> -	       console.assert(row, "Visible rows should always be in the
> -	       if (row)
> -		   row.scrollIntoViewIfNeeded(false);
> -	       this.needsLayout();

I still think we want to do this, as we should try to use the browser method
whenever possible. This works exactly as we want it to, so there's no reason to
change it.

> Source/WebInspectorUI/UserInterface/Views/Table.js:-379
> -	       this.updateLayout();

This seems to have been removed.  We still want to trigger a `layout()` at some
point.	This will cause a "scroll" event to be fired, but the code for the
`_handleScroll` will (I think) early return because the `event.wheelDeltaY`
would be `0`.

> Source/WebInspectorUI/UserInterface/Views/Table.js:371
> +	   if (isNaN(this._cachedHeight))

Ditto (>368).

> Source/WebInspectorUI/UserInterface/Views/Table.js:372
> +	       this._cachedHeight =

Why is it that we need `getBoundingClientRect`?  Could we not just use the

NIT: you could use `Math.floor(this._scrollContainerElement.realOffsetHeight)`

More information about the webkit-reviews mailing list