AutoFill: Add support for multi-select and delete.

AutoFill: Add support for multi-select and delete.

BUG=46075
TEST=see bug

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

[tfarina, 2010-06-20 23:06:14]

Hi dhollowa, could you review this to me?

[dhollowa, 2010-06-21 14:40:25]

On 2010/06/20 23:06:14, tfarina wrote:
> Hi dhollowa, could you review this to me?

Passed to George.  He's more familiar with this code.

[GeorgeY, 2010-06-22 00:22:08]

On 2010/06/21 14:40:25, dhollowa wrote:
> On 2010/06/20 23:06:14, tfarina wrote:
> > Hi dhollowa, could you review this to me?
> 
> Passed to George.  He's more familiar with this code.

The problem with this patch that buttons need to be updated according to
selection: only delete button must be enabled if more than one item selected.

[GeorgeY, 2010-06-22 00:22:15]

http://codereview.chromium.org/2828014/diff/1/3
File chrome/browser/views/autofill_profiles_view_win.h (left):

http://codereview.chromium.org/2828014/diff/1/3#oldcode421
chrome/browser/views/autofill_profiles_view_win.h:421: protected:
Why is it removed?

[GeorgeY, 2010-06-22 00:27:35]

http://codereview.chromium.org/2828014/diff/1/2
File chrome/browser/views/autofill_profiles_view_win.cc (right):

http://codereview.chromium.org/2828014/diff/1/2#newcode171
chrome/browser/views/autofill_profiles_view_win.cc:171:
table_model_->RemoveItem(*i);
Won't work correctly:
Let's say you have 4 items 0-3 and 1-2 selected. First it will delete #1, and
move all subsequent items up, then it will delete #3, which is now is in #2
position, so instead of deleting ##1-2 it will delete #1 and #3 and leave #2
undeleted.

[dhollowa, 2010-06-22 00:28:43]

On 2010/06/21 14:40:25, dhollowa wrote:
> On 2010/06/20 23:06:14, tfarina wrote:
> > Hi dhollowa, could you review this to me?
> 
> Passed to George.  He's more familiar with this code.

One implication here is that when we move to the non-modal version of this
dialog we *may* need to support multi-delete in the backend as well.  In a prior
discussion with James he was not planning to support this.  It seems possible to
iterate over the items and delete one at a time, but since this is async with
the web data service it becomes tricky.

[tfarina, 2010-06-22 01:25:28]

http://codereview.chromium.org/2828014/diff/1/2
File chrome/browser/views/autofill_profiles_view_win.cc (right):

http://codereview.chromium.org/2828014/diff/1/2#newcode171
chrome/browser/views/autofill_profiles_view_win.cc:171:
table_model_->RemoveItem(*i);
On 2010/06/22 00:27:35, GeorgeY wrote:
> Won't work correctly:
> Let's say you have 4 items 0-3 and 1-2 selected. First it will delete #1, and
> move all subsequent items up, then it will delete #3, which is now is in #2
> position, so instead of deleting ##1-2 it will delete #1 and #3 and leave #2
> undeleted.
Have you tried this combination?

OK, I have tried the following:
I added the items: a, b, c, d.
Then, I selected b and c.
Then, I clicked on Remove. The items b and c were deleted.

So I'm not sure what you mean by "Won't work correctly."

http://codereview.chromium.org/2828014/diff/1/3
File chrome/browser/views/autofill_profiles_view_win.h (left):

http://codereview.chromium.org/2828014/diff/1/3#oldcode421
chrome/browser/views/autofill_profiles_view_win.h:421: protected:
On 2010/06/22 00:22:15, GeorgeY wrote:
> Why is it removed?

Because if it is protected, I could only access them if I was inheriting from
ContentListTableModel, which is not the case.

[tfarina, 2010-06-22 02:15:38]

On 2010/06/22 00:22:08, GeorgeY wrote:
> On 2010/06/21 14:40:25, dhollowa wrote:
> > On 2010/06/20 23:06:14, tfarina wrote:
> > > Hi dhollowa, could you review this to me?
> > 
> > Passed to George.  He's more familiar with this code.
> 
> The problem with this patch that buttons need to be updated according to
> selection: only delete button must be enabled if more than one item selected.

I fixed that. Now when more than one item is selected, the "Edit" button is
disabled. This match with the other dialogs.

Please, could you take another look?

[tfarina, 2010-06-23 17:37:23]

Ping! (Hope this time, you will not IGNORING me again (like you did on the other
patch)).

[tfarina, 2010-06-28 22:18:11]

Friendly ping.

[dhollowa, 2010-06-28 23:37:30]

On 2010/06/22 00:28:43, dhollowa wrote:
> On 2010/06/21 14:40:25, dhollowa wrote:
> > On 2010/06/20 23:06:14, tfarina wrote:
> > > Hi dhollowa, could you review this to me?
> > 
> > Passed to George.  He's more familiar with this code.
> 
> One implication here is that when we move to the non-modal version of this
> dialog we *may* need to support multi-delete in the backend as well.  In a
prior
> discussion with James he was not planning to support this.  It seems possible
to
> iterate over the items and delete one at a time, but since this is async with
> the web data service it becomes tricky.

Spoke with James.  Should be ok here.

[GeorgeY, 2010-06-28 23:39:17]

On 2010/06/28 23:37:30, dhollowa wrote:
> On 2010/06/22 00:28:43, dhollowa wrote:
> > On 2010/06/21 14:40:25, dhollowa wrote:
> > > On 2010/06/20 23:06:14, tfarina wrote:
> > > > Hi dhollowa, could you review this to me?
> > > 
> > > Passed to George.  He's more familiar with this code.
> > 
> > One implication here is that when we move to the non-modal version of this
> > dialog we *may* need to support multi-delete in the backend as well.  In a
> prior
> > discussion with James he was not planning to support this.  It seems
possible
> to
> > iterate over the items and delete one at a time, but since this is async
with
> > the web data service it becomes tricky.
> 
> Spoke with James.  Should be ok here.

LGTM