Adding a visibility property to cr.ui.table.TableColumn.

Adding a visibility property to cr.ui.table.TableColumn.

Setting the new #visible property on a column will show and hide it.  Hidden
columns are still added to the DOM, but as hidden elements.  Hidden columns
cannot be resized via the UI, but if their width is programatically set, they
will appear at the requested size the next time they are made visible.

BUG=468022
Committed: https://crrev.com/a0e8a714c577e98e26a1194c40b98ec0854c1c32
Cr-Commit-Position: refs/heads/master@{#321210}

[Ben Kwa, 2015-03-17 20:14:05]

kenobi@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-03-17 23:04:13]

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/table/table_column.js (right):

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column.js:64: div.style.display =
'none';
div.hidden = !this.visible;

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column.js:110:
cr.defineProperty(TableColumn, 'width');
can't you just create a getter for this and return 0 if hidden?

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/table/table_column_model.js (right):

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:111:
cr.dispatchSimpleEvent(this, 'resize');
wouldn't we dispatch this even if a column is hidden?

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:199: if (index < 0 ||
index > this.columns_.size - 1)
should this be an assert?

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:203: if (column.visible
=== visible) {
prefer ==

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:203: if (column.visible
=== visible) {
no curlies

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:219: isVisible:
function(index) {
assert(this.columns_[index]);

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/table/table_header.js (right):

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_header.js:98: // Don't display cells for
hidden columns.  Don't omit the cell
\s\s -> \s

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_header.js:101: if (!cm.isVisible(i)) {
no curlies

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_header.js:126:
this.handleDblClick_.bind(this, i));
splitter.addEventListener('dblclick',
                          this.handleDblClick_.bind(this, i));

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_header.js:131: splitter.style.display =
'none';
how exactly does this work?  this seems unintuitive depending on where/how
splitters work.

[Ben Kwa, 2015-03-18 02:48:11]

Quick responses to some stuff first.  Code changes to follow.

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/table/table_column_model.js (right):

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:111:
cr.dispatchSimpleEvent(this, 'resize');
On 2015/03/17 23:04:13, Dan Beam wrote:
> wouldn't we dispatch this even if a column is hidden?

If the column is hidden, resizing it does nothing in the UI, so there's no point
dispatching this event.

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:199: if (index < 0 ||
index > this.columns_.size - 1)
On 2015/03/17 23:04:13, Dan Beam wrote:
> should this be an assert?

It could be.  I was following the pattern in the rest of the class.

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:219: isVisible:
function(index) {
On 2015/03/17 23:04:13, Dan Beam wrote:
> assert(this.columns_[index]);

This was me going with convention again - the other getters in this class just
dereference the columns_ array.

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/table/table_header.js (right):

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_header.js:131: splitter.style.display =
'none';
On 2015/03/17 23:04:13, Dan Beam wrote:
> how exactly does this work?  this seems unintuitive depending on where/how
> splitters work.

The arrays of splitters elements (and header cells, see above) are
index-corresponded with the array of columns in a number of places in this
class.  Rather than omit the hidden elements and then have to write special-case
iteration code, I figured it was easier to leave the elements in the DOM but
just hide them.

[Ben Kwa, 2015-03-18 16:00:46]

Patchset #2 (id:20001) has been deleted

[Ben Kwa, 2015-03-18 16:03:47]

Patchset #2 (id:40001) has been deleted

[Ben Kwa, 2015-03-18 16:11:38]

Patchset #2 (id:60001) has been deleted

[Ben Kwa, 2015-03-18 16:13:51]

Done.  I also had to add some new code (#needsFullRedraw_) to TableHeader and
TableList in order to fix some UI jankiness.  PTAL.

Thanks!

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/table/table_column.js (right):

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column.js:64: div.style.display =
'none';
On 2015/03/17 23:04:13, Dan Beam wrote:
> div.hidden = !this.visible;

Done.

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column.js:110:
cr.defineProperty(TableColumn, 'width');
On 2015/03/17 23:04:13, Dan Beam wrote:
> can't you just create a getter for this and return 0 if hidden?

I didn't realize you could define custom getters and still use
cr.defineProperty.  Much cleaner now.  Thanks!

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/table/table_column_model.js (right):

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:203: if (column.visible
=== visible) {
On 2015/03/17 23:04:13, Dan Beam wrote:
> prefer ==

Done.

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_column_model.js:203: if (column.visible
=== visible) {
On 2015/03/17 23:04:13, Dan Beam wrote:
> no curlies

Done.

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/table/table_header.js (right):

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_header.js:98: // Don't display cells for
hidden columns.  Don't omit the cell
On 2015/03/17 23:04:13, Dan Beam wrote:
> \s\s -> \s

Done.

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_header.js:101: if (!cm.isVisible(i)) {
On 2015/03/17 23:04:13, Dan Beam wrote:
> no curlies

Done.

https://codereview.chromium.org/1006003004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/table/table_header.js:126:
this.handleDblClick_.bind(this, i));
On 2015/03/17 23:04:13, Dan Beam wrote:
> splitter.addEventListener('dblclick',
>                           this.handleDblClick_.bind(this, i));

Done.

[Dan Beam, 2015-03-18 21:09:08]

lgtm

[Ben Kwa, 2015-03-18 21:10:16]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2015-03-18 21:13:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1006003004/80001

[commit-bot: I haz the power, 2015-03-18 21:33:04]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2015-03-18 21:33:52]

Patchset 2 (id:??) landed as
https://crrev.com/a0e8a714c577e98e26a1194c40b98ec0854c1c32
Cr-Commit-Position: refs/heads/master@{#321210}