Add metrics for missing form managers |
||
Issue descriptionPasswordManager needs to look up corresponding (New)PasswordFormManagers at various times. Most failure cases are not reported. It would be useful to have such reporting to monitor regressions during the launch of new form parser. Design: https://docs.google.com/document/d/1aEWWzDpna4nTIhIF8PGbQmaKWoQ9sjyDg2BMOa6uBoY/edit?usp=sharing
,
Nov 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/957985bad00008123811e51d68dfc6648ced3a6c commit 957985bad00008123811e51d68dfc6648ced3a6c Author: Vaclav Brozek <vabr@chromium.org> Date: Mon Nov 26 10:39:41 2018 Avoid leaking unique_ptr args in PasswordManagerTest Due to issues with moveable arguments in GMock, the password_manager_unittest.cc uses the technique where instead of mocking a method with a std::unique_ptr argument, it forwards the raw pointer of that argument to a *Ptr variant of the original method and mocks that. This works as long as the expectations on the *Ptr method are always specified and handle freeing the argument properly. Assuming that every EXPECT_CALL on the *Ptr method is handling the argument correctly, there are two ways to complete the protection against leaks: (A) Make the whole mocked object a StrictMock. (B) Provide an ON_CALL default which deletes the raw pointer argument This CL goes with (B) because it does not take away the flexibility to use normal or NiceMock for the other mocked methods. Bug: 907794 Change-Id: I498c2cb4fd69c232406baad41278fab4defc5ac9 Reviewed-on: https://chromium-review.googlesource.com/c/1349656 Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#610803} [modify] https://crrev.com/957985bad00008123811e51d68dfc6648ced3a6c/components/password_manager/core/browser/password_manager_unittest.cc
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/346f21451cee758142d1ddef425de8f8bd0bb0b4 commit 346f21451cee758142d1ddef425de8f8bd0bb0b4 Author: Vaclav Brozek <vabr@chromium.org> Date: Tue Nov 27 08:08:13 2018 Add UKM helper in password_manager_unittest.cc Checking for values in UKM metrics is a lot of boilerplate in the password manager unittest, so this CL adds two helper methods for brevity. Bug: 907794 Change-Id: Iecf86eb564f9f0f239e7b337725a9666f59d56d4 Reviewed-on: https://chromium-review.googlesource.com/c/1350959 Commit-Queue: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#611039} [modify] https://crrev.com/346f21451cee758142d1ddef425de8f8bd0bb0b4/components/password_manager/core/browser/password_manager_unittest.cc
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a13c906dccbe17a8934e5d97826b7cce073e564 commit 0a13c906dccbe17a8934e5d97826b7cce073e564 Author: Vaclav Brozek <vabr@chromium.org> Date: Tue Nov 27 09:19:23 2018 Fix PasswordManagerTest.ParsingOnSavingMetricRecorded The test currently passes by accident: it fails to check that a metric has a value. It also has two issues: one is mixing null and non-null drivers, and the other is calling old-parser-only ProvisionallySavePassword instead of OnPasswordFormSubmitted, while at the same time using the new parser. This CL fixes the test: * Activates the check, and * Fixes the issues, so that the test passes. Bug: 907794 Change-Id: Ia24518aa2bae3c0a25db7722c89eb59f70c61ef1 Reviewed-on: https://chromium-review.googlesource.com/c/1350898 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#611051} [modify] https://crrev.com/0a13c906dccbe17a8934e5d97826b7cce073e564/components/password_manager/core/browser/password_manager_unittest.cc
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/136459657a1f02c540322d30b1a160260c095123 commit 136459657a1f02c540322d30b1a160260c095123 Author: Vaclav Brozek <vabr@chromium.org> Date: Tue Nov 27 14:48:57 2018 New UKM: PageWithPassword.FormManagerAvailable This CL introduces a new UKM for measuring success of finding corresponding password form managers during the stages of saving a password form. There are three stages during which one of four values is reported. The reporting only takes place if the page has at least one password form, and only one value per page load is reported. Privacy review of this metric: The whole UKM event "PageWithPassword" has been reviewed in crbug.com/728707 and go/gkmnc, where the privacy TL agreed that adding new metrics for this event is OK under certain conditions (see the linked doc). The new metric satisfies such conditions, in the opinion of the CL author, and hence are covered by that review. The new metrics have been added to the requested spreadsheet listing all passwords-related metrics. Bug: 907794 Change-Id: I3cdcffeea9546474acd71ee85f74f4d2e60a6a2e Reviewed-on: https://chromium-review.googlesource.com/c/1348043 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Cr-Commit-Position: refs/heads/master@{#611101} [modify] https://crrev.com/136459657a1f02c540322d30b1a160260c095123/components/password_manager/core/browser/password_manager.cc [modify] https://crrev.com/136459657a1f02c540322d30b1a160260c095123/components/password_manager/core/browser/password_manager_metrics_recorder.cc [modify] https://crrev.com/136459657a1f02c540322d30b1a160260c095123/components/password_manager/core/browser/password_manager_metrics_recorder.h [modify] https://crrev.com/136459657a1f02c540322d30b1a160260c095123/components/password_manager/core/browser/password_manager_unittest.cc [modify] https://crrev.com/136459657a1f02c540322d30b1a160260c095123/tools/metrics/ukm/ukm.xml
,
Nov 27
One more minor fix of tests is in the commit queue: https://crrev.com/c/1350899, but the metrics are already added.
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5a49664f21e73cf1497fee10bb84e73c29c06c0f commit 5a49664f21e73cf1497fee10bb84e73c29c06c0f Author: Vaclav Brozek <vabr@chromium.org> Date: Tue Nov 27 18:31:16 2018 Reset PasswordManager in ReportMissingFormManager The test PasswordManagerTest.ReportMissingFormManager executes a number of test-cases, and for each test-case, two runs, one with the old FormData parser, one with the new one. Each test-case can leave the PasswordManager in a state not expected by the next case, so it is good to reset the PasswordManager at the beginning of each run. This happens as part of setting the flags for the new parser automatically, but does not happen when the old parser is used. Thus, the tests failed if for each test-case the old parser was run first. This CL fixes that by ensuring that the PasswordManager is reset before each run. Bug: 907794 Change-Id: Id3ec0acf34d1a9d7b3530edd3eec0d6351fa4e1d Reviewed-on: https://chromium-review.googlesource.com/c/1350899 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#611205} [modify] https://crrev.com/5a49664f21e73cf1497fee10bb84e73c29c06c0f/components/password_manager/core/browser/password_manager_unittest.cc |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Nov 22