Issue 11280253: Fixing column widths in tables.

Issue 11280253: Fixing column widths in tables. (Closed)

Created:
8 years ago by SeRya

Modified:
8 years ago

Reviewers:
arv (Not doing code reviews), dgozman, Dmitry Zvorygin

CC:
chromium-reviews, rginda+watch_chromium.org, arv (Not doing code reviews), yoshiki+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Fixing column widths in tables. Affects File Manager table view and Task Manager. Previously columns was stretched to occupy full list width. As we discussed width @kenmoore and @joshwoodward won't depend on list width and have values in pixels. Total columns width may be less or more of list width. In the last case horizontal scroll bar appears. Column widths supposed to be persistent. But it is not implemented yet. BUG=126738 TEST=Check how File Manager and Task Manager tables behave when window and column resize. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170789

Patch Set 1 #

Total comments: 25

Patch Set 2 : Code review fixes. #

Total comments: 8

Patch Set 3 : Code review fixes. #

Created: 8 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+177 lines, -92 lines)			Patch
M	chrome/browser/resources/file_manager/js/file_manager.js	View	1 2	5 chunks	+25 lines, -21 lines	0 comments	Download
M	chrome/browser/resources/shared/css/table.css	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/shared/js/cr/ui/list.js	View		5 chunks	+7 lines, -11 lines	0 comments	Download
M	chrome/browser/resources/shared/js/cr/ui/table.js	View	1 2	4 chunks	+14 lines, -6 lines	0 comments	Download
M	chrome/browser/resources/shared/js/cr/ui/table/table_column_model.js	View		4 chunks	+14 lines, -24 lines	0 comments	Download
M	chrome/browser/resources/shared/js/cr/ui/table/table_header.js	View		4 chunks	+7 lines, -10 lines	0 comments	Download
M	chrome/browser/resources/shared/js/cr/ui/table/table_list.js	View	1 2	3 chunks	+104 lines, -12 lines	0 comments	Download
M	chrome/browser/resources/shared/js/cr/ui/table/table_splitter.js	View		1 chunk	+2 lines, -7 lines	0 comments	Download
M	chrome/browser/resources/task_manager/main.js	View		1 chunk	+3 lines, -1 line	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

SeRya

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shared/js/cr/ui/list.js File chrome/browser/resources/shared/js/cr/ui/list.js (left): https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shared/js/cr/ui/list.js#oldcode352 chrome/browser/resources/shared/js/cr/ui/list.js:352: if (height && height.substr(-2) == 'px') This code ignored ...

8 years ago (2012-11-30 13:20:01 UTC) #1

arv (Not doing code reviews)

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/file_manager/js/file_manager.js File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/file_manager/js/file_manager.js#newcode1080 chrome/browser/resources/file_manager/js/file_manager.js:1080: columnModel.__defineGetter__('size', function() { Don't use __defineGetter___. Use ES5 Object.defineProperty ...

8 years ago (2012-11-30 19:05:52 UTC) #2

SeRya

