Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(129)

Issue 2828014: AutoFill: Add support for multi-select and delete. (Closed)

Created:
10 years, 6 months ago by tfarina
Modified:
9 years, 7 months ago
Reviewers:
GeorgeY, dhollowa
CC:
chromium-reviews, ben+cc_chromium.org
Visibility:
Public.

Description

AutoFill: Add support for multi-select and delete. BUG=46075 TEST=see bug Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51202

Patch Set 1 #

Total comments: 4

Patch Set 2 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -10 lines) Patch
M chrome/browser/views/autofill_profiles_view_win.h View 4 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/views/autofill_profiles_view_win.cc View 1 3 chunks +15 lines, -7 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
tfarina
Hi dhollowa, could you review this to me?
10 years, 6 months ago (2010-06-20 23:06:14 UTC) #1
dhollowa
On 2010/06/20 23:06:14, tfarina wrote: > Hi dhollowa, could you review this to me? Passed ...
10 years, 6 months ago (2010-06-21 14:40:25 UTC) #2
GeorgeY
On 2010/06/21 14:40:25, dhollowa wrote: > On 2010/06/20 23:06:14, tfarina wrote: > > Hi dhollowa, ...
10 years, 6 months ago (2010-06-22 00:22:08 UTC) #3
GeorgeY
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?
10 years, 6 months ago (2010-06-22 00:22:15 UTC) #4
GeorgeY
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 ...
10 years, 6 months ago (2010-06-22 00:27:35 UTC) #5
dhollowa
On 2010/06/21 14:40:25, dhollowa wrote: > On 2010/06/20 23:06:14, tfarina wrote: > > Hi dhollowa, ...
10 years, 6 months ago (2010-06-22 00:28:43 UTC) #6
tfarina
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 ...
10 years, 6 months ago (2010-06-22 01:25:28 UTC) #7
tfarina
On 2010/06/22 00:22:08, GeorgeY wrote: > On 2010/06/21 14:40:25, dhollowa wrote: > > On 2010/06/20 ...
10 years, 6 months ago (2010-06-22 02:15:38 UTC) #8
tfarina
Ping! (Hope this time, you will not IGNORING me again (like you did on the ...
10 years, 6 months ago (2010-06-23 17:37:23 UTC) #9
tfarina
Friendly ping.
10 years, 5 months ago (2010-06-28 22:18:11 UTC) #10
dhollowa
On 2010/06/22 00:28:43, dhollowa wrote: > On 2010/06/21 14:40:25, dhollowa wrote: > > On 2010/06/20 ...
10 years, 5 months ago (2010-06-28 23:37:30 UTC) #11
GeorgeY
10 years, 5 months ago (2010-06-28 23:39:17 UTC) #12
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

Powered by Google App Engine
This is Rietveld 408576698