[webkit-reviews] review canceled: [Bug 177206] Web Inspector: Include a table in New Network Tab : [Attachment 321260] [PATCH] Network Table

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 177206: Web Inspector: Include a table in New Network Tab
https://bugs.webkit.org/show_bug.cgi?id=177206

Attachment 321260: [PATCH] Network Table

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




--- Comment #8 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 321260
  --> https://bugs.webkit.org/attachment.cgi?id=321260
[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.