Add Delete Support to New Autofill UI

Add Delete Support to New Autofill UI

Adds the backend support for the new Autofill UI so that hitting shift delete with remove Autocomplete entries from the popup and the database.


BUG=51644
TEST=Autocomplete entries can be deleted when using the new Autofill UI


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

[csharp, 2012-04-13 20:19:59]

This only deletes Autocomplete entries. It just closes the popup if you try and
delete an Autofill Profile (which seems to be the same behaviour the current
system has).

Should hitting shift-delete also remove Profiles?

[Ilya Sherman, 2012-04-17 08:31:20]

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete_history_manager.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete_history_manager.cc:228: }
Are the changes in this file just re-ordering the functions, or are there
functionality changes embedded here as well?

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:175: if
(tab_contents_wrapper_) {
nit: Is it actually possible for this to be NULL?

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_external_delegate.h (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_external_delegate.h:67: // Remove the
Autocomplete entry specific from the DB.
nit: What does "specific" mean?

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_popup_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_unittest.cc:37: Show(autofill_values,
autofill_values, autofill_values, autofill_ids, 1);
Hmm, it's not great that there is this long-distance dependency between the
constructor and the tests that expect these particular values to be shown.  It
would be better to do this set up in each test separately (it's not a whole lot
of code), so that the tests are more independent.

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_unittest.cc:128: EXPECT_NE(kNoSelection,
autofill_popup_view_->selected_line());
nit: Perhaps EXPECT_GE(0, ...) so that kNoSelection is not part of the public
contract for the class?  Or, if you prefer to use the constant in the test,
please declare it in a place that both the test code and the regular
implementation can access.

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_view.cc:117: return false;
It should also be impossible to delete a warning.

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_view.cc:122: // If the user attempts to
delete an Autofill entry, just hide the popup.
Ideally we would delete the autofill entry as well.  If that's easy to
implement, it would be great to implement it as part of this CL.

If it's not easy to implement, we should at least have a TODO or a bug filed :) 
Also, I'm not convinced that we really should be hiding the popup if we can't
delete the entry -- I'd prefer to just not handle the event at all.

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/ui/gtk/...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:241: return
(event->state & GDK_SHIFT_MASK) && RemoveSelectedLine();
Does this mean that ctrl-shift-delete and other such shortcuts will also delete
the item?  I'd prefer that shift-delete and nothing else be the keyboard
shortcut for deleting.

[csharp, 2012-04-18 15:35:58]

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete_history_manager.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete_history_manager.cc:228: }
On 2012/04/17 08:31:20, Ilya Sherman wrote:
> Are the changes in this file just re-ordering the functions, or are there
> functionality changes embedded here as well?

All just reordering from moving OnRemoveAutocompleteEntry to be public.

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:175: if
(tab_contents_wrapper_) {
On 2012/04/17 08:31:20, Ilya Sherman wrote:
> nit: Is it actually possible for this to be NULL?

In some tests right now, but I don't think it could happen outside of tests.

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_external_delegate.h (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_external_delegate.h:67: // Remove the
Autocomplete entry specific from the DB.
On 2012/04/17 08:31:20, Ilya Sherman wrote:
> nit: What does "specific" mean?

reworded

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_popup_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_unittest.cc:37: Show(autofill_values,
autofill_values, autofill_values, autofill_ids, 1);
On 2012/04/17 08:31:20, Ilya Sherman wrote:
> Hmm, it's not great that there is this long-distance dependency between the
> constructor and the tests that expect these particular values to be shown.  It
> would be better to do this set up in each test separately (it's not a whole
lot
> of code), so that the tests are more independent.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_unittest.cc:128: EXPECT_NE(kNoSelection,
autofill_popup_view_->selected_line());
On 2012/04/17 08:31:20, Ilya Sherman wrote:
> nit: Perhaps EXPECT_GE(0, ...) so that kNoSelection is not part of the public
> contract for the class?  Or, if you prefer to use the constant in the test,
> please declare it in a place that both the test code and the regular
> implementation can access.

Replaced with EXPECT_LE(0,

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_view.cc:117: return false;
On 2012/04/17 08:31:20, Ilya Sherman wrote:
> It should also be impossible to delete a warning.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_view.cc:122: // If the user attempts to
delete an Autofill entry, just hide the popup.
On 2012/04/17 08:31:20, Ilya Sherman wrote:
> Ideally we would delete the autofill entry as well.  If that's easy to
> implement, it would be great to implement it as part of this CL.
> 
> If it's not easy to implement, we should at least have a TODO or a bug filed
:) 
> Also, I'm not convinced that we really should be hiding the popup if we can't
> delete the entry -- I'd prefer to just not handle the event at all.

K, I dug into the code a bit and got deletion of profiles and credit cards
hooked up.

I also noticed that shift-delete doesn't seem to delete profile in the current
Autofill UI, it just closes the popup. Is this a bug or was this just the old
way to do it?

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/ui/gtk/...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:241: return
(event->state & GDK_SHIFT_MASK) && RemoveSelectedLine();
On 2012/04/17 08:31:20, Ilya Sherman wrote:
> Does this mean that ctrl-shift-delete and other such shortcuts will also
delete
> the item?  I'd prefer that shift-delete and nothing else be the keyboard
> shortcut for deleting.

Fixed. There seem to be other modifiers on by default, I grabbed a fix the other
gtk seems to use.

[Ilya Sherman, 2012-04-18 18:12:50]

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete_history_manager.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete_history_manager.cc:228: }
On 2012/04/18 15:35:58, csharp wrote:
> On 2012/04/17 08:31:20, Ilya Sherman wrote:
> > Are the changes in this file just re-ordering the functions, or are there
> > functionality changes embedded here as well?
> 
> All just reordering from moving OnRemoveAutocompleteEntry to be public.

Ok, thanks for confirming :)

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_external_delegate.cc:175: if
(tab_contents_wrapper_) {
On 2012/04/18 15:35:58, csharp wrote:
> On 2012/04/17 08:31:20, Ilya Sherman wrote:
> > nit: Is it actually possible for this to be NULL?
> 
> In some tests right now, but I don't think it could happen outside of tests.

Ok.  I think it would be better to use mocked objects in tests, rather than
passing NULL, so that we don't risk silently handling non-test failures here in
the future.  What do you think?

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_popup_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_unittest.cc:128: EXPECT_NE(kNoSelection,
autofill_popup_view_->selected_line());
On 2012/04/18 15:35:58, csharp wrote:
> On 2012/04/17 08:31:20, Ilya Sherman wrote:
> > nit: Perhaps EXPECT_GE(0, ...) so that kNoSelection is not part of the
public
> > contract for the class?  Or, if you prefer to use the constant in the test,
> > please declare it in a place that both the test code and the regular
> > implementation can access.
> 
> Replaced with EXPECT_LE(0, 

Sorry, I meant EXPECT_GE(..., 0) -- for the EXPECT_LE-type macros, the more
constant-y number typically goes on the right.

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/ui/gtk/...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:241: return
(event->state & GDK_SHIFT_MASK) && RemoveSelectedLine();
On 2012/04/18 15:35:58, csharp wrote:
> On 2012/04/17 08:31:20, Ilya Sherman wrote:
> > Does this mean that ctrl-shift-delete and other such shortcuts will also
> delete
> > the item?  I'd prefer that shift-delete and nothing else be the keyboard
> > shortcut for deleting.
> 
> Fixed. There seem to be other modifiers on by default, I grabbed a fix the
other
> gtk seems to use.

Ah, that's tricky.  Nice job finding how to do that :)

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_external_delegate.cc:176:
AdjustIndicesAfterDeletion();
nit: This probably makes sense to do in a follow-up CL, but we probably want to
migrate this code to match the changes in [
http://codereview.chromium.org/10024059 ], i.e. to use ids rather than indices
to identify various special entries, as there's less bookkeeping.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_external_delegate.h (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_external_delegate.h:34: static const int
kWarningId = -1;
nit: Let's use the new constants added in the WebKit part of [
http://codereview.chromium.org/10024059 ], i.e.
WebKit::WebAutofillClient::MenuItemIDWarningMessage for warnings.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_manager.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:691: const
std::vector<AutofillProfile*>& profiles = personal_data_->profiles();
nit: indentation?

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:695: size_t variant = 0;
Hmm, I wonder if we should really delete the whole profile if the variant is not
the default one...  More likely, we should just delete that variant.  WDYT?

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:698: return;
nit: I think we can add a NOTREACHED() to this if-stmt.  Does that sound right
to you, or can you think of a legitimate reason why this might fail?

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:701: return;
You already used personal_data_ above... (lines 691 and 692)

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:705: } else if (credit_card) {
nit: No need for the "if" part of the else stmt -- at least one of the two is
guaranteed to be non-null.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:707: }
nit: No need for curly braces.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_manager.h (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.h:94: void
RemoveAutofillProfileOrCreditCard(int unique_id);
nit: Please add a blank line preceding this method declaration, and also brief
comment describing it.  Unlike the other methods here, it does not correspond to
a RVH message, so it probably shouldn't be grouped together with 'em.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:114: }
nit: Perhaps replace this loop with a call to std::remove() (defined in the
<algorithm> header)?

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:126: }
nit: Perhaps replace this loop with a call to std::remove() (defined in the
<algorithm> header)?

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:2973: std::string guid
="00000000-0000-0000-0000-000000000102";
nit: Missing space between the = sign and the quote.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:2983: EXPECT_EQ(NULL,
autofill_manager_->GetProfileWithGUID(guid.c_str()));
nit: I don't think this compiles on all platforms.  You either need to write
this as EXPECT_EQ(static_cast<AutofillProfile*>(NULL), ...), or as
EXPECT_FALSE(...).

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:2987: // Add and remove and
Autofill credit card.
nit: "remove and" -> "remove an"

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_popup_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_popup_unittest.cc:76: // Setup the popup.
nit: "Setup" -> "Set up"

[csharp, 2012-04-19 15:33:24]

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_external_delegate.cc:176:
AdjustIndicesAfterDeletion();
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: This probably makes sense to do in a follow-up CL, but we probably want
to
> migrate this code to match the changes in [
> http://codereview.chromium.org/10024059 ], i.e. to use ids rather than indices
> to identify various special entries, as there's less bookkeeping.

I just added that to this change. Let me remove a few lines without having to
add much.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_external_delegate.h (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_external_delegate.h:34: static const int
kWarningId = -1;
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: Let's use the new constants added in the WebKit part of [
> http://codereview.chromium.org/10024059 ], i.e.
> WebKit::WebAutofillClient::MenuItemIDWarningMessage for warnings.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_manager.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:691: const
std::vector<AutofillProfile*>& profiles = personal_data_->profiles();
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: indentation?

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:695: size_t variant = 0;
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> Hmm, I wonder if we should really delete the whole profile if the variant is
not
> the default one...  More likely, we should just delete that variant.  WDYT?

That sounds good to me. I did a little poking around though and I'm not sure how
to delete just a variant of a profile (I don't see any existing functions that
do that).

I've add a todo and filled a bug (crbug.com/124211). I didn't make myself owner
because I haven't poked around with the internals of Autofill too much. I'd be
willing to tackle it after wrapping up the other UI parts though.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:698: return;
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: I think we can add a NOTREACHED() to this if-stmt.  Does that sound right
> to you, or can you think of a legitimate reason why this might fail?

Seems reasonable. If we get here then so thing has already gone wrong, since the
unique_ids should always be mapping to something.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:701: return;
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> You already used personal_data_ above... (lines 691 and 692)

That's what I get for copy the GetProfileOrCreditCard function call

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:705: } else if (credit_card) {
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: No need for the "if" part of the else stmt -- at least one of the two is
> guaranteed to be non-null.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.cc:707: }
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: No need for curly braces.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_manager.h (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager.h:94: void
RemoveAutofillProfileOrCreditCard(int unique_id);
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: Please add a blank line preceding this method declaration, and also brief
> comment describing it.  Unlike the other methods here, it does not correspond
to
> a RVH message, so it probably shouldn't be grouped together with 'em.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:114: }
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: Perhaps replace this loop with a call to std::remove() (defined in the
> <algorithm> header)?

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:126: }
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: Perhaps replace this loop with a call to std::remove() (defined in the
> <algorithm> header)?

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:2973: std::string guid
="00000000-0000-0000-0000-000000000102";
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: Missing space between the = sign and the quote.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:2983: EXPECT_EQ(NULL,
autofill_manager_->GetProfileWithGUID(guid.c_str()));
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: I don't think this compiles on all platforms.  You either need to write
> this as EXPECT_EQ(static_cast<AutofillProfile*>(NULL), ...), or as
> EXPECT_FALSE(...).

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_unittest.cc:2987: // Add and remove and
Autofill credit card.
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: "remove and" -> "remove an"

Done.

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
File chrome/browser/autofill/autofill_popup_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/8001/chrome/browser/auto...
chrome/browser/autofill/autofill_popup_unittest.cc:76: // Setup the popup.
On 2012/04/18 18:12:51, Ilya Sherman wrote:
> nit: "Setup" -> "Set up"

Done.

[Ilya Sherman, 2012-04-19 21:01:57]

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_external_delegate.cc:38: int list_index) {
nit: It looks like |list_index| is no longer used.

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_external_delegate.cc:114: int separator_index =
ids.size();
Do we still need to keep track of the separator_index, given that we're now
using IDs to identify separators instead?

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:65: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h"
nit: alpha-sort (should come before ui/)

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:706: // http://crbug.com/124211
Hmm, this has the potential for accidental data loss.  I'd prefer that we return
early if the variant is non-zero rather than deleting the whole profile, in the
intermediate phase while we don't support deleting specific variants.

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:47: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h"
nit: alpha-sort

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:112: web_profiles_.end());
nit: Isn't just """std::remove(web_profiles_.begin(), web_profiles_.end(),
profile)""" sufficient?  (Same applies below.)

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:139: --separator_index_;
Do we still need the separator index here, or can we just use the item ids
instead?

[csharp, 2012-04-20 15:03:18]

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_external_delegate.cc:38: int list_index) {
On 2012/04/19 21:01:57, Ilya Sherman wrote:
> nit: It looks like |list_index| is no longer used.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_external_delegate.cc:114: int separator_index =
ids.size();
On 2012/04/19 21:01:57, Ilya Sherman wrote:
> Do we still need to keep track of the separator_index, given that we're now
> using IDs to identify separators instead?

See other comment for reason to keep this.

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:65: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h"
On 2012/04/19 21:01:57, Ilya Sherman wrote:
> nit: alpha-sort (should come before ui/)

Done.

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:706: // http://crbug.com/124211
On 2012/04/19 21:01:57, Ilya Sherman wrote:
> Hmm, this has the potential for accidental data loss.  I'd prefer that we
return
> early if the variant is non-zero rather than deleting the whole profile, in
the
> intermediate phase while we don't support deleting specific variants.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:47: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h"
On 2012/04/19 21:01:57, Ilya Sherman wrote:
> nit: alpha-sort

Done.

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:112: web_profiles_.end());
On 2012/04/19 21:01:57, Ilya Sherman wrote:
> nit: Isn't just """std::remove(web_profiles_.begin(), web_profiles_.end(),
> profile)""" sufficient?  (Same applies below.)

I don't believe so. std::remove (as I understand it), basically moves all the
non deleted elements to the front of the vector, but it doesn't actually change
the length of the vector. However many elements were deleted is how many
undefined elements there are at the end of the vector, so we then delete them.

I could be wrong in this understanding, but I did try just
"std:remove*web_profiles_.begin(), web_profiles_.end(), profile)" and it didn't
work.

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:139: --separator_index_;
On 2012/04/19 21:01:57, Ilya Sherman wrote:
> Do we still need the separator index here, or can we just use the item ids
> instead?

The only reason to keep the separator index is so that the drawing code knows
where it needs to draw the separator line.

We could modify the drawing code to figure out where to draw it on its own, but
since ever UI will need it I feel that its worth keeping it in.

[Ilya Sherman, 2012-04-20 21:30:47]

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:112: web_profiles_.end());
On 2012/04/20 15:03:18, csharp wrote:
> On 2012/04/19 21:01:57, Ilya Sherman wrote:
> > nit: Isn't just """std::remove(web_profiles_.begin(), web_profiles_.end(),
> > profile)""" sufficient?  (Same applies below.)
> 
> I don't believe so. std::remove (as I understand it), basically moves all the
> non deleted elements to the front of the vector, but it doesn't actually
change
> the length of the vector. However many elements were deleted is how many
> undefined elements there are at the end of the vector, so we then delete them.
> 
> I could be wrong in this understanding, but I did try just
> "std:remove*web_profiles_.begin(), web_profiles_.end(), profile)" and it
didn't
> work.

Ah, I think you're right -- sorry 'bout that.  I guess that actually makes sense
given that std::remove is supposed to work with raw arrays as well...

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:139: --separator_index_;
On 2012/04/20 15:03:18, csharp wrote:
> On 2012/04/19 21:01:57, Ilya Sherman wrote:
> > Do we still need the separator index here, or can we just use the item ids
> > instead?
> 
> The only reason to keep the separator index is so that the drawing code knows
> where it needs to draw the separator line.
> 
> We could modify the drawing code to figure out where to draw it on its own,
but
> since ever UI will need it I feel that its worth keeping it in.
> 

In order to support DataList entries -- which is what [
https://chromiumcodereview.appspot.com/10024059 ] added to the renderer-based
popup -- we'll need to be able to support multiple separators.  So, long-term,
we won't be able to use the separator_index anyway; given that, I think we might
as well go ahead and drop it here.

https://chromiumcodereview.appspot.com/10073018/diff/25001/chrome/browser/aut...
File chrome/browser/autofill/autofill_external_delegate.h (right):

https://chromiumcodereview.appspot.com/10073018/diff/25001/chrome/browser/aut...
chrome/browser/autofill/autofill_external_delegate.h:37: // TODO(jrg): add
feedback mechanism for hover on relevant platforms.
nit: Is this TODO still relevant, or has this been addressed?

https://chromiumcodereview.appspot.com/10073018/diff/25001/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/25001/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:2972: GUIDPair
guid_pair(guid, 0);
nit: Might be worth adding a test-case that verifies that currently deleting a
profile with a non-zero variant is a no-op (and including a comment explaining
that the test should be updated once the corresponding bug is fixed).

[csharp, 2012-04-24 14:18:48]

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/11003/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:139: --separator_index_;
On 2012/04/20 21:30:48, Ilya Sherman wrote:
> On 2012/04/20 15:03:18, csharp wrote:
> > On 2012/04/19 21:01:57, Ilya Sherman wrote:
> > > Do we still need the separator index here, or can we just use the item ids
> > > instead?
> > 
> > The only reason to keep the separator index is so that the drawing code
knows
> > where it needs to draw the separator line.
> > 
> > We could modify the drawing code to figure out where to draw it on its own,
> but
> > since ever UI will need it I feel that its worth keeping it in.
> > 
> 
> In order to support DataList entries -- which is what [
> https://chromiumcodereview.appspot.com/10024059 ] added to the renderer-based
> popup -- we'll need to be able to support multiple separators.  So, long-term,
> we won't be able to use the separator_index anyway; given that, I think we
might
> as well go ahead and drop it here.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/25001/chrome/browser/aut...
File chrome/browser/autofill/autofill_external_delegate.h (right):

https://chromiumcodereview.appspot.com/10073018/diff/25001/chrome/browser/aut...
chrome/browser/autofill/autofill_external_delegate.h:37: // TODO(jrg): add
feedback mechanism for hover on relevant platforms.
On 2012/04/20 21:30:48, Ilya Sherman wrote:
> nit: Is this TODO still relevant, or has this been addressed?

This has been addressed for Linux, so I'll remove the todo (other platforms have
a lot of things they'll need to implement and they aren't marked with todos).

https://chromiumcodereview.appspot.com/10073018/diff/25001/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/25001/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:2972: GUIDPair
guid_pair(guid, 0);
On 2012/04/20 21:30:48, Ilya Sherman wrote:
> nit: Might be worth adding a test-case that verifies that currently deleting a
> profile with a non-zero variant is a no-op (and including a comment explaining
> that the test should be updated once the corresponding bug is fixed).

Done.

[Ilya Sherman, 2012-04-24 20:08:12]

https://chromiumcodereview.appspot.com/10073018/diff/35001/chrome/browser/aut...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/35001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:142: if (autofill_values_.size()
== 0 || !CanDelete(autofill_unique_ids_[0]))
I don't think this check will quite make sense once we include DataList
suggestions, since they might be above Autofill suggestions.

https://chromiumcodereview.appspot.com/10073018/diff/35001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:150: autofill_unique_ids_[index]
< 0);
Hmm, why isn't this just "return autofill_unique_ids[index] ==
WebAutofillClient::MenuItemIDSeparator"?

https://chromiumcodereview.appspot.com/10073018/diff/35001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:156: id !=
WebAutofillClient::MenuItemIDAutofillOptions;
Since the full set of ids might grow over time, it's probably better to check
for equality to deletable types rather than inequality to non-deletable types
here.

[csharp, 2012-04-25 15:05:09]

https://chromiumcodereview.appspot.com/10073018/diff/35001/chrome/browser/aut...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/35001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:142: if (autofill_values_.size()
== 0 || !CanDelete(autofill_unique_ids_[0]))
On 2012/04/24 20:08:12, Ilya Sherman wrote:
> I don't think this check will quite make sense once we include DataList
> suggestions, since they might be above Autofill suggestions.

Done.

https://chromiumcodereview.appspot.com/10073018/diff/35001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:150: autofill_unique_ids_[index]
< 0);
On 2012/04/24 20:08:12, Ilya Sherman wrote:
> Hmm, why isn't this just "return autofill_unique_ids[index] ==
> WebAutofillClient::MenuItemIDSeparator"?

WebAutofillClient::MenuItemIDsperator is only set in the old Autofill UI right
now. I've added a bug to change this to use that id since its more work than I'd
like to add to this change as it would introduce an elements that doesn't appear
as a full row when drawing the popup.

MenuItemIDSeparator isn't set for any of the lists passed to the new Autofill UI
right now, only

https://chromiumcodereview.appspot.com/10073018/diff/35001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:156: id !=
WebAutofillClient::MenuItemIDAutofillOptions;
On 2012/04/24 20:08:12, Ilya Sherman wrote:
> Since the full set of ids might grow over time, it's probably better to check
> for equality to deletable types rather than inequality to non-deletable types
> here.

Done.

[Ilya Sherman, 2012-04-25 22:19:51]

LGTM

https://chromiumcodereview.appspot.com/10073018/diff/51001/chrome/browser/aut...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/51001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:155: return id >= 0 ||
nit: Please write this as "id > 0 || id ==
WebAutofillClient::MenuItemIDAutocompleteEntry"

https://chromiumcodereview.appspot.com/10073018/diff/51001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:161: (autofill_unique_ids_[0] >=0
||
nit: Ditto

[commit-bot: I haz the power, 2012-04-26 12:47:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10073018/57001

[commit-bot: I haz the power, 2012-04-26 12:47:30]

Presubmit check for 10073018-57001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/gtk

Presubmit checks took 4.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[csharp, 2012-04-26 13:07:24]

Elliot, can you take a look at the small gtk changes? Thanks

https://chromiumcodereview.appspot.com/10073018/diff/51001/chrome/browser/aut...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10073018/diff/51001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:155: return id >= 0 ||
On 2012/04/25 22:19:51, Ilya Sherman wrote:
> nit: Please write this as "id > 0 || id ==
> WebAutofillClient::MenuItemIDAutocompleteEntry"

Done.

https://chromiumcodereview.appspot.com/10073018/diff/51001/chrome/browser/aut...
chrome/browser/autofill/autofill_popup_view.cc:161: (autofill_unique_ids_[0] >=0
||
On 2012/04/25 22:19:51, Ilya Sherman wrote:
> nit: Ditto

Done.

[Elliot Glaysher, 2012-05-01 18:14:59]

gtk lgtm

[commit-bot: I haz the power, 2012-05-01 18:26:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10073018/57001

[commit-bot: I haz the power, 2012-05-01 18:26:50]

Can't apply patch for file
chrome/browser/autofill/autofill_external_delegate.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/autofill/autofill_external_delegate.cc
Hunk #2 FAILED at 31.
Hunk #3 succeeded at 83 (offset 1 line).
Hunk #4 succeeded at 116 (offset 1 line).
Hunk #5 succeeded at 128 (offset 1 line).
Hunk #6 succeeded at 156 (offset 1 line).
Hunk #7 succeeded at 184 (offset 1 line).
Hunk #8 succeeded at 197 (offset 2 lines).
Hunk #9 succeeded at 220 (offset -2 lines).
1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/autofill/autofill_external_delegate.cc.rej

[commit-bot: I haz the power, 2012-05-01 19:46:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10073018/64024

[commit-bot: I haz the power, 2012-05-01 20:13:57]

Try job failure for 10073018-64024 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-01 20:22:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10073018/64024

[commit-bot: I haz the power, 2012-05-01 20:34:13]

Try job failure for 10073018-64024 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-02 13:13:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10073018/64024

[commit-bot: I haz the power, 2012-05-02 14:30:21]

Change committed as 134909