requestAutocomplete - more elaborate suggestion labels

requestAutocomplete - more elaborate suggestion labels

- more profile info
- updated credit card label
- credit card icon

BUG=157270, 157273
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178783

[Evan Stade, 2013-01-24 02:24:18]

sky for ui/views, isherman for the rest

[Ilya Sherman, 2013-01-24 04:27:27]

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/autofill/cr...
File chrome/browser/autofill/credit_card.cc (right):

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/autofill/cr...
chrome/browser/autofill/credit_card.cc:445: }
This method seems like it should be implemented by the UI code, rather than by
the CreditCard class.

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/autofill/cr...
chrome/browser/autofill/credit_card.cc:467: }
Can AutofillPopupControllerImpl::GetIconResourceID() be updated to use this
method as well, rather than re-defining the mapping?

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:334: return
string16();
When would card be null?  The TODO on line 356 makes it sound like this
shouldn't happen, but there's no similar TODO associated with this early return.

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:352:
profile->GetInfo(ADDRESS_HOME_ZIP, app_locale);
This code is very US-centric.  That's fine for now, but please add a TODO and/or
file a bug to internationalize this code.

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:358: return
string16();
This code doesn't seem to be reachable anymore...

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:369: if (!card)
When is this case reachable?  Can it be a DCHECK instead?

[sky, 2013-01-24 18:10:55]

LGTM

[Evan Stade, 2013-01-24 23:34:24]

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/autofill/cr...
File chrome/browser/autofill/credit_card.cc (right):

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/autofill/cr...
chrome/browser/autofill/credit_card.cc:445: }
On 2013/01/24 04:27:27, Ilya Sherman wrote:
> This method seems like it should be implemented by the UI code, rather than by
> the CreditCard class.

I wanted to keep this close to similar functions, i.e. all those functions which
are dealing with presentation and returning string16.

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/autofill/cr...
chrome/browser/autofill/credit_card.cc:467: }
On 2013/01/24 04:27:27, Ilya Sherman wrote:
> Can AutofillPopupControllerImpl::GetIconResourceID() be updated to use this
> method as well, rather than re-defining the mapping?

I actually spent a while looking at this, but I'd rather not. Eventually we
should be able to delete AutofillPopupControllerImpl's code, and pass around
IDRs for icons instead of string identifiers (string16 is especially weird given
that it's not something that's used for display). So for now, I'd prefer to
leave the code as is rather than adding new code that I plan to delete. I filed
a bug: crbug.com/172101

Currently this mapping exists 4 times in chrome. This patch reduces that to 3.
Fixing the bug I filed will reduce it to 1.

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:334: return
string16();
On 2013/01/24 04:27:27, Ilya Sherman wrote:
> When would card be null?  The TODO on line 356 makes it sound like this
> shouldn't happen, but there's no similar TODO associated with this early
return.

same reason. I just get tired of repeating the same TODO over and over (it
already exists 5 times in this file).

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:352:
profile->GetInfo(ADDRESS_HOME_ZIP, app_locale);
On 2013/01/24 04:27:27, Ilya Sherman wrote:
> This code is very US-centric.  That's fine for now, but please add a TODO
and/or
> file a bug to internationalize this code.

the entire dialog needs localization, so this fits under http://crbug.com/163656

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:358: return
string16();
On 2013/01/24 04:27:27, Ilya Sherman wrote:
> This code doesn't seem to be reachable anymore...

good catch, wonder why the compiler doesn't warn.

https://codereview.chromium.org/12052059/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:369: if (!card)
On 2013/01/24 04:27:27, Ilya Sherman wrote:
> When is this case reachable?  Can it be a DCHECK instead?

it's potentially reachable if the credit card is deleted while the dialog is
showing. I don't think it can be a DCHECK, but it should go away when I fix the
TODO about listening for PDM changes.

[Ilya Sherman, 2013-01-25 00:01:57]

lgtm

[commit-bot: I haz the power, 2013-01-25 00:07:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/12052059/8005

[commit-bot: I haz the power, 2013-01-25 08:04:30]

Change committed as 178783