allow wallet items to appear in requestAutocomplete UI

allow wallet items to appear in requestAutocomplete UI

this CL adds a glue interface so that AutofillDialogController can be more agnostic about where its data is coming from.

BUG=174937
TBR=thestig@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182348

[Evan Stade, 2013-02-12 02:49:43]

isherman: review
dbeam, ahutter: FYI

this depends on dbeam's https://codereview.chromium.org/12208070/

Thus far, I have only tested that it works for the wallet error case (as I wait
to join sugar-v2-bug-basher), and it doesn't break the Autofill case.

[Dan Beam, 2013-02-12 03:00:52]

On 2013/02/12 02:49:43, Evan Stade wrote:
> isherman: review
> dbeam, ahutter: FYI
> 
> this depends on dbeam's https://codereview.chromium.org/12208070/

Link overflow: Maximum click count exceeded (it's a link back to this CL).

> 
> Thus far, I have only tested that it works for the wallet error case (as I
wait
> to join sugar-v2-bug-basher), and it doesn't break the Autofill case.

[Evan Stade, 2013-02-12 03:21:27]

should have been: https://codereview.chromium.org/12221040/

[ahutter, 2013-02-12 15:46:42]

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

https://codereview.chromium.org/12208070/diff/4002/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:399: string16
label = wrapper->GetInfo(NAME_FULL) +
Are we sure this way of displaying addresses localizes well?   For instance
here's an address in France: 1 Rue Jules Verne, 22360 Langueux, France

[Evan Stade, 2013-02-12 18:10:22]

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

https://codereview.chromium.org/12208070/diff/4002/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:399: string16
label = wrapper->GetInfo(NAME_FULL) +
On 2013/02/12 15:46:42, ahutter wrote:
> Are we sure this way of displaying addresses localizes well?   For instance
> here's an address in France: 1 Rue Jules Verne, 22360 Langueux, France 

it definitely doesn't.
https://code.google.com/p/chromium/issues/detail?id=163656 has not been started
yet.

[Evan Stade, 2013-02-12 18:28:15]

although there are rough edges (e.g. lack of spinner while wallet data is
loading) and lots of TODOs, I've confirmed this works for displaying wallet
data.

[Evan Stade, 2013-02-12 22:26:39]

ping isherman

[Ilya Sherman, 2013-02-12 22:50:34]

LGTM with nits:

https://codereview.chromium.org/12208070/diff/9/chrome/browser/autofill/walle...
File chrome/browser/autofill/wallet/wallet_address.cc (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/autofill/walle...
chrome/browser/autofill/wallet/wallet_address.cc:71: string16
Address::GetInfo(AutofillFieldType type) const {
Optional nit: Mebbe add a DCHECK that the type is an address type?

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:402: NOTREACHED();
Optional nit: IMO these two lines would be clearer as

bool success = base::StringToInt(item_key, &index);
DCHECK(success);

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:405: return new
WalletInstrumentWrapper(wallet_items_->instruments()[index]);
nit: Is it worth DCHECK'ing that the index is in bounds?

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:922:
PersonalDataManager* manager = GetManager();
nit: Move this into the else stmt?

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:156: const
AutofillField& field)> InputFieldComparator;
nit: Remove this, since it's now in autofill_dialog_types?

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:188:
DataModelWrapper* CreateWrapper(DialogSection section);
Please return a scoped_ptr<>

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_types.h:11: #include
"base/callback.h"
nit: callback_forward

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.cc:138: for (size_t j = 0; j <
inputs->size(); ++j)
Optional nit: I prefer always using curly braces to enclose loop bodies.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.cc:154: }
This implementation looks almost identical to
AutofillDataModelWrapper::FillFormStructure().  How about moving most of the
code into the shared base class, and defining a single method
FillFormField(AutofillField* field) that the subclasses override for their
custom functionality?

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.h:91: class
WalletInstrumentWrapper : public DataModelWrapper {
Optional nit: IMO it would be better to order this class after the
AddressWrapper one, for closer symmetry to the Autofill class ordering.

[Evan Stade, 2013-02-13 00:11:58]

https://codereview.chromium.org/12208070/diff/9/chrome/browser/autofill/walle...
File chrome/browser/autofill/wallet/wallet_address.cc (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/autofill/walle...
chrome/browser/autofill/wallet/wallet_address.cc:71: string16
Address::GetInfo(AutofillFieldType type) const {
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> Optional nit: Mebbe add a DCHECK that the type is an address type?

Done.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:402: NOTREACHED();
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> Optional nit: IMO these two lines would be clearer as
> 
> bool success = base::StringToInt(item_key, &index);
> DCHECK(success);

Done.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:405: return new
WalletInstrumentWrapper(wallet_items_->instruments()[index]);
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> nit: Is it worth DCHECK'ing that the index is in bounds?

IMO no, because it's already self-evident that we expect the index to be in
bounds.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:922:
PersonalDataManager* manager = GetManager();
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> nit: Move this into the else stmt?

Done.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:156: const
AutofillField& field)> InputFieldComparator;
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> nit: Remove this, since it's now in autofill_dialog_types?

Done.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:188:
DataModelWrapper* CreateWrapper(DialogSection section);
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> Please return a scoped_ptr<>

Done.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_types.h:11: #include
"base/callback.h"
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> nit: callback_forward

Done.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.cc:138: for (size_t j = 0; j <
inputs->size(); ++j)
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> Optional nit: I prefer always using curly braces to enclose loop bodies.

me too. Done.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.cc:154: }
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> This implementation looks almost identical to
> AutofillDataModelWrapper::FillFormStructure().  How about moving most of the
> code into the shared base class, and defining a single method
> FillFormField(AutofillField* field) that the subclasses override for their
> custom functionality?

Done.

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://codereview.chromium.org/12208070/diff/9/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.h:91: class
WalletInstrumentWrapper : public DataModelWrapper {
On 2013/02/12 22:50:34, Ilya Sherman wrote:
> Optional nit: IMO it would be better to order this class after the
> AddressWrapper one, for closer symmetry to the Autofill class ordering.

Done.

[Evan Stade, 2013-02-13 23:36:17]

TBR'ing Lei for gypi change.

[Lei Zhang, 2013-02-13 23:38:49]

.gypi lgtm

[Dan Beam, 2013-02-15 02:11:45]

https://codereview.chromium.org/12208070/diff/19003/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12208070/diff/19003/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:397: bool success
= !base::StringToInt(item_key, &index);
shouldn't this be without the "!" ?

[Evan Stade, 2013-02-20 00:03:38]

https://codereview.chromium.org/12208070/diff/19003/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12208070/diff/19003/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:397: bool success
= !base::StringToInt(item_key, &index);
On 2013/02/15 02:11:45, Dan Beam wrote:
> shouldn't this be without the "!" ?

yes