Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
157093002:
rAc - hardcode wallet billing address to US in i18n mode (Closed)
|
Created:
6 years, 10 months ago by Evan Stade Modified:
6 years, 10 months ago CC:
chromium-reviews, benquan, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionrAc - hardcode wallet billing address to US in i18n mode
BUG=341586
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251288
Patch Set 1 #
Total comments: 13
Patch Set 2 : else #
Total comments: 2
Patch Set 3 : fix dcheck #Patch Set 4 : tests find bugs, me fix bugs, tests have bugs, me fix tests #
Total comments: 10
Patch Set 5 : fix test #
Total comments: 2
Patch Set 6 : more on test #Patch Set 7 : sync #
Messages
Total messages: 29 (0 generated)
https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_common.cc (right): https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_common.cc:25: namespace { const char kHardcodedCountryCode[] = "US"; } https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_common.cc:204: case SECTION_CC_BILLING: ^ in general I'd prefer to minimize the amount of assumptions we make that SECTION_CC_BILLING == Wallet but I guess we're a ways from needing to break that assumption now... https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_common.cc:216: base::ASCIIToUTF16(hardcoded_country_code); s/hardcoded_country_code/kHardcodedCountryCode/g https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_common.cc:281: AutofillCountry country("US", g_browser_process->GetApplicationLocale()); s/"US"/kHardcodedCountryCode/ https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1164: } } else { https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1168: if (!i18ninput::Enabled()) ^ remove
https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_common.cc (right): https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_common.cc:25: On 2014/02/07 01:30:44, Dan Beam wrote: > namespace { > const char kHardcodedCountryCode[] = "US"; > } why? https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_common.cc:204: case SECTION_CC_BILLING: On 2014/02/07 01:30:44, Dan Beam wrote: > ^ in general I'd prefer to minimize the amount of assumptions we make that > SECTION_CC_BILLING == Wallet but I guess we're a ways from needing to break that > assumption now... Wallet is the only reason SECTION_CC_BILLING even exists. It should really be called SECTION_WALLET_BILLING.
https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_common.cc (right): https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_common.cc:25: On 2014/02/07 01:52:43, Evan Stade wrote: > On 2014/02/07 01:30:44, Dan Beam wrote: > > namespace { > > const char kHardcodedCountryCode[] = "US"; > > } > > why? because of all the other comments in this file. alternatively, why not just inline "US" then? it's shorter... https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_common.cc:204: case SECTION_CC_BILLING: On 2014/02/07 01:52:43, Evan Stade wrote: > On 2014/02/07 01:30:44, Dan Beam wrote: > > ^ in general I'd prefer to minimize the amount of assumptions we make that > > SECTION_CC_BILLING == Wallet but I guess we're a ways from needing to break > that > > assumption now... > > Wallet is the only reason SECTION_CC_BILLING even exists. It should really be > called SECTION_WALLET_BILLING. it's not a big deal until we encounter another data source that requires combined billing (which we have no current prospects to do)
https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_common.cc (right): https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_common.cc:25: On 2014/02/07 03:33:23, Dan Beam wrote: > On 2014/02/07 01:52:43, Evan Stade wrote: > > On 2014/02/07 01:30:44, Dan Beam wrote: > > > namespace { > > > const char kHardcodedCountryCode[] = "US"; > > > } > > > > why? > > because of all the other comments in this file. alternatively, why not just > inline "US" then? it's shorter... ah, I missed the last one. I don't think we need to share the constant in these two cases as they are distinct. One is because we were previously technologically unable to accept i18n input. The other is for a business reason (Wallet can't offer the service outside of the US). Therefore they are unlikely to be updated at the same time; sharing a constant would falsely suggest otherwise. The reason I didn't inline "US" is because a) I was trying to make it harder to break the code (two different inlines would make it easier to update L211 without also updating L216) b) the variable name helps document its purpose. but if the new code only used "US" once I probably would have just inlined it.
lgtm w/last 2 nits addressed
The CQ bit was checked by estade@chromium.org
https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1164: } On 2014/02/07 01:30:44, Dan Beam wrote: > } else { Done. https://codereview.chromium.org/157093002/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1168: if (!i18ninput::Enabled()) On 2014/02/07 01:30:44, Dan Beam wrote: > ^ remove Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/157093002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
https://codereview.chromium.org/157093002/diff/300001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_dialog_common.cc (right): https://codereview.chromium.org/157093002/diff/300001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_common.cc:213: DCHECK_EQ(inputs->back().type, ADDRESS_HOME_COUNTRY); ^ did you mean ADDRESS_BILLING_COUNTRY?
https://codereview.chromium.org/157093002/diff/300001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_dialog_common.cc (right): https://codereview.chromium.org/157093002/diff/300001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_common.cc:213: DCHECK_EQ(inputs->back().type, ADDRESS_HOME_COUNTRY); On 2014/02/08 03:43:09, Dan Beam wrote: > ^ did you mean ADDRESS_BILLING_COUNTRY? probably
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/157093002/510002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
there were bugs found by the tests, PTAL
https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc (right): https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1444: // Select "Add new billing/shipping address...". ^ update https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1469: view->SetTextContentsOfInput(ADDRESS_BILLING_COUNTRY, ASCIIToUTF16("China")); view->ActivateInput(ADDRESS_BILLING_COUNTRY); https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1471: view->GetTextContentsOfInput(ADDRESS_BILLING_COUNTRY)); ^ I don't see much value to this EXPECT_EQ() call (on L1470-1471) https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1475: view->GetTextContentsOfInput(ADDRESS_BILLING_COUNTRY)); please verify China/DEPENDENT_LOCALITY are still present in shipping after switching from Autofill -> Wallet https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3220: bool should_clobber) { #docwelcome
https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc (right): https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1444: // Select "Add new billing/shipping address...". On 2014/02/11 03:16:07, Dan Beam wrote: > ^ update Done. https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1469: view->SetTextContentsOfInput(ADDRESS_BILLING_COUNTRY, ASCIIToUTF16("China")); On 2014/02/11 03:16:07, Dan Beam wrote: > view->ActivateInput(ADDRESS_BILLING_COUNTRY); Done. https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1471: view->GetTextContentsOfInput(ADDRESS_BILLING_COUNTRY)); On 2014/02/11 03:16:07, Dan Beam wrote: > ^ I don't see much value to this EXPECT_EQ() call (on L1470-1471) making sure the SetTextContentsOfInput call actually worked. In fact this check just caught a bug in the test, which is that L1468 should have said SECTION_BILLING. https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1475: view->GetTextContentsOfInput(ADDRESS_BILLING_COUNTRY)); On 2014/02/11 03:16:07, Dan Beam wrote: > please verify China/DEPENDENT_LOCALITY are still present in shipping after > switching from Autofill -> Wallet Done. https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/157093002/diff/890001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3220: bool should_clobber) { On 2014/02/11 03:16:07, Dan Beam wrote: > #docwelcome Done.
https://codereview.chromium.org/157093002/diff/1170001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc (right): https://codereview.chromium.org/157093002/diff/1170001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1472: view->GetTextContentsOfInput(ADDRESS_BILLING_COUNTRY)); \n https://codereview.chromium.org/157093002/diff/1170001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1480: view->GetTextContentsOfInput(ADDRESS_HOME_COUNTRY)); EXPECT_FALSE( SectionHasField(SECTION_CC_BILLING, ADDRESS_BILLING_DEPENDENT_LOCALITY)); EXPECT_TRUE( SectionHasField(SECTION_SHIPPING, ADDRESS_HOME_DEPENDENT_LOCALITY));
lgtm
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/157093002/1270001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_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/157093002/1600001
Message was sent while issue was closed.
Change committed as 251288 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
