cr/ui/list.js: Support rows with variable heights.

cr/ui/list.js: Support rows with variable heights. 

This patch also enables partial refreshing on WebUI Task Manager.

BUG=104136
TEST=manual on TaskManager, CookieManager and FileManager


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113775

[yoshiki, 2011-11-21 12:01:48]

Could you take a look? If you have any questions, please feel free to ask.

[arv (Not doing code reviews), 2011-11-21 20:38:01]

Thanks for doing this.

Please do thorough testing with zoomed lists since we had numerous bugs related
to that before.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/js/cr/ui/grid.js (right):

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/grid.js:89: set
fixedHeight(fixedHeight) {
missing getter

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/grid.js:91: console.warn("cr.ui.Grid
does not support fixedHeight = false");
Use single quotes

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:119: * The height of the lead
item, which is allowed to have a different height
Is there some way we can simplify this so that we do not need a special lead
item height and let the cases that used that use non fixed height rows instead?

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:143: cachedItems_: [],
These two arrays needs to go on the instance of they will be shared between all
instances.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:281: return this.fixedHeight;
iloop

I assume you meant this.fixedHeight_

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:412: getItemHeightByIndex_:
function(index) {
missing jsdoc

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:670: (e.index < this.lastIndex_
|| this.remainSpace_)) {
rename to remainingSpace_?

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:692: for (var i = 0; i < index;
i++)
Only skip {} for if/else

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:732: var scrollToAdjustTopFunc
= function() {
function scrollToAdjustTop() {

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:737: var
scrollToAdjustBottomFunc = function () {
function scrollToAdjustBottom() {

}

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:752: else ((scrollTop +
clientHeight) < (top + itemHeight))
Syntax error

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:757: else if ((top +
itemHeight) < (scrollTop + clientHeight))
no else after return

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:757: else if ((top +
itemHeight) < (scrollTop + clientHeight))
Too many parentheses

if (top + itemHeight < scrollTop + clientHeight)

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:861: if (itemHeight == 0)
if (!itemHeight)

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:862: return
this.dataModel.length;
fix indentation

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:876: if (offset < leadTop +
leadItemHeight)
please add some newlines somewhere... this could use some grouping

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:893: this.dataModel.length -
1);
fix indentation

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:934: return {first: 0,
return {
  first: ...
};

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:965: this.cachedItemSizes_[y] =
measureItem(this, listItem);
This is bad. it will regress performance since it will make a reflow for every
iteration.

To solve this you need to add all the elements first before any measuring is
done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:1044: this.addItems(firstIndex,
lastIndex, cachedItems, newCachedItems);
Make sure there are no measuring done in here.

Basically, you need to ensure that you do not interleave measuring with mutating
the dom (appendChild, element.style.foo assignment etc). You should be able to
see this in the inspector.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/js/cr/ui/list_selection_model.js (right):

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list_selection_model.js:180: // Calls
delayed |dispatchPropertyChange|s.
Can you explain why this is needed?

[yoshiki, 2011-11-28 11:03:31]

Thank you for comments. I confirmed that it works well even with zoomed lists.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/js/cr/ui/grid.js (right):

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/grid.js:89: set
fixedHeight(fixedHeight) {
On 2011/11/21 20:38:01, arv wrote:
> missing getter

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/grid.js:91: console.warn("cr.ui.Grid
does not support fixedHeight = false");
On 2011/11/21 20:38:01, arv wrote:
> Use single quotes

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:119: * The height of the lead
item, which is allowed to have a different height
On 2011/11/21 20:38:01, arv wrote:
> Is there some way we can simplify this so that we do not need a special lead
> item height and let the cases that used that use non fixed height rows
instead?

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:143: cachedItems_: [],
On 2011/11/21 20:38:01, arv wrote:
> These two arrays needs to go on the instance of they will be shared between
all
> instances.

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:281: return this.fixedHeight;
On 2011/11/21 20:38:01, arv wrote:
> iloop
> 
> I assume you meant this.fixedHeight_

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:412: getItemHeightByIndex_:
function(index) {
On 2011/11/21 20:38:01, arv wrote:
> missing jsdoc

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:670: (e.index < this.lastIndex_
|| this.remainSpace_)) {
On 2011/11/21 20:38:01, arv wrote:
> rename to remainingSpace_?

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:692: for (var i = 0; i < index;
i++)
On 2011/11/21 20:38:01, arv wrote:
> Only skip {} for if/else

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:732: var scrollToAdjustTopFunc
= function() {
On 2011/11/21 20:38:01, arv wrote:
> function scrollToAdjustTop() {

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:737: var
scrollToAdjustBottomFunc = function () {
On 2011/11/21 20:38:01, arv wrote:
> function scrollToAdjustBottom() {
> 
> }

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:752: else ((scrollTop +
clientHeight) < (top + itemHeight))
On 2011/11/21 20:38:01, arv wrote:
> Syntax error

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:757: else if ((top +
itemHeight) < (scrollTop + clientHeight))
On 2011/11/21 20:38:01, arv wrote:
> Too many parentheses
> 
> if (top + itemHeight < scrollTop + clientHeight)

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:757: else if ((top +
itemHeight) < (scrollTop + clientHeight))
On 2011/11/21 20:38:01, arv wrote:
> no else after return

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:861: if (itemHeight == 0)
On 2011/11/21 20:38:01, arv wrote:
> if (!itemHeight)

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:862: return
this.dataModel.length;
On 2011/11/21 20:38:01, arv wrote:
> fix indentation

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:876: if (offset < leadTop +
leadItemHeight)
On 2011/11/21 20:38:01, arv wrote:
> please add some newlines somewhere... this could use some grouping

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:893: this.dataModel.length -
1);
On 2011/11/21 20:38:01, arv wrote:
> fix indentation

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:934: return {first: 0,
On 2011/11/21 20:38:01, arv wrote:
> return {
>   first: ...
> };

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:965: this.cachedItemSizes_[y] =
measureItem(this, listItem);
On 2011/11/21 20:38:01, arv wrote:
> This is bad. it will regress performance since it will make a reflow for every
> iteration.
> 
> To solve this you need to add all the elements first before any measuring is
> done.

Done.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:1044: this.addItems(firstIndex,
lastIndex, cachedItems, newCachedItems);
I added the function ensureAllItemSizesInCache() and call it in advance before
redrawing. I checked that the dom is not mutated after that before addItems().

On 2011/11/21 20:38:01, arv wrote:
> Make sure there are no measuring done in here.
> 
> Basically, you need to ensure that you do not interleave measuring with
mutating
> the dom (appendChild, element.style.foo assignment etc). You should be able to
> see this in the inspector.

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/js/cr/ui/list_selection_model.js (right):

http://codereview.chromium.org/8608007/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list_selection_model.js:180: // Calls
delayed |dispatchPropertyChange|s.
On 2011/11/21 20:38:01, arv wrote:
> Can you explain why this is needed?

Done.

[arv (Not doing code reviews), 2011-11-29 21:46:55]

http://codereview.chromium.org/8608007/diff/12001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/8608007/diff/12001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:122: * lead item is needed, use
this value instead of fixedHeight = false, to
This sentence does not parse.

http://codereview.chromium.org/8608007/diff/12001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:680: if
(this.selectionModel.leadIndex > -1 &&
Can we treat fixedHeight_ as not having a custom leadItemHeight_ and then in the
other code path have getItemHeightByIndex_ return leadItemHeight?

[yoshiki, 2011-11-30 12:01:39]

I changed the comment text. Could you take an another look?

http://codereview.chromium.org/8608007/diff/12001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/8608007/diff/12001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:122: * lead item is needed, use
this value instead of fixedHeight = false, to
Changed the sentence.
And moved it to the comment of fixedHeight() setter/getter.

On 2011/11/29 21:46:55, arv wrote:
> This sentence does not parse.

http://codereview.chromium.org/8608007/diff/12001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:680: if
(this.selectionModel.leadIndex > -1 &&
On 2011/11/29 21:46:55, arv wrote:
> Can we treat fixedHeight_ as not having a custom leadItemHeight_ and then in
the
> other code path have getItemHeightByIndex_ return leadItemHeight?

Done.

[arv (Not doing code reviews), 2011-11-30 18:57:48]

I think there is some miscommunication going on here.

Now that you are adding support for custom row heights I think we can get rid of
the custom lead item height or at least let the custom lead item height use the
custom row height code path.

[yoshiki, 2011-12-01 15:27:05]

Thanks, I omitted the leadItemHeight and made sure that CookieManager works
well.

[yoshiki, 2011-12-05 14:41:05]

PATL

[arv (Not doing code reviews), 2011-12-05 18:31:42]

Maybe I'm missing something, but doesn't the cookies and intents list code be
updated to set the desired height when the lead item changes?

[yoshiki, 2011-12-08 10:13:15]

In cookies_list.js and intents_list.js the height of item element is changed
when the item is changing, separately from List.leadItemHeight.

On 2011/12/05 18:31:42, arv wrote:
> Maybe I'm missing something, but doesn't the cookies and intents list code be
> updated to set the desired height when the lead item changes?

[arv (Not doing code reviews), 2011-12-08 20:08:13]

LGTM

[commit-bot: I haz the power, 2011-12-09 07:21:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/8608007/18009

[commit-bot: I haz the power, 2011-12-09 08:41:36]

Change committed as 113775