Form signatures are not stiched to new Autofill API requests |
|||||||||||
Issue descriptionChrome Version: M72 beta OS: all Severity: blocks our feature launch https://bugs.chromium.org/p/chromium/issues/approval?id=903801 Problem: The function CreateApiFormFromLegacyForm (https://cs.chromium.org/chromium/src/components/autofill/core/browser/proto/legacy_proto_bridge.cc) that parses legacy query to API query does not set the form signatures and other fields that enable other prediction features. Fix is a few lines change: https://chromium-review.googlesource.com/c/chromium/src/+/1405696 The change is trivial and does not affect existing users. It is only to fix our Autofill API feature that is gated by a feature flag (that we flip using a Finch experiment). What is the expected result? Requests made to the new Autofill API should include the form signatures in their encoded proto. What happens instead? There are no form signature, so the server is not able to provide the right field predictions.
,
Jan 11
,
Jan 11
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 11
,
Jan 11
,
Jan 11
Have you verified the fix in canary?
,
Jan 11
vincb@: we usually wait until the CL lands and runs in the canary for one day to request a merge. abdulsyed@: I removed the merge request annotation from now, and will add it back once the CL lands. The CL is in the CQ now and should land soon.
,
Jan 11
abdulsyed@, sorry I labeled that bug too quickly... The fix CL has not landed yet, I had to update a test to take the new change. My plan is to test on canary for a day before merging into the release branch.
,
Jan 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/81244830570aabef46b41d83c50f9ba1c60b8cd5 commit 81244830570aabef46b41d83c50f9ba1c60b8cd5 Author: Vincent Boisselle <vincb@chromium.org> Date: Fri Jan 11 15:42:13 2019 Added missing fields to the function parsing a legacy Autofill server request to a new API request. Bug: 920745 Change-Id: I84f55f70e5de729810f293289ddc74cbf6515e86 Reviewed-on: https://chromium-review.googlesource.com/c/1405696 Commit-Queue: Vincent Boisselle <vincb@chromium.org> Reviewed-by: Fabio Tirelo <ftirelo@chromium.org> Cr-Commit-Position: refs/heads/master@{#622014} [modify] https://crrev.com/81244830570aabef46b41d83c50f9ba1c60b8cd5/components/autofill/core/browser/autofill_download_manager_unittest.cc [modify] https://crrev.com/81244830570aabef46b41d83c50f9ba1c60b8cd5/components/autofill/core/browser/proto/api_v1.proto [modify] https://crrev.com/81244830570aabef46b41d83c50f9ba1c60b8cd5/components/autofill/core/browser/proto/legacy_proto_bridge.cc [modify] https://crrev.com/81244830570aabef46b41d83c50f9ba1c60b8cd5/components/autofill/core/browser/proto/legacy_proto_bridge_unittest.cc
,
Jan 11
,
Jan 11
abdulsyed@, the CL has landed, I will let it bake a few days in the canary build. Let me know if you have any concerns.
,
Jan 12
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 14
Thanks vincb@ - how does it look now on canary? I see there is a proto change, adding a new metadata field. Are we sure this won't introduce any potential other regressions? Is this change gated behind a flag? To give more background for why I'm asking this, we're only 2 weeks away from M72 stable (only 2 more betas). We'd like to ensure that all fixes that are landed now are 1) absolutely critical 2) 0 chances of introducing any new regressions.
,
Jan 14
Thanks for feedback abdulsyed@!, I haven't seen any issues with the canary so far. When our "AutofillUseApi" feature is enabled (disabled by default): The metric "Autofill.ServerResponseHasDataForForm" now looks good whereas it was looking bad before this patch. I performed the suite of tests that we use for beta qualification: https://docs.google.com/document/d/1ygSTtCdGsNpgED5aRKLR_F3ifKijX6utSlCek2GnAbo The code change does not touch any code that is not gated by the "AutofillUseApi" feature flag. We are fairly confident (99.99%) that this change will not break stable. This change is not absolutely critical (nobody outside the feature experiment will be broken). Why merging here? We would like 1) fix the broken code to perform beta experiments before using our feature with non-beta traffic (the sooner the better, but without risk), and 2) not have a broken feature in stable Chrome. The next beta branch is in Feb 7th, we can wait up until then if releasers really don't want to see new code in the stable branch that is not **absolutely** critical.
,
Jan 15
Great thank you - since it's behind a flag, approving merge to M72. Branch:3626
,
Jan 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/39b36508bf765dbf9811c5d4c49fe22c46ad8267 commit 39b36508bf765dbf9811c5d4c49fe22c46ad8267 Author: Vincent Boisselle <vincb@chromium.org> Date: Tue Jan 15 19:48:38 2019 Added missing fields to the function parsing a legacy Autofill server request to a new API request. Bug: 920745 Change-Id: I84f55f70e5de729810f293289ddc74cbf6515e86 Reviewed-on: https://chromium-review.googlesource.com/c/1405696 Commit-Queue: Vincent Boisselle <vincb@chromium.org> Reviewed-by: Fabio Tirelo <ftirelo@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#622014}(cherry picked from commit 316fbbd9c2f4617de088549297ddfa7aa5e8e6a8) Reviewed-on: https://chromium-review.googlesource.com/c/1413064 Cr-Commit-Position: refs/branch-heads/3626@{#693} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/39b36508bf765dbf9811c5d4c49fe22c46ad8267/components/autofill/core/browser/autofill_download_manager_unittest.cc [modify] https://crrev.com/39b36508bf765dbf9811c5d4c49fe22c46ad8267/components/autofill/core/browser/proto/api_v1.proto [modify] https://crrev.com/39b36508bf765dbf9811c5d4c49fe22c46ad8267/components/autofill/core/browser/proto/legacy_proto_bridge.cc [modify] https://crrev.com/39b36508bf765dbf9811c5d4c49fe22c46ad8267/components/autofill/core/browser/proto/legacy_proto_bridge_unittest.cc
,
Jan 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/39b36508bf765dbf9811c5d4c49fe22c46ad8267 Commit: 39b36508bf765dbf9811c5d4c49fe22c46ad8267 Author: vincb@chromium.org Commiter: ftirelo@chromium.org Date: 2019-01-15 19:48:38 +0000 UTC Added missing fields to the function parsing a legacy Autofill server request to a new API request. Bug: 920745 Change-Id: I84f55f70e5de729810f293289ddc74cbf6515e86 Reviewed-on: https://chromium-review.googlesource.com/c/1405696 Commit-Queue: Vincent Boisselle <vincb@chromium.org> Reviewed-by: Fabio Tirelo <ftirelo@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#622014}(cherry picked from commit 316fbbd9c2f4617de088549297ddfa7aa5e8e6a8) Reviewed-on: https://chromium-review.googlesource.com/c/1413064 Cr-Commit-Position: refs/branch-heads/3626@{#693} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by vincb@google.com
, Jan 11