[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]

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
https://bugs.webkit.org/show_bug.cgi?id=193841

Attachment 373280: Patch

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




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

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
cache.");
> -	       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 =
Math.floor(this._scrollContainerElement.getBoundingClientRect().height);

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

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