New issue
Advanced search Search tips

Issue 766226 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Code refactor partially broke M62 UKM for credit card upload save

Project Member Reported by jsaul@google.com, Sep 18 2017

Issue description

This CL accidentally broke a portion of credit card upload UKM in M62:
https://chromium-review.googlesource.com/c/chromium/src/+/582239
Screenshot of the relevant portion:
https://screenshot.googleplex.com/cjaHu5rEYNf.png

The fix is small:
https://chromium-review.googlesource.com/c/chromium/src/+/669770

This bug is for tracking the fix, the merge into M62, and adding whatever unit tests were apparently missing.
 

Comment 1 by jsaul@google.com, Sep 18 2017

Labels: Merge-Request-62
Adding Merge-Request-62 label for https://chromium-review.googlesource.com/c/chromium/src/+/669770
Project Member

Comment 2 by sheriffbot@chromium.org, Sep 18 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Has the change been tested and confirmed in Canary?

Comment 4 by jsaul@google.com, Sep 18 2017

Not yet; it just landed in the code base today.  I'm half-OOO this week but I can try it out as soon as there's a Canary cut with it.
Ah ok - Canary will cut around 8PM PDT tonight. It'd be great if you can test it tomorrow and update this bug. Thanks

Comment 6 by jsaul@google.com, Sep 20 2017

Sorry for the delay!  I tested it out in Canary and the functionality works as expected (as in, it doesn't crash).  The bug was an old if statement that shouldn't have still existed which aborted UKM logging in certain situations, so to completely confirm the fix I'll probably need to investigate today's UKM logs tomorrow.  (I hope my Canary is opted-in to UKM; I'm not sure how to check.)

Comment 7 by jsaul@google.com, Sep 21 2017

I managed to confirm the fix works via chrome://ukm.  I think that's everything from my end; please let me know if you have any further questions!
Labels: -Merge-Review-62 Merge-Approved-62
Approving for M62. Branch:3202
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 21 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cace7aaa9bd36ed6b421d1be1d000c92c50dba8e

commit cace7aaa9bd36ed6b421d1be1d000c92c50dba8e
Author: Jared Saul <jsaul@google.com>
Date: Thu Sep 21 18:11:43 2017

Fix refactor that broke UKM

Bug:  766226 
Change-Id: Iaf53627226d2b9db00aedfaeecabbd3681e2c751
Reviewed-on: https://chromium-review.googlesource.com/669770
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Commit-Queue: Jared Saul <jsaul@google.com>
Cr-Original-Commit-Position: refs/heads/master@{#502600}(cherry picked from commit 79f0fbc4435422ff406a486c7949efeeda78a144)
Reviewed-on: https://chromium-review.googlesource.com/676953
Cr-Commit-Position: refs/branch-heads/3202@{#380}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/cace7aaa9bd36ed6b421d1be1d000c92c50dba8e/components/autofill/core/browser/autofill_manager.cc

Comment 10 by jsaul@google.com, Sep 21 2017

Thanks!  Fix successfully merged.  Leaving this bug open for unit test improvements.
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e6afd312281368575d726055f1468d5bf26b204

commit 6e6afd312281368575d726055f1468d5bf26b204
Author: Jared Saul <jsaul@google.com>
Date: Mon Dec 18 19:31:58 2017

Add missing unit test for UKM breakage

Bug:  766226 
Change-Id: I2b968038ae86fe23af9055c689b6e5491db08a04
Reviewed-on: https://chromium-review.googlesource.com/820639
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Commit-Queue: Jared Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/master@{#524762}
[modify] https://crrev.com/6e6afd312281368575d726055f1468d5bf26b204/components/autofill/core/browser/credit_card_save_manager_unittest.cc

Comment 12 by jsaul@google.com, Dec 18 2017

Status: Fixed (was: Started)

Sign in to add a comment