Devtools: DataGrid: do not render hidden columns.

Devtools: DataGrid: do not render hidden columns.

Previous implementation used display:none to hide columns.
This caused UI glitches, tremble on resize and performance issues.

In new implementation hidden columns are not rendered at all.

BUG=372246, 316092
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175273

[aandrey, 2014-05-28 14:53:50]

i'm not doing this review, just few drive-by comments

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/ui/DataGrid.js (right):

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:124: this._fillerRow = fillerRow;
you can remove local vars headerRow, fillerRow and columnGroup

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:696: var offset = (sum * tableWidth /
sumOfWeights) | 0;
what is this trick?
if float numbers work, I'd not convert them to ints. AFAIR when I touched this
kind of code, I observed resize glitches in some corner cases.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:771: while
(this._columnsArray[columnIndex].hidden)
can this._columnsArray[columnIndex] be undefined?

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:799: while
(this._columnsArray[columnIndex].hidden)
ditto

[alph, 2014-05-28 15:25:58]

lgtm w/ nits

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/network/NetworkPanel.js (right):

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:2521: _createCell:
function(columnIdentifier)
nuke this

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:2684: * @param cell {!Element}
Here and elsewhere:
@param {!Element} cell

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/ui/DataGrid.js (left):

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:1145: First: "first",
I'd keep this resize method and use it in the heap profiler.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/ui/DataGrid.js (right):

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:136: WebInspector.DataGrid.CornerWidth
= 14;
Does it work for all platforms?

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:254: if (column.width)
merge if's

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:694: if (this.isColumnVisible(column))
{
change to continue.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:794: resizer.leftNeighboringColumnIndex
= columnIndex;
just curious, why some properties we set to div have '_' prefix, some '__', and
some have no _'s?

[alph, 2014-05-28 15:25:59]

lgtm w/ nits

[eustas, 2014-05-29 13:09:46]

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/network/NetworkPanel.js (right):

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:2521: _createCell:
function(columnIdentifier)
On 2014/05/28 15:25:58, alph wrote:
> nuke this

Done.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:2684: * @param cell {!Element}
On 2014/05/28 15:25:58, alph wrote:
> Here and elsewhere:
> @param {!Element} cell

Done.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/ui/DataGrid.js (left):

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:1145: First: "first",
On 2014/05/28 15:25:58, alph wrote:
> I'd keep this resize method and use it in the heap profiler.

Done.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/ui/DataGrid.js (right):

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:124: this._fillerRow = fillerRow;
On 2014/05/28 14:53:50, aandrey wrote:
> you can remove local vars headerRow, fillerRow and columnGroup

I did in in first version of patch, but then reverted, to minimize patch size.

Done.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:136: WebInspector.DataGrid.CornerWidth
= 14;
On 2014/05/28 15:25:58, alph wrote:
> Does it work for all platforms?

Yes. See .data-grid col.corner

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:254: if (column.width)
On 2014/05/28 15:25:58, alph wrote:
> merge if's

Done.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:694: if (this.isColumnVisible(column))
{
On 2014/05/28 15:25:58, alph wrote:
> change to continue.

Done.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:696: var offset = (sum * tableWidth /
sumOfWeights) | 0;
On 2014/05/28 14:53:50, aandrey wrote:
> what is this trick?
> if float numbers work, I'd not convert them to ints. AFAIR when I touched this
> kind of code, I observed resize glitches in some corner cases.

When resizer is dragged, then we use offsetWidths of th elements, that are
always integer. So if we don't truncate numbers here, then first drag gesture
will shutter some columns.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:771: while
(this._columnsArray[columnIndex].hidden)
On 2014/05/28 14:53:50, aandrey wrote:
> can this._columnsArray[columnIndex] be undefined?

Only if all columns are hidden. I hope we will never get in such situation.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:794: resizer.leftNeighboringColumnIndex
= columnIndex;
On 2014/05/28 15:25:58, alph wrote:
> just curious, why some properties we set to div have '_' prefix, some '__',
and
> some have no _'s?

With no _'s is "public" - they are accessed from NetworkPanel.

__ - this is used to add "local-private" property to entity defined elsewhere.

_ - this shouldn't be used in this case; it appeared long ago and I decided to
leave it as is for now.

https://codereview.chromium.org/297893003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:799: while
(this._columnsArray[columnIndex].hidden)
On 2014/05/28 14:53:50, aandrey wrote:
> ditto

Number of resizers is guaranteed to be number of visible columns minus one.

So, we will never meed undefined here.

[yurys, 2014-05-30 11:31:45]

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/network/NetworkPanel.js (right):

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:2420: delete this._nameCell;
this._nameCell = null;

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/ui/DataGrid.js (right):

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:615: var tableWidth =
this.element.offsetWidth - this._cornerWidth;
Could you add a comment here?

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:675: applyColumnWeights: function()
Let's make it private.

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:710: isColumnVisible: function(column)
This method could be inlined. In a separate change probably.

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:768: while
(this._columnsArray[columnIndex].hidden)
There are too many calculations like this related to the hidden property. Let's
introduce _visibleColumns array to avoid them.

[eustas, 2014-06-02 08:51:47]

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/network/NetworkPanel.js (right):

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:2420: delete this._nameCell;
On 2014/05/30 11:31:46, yurys wrote:
> this._nameCell = null;

Done.

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/ui/DataGrid.js (right):

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:615: var tableWidth =
this.element.offsetWidth - this._cornerWidth;
On 2014/05/30 11:31:46, yurys wrote:
> Could you add a comment here?

Done.

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:675: applyColumnWeights: function()
On 2014/05/30 11:31:46, yurys wrote:
> Let's make it private.

Done.

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:710: isColumnVisible: function(column)
On 2014/05/30 11:31:46, yurys wrote:
> This method could be inlined. In a separate change probably.

Done.

https://codereview.chromium.org/297893003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/ui/DataGrid.js:768: while
(this._columnsArray[columnIndex].hidden)
On 2014/05/30 11:31:46, yurys wrote:
> There are too many calculations like this related to the hidden property.
Let's
> introduce _visibleColumns array to avoid them.

Done.

[yurys, 2014-06-02 09:16:21]

lgtm

https://codereview.chromium.org/297893003/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/network/NetworkPanel.js (right):

https://codereview.chromium.org/297893003/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:100:
WebInspector.NetworkLogView._columnTitles = {
What's the point in this map? We already have an array of all columns and each
column has a title, wasn't that enough?

https://codereview.chromium.org/297893003/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:369: title:
WebInspector.UIString("timeline"),
Why not NetworkLogView._columnTitles["timeline"] ?

[eustas, 2014-06-02 09:43:18]

https://codereview.chromium.org/297893003/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/network/NetworkPanel.js (right):

https://codereview.chromium.org/297893003/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:100:
WebInspector.NetworkLogView._columnTitles = {
On 2014/06/02 09:16:21, yurys wrote:
> What's the point in this map? We already have an array of all columns and each
> column has a title, wasn't that enough?

This map is used for unwireing from dataGrid.

https://codereview.chromium.org/297893003/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/network/NetworkPanel.js:369: title:
WebInspector.UIString("timeline"),
On 2014/06/02 09:16:21, yurys wrote:
> Why not NetworkLogView._columnTitles["timeline"] ?

Done.

[eustas, 2014-06-02 09:43:25]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-06-02 09:43:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/297893003/80001

[commit-bot: I haz the power, 2014-06-02 10:58:17]

Change committed as 175273