Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2493253002:
[autofill] Add address comparison/merge logic for dependent locality and sorting codes (Closed)
|
Created:
4 years, 1 month ago by Roger McFarlane (Chromium) Modified:
4 years, 1 month ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[autofill] Add address comparison/merge logic for dependent locality and sorting codes
BUG=664540
Committed: https://crrev.com/64fdf583b9fbbe70b65928344713ac07c3e53f35
Cr-Commit-Position: refs/heads/master@{#433647}
Patch Set 1 #
Total comments: 2
Patch Set 2 : moar logging #Patch Set 3 : Found and fixed a latent bug #Patch Set 4 : fix try bots (redux) #
Total comments: 4
Messages
Total messages: 27 (20 generated)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [autofill] Add address comparison/merge logic for dependent locality and sorting codes BUG=664540 R=mathp@chromium.org ========== to ========== [autofill] Add address comparison/merge logic for dependent locality and sorting codes BUG=664540 ==========
PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay, lgtm https://codereview.chromium.org/2493253002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_profile_comparator.cc (right): https://codereview.chromium.org/2493253002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_profile_comparator.cc:548: NOTREACHED(); suggestion: let's be more specific on the NOTREACHED(): NOTREACHED() << "Localities not mergeable " << locality1 << " " << locality2; (Same with other NOTREACHED(). A Debug crash was filed with us the other day and I would have loved to have the values responsible for the DCHECK in the stack trace. What do you think?)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
+sebsg I've made the logging more informative. Along the way, I found and fixed a merge bug for addresses (the wrong address was being retained due to a typo/copy-paste error), depending on the order in which the profiles were passed to the merge function. I've updated the unittests to validate that the merge logic is independent of argument order. Seb, can you take a look, plz.. Thanks. https://codereview.chromium.org/2493253002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_profile_comparator.cc (right): https://codereview.chromium.org/2493253002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_profile_comparator.cc:548: NOTREACHED(); On 2016/11/12 13:02:30, Mathieu Perreault wrote: > suggestion: let's be more specific on the NOTREACHED(): > > NOTREACHED() << "Localities not mergeable " << locality1 << " " << locality2; > > (Same with other NOTREACHED(). A Debug crash was filed with us the other day and > I would have loved to have the values responsible for the DCHECK in the stack > trace. What do you think?) Done. But note that this only applies to dev/debug builds. NOTREACHED() is a DCHECK. https://codereview.chromium.org/2493253002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_comparator.cc (right): https://codereview.chromium.org/2493253002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile_comparator.cc:610: address->SetInfo(kStreetAddress, address2, app_locale_); Latent copy-paste bug. Should have been address2 here. Fixed. https://codereview.chromium.org/2493253002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_comparator_unittest.cc (right): https://codereview.chromium.org/2493253002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile_comparator_unittest.cc:940: TEST_F(AutofillProfileComparatorTest, MergeAddressesMostUniqueTokens) { Added this test to validate the bug fix for the address1 vs address2 merge bug.
rogerm@chromium.org changed reviewers: + sebsg@chromium.org
LGTM! https://codereview.chromium.org/2493253002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_comparator.cc (right): https://codereview.chromium.org/2493253002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile_comparator.cc:610: address->SetInfo(kStreetAddress, address2, app_locale_); On 2016/11/21 18:39:51, Roger McFarlane (Chromium) wrote: > Latent copy-paste bug. Should have been address2 here. Fixed. Good catch :) https://codereview.chromium.org/2493253002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_comparator_unittest.cc (right): https://codereview.chromium.org/2493253002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_profile_comparator_unittest.cc:940: TEST_F(AutofillProfileComparatorTest, MergeAddressesMostUniqueTokens) { On 2016/11/21 18:39:51, Roger McFarlane (Chromium) wrote: > Added this test to validate the bug fix for the address1 vs address2 merge bug. Acknowledged.
Thanks! Awaiting trybot completion...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2493253002/#ps60001 (title: "fix try bots (redux)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479761794401140,
"parent_rev": "5d811c1430641528189460e7eedbae5797638a50", "commit_rev":
"0fce6dfbbfdd2579e410b89679b234dfff0434dd"}
Message was sent while issue was closed.
Description was changed from ========== [autofill] Add address comparison/merge logic for dependent locality and sorting codes BUG=664540 ========== to ========== [autofill] Add address comparison/merge logic for dependent locality and sorting codes BUG=664540 ==========
Message was sent while issue was closed.
Description was changed from ========== [autofill] Add address comparison/merge logic for dependent locality and sorting codes BUG=664540 ========== to ========== [autofill] Add address comparison/merge logic for dependent locality and sorting codes BUG=664540 Committed: https://crrev.com/64fdf583b9fbbe70b65928344713ac07c3e53f35 Cr-Commit-Position: refs/heads/master@{#433647} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/64fdf583b9fbbe70b65928344713ac07c3e53f35 Cr-Commit-Position: refs/heads/master@{#433647} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
