Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
178263004:
rAc - Only show countries we're able to fill in. (Closed)
|
Created:
6 years, 9 months ago by Evan Stade Modified:
6 years, 9 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org, aruslan Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionrAc - Only show countries we're able to fill in.
If there is a country field that's a plain text input, we can fill anything into it, so show all countries.
Likewise, if there's no country field, show all countries.
If there is a limited set of countries in a <select>, only show those in the dialog.
Note that "Use billing for shipping" doesn't respect the shipping country set. It seems highly unlikely that an address would be acceptable for billing but not shipping.
BUG=346440
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256343
Patch Set 1 #Patch Set 2 : checkpoint #Patch Set 3 : . #
Total comments: 38
Patch Set 4 : nits #Patch Set 5 : 500s begone #
Total comments: 6
Patch Set 6 : country codes only #
Total comments: 7
Patch Set 7 : ^H #
Total comments: 2
Patch Set 8 : add FormStructure unit test #
Total comments: 2
Patch Set 9 : fix test #Patch Set 10 : merge #Patch Set 11 : vtable crappola #Patch Set 12 : remove debug line #Patch Set 13 : fix win compile #Patch Set 14 : fix android build #Messages
Total messages: 51 (0 generated)
can haz test in form_structure_unittest.cc also plz? https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1503: } nit: revert https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3276: controller()->Show(); ^ any reason you're not just writing this as a browser test and using HTML? (i.e. SetupAndInvoke() in autofill_dialog_controller_browsertest.cc) https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3292: verified_profile.SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16("US")); ^ can you name this variable us_profile or american_profile? https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:27: const std::set<base::string16> candidate_countries) { nit: const-ref https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:34: if (candidate_countries.empty()) return true; // the rest... https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model_unittest.cc:80: } can you add a test of the country filtering logic here?
https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3276: controller()->Show(); On 2014/03/01 03:38:10, Dan Beam wrote: > ^ any reason you're not just writing this as a browser test and using HTML? > (i.e. SetupAndInvoke() in autofill_dialog_controller_browsertest.cc) unit tests are preferable to browser tests https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model_unittest.cc:80: } On 2014/03/01 03:38:10, Dan Beam wrote: > can you add a test of the country filtering logic here? Adding a test here or in formstructure would not add any more coverage than the test I have already added.
https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3276: controller()->Show(); On 2014/03/03 18:37:00, Evan Stade wrote: > On 2014/03/01 03:38:10, Dan Beam wrote: > > ^ any reason you're not just writing this as a browser test and using HTML? > > (i.e. SetupAndInvoke() in autofill_dialog_controller_browsertest.cc) > > unit tests are preferable to browser tests fine https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model_unittest.cc:80: } On 2014/03/03 18:37:00, Evan Stade wrote: > On 2014/03/01 03:38:10, Dan Beam wrote: > > can you add a test of the country filtering logic here? > > Adding a test here or in formstructure would not add any more coverage than the > test I have already added. fine
https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3747: // Fall back to the first non-suggestion key. I think this code chooses the last non-suggestion key, not the first, since there's no break stmt or anything like that. https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:41: } What happens if the <select> dropdown includes localized country names, and doesn't use country codes for the option values? For example, Wikipedia generally lists each country name as it is written by residents of that country, character set and all. I'd expect that you'd want to extend candidate_countries to contain all of the country codes that we can deduce from existing members of the set. https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.h (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.h:28: const std::set<base::string16>& country_filter, Please document this parameter. https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model_unittest.cc:80: } On 2014/03/03 18:37:00, Evan Stade wrote: > On 2014/03/01 03:38:10, Dan Beam wrote: > > can you add a test of the country filtering logic here? > > Adding a test here or in formstructure would not add any more coverage than the > test I have already added. IMO that means you're testing the wrong things at the wrong levels. Unit tests should test a class's public API. If I make changes to foo.cc and foo_unittests.cc pass, then I should feel pretty confident that the changes to foo.cc were correct. Longer-distance dependencies create confusion and make it less likely that e.g. tests will be updated as the implementation evolves. https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... File components/autofill/core/browser/form_structure.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.cc:1181: field->Type().group() != target_type.group()) { Optional nit: Maybe it makes sense to add an IsEquivalentTo() method to the AutofillType class? This if-stmt requires a bit too much thought to understand, IMO. https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... File components/autofill/core/browser/form_structure.h (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.h:161: std::set<base::string16> PossibleValues(ServerFieldType type); 'ServerFieldType' is a strange choice of type to pass to this method. You probably want an AutofillType instead. https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.h:161: std::set<base::string16> PossibleValues(ServerFieldType type); nit: This method name is rather vague. Perhaps "GetPossibleValuesForFieldType()" or something like that? https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.h:161: std::set<base::string16> PossibleValues(ServerFieldType type); You are expanding the public API of this class, so please add a unit test for this method.
https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1503: } On 2014/03/01 03:38:10, Dan Beam wrote: > nit: revert Done. https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3747: // Fall back to the first non-suggestion key. On 2014/03/03 23:55:54, Ilya Sherman wrote: > I think this code chooses the last non-suggestion key, not the first, since > there's no break stmt or anything like that. that's what the guid->empty() check is for https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3292: verified_profile.SetRawInfo(ADDRESS_HOME_COUNTRY, ASCIIToUTF16("US")); On 2014/03/01 03:38:10, Dan Beam wrote: > ^ can you name this variable us_profile or american_profile? Done. https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:27: const std::set<base::string16> candidate_countries) { On 2014/03/01 03:38:10, Dan Beam wrote: > nit: const-ref good catch, done. https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:34: On 2014/03/01 03:38:10, Dan Beam wrote: > if (candidate_countries.empty()) > return true; > > // the rest... not really an improvement, but done https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:41: } On 2014/03/03 23:55:54, Ilya Sherman wrote: > What happens if the <select> dropdown includes localized country names, and > doesn't use country codes for the option values? For example, Wikipedia > generally lists each country name as it is written by residents of that country, > character set and all. I'd expect that you'd want to extend candidate_countries > to contain all of the country codes that we can deduce from existing members of > the set. what happens is that requestAutocomplete won't work properly. But given that we're creating a new "standard" and authors are coding to our implementation, they should just use the country codes. In fact, I'd be willing to remove the country.name() check altogether. https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.h (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.h:28: const std::set<base::string16>& country_filter, On 2014/03/03 23:55:54, Ilya Sherman wrote: > Please document this parameter. Done. https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model_unittest.cc:80: } On 2014/03/03 23:55:54, Ilya Sherman wrote: > On 2014/03/03 18:37:00, Evan Stade wrote: > > On 2014/03/01 03:38:10, Dan Beam wrote: > > > can you add a test of the country filtering logic here? > > > > Adding a test here or in formstructure would not add any more coverage than > the > > test I have already added. > > IMO that means you're testing the wrong things at the wrong levels. Unit tests > should test a class's public API. If I make changes to foo.cc and > foo_unittests.cc pass, then I should feel pretty confident that the changes to > foo.cc were correct. Longer-distance dependencies create confusion and make it > less likely that e.g. tests will be updated as the implementation evolves. This is what the bots are for. They make sure to catch all relevant failures. If I move the tests to this level, I will still have to leave the tests that are at the AutofillDialogController unit test level. Thus I will be adding more tests for no greater coverage. Adding tests here will (a) take time today (b) take time tomorrow to update when I want to change something (c) test implementation rather than behavior (all we really care about is that the dialog itself works, not the fact it uses CountryComboboxModel to work). https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... File components/autofill/core/browser/form_structure.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.cc:1181: field->Type().group() != target_type.group()) { On 2014/03/03 23:55:54, Ilya Sherman wrote: > Optional nit: Maybe it makes sense to add an IsEquivalentTo() method to the > AutofillType class? This if-stmt requires a bit too much thought to understand, > IMO. Using IsEquivalentTo would just force the next developer to read this code to open a new file to figure out what the heck it actually means. https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... File components/autofill/core/browser/form_structure.h (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.h:161: std::set<base::string16> PossibleValues(ServerFieldType type); On 2014/03/03 23:55:54, Ilya Sherman wrote: > 'ServerFieldType' is a strange choice of type to pass to this method. You > probably want an AutofillType instead. I started with that, but it makes the call sites more verbose. ServerFieldType is expressive enough to describe any type we could want to ask this question about. https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.h:161: std::set<base::string16> PossibleValues(ServerFieldType type); On 2014/03/03 23:55:54, Ilya Sherman wrote: > nit: This method name is rather vague. Perhaps > "GetPossibleValuesForFieldType()" or something like that? Do you think it's vague when you see it in the context of the call site? https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.h:161: std::set<base::string16> PossibleValues(ServerFieldType type); On 2014/03/03 23:55:54, Ilya Sherman wrote: > You are expanding the public API of this class, so please add a unit test for > this method. See my other comment on why I think that is a bad idea.
https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:34: On 2014/03/04 00:11:30, Evan Stade wrote: > On 2014/03/01 03:38:10, Dan Beam wrote: > > if (candidate_countries.empty()) > > return true; > > > > // the rest... > > not really an improvement, but done doesn't construct an AutofillCountry needlessly... https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:41: } On 2014/03/04 00:11:30, Evan Stade wrote: > On 2014/03/03 23:55:54, Ilya Sherman wrote: > > What happens if the <select> dropdown includes localized country names, and > > doesn't use country codes for the option values? For example, Wikipedia > > generally lists each country name as it is written by residents of that > country, > > character set and all. I'd expect that you'd want to extend > candidate_countries > > to contain all of the country codes that we can deduce from existing members > of > > the set. > > what happens is that requestAutocomplete won't work properly. But given that > we're creating a new "standard" and authors are coding to our implementation, > they should just use the country codes. In fact, I'd be willing to remove the > country.name() check altogether. in general i think this was a cool idea, but +1 to just removing name check assuming it makes our code faster/easier to understand and eases cognitive load on web authors.
https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3747: // Fall back to the first non-suggestion key. On 2014/03/04 00:11:30, Evan Stade wrote: > On 2014/03/03 23:55:54, Ilya Sherman wrote: > > I think this code chooses the last non-suggestion key, not the first, since > > there's no break stmt or anything like that. > > that's what the guid->empty() check is for Ah, true dat. Maybe DCHECK at the beginning of this method, then, that the |guid| starts out empty? https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:41: } On 2014/03/04 00:19:50, Dan Beam wrote: > On 2014/03/04 00:11:30, Evan Stade wrote: > > On 2014/03/03 23:55:54, Ilya Sherman wrote: > > > What happens if the <select> dropdown includes localized country names, and > > > doesn't use country codes for the option values? For example, Wikipedia > > > generally lists each country name as it is written by residents of that > > country, > > > character set and all. I'd expect that you'd want to extend > > candidate_countries > > > to contain all of the country codes that we can deduce from existing members > > of > > > the set. > > > > what happens is that requestAutocomplete won't work properly. But given that > > we're creating a new "standard" and authors are coding to our implementation, > > they should just use the country codes. In fact, I'd be willing to remove the > > country.name() check altogether. > > in general i think this was a cool idea, but +1 to just removing name check > assuming it makes our code faster/easier to understand and eases cognitive load > on web authors. IMO we should either only support country codes, or try harder to support localized names. I'd expect the in-between state that this CL puts us in to be confusing to developers, who'd essentially see that some fraction of their countries aren't usable, and then they'd have to debug a black box to figure out why. https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model_unittest.cc:80: } On 2014/03/04 00:11:30, Evan Stade wrote: > On 2014/03/03 23:55:54, Ilya Sherman wrote: > > On 2014/03/03 18:37:00, Evan Stade wrote: > > > On 2014/03/01 03:38:10, Dan Beam wrote: > > > > can you add a test of the country filtering logic here? > > > > > > Adding a test here or in formstructure would not add any more coverage than > > the > > > test I have already added. > > > > IMO that means you're testing the wrong things at the wrong levels. Unit > tests > > should test a class's public API. If I make changes to foo.cc and > > foo_unittests.cc pass, then I should feel pretty confident that the changes to > > foo.cc were correct. Longer-distance dependencies create confusion and make > it > > less likely that e.g. tests will be updated as the implementation evolves. > > This is what the bots are for. They make sure to catch all relevant failures. That's true, though bots are much slower than running the tests locally. > If I move the tests to this level, I will still have to leave the tests that are > at the AutofillDialogController unit test level. Thus I will be adding more > tests for no greater coverage. Adding tests here will (a) take time today (b) > take time tomorrow to update when I want to change something (c) test > implementation rather than behavior (all we really care about is that the dialog > itself works, not the fact it uses CountryComboboxModel to work). The logical extreme of your argument seems to me to be something like "We really only care whether features work, so there's no point to testing intermediate classes, as long as their code paths are tested at the feature level." I suspect you don't really think that, but I honestly don't see much difference between that stance and the narrower one that you're adopting here. IMO "tests should test a class's full public API" is a simpler rule to follow that results in better test coverage overall. Replying to your objections specifically: (a) Yes, that's true. Tests are never free; but IMO it's worth taking the time to write them. (b) This might be true. On the other hand, it might save time tomorrow for a developer who hits a clearer test failure. Again; tests aren't expected to have zero maintenance cost. In this case, I'd expect the maintenance cost of a test for this class to be quite low over the lifetime of the test. (c) I'm asking you to test the *behavior* of the CountryComboboxModel class, which is what is exposed to clients of the class. This has nothing to do with decisions that are private to the implementation of the class. https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... File components/autofill/core/browser/form_structure.h (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.h:161: std::set<base::string16> PossibleValues(ServerFieldType type); On 2014/03/04 00:11:30, Evan Stade wrote: > On 2014/03/03 23:55:54, Ilya Sherman wrote: > > nit: This method name is rather vague. Perhaps > > "GetPossibleValuesForFieldType()" or something like that? > > Do you think it's vague when you see it in the context of the call site? Yes, I do. The call site just forwards a bunch of stuff to a constructor, so the call site is really not very informative at all. Besides, you're adding a public method that might be called from other call sites in the future; so even if the current call site was overwhelmingly clear, that would not mean that future call sites would be. https://chromiumcodereview.appspot.com/178263004/diff/30001/components/autofi... components/autofill/core/browser/form_structure.h:161: std::set<base::string16> PossibleValues(ServerFieldType type); On 2014/03/04 00:11:30, Evan Stade wrote: > On 2014/03/03 23:55:54, Ilya Sherman wrote: > > You are expanding the public API of this class, so please add a unit test for > > this method. > > See my other comment on why I think that is a bad idea. This code lives in a different component from the unittest code that you've added. Soon, it will run as part of a completely separate test binary. If you prefer not to add test coverage to this class, then please move the implementation of this code into the AutofillDialogController's anonymous namespace. In fact, now that I think about it, that's probably a better place for this code regardless -- there's really no need to drop it into the shared code.
https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model_unittest.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/30001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model_unittest.cc:80: } On 2014/03/04 00:53:57, Ilya Sherman wrote: > On 2014/03/04 00:11:30, Evan Stade wrote: > > On 2014/03/03 23:55:54, Ilya Sherman wrote: > > > On 2014/03/03 18:37:00, Evan Stade wrote: > > > > On 2014/03/01 03:38:10, Dan Beam wrote: > > > > > can you add a test of the country filtering logic here? > > > > > > > > Adding a test here or in formstructure would not add any more coverage > than > > > the > > > > test I have already added. > > > > > > IMO that means you're testing the wrong things at the wrong levels. Unit > > tests > > > should test a class's public API. If I make changes to foo.cc and > > > foo_unittests.cc pass, then I should feel pretty confident that the changes > to > > > foo.cc were correct. Longer-distance dependencies create confusion and make > > it > > > less likely that e.g. tests will be updated as the implementation evolves. > > > > This is what the bots are for. They make sure to catch all relevant failures. > > That's true, though bots are much slower than running the tests locally. > > > If I move the tests to this level, I will still have to leave the tests that > are > > at the AutofillDialogController unit test level. Thus I will be adding more > > tests for no greater coverage. Adding tests here will (a) take time today (b) > > take time tomorrow to update when I want to change something (c) test > > implementation rather than behavior (all we really care about is that the > dialog > > itself works, not the fact it uses CountryComboboxModel to work). > > The logical extreme of your argument seems to me to be something like "We really > only care whether features work, so there's no point to testing intermediate > classes, as long as their code paths are tested at the feature level." In fact I do think this, but unit tests are frequently the most expedient way to get complete code coverage. > I > suspect you don't really think that, but I honestly don't see much difference > between that stance and the narrower one that you're adopting here. IMO "tests > should test a class's full public API" is a simpler rule to follow that results > in better test coverage overall. It is a simple rule, but it's not a rule that we follow in Chrome, and it's not one that I would espouse given the extreme cost of doing this. > > Replying to your objections specifically: > (a) Yes, that's true. Tests are never free; but IMO it's worth taking the > time to write them. when there's value to them, I agree with you. > (b) This might be true. On the other hand, it might save time tomorrow for a > developer who hits a clearer test failure. Again; tests aren't expected to have > zero maintenance cost. In this case, I'd expect the maintenance cost of a test > for this class to be quite low over the lifetime of the test. > (c) I'm asking you to test the *behavior* of the CountryComboboxModel class, > which is what is exposed to clients of the class. This has nothing to do with > decisions that are private to the implementation of the class. If you can think of a way to change CountryComboboxModel that will not cause the test that is already written to fail, and which could not be covered by improving the test that's already written, then I'll agree CountryComboboxModel needs more testing.
some nits while you make this country-code-only https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:330: CountryComboboxModel* CountryComboboxModelForSection(DialogSection section); revert https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3113: controller()->SuggestionStateForSection(SECTION_BILLING).visible); ^ what's the advantage to this change? https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3253: controller()->CountryComboboxModelForSection(SECTION_SHIPPING); this can be ui::ComboboxModel* model = controller()->ComboboxModelForType(ADDRESS_HOME_COUNTRY); instead as you only use GetItemCount() and GetItemAt()
https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:330: CountryComboboxModel* CountryComboboxModelForSection(DialogSection section); On 2014/03/05 00:40:03, Dan Beam wrote: > revert Done. https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3113: controller()->SuggestionStateForSection(SECTION_BILLING).visible); On 2014/03/05 00:40:03, Dan Beam wrote: > ^ what's the advantage to this change? it correctly tests the new behavior, which is to show addresses with invalid countries but just make them unselectable https://codereview.chromium.org/178263004/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3253: controller()->CountryComboboxModelForSection(SECTION_SHIPPING); On 2014/03/05 00:40:03, Dan Beam wrote: > this can be > > ui::ComboboxModel* model = > controller()->ComboboxModelForType(ADDRESS_HOME_COUNTRY); > > instead as you only use GetItemCount() and GetItemAt() fair enough
(updated the code to only check for country codes, not country names)
+aruslan FYI
lgtm https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:434: bool CanAcceptCountry(DialogSection section, const std::string& country_code); this looks familiar :) https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:34: ^H https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:41: return true; nit: i think this all could be return candidate_countries.count(base::ASCIIToUTF16(country_code));
https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:34: On 2014/03/05 04:19:16, Dan Beam wrote: > ^H Done. https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:41: return true; On 2014/03/05 04:19:16, Dan Beam wrote: > nit: i think this all could be > > return candidate_countries.count(base::ASCIIToUTF16(country_code)); no, if it's empty, anything goes
https://codereview.chromium.org/178263004/diff/90012/components/autofill/core... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/178263004/diff/90012/components/autofill/core... components/autofill/core/browser/form_structure.cc:1141: } Since you prefer not to add tests to form_structure_unittest.cc, please move this code out of the FormStructure class into the anonymous namespace of the single class implementation that needs it.
slgtm https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... File chrome/browser/ui/autofill/country_combobox_model.cc (right): https://chromiumcodereview.appspot.com/178263004/diff/80001/chrome/browser/ui... chrome/browser/ui/autofill/country_combobox_model.cc:41: return true; On 2014/03/05 04:21:05, Evan Stade wrote: > On 2014/03/05 04:19:16, Dan Beam wrote: > > nit: i think this all could be > > > > return candidate_countries.count(base::ASCIIToUTF16(country_code)); > > no, if it's empty, anything goes nit: if (candidate_countries.empty()) return true; return candidate_countries.count(base::ASCIIToUTF16(country_code));
https://codereview.chromium.org/178263004/diff/80001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/country_combobox_model.cc (right): https://codereview.chromium.org/178263004/diff/80001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/country_combobox_model.cc:41: return true; On 2014/03/05 19:52:12, Dan Beam wrote: > On 2014/03/05 04:21:05, Evan Stade wrote: > > On 2014/03/05 04:19:16, Dan Beam wrote: > > > nit: i think this all could be > > > > > > return candidate_countries.count(base::ASCIIToUTF16(country_code)); > > > > no, if it's empty, anything goes > > nit: > > if (candidate_countries.empty()) > return true; > > return candidate_countries.count(base::ASCIIToUTF16(country_code)); doesn't seem better... I prefer the layout of if (...) return false if (...) return false if (...) return false if (...) return false if (...) return false [...] // OK, all checks passed. return true; https://codereview.chromium.org/178263004/diff/90012/components/autofill/core... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/178263004/diff/90012/components/autofill/core... components/autofill/core/browser/form_structure.cc:1141: } On 2014/03/05 06:09:37, Ilya Sherman wrote: > Since you prefer not to add tests to form_structure_unittest.cc, please move > this code out of the FormStructure class into the anonymous namespace of the > single class implementation that needs it. Don't think I can do that because iOS and Android will likely want this code. Added a unit test.
LGTM w/ comment addressed: https://codereview.chromium.org/178263004/diff/110001/components/autofill/cor... File components/autofill/core/browser/form_structure_unittest.cc (right): https://codereview.chromium.org/178263004/diff/110001/components/autofill/cor... components/autofill/core/browser/form_structure_unittest.cc:2400: EXPECT_EQ(0U, form_structure2.PossibleValues(ADDRESS_BILLING_COUNTRY).size()); You probably need to call ParseFieldTypesFromAutocompleteAttributes() on form_structure2 in order for this check to test what you want it to.
https://codereview.chromium.org/178263004/diff/110001/components/autofill/cor... File components/autofill/core/browser/form_structure_unittest.cc (right): https://codereview.chromium.org/178263004/diff/110001/components/autofill/cor... components/autofill/core/browser/form_structure_unittest.cc:2400: EXPECT_EQ(0U, form_structure2.PossibleValues(ADDRESS_BILLING_COUNTRY).size()); On 2014/03/05 23:30:57, Ilya Sherman wrote: > You probably need to call ParseFieldTypesFromAutocompleteAttributes() on > form_structure2 in order for this check to test what you want it to. ah, yes. Thanks.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/130001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
Hunk #1 succeeded at 623 (offset -10 lines).
Hunk #2 succeeded at 1370 (offset -14 lines).
Hunk #3 succeeded at 2655 (offset -51 lines).
Hunk #4 succeeded at 2810 (offset -51 lines).
Hunk #5 succeeded at 2834 (offset -51 lines).
Hunk #6 FAILED at 2958.
Hunk #7 succeeded at 2915 (offset -52 lines).
Hunk #8 succeeded at 3134 (offset -52 lines).
Hunk #9 succeeded at 3188 (offset -52 lines).
Hunk #10 succeeded at 3680 (offset -61 lines).
1 out of 10 hunks FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc.rej
Patch: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
Index: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index
1ce3532083495eccfe1d023508f9568a4c8daf16..cbbde83192e95e2f70220a4d937d594bad27b37a
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -633,6 +633,15 @@ void AutofillDialogControllerImpl::Show() {
return;
}
+ billing_country_combobox_model_.reset(new CountryComboboxModel(
+ *GetManager(),
+ form_structure_.PossibleValues(ADDRESS_BILLING_COUNTRY),
+ false));
+ shipping_country_combobox_model_.reset(new CountryComboboxModel(
+ *GetManager(),
+ form_structure_.PossibleValues(ADDRESS_HOME_COUNTRY),
+ false));
+
// Log any relevant UI metrics and security exceptions.
GetMetricLogger().LogDialogUiEvent(AutofillMetrics::DIALOG_UI_SHOWN);
@@ -1375,10 +1384,10 @@ ui::ComboboxModel*
AutofillDialogControllerImpl::ComboboxModelForAutofillType(
return &cc_exp_year_combobox_model_;
case ADDRESS_BILLING_COUNTRY:
- return &billing_country_combobox_model_;
+ return billing_country_combobox_model_.get();
case ADDRESS_HOME_COUNTRY:
- return &shipping_country_combobox_model_;
+ return shipping_country_combobox_model_.get();
default:
return NULL;
@@ -2697,8 +2706,6 @@
AutofillDialogControllerImpl::AutofillDialogControllerImpl(
wallet_items_requested_(false),
handling_use_wallet_link_click_(false),
passive_failed_(false),
- billing_country_combobox_model_(*GetManager(), false),
- shipping_country_combobox_model_(*GetManager(), false),
suggested_cc_(this),
suggested_billing_(this),
suggested_cc_billing_(this),
@@ -2854,6 +2861,10 @@ void AutofillDialogControllerImpl::SuggestionsUpdated() {
key,
addresses[i]->DisplayName(),
addresses[i]->DisplayNameDetail());
+ suggested_shipping_.SetEnabled(
+ key,
+ CanAcceptCountry(SECTION_SHIPPING,
+ addresses[i]->country_name_code()));
// TODO(scr): Move this assignment outside the loop or comment why it
// can't be there.
@@ -2874,7 +2885,9 @@ void AutofillDialogControllerImpl::SuggestionsUpdated() {
std::string first_active_instrument_key;
std::string default_instrument_key;
for (size_t i = 0; i < instruments.size(); ++i) {
- bool allowed = IsInstrumentAllowed(*instruments[i]);
+ bool allowed = IsInstrumentAllowed(*instruments[i]) &&
+ CanAcceptCountry(SECTION_BILLING,
+ instruments[i]->address().country_name_code());
gfx::Image icon = instruments[i]->CardIcon();
if (!allowed && !icon.IsEmpty()) {
// Create a grayed disabled icon.
@@ -2945,8 +2958,6 @@ void AutofillDialogControllerImpl::SuggestionsUpdated() {
for (size_t i = 0; i < profiles.size(); ++i) {
const AutofillProfile& profile = *profiles[i];
if (!i18ninput::AddressHasCompleteAndVerifiedData(profile) ||
- !i18ninput::CountryIsFullySupported(
- UTF16ToASCII(profile.GetRawInfo(ADDRESS_HOME_COUNTRY))) ||
(!i18ninput::Enabled() && HasInvalidAddress(*profiles[i]))) {
continue;
}
@@ -2954,9 +2965,19 @@ void AutofillDialogControllerImpl::SuggestionsUpdated() {
// Don't add variants for addresses: name is part of credit card and
// we'll just ignore email and phone number variants.
suggested_shipping_.AddKeyedItem(profile.guid(), labels[i]);
+ suggested_shipping_.SetEnabled(
+ profile.guid(),
+ CanAcceptCountry(
+ SECTION_SHIPPING,
+ UTF16ToUTF8(profile.GetRawInfo(ADDRESS_HOME_COUNTRY))));
if (!profile.GetRawInfo(EMAIL_ADDRESS).empty() &&
!profile.IsPresentButInvalid(EMAIL_ADDRESS)) {
suggested_billing_.AddKeyedItem(profile.guid(), labels[i]);
+ suggested_billing_.SetEnabled(
+ profile.guid(),
+ CanAcceptCountry(
+ SECTION_BILLING,
+ UTF16ToUTF8(profile.GetRawInfo(ADDRESS_HOME_COUNTRY))));
}
}
}
@@ -3163,6 +3184,19 @@ base::string16
AutofillDialogControllerImpl::GetValueFromSection(
return output[type];
}
+bool AutofillDialogControllerImpl::CanAcceptCountry(
+ DialogSection section,
+ const std::string& country_code) {
+ CountryComboboxModel* model = CountryComboboxModelForSection(section);
+ const std::vector<AutofillCountry*>& countries = model->countries();
+ for (size_t i = 0; i < countries.size(); ++i) {
+ if (countries[i] && countries[i]->country_code() == country_code)
+ return true;
+ }
+
+ return false;
+}
+
SuggestionsMenuModel* AutofillDialogControllerImpl::
SuggestionsMenuModelForSection(DialogSection section) {
switch (section) {
@@ -3204,10 +3238,10 @@ DialogSection
AutofillDialogControllerImpl::SectionForSuggestionsMenuModel(
CountryComboboxModel* AutofillDialogControllerImpl::
CountryComboboxModelForSection(DialogSection section) {
if (section == SECTION_BILLING)
- return &billing_country_combobox_model_;
+ return billing_country_combobox_model_.get();
if (section == SECTION_SHIPPING)
- return &shipping_country_combobox_model_;
+ return shipping_country_combobox_model_.get();
return NULL;
}
@@ -3705,9 +3739,13 @@ void
AutofillDialogControllerImpl::GetDefaultAutofillChoice(
// item.
SuggestionsMenuModel* model = SuggestionsMenuModelForSection(section);
for (int i = 0; i < model->GetItemCount(); ++i) {
- if (IsASuggestionItemKey(model->GetItemKeyAt(i))) {
+ // Try the first suggestion item that is enabled.
+ if (IsASuggestionItemKey(model->GetItemKeyAt(i)) && model->IsEnabledAt(i))
{
+ *guid = model->GetItemKeyAt(i);
+ return;
+ // Fall back to the first non-suggestion key.
+ } else if (!IsASuggestionItemKey(model->GetItemKeyAt(i)) && guid->empty())
{
*guid = model->GetItemKeyAt(i);
- break;
}
}
}
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/170001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/190001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/210001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/230001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/230001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/230001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/230001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/178263004/230001
Message was sent while issue was closed.
Change committed as 256343 |