8 years ago (2012-12-01 20:14:37 UTC) #3

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:1080:
columnModel.__defineGetter__('size', function() {
On 2012/11/30 19:05:52, arv wrote:
> Don't use __defineGetter___. Use ES5 Object.defineProperty instead.

Done.

> Why are you overriding the getter here though? Would it be cleaner if you
> extracted this into a subclass?

Just to keep this change simple. It indeed needs some reorganizing (even comment
in line 1064 states so).

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:1087: function
isOnDrive(entry) {
On 2012/11/30 19:05:52, arv wrote:
> FunctionDeclaration is not allowed here. "use strict" would have told you so.
> 
> The simplest thing is to change to a FunctionExpression.

Done.

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
File chrome/browser/resources/shared/js/cr/ui/table.js (left):

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table.js:174:
this.ownerDocument.defaultView.addEventListener(
On 2012/11/30 19:05:52, arv wrote:
> Why is this removed? If the table is using a size relative to the window size
it
> needs to be updated.

A moved it to Task Manager. File manager has already have this logic. Why I
think it's a wrong place to handle window resize:
1. List only need to be resized if its size is relative to window size.
2. Not only window resizing could cause resiling list. In case of Task Manager
is is the only reason but in Fine Manager there are a lot of other reasons:
dragging vertical splitter, showing/hidding the bottom panel and so on.

So this code is not competent enough to make decision when list must be updated.
Handling it here partially blures responsibility.

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
File chrome/browser/resources/shared/js/cr/ui/table/table_list.js (right):

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table/table_list.js:39: if
(this.updateScrollbars_()) {
On 2012/11/30 19:05:52, arv wrote:
> No {} for single line ifs

Done.

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table/table_list.js:40:
List.prototype.redraw.call(this);
On 2012/11/30 19:05:52, arv wrote:
> Why not this.redraw()?

this.redraw updated scrollbars and calls List.prototype.redraw. There is no need
to update scrollbars one again.

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table/table_list.js:92:
updateScrollbars_: function() {
On 2012/11/30 19:05:52, arv wrote:
> How many reflows does this cause? It is really hard to follow. It seems like
it
> might do 4 reflows. Can we do this in a single reflow?

In worse case it could be 4 reflows indeed:
1. if cm.totalWidth > offsetWidth && style.overflowX != 'scroll'
2. Item height is not fixed and heights are not cached (areAllItemsVisible_
cause reflow in ths case).
3. !this.areAllItemsVisible_(height) && style.overflowY == 'scroll'

We could cache (offsetWidth - clientWidth) assuming it doesn't change. The same
with height. So there is a room for some optimization. But actually I don't
think it worth additional complication?

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table/table_list.js:94: var s =
this.style;
On 2012/11/30 19:05:52, arv wrote:
> s/s/style/

Done.

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table/table_list.js:94: var s =
this.style;
On 2012/11/30 19:05:52, arv wrote:
> Maybe this should be the computed style?

this.style.overflow should always have value to reduce complexity. So it's
correct.

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table/table_list.js:108: s.overflowX =
'scroll';
On 2012/11/30 19:05:52, arv wrote:
> Can this code needs to be reordered to do all the  querying of the layout
before
> you start mutating the style?

I've done it for offsetWidth. But it is important to query clientWidth/Height
after appropriate style set (in order to measure it with scrollbars). I don't
expect performance impart since additional reflow happen only style changed that
happen relatively rare.

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table/table_list.js:130: * SHows or
hides vertical scroll bar.
On 2012/11/30 19:05:52, arv wrote:
> typo

Done.

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table/table_list.js:133:
showVerticalScrollBar_: function(on) {
On 2012/11/30 19:05:52, arv wrote:
> rename on to something more descriptive

It looks descriptive for me. If you see something more descriptive please write
it here.

https://codereview.chromium.org/11280253/diff/1/chrome/browser/resources/shar...
chrome/browser/resources/shared/js/cr/ui/table/table_list.js:134: var s =
this.style;
On 2012/11/30 19:05:52, arv wrote:
> computed style?

this.style.overflow expected to have own value. So this.style is correct.

arv (Not doing code reviews)

LGTM Please fix all the typos in the CL description (there are a lot of ...

8 years ago (2012-12-03 15:53:48 UTC) #4

SeRya

https://codereview.chromium.org/11280253/diff/8002/chrome/browser/resources/file_manager/js/file_manager.js File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/11280253/diff/8002/chrome/browser/resources/file_manager/js/file_manager.js#newcode1080 chrome/browser/resources/file_manager/js/file_manager.js:1080: Object.defineProperty(columnModel, 'size', { get: function() { On 2012/12/03 15:53:48, ...

8 years ago (2012-12-03 16:59:36 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/11280253/2011

8 years ago (2012-12-03 16:59:51 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/11280253/2011

8 years ago (2012-12-03 18:46:58 UTC) #8

Message was sent while issue was closed.

Change committed as 170789

Expand Messages | Collapse Messages