Move sorting logic from table to list.

Move sorting logic from table to list.


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

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

[rginda, 2011-05-17 23:56:43]

A few quick comments.  I'll poke at it a bit more tomorrow morning.

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:726:
self.grid_.redraw();
I think this will need to become invalidateAndRedraw() too.

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/main.html (right):

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/main.html:33:
'cr/ui/list_selection_model.js',
These three files are already included above.

[vsevik, 2011-05-18 11:32:16]

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:726:
self.grid_.redraw();
On 2011/05/17 23:56:43, rginda wrote:
> I think this will need to become invalidateAndRedraw() too.
This seems unrelated to this change which is complex enough already. Besides
that, I don't really understand what happens here. This onResize is used not
only as a resize handler, shouldn't we rename it?

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/main.html (right):

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/main.html:33:
'cr/ui/list_selection_model.js',
On 2011/05/17 23:56:43, rginda wrote:
> These three files are already included above.

Done.

[rginda, 2011-05-18 23:45:19]

This CL seems regression free so far, but I have a few questions inline.

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:726:
self.grid_.redraw();
Where did invalidateAndRedraw come from?  I thought you mentioned over chat that
you renamed redraw(), but that doesn't seem to be the case.

The grid codepath is there to make sure the grid recalculates the number of
columns for the new size.  It's done on a timeout to give the DOM a chance to
stabilize (though the grid column code has changed since this was written, and
the timeout may no longer be required.)

On 2011/05/18 11:32:17, vsevik wrote:
> On 2011/05/17 23:56:43, rginda wrote:
> > I think this will need to become invalidateAndRedraw() too.
> This seems unrelated to this change which is complex enough already. Besides
> that, I don't really understand what happens here. This onResize is used not
> only as a resize handler, shouldn't we rename it?

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

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/table.js:164: for (var i = 0; i <
this.list_.dataModel.length; i++) {
Apparently in chrome js code we're only supposed to omit the braces on one-line
if statements.

[vsevik, 2011-05-19 10:01:10]

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
> chrome/browser/resources/file_manager/js/file_manager.js:726:
> self.grid_.redraw();
> Where did invalidateAndRedraw come from?  I thought you mentioned over chat
that
> you renamed redraw(), but that doesn't seem to be the case.
> 
> The grid codepath is there to make sure the grid recalculates the number of
> columns for the new size.  It's done on a timeout to give the DOM a chance to
> stabilize (though the grid column code has changed since this was written, and
> the timeout may no longer be required.)
Fine, I see the reason of misunderstanding now.
Before the list.redraw() was overridden in table. Because of that it was called
each time list wanted to redraw itself. Now I removed redraw() from table and
moved the logic from it to invalidateAndRedraw(), which is only called from
table internally. file_manager was the only place where redraw() was called on
table externally, so I made it call invalidateAndRedraw() as well to be sure  
 that this patch doesn't regress it. We can probably optimize performance of
resize logic in file_manager, but I suggest that we do that in a separate CL.

http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/shared...
> chrome/browser/resources/shared/js/cr/ui/table.js:164: for (var i = 0; i <
> this.list_.dataModel.length; i++) {
> Apparently in chrome js code we're only supposed to omit the braces on
one-line
> if statements.
Done.

[arv (Not doing code reviews), 2011-05-19 16:17:56]

http://codereview.chromium.org/7038024/diff/5001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/array_data_model.js (right):

http://codereview.chromium.org/7038024/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/array_data_model.js:46:
getItemByUnsortedIndex_: function(unsortedIndex) {
Why is this needed?

http://codereview.chromium.org/7038024/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/array_data_model.js:66: * @return
{Function(*, *): number} Compare function set for given field.
{function(*, *) : number}

http://codereview.chromium.org/7038024/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/array_data_model.js:128: var addCount =
arguments.length - 2;
Can we skip this work in case there isn't any sorting happening?

[vsevik, 2011-05-19 20:07:02]

PTAL

http://codereview.chromium.org/7038024/diff/5001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/array_data_model.js (right):

http://codereview.chromium.org/7038024/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/array_data_model.js:46:
getItemByUnsortedIndex_: function(unsortedIndex) {
On 2011/05/19 16:17:56, arv wrote:
> Why is this needed?
> 
Inlined.

http://codereview.chromium.org/7038024/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/array_data_model.js:66: * @return
{Function(*, *): number} Compare function set for given field.
On 2011/05/19 16:17:56, arv wrote:
> {function(*, *) : number}

Done.

http://codereview.chromium.org/7038024/diff/5001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/array_data_model.js:128: var addCount =
arguments.length - 2;
On 2011/05/19 16:17:56, arv wrote:
> Can we skip this work in case there isn't any sorting happening?
As discussed, I've done benchmarking that showed regression. I've made some
changes to ensure only one redraw is done on splice, now it looks like we get
some performance improvement.

[arv (Not doing code reviews), 2011-05-19 20:56:28]

LGTM but please wait for Rob to look at it again.

[rginda, 2011-05-19 21:04:11]

On 2011/05/19 10:01:10, vsevik wrote:
>
http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/file_m...
> > chrome/browser/resources/file_manager/js/file_manager.js:726:
> > self.grid_.redraw();
> > Where did invalidateAndRedraw come from?  I thought you mentioned over chat
> that
> > you renamed redraw(), but that doesn't seem to be the case.
> > 
> > The grid codepath is there to make sure the grid recalculates the number of
> > columns for the new size.  It's done on a timeout to give the DOM a chance
to
> > stabilize (though the grid column code has changed since this was written,
and
> > the timeout may no longer be required.)
> Fine, I see the reason of misunderstanding now.
> Before the list.redraw() was overridden in table. Because of that it was
called
> each time list wanted to redraw itself. Now I removed redraw() from table and
> moved the logic from it to invalidateAndRedraw(), which is only called from
> table internally. file_manager was the only place where redraw() was called on
> table externally, so I made it call invalidateAndRedraw() as well to be sure  
>  that this patch doesn't regress it. We can probably optimize performance of
> resize logic in file_manager, but I suggest that we do that in a separate CL.
> 

How about adding invalidateAndRedraw to the cr.ui.List base class, so it's
available on List, Table and Grid?

>
http://codereview.chromium.org/7038024/diff/1/chrome/browser/resources/shared...
> > chrome/browser/resources/shared/js/cr/ui/table.js:164: for (var i = 0; i <
> > this.list_.dataModel.length; i++) {
> > Apparently in chrome js code we're only supposed to omit the braces on
> one-line
> > if statements.
> Done.

[vsevik, 2011-05-19 22:29:36]

> How about adding invalidateAndRedraw to the cr.ui.List base class, so it's
> available on List, Table and Grid?
As discussed in chat, I created invalidate() in list that clears cachedItems_
and removed invalidateAndRedraw

[rginda, 2011-05-19 22:32:08]

On 2011/05/19 22:29:36, vsevik wrote:
> > How about adding invalidateAndRedraw to the cr.ui.List base class, so it's
> > available on List, Table and Grid?
> As discussed in chat, I created invalidate() in list that clears cachedItems_
> and removed invalidateAndRedraw

LGTM, thanks!

[Evan Stade, 2011-05-21 01:11:07]

I believe this CL broke lead

[Mike Mammarella, 2011-05-23 20:25:45]

On 2011/05/21 01:11:07, Evan Stade wrote:
> I believe this CL broke lead

Yes, this CL has broken (at least) being able to actually click on any of the
items in the cookies list at chrome://settings/cookies which uses the list lead
element feature.

[vsevik, 2011-05-24 10:02:20]

Uploaded patch with bug fixed.

[dhollowa, 2011-05-24 15:45:34]

I'm noticing a glitch where the "placeholder" text is retaining the value of the
last item in the list.  To repro:
 (1) Open Autofill preferences : "Manage Autofill Settings..."
 (2) Click "Add New Street Address..."
 (3) Click in "Full name" field (containing placeholder text "Add new name")
 (4) Type "Joe"
 (5) Click outside of the text field to commit the value

Result:
  "Joe" shows up twice, once in the top/edited entry, and once in the
"placeholder" entry

Expect:
  "Joe" shows up *once* in the top/edited entry.  The the "placeholder" entry
should be empty and display the "Add new name" placeholder text.

It looks like somehow the "Joe" value is getting transfered to the new
placeholder item.  This is different from the original behavior.

On 2011/05/24 10:02:20, vsevik wrote:
> Uploaded patch with bug fixed.

[dhollowa, 2011-05-24 15:45:49]

http://codereview.chromium.org/7038024/diff/20001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/array_data_model.js (right):

http://codereview.chromium.org/7038024/diff/20001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/array_data_model.js:82: 
nit: extra spaces here.

[vsevik, 2011-05-25 18:12:04]

PTAL, autofill bug is fixed now.

http://codereview.chromium.org/7038024/diff/20001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/array_data_model.js (right):

http://codereview.chromium.org/7038024/diff/20001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/array_data_model.js:82: 
On 2011/05/24 15:45:49, dhollowa wrote:
> nit: extra spaces here.

Done.

[dhollowa, 2011-05-25 19:28:42]

When I manipulate the Phone or Fax lists I see the following JavaScript console
errors:

Uncaught Error: Invalid index, 1
ArrayDataModel.updateIndexarray_data_model.js:187
ValuesAddRowListItem.onEditCommitted_options_bundle.js:4448
dispatchSimpleEventcr.js:265
editingoptions_bundle.js:1026
InlineEditableItemList.handleListFocusChange_options_bundle.js:1239
dispatchPropertyChangecr.js:102
objcr.js:199
List.handleElementBlur_list.js:458
array_data_model.js:187Uncaught Error: Invalid index, 2
ArrayDataModel.updateIndexarray_data_model.js:187
ValuesAddRowListItem.onEditCommitted_options_bundle.js:4448
dispatchSimpleEventcr.js:265
editingoptions_bundle.js:1026
InlineEditableItemList.handleListFocusChange_options_bundle.js:1239
dispatchPropertyChangecr.js:102
objcr.js:199
List.handleElementBlur_list.js:458

On 2011/05/25 18:12:04, vsevik wrote:
> PTAL, autofill bug is fixed now.
> 
>
http://codereview.chromium.org/7038024/diff/20001/chrome/browser/resources/sh...
> File chrome/browser/resources/shared/js/cr/ui/array_data_model.js (right):
> 
>
http://codereview.chromium.org/7038024/diff/20001/chrome/browser/resources/sh...
> chrome/browser/resources/shared/js/cr/ui/array_data_model.js:82: 
> On 2011/05/24 15:45:49, dhollowa wrote:
> > nit: extra spaces here.
> 
> Done.

[vsevik, 2011-05-25 21:34:51]

Fixed that.

[dhollowa, 2011-05-25 22:29:14]

Yup.  Seems fixed.

On 2011/05/25 21:34:51, vsevik wrote:
> Fixed that.

[rginda, 2011-05-26 05:54:38]

Lgtm, I've merged my changes so you can land this when you're ready.
On May 25, 2011 3:29 PM, <dhollowa@chromium.org> wrote:
> Yup. Seems fixed.
>
> On 2011/05/25 21:34:51, vsevik wrote:
>> Fixed that.
>
>
>
> http://codereview.chromium.org/7038024/