Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1207373002:
Implement Mac Keychain migration algorithm. (Closed)
|
Created:
5 years, 6 months ago by vasilii Modified:
5 years, 5 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlist_chromium.org, asvitkine+watch_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement Mac Keychain migration algorithm.
This code is currently not actually running, so no migration is happening yet.
Design doc: https://docs.google.com/a/google.com/document/d/1A8ZG16bLuUH1u21K0GoABKz_wpz1kchXMnMlpmq_ecA/edit?usp=sharing
BUG=466638
Committed: https://crrev.com/0b57b117fdabefd8584a99064822d50deea6d52a
Cr-Commit-Position: refs/heads/master@{#336756}
Patch Set 1 #
Total comments: 16
Patch Set 2 : comments #
Total comments: 28
Patch Set 3 : more comments #
Total comments: 6
Patch Set 4 : Ilya's comments #
Messages
Total messages: 16 (3 generated)
vasilii@chromium.org changed reviewers: + isherman@chromium.org, rsleevi@chromium.org, stuartmorgan@chromium.org
Hi all, stuartmorgan@chromium.org: Please review changes in chrome/browser/password_manager/ rsleevi@chromium.org: Please review changes in crypto/ isherman@chromium.org: Please review changes in histograms.xml
I'm a little worried that users in particularly errorful states might hit these histogram code paths multiple times, effectively magnifying their votes. Is this a reasonable concern, or is the effect likely to be in the noise? https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:969: using namespace internal_keychain_helpers; Hmm, why isn't this at the top of the file? https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:1035: unmerged_forms_count); It looks like you emit "unmerged_forms_count" to both of the latter two histograms. Why do you need both? https://codereview.chromium.org/1207373002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1207373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27681: + name="PasswordManager.KeychainMigration.NumChromeOwnedLockedPasswords"> nit: Probably mention "failure" somewhere in the histogram name. Ditto for the others. https://codereview.chromium.org/1207373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27685: + owned by Chrome but not accessible by it. nit: Suggested phrasing: "After migration from the Mac Keychain fails, records the number of passwords ...". Ditto for the other histograms. https://codereview.chromium.org/1207373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27701: + the Chrome Password Manager. Is it worth having a (set of) success histogram(s) as well?
https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:969: using namespace internal_keychain_helpers; On 2015/06/25 21:46:25, Ilya Sherman wrote: > Hmm, why isn't this at the top of the file? It's not required to be: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces You may use a using-declaration anywhere in a .cc file, and in functions, methods or classes in .h files. // OK in .cc files. // Must be in a function, method or class in .h files. using ::foo::bar; https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:1027: } STYLE: You're inconsistent with the braces for single-lines within this function/file (compare w/ 972, 975, 981, 988, 1040) https://codereview.chromium.org/1207373002/diff/1/crypto/mock_apple_keychain.h File crypto/mock_apple_keychain.h (right): https://codereview.chromium.org/1207373002/diff/1/crypto/mock_apple_keychain.... crypto/mock_apple_keychain.h:122: static void set_locked(bool locked) { locked_ = locked; } I feel that, design wise, adding a static member variable here is somewhat inconsistent / problematic from a design point. This is especially problematic from a unit test perspective, because if you fail to reset this flag, the test can have side-effects. Could you explain/explore why using https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas... wouldn't work, and making this an instance-member on the class - either part of the base crypto::AppleKeychain interface (with noop in for the concrete impl) or via casting for the test? Just wanting to understand this design better, and what the challenges are to doing it as an instance member, because this represents a bit of a red flag.
The migration is performed only twice at max. Therefore, those who failed twice will magnify the counters. On the other hand, these users are of interest for us. https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:969: using namespace internal_keychain_helpers; On 2015/06/25 21:46:25, Ilya Sherman wrote: > Hmm, why isn't this at the top of the file? I removed it. https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:969: using namespace internal_keychain_helpers; On 2015/06/26 10:46:37, Ryan Sleevi (slow through 7-15 wrote: > On 2015/06/25 21:46:25, Ilya Sherman wrote: > > Hmm, why isn't this at the top of the file? > > It's not required to be: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces > > You may use a using-declaration anywhere in a .cc file, and in functions, > methods or classes in .h files. > // OK in .cc files. > // Must be in a function, method or class in .h files. > using ::foo::bar; Though it prohibits "using namespace foo;" so I removed it. https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:1027: } On 2015/06/26 10:46:37, Ryan Sleevi (slow through 7-15 wrote: > STYLE: You're inconsistent with the braces for single-lines within this > function/file (compare w/ 972, 975, 981, 988, 1040) I'm consistent. It was a copy-paste :-) https://codereview.chromium.org/1207373002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_store_mac.cc:1035: unmerged_forms_count); On 2015/06/25 21:46:24, Ilya Sherman wrote: > It looks like you emit "unmerged_forms_count" to both of the latter two > histograms. Why do you need both? Oh, that's a bug. It should be chrome_owned_locked_forms_count. https://codereview.chromium.org/1207373002/diff/1/crypto/mock_apple_keychain.h File crypto/mock_apple_keychain.h (right): https://codereview.chromium.org/1207373002/diff/1/crypto/mock_apple_keychain.... crypto/mock_apple_keychain.h:122: static void set_locked(bool locked) { locked_ = locked; } On 2015/06/26 10:46:38, Ryan Sleevi (slow through 7-15 wrote: > I feel that, design wise, adding a static member variable here is somewhat > inconsistent / problematic from a design point. This is especially problematic > from a unit test perspective, because if you fail to reset this flag, the test > can have side-effects. > > Could you explain/explore why using > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas... > wouldn't work, and making this an instance-member on the class - either part of > the base crypto::AppleKeychain interface (with noop in for the concrete impl) or > via casting for the test? > > Just wanting to understand this design better, and what the challenges are to > doing it as an instance member, because this represents a bit of a red flag. You're absolutely right. The original idea was to make it static so OSCrypt::EncryptString() also fails. However, the approach failed for the reasons you mentioned. https://codereview.chromium.org/1207373002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1207373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27681: + name="PasswordManager.KeychainMigration.NumChromeOwnedLockedPasswords"> On 2015/06/25 21:46:25, Ilya Sherman wrote: > nit: Probably mention "failure" somewhere in the histogram name. Ditto for the > others. Done. https://codereview.chromium.org/1207373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27685: + owned by Chrome but not accessible by it. On 2015/06/25 21:46:25, Ilya Sherman wrote: > nit: Suggested phrasing: "After migration from the Mac Keychain fails, records > the number of passwords ...". Ditto for the other histograms. Done. https://codereview.chromium.org/1207373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27701: + the Chrome Password Manager. On 2015/06/25 21:46:25, Ilya Sherman wrote: > Is it worth having a (set of) success histogram(s) as well? Sure. I'll add one in another patch. It will record percentage of migrated users.
//crypto LGTM; some drive-by nits but *shrug* :) https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:975: ScopedVector<PasswordForm> uninteresting_forms; nits (feel free to ignore) - I found the transition of lines 971->972, 974->975, and 987->989 a bit hard to read. It wouldn't hurt to add newlines in between (for example, you do this on 980 -> 981 as a separator for "edge condition" from "code flow". Not required, worth considering :) https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1004: int chrome_owned_locked_forms_count = 0; STYLE: (usually a SECURITY BUG, but here it just seems 'danger') https://www.chromium.org/developers/coding-style#TOC-Types Use size_t for object and allocation sizes, object counts, array and pointer offsets, vector indices, and so on. The signed types are incorrect and unsafe for these purposes. (Integer overflow behavior for signed types is undefined in the C and C++ standards, while the behavior is defined for unsigned types.) The C++ STL is a guide here: they use size_t and foo::size_type for very good reasons. (Namely, you're iterating though database_forms, which is of size size_t, but you're counting with int, which may lead to undefined overflow; this would throw off your metrics, since you don't use these for offsets, but you hopefully see the issue) https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1030: } nit: Since it looks like the previous comment wasn't clear - you use {} here for a one-line conditional, but you don't elsewhere in this function. That's the consistency bit. That is, your early return conditionals (e.g. 986-987) and your other loop (line 1044-1045) don't use braces, so it's inconsistent that you do. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals 's mantra is about ensuring consistency - it's fine to pick either, but if you do, it's better to be consistent within a function, file, and 'section' of code. That's why I flagged this - it simply stood out to me as "one of these things it not like the other"
LGTM, but please flesh out the CL description to make it clear that this code is currently not actually run, so no migration is happening yet. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:442: SecKeychainAttributeInfo attrInfo; attr_info https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:446: attrInfo.format = NULL; nullptr https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:448: SecKeychainAttributeList* attrList; attr_list https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:986: if (!OSCrypt::EncryptString(database_forms[0]->origin.spec(), &ciphertext)) Why are you pulling a random field out for this, instead of just using some hard-coded dummy text? https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:997: ExtractAllKeychainItemAttributesIntoPasswordForms(&keychain_items, Indent 4 more; it's a continuation https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1007: internal_keychain_helpers::ExtractPasswordsMergeableWithForm( This seems wasteful (it may do extra Keychain lookups that you'll just discard below), but I guess given the current structure of the existing flows it's the best option. I will not miss this class when it's gone... https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1016: // Check if the corresponding keychain items are created by Chrome s/the/any/ (since there may be none). Also, missing a final period. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1033: "PasswordManager.KeychainMigration.NumPasswordsOnFailure", Indent these lines 4, not 2.
https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:1881: "PasswordManager.KeychainMigration.NumChromeOwnedFailedPasswords", 2, 1); How is it possible that the number of Chrome-owned failed passwords is larger than the total number of failed passwords? https://codereview.chromium.org/1207373002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1207373002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27701: + in the Chrome Password Manager to be migrated. I'm not exactly sure how this number differs from the one above. Could you please clarify?
https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:442: SecKeychainAttributeInfo attrInfo; On 2015/06/26 21:26:08, stuartmorgan wrote: > attr_info Done. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:446: attrInfo.format = NULL; On 2015/06/26 21:26:08, stuartmorgan wrote: > nullptr Done. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:448: SecKeychainAttributeList* attrList; On 2015/06/26 21:26:08, stuartmorgan wrote: > attr_list Done. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:975: ScopedVector<PasswordForm> uninteresting_forms; On 2015/06/26 16:44:35, Ryan Sleevi (slow through 7-15 wrote: > nits (feel free to ignore) - I found the transition of lines 971->972, 974->975, > and 987->989 a bit hard to read. It wouldn't hurt to add newlines in between > (for example, you do this on 980 -> 981 as a separator for "edge condition" from > "code flow". Not required, worth considering :) Done. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:986: if (!OSCrypt::EncryptString(database_forms[0]->origin.spec(), &ciphertext)) On 2015/06/26 21:26:07, stuartmorgan wrote: > Why are you pulling a random field out for this, instead of just using some > hard-coded dummy text? Done. Binary size? https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:997: ExtractAllKeychainItemAttributesIntoPasswordForms(&keychain_items, On 2015/06/26 21:26:08, stuartmorgan wrote: > Indent 4 more; it's a continuation Done. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1004: int chrome_owned_locked_forms_count = 0; On 2015/06/26 16:44:35, Ryan Sleevi (slow through 7-15 wrote: > STYLE: (usually a SECURITY BUG, but here it just seems 'danger') > > https://www.chromium.org/developers/coding-style#TOC-Types > Use size_t for object and allocation sizes, object counts, array and pointer > offsets, vector indices, and so on. The signed types are incorrect and unsafe > for these purposes. (Integer overflow behavior for signed types is undefined in > the C and C++ standards, while the behavior is defined for unsigned types.) The > C++ STL is a guide here: they use size_t and foo::size_type for very good > reasons. > > (Namely, you're iterating though database_forms, which is of size size_t, but > you're counting with int, which may lead to undefined overflow; this would throw > off your metrics, since you don't use these for offsets, but you hopefully see > the issue) Done. Though implicit conversion to int in UMA_HISTOGRAM_COUNTS isn't defined too. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1007: internal_keychain_helpers::ExtractPasswordsMergeableWithForm( On 2015/06/26 21:26:08, stuartmorgan wrote: > This seems wasteful (it may do extra Keychain lookups that you'll just discard > below), but I guess given the current structure of the existing flows it's the > best option. > > I will not miss this class when it's gone... I think it should return exactly one form (which is the best match below) in most of the cases. PSL-matched passwords are discarded in FormIsValidAndMatchesOtherForm. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1016: // Check if the corresponding keychain items are created by Chrome On 2015/06/26 21:26:07, stuartmorgan wrote: > s/the/any/ (since there may be none). Also, missing a final period. Done. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1030: } On 2015/06/26 16:44:35, Ryan Sleevi (slow through 7-15 wrote: > nit: Since it looks like the previous comment wasn't clear - you use {} here for > a one-line conditional, but you don't elsewhere in this function. That's the > consistency bit. > > That is, your early return conditionals (e.g. 986-987) and your other loop (line > 1044-1045) don't use braces, so it's inconsistent that you do. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals 's > mantra is about ensuring consistency - it's fine to pick either, but if you do, > it's better to be consistent within a function, file, and 'section' of code. > That's why I flagged this - it simply stood out to me as "one of these things it > not like the other" Done. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1033: "PasswordManager.KeychainMigration.NumPasswordsOnFailure", On 2015/06/26 21:26:08, stuartmorgan wrote: > Indent these lines 4, not 2. Done. https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac_unittest.cc:1881: "PasswordManager.KeychainMigration.NumChromeOwnedFailedPasswords", 2, 1); On 2015/06/27 05:45:34, Ilya Sherman wrote: > How is it possible that the number of Chrome-owned failed passwords is larger > than the total number of failed passwords? NumChromeOwnedFailedPasswords counts entries in the Keychain. NumFailedPasswords in the local database. They are independent on each other. https://codereview.chromium.org/1207373002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1207373002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27701: + in the Chrome Password Manager to be migrated. On 2015/06/27 05:45:34, Ilya Sherman wrote: > I'm not exactly sure how this number differs from the one above. Could you > please clarify? Done. Note that there is no partial migration. NumPasswordsOnFailure shows the total number of passwords with values hidden in the Keychain. NumFailedPasswords are only those inaccessible by Chrome.
LGTM % the remaining nits: https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1004: int chrome_owned_locked_forms_count = 0; On 2015/06/29 14:12:44, vasilii wrote: > On 2015/06/26 16:44:35, Ryan Sleevi (slow through 7-15 wrote: > > STYLE: (usually a SECURITY BUG, but here it just seems 'danger') > > > > https://www.chromium.org/developers/coding-style#TOC-Types > > Use size_t for object and allocation sizes, object counts, array and pointer > > offsets, vector indices, and so on. The signed types are incorrect and unsafe > > for these purposes. (Integer overflow behavior for signed types is undefined > in > > the C and C++ standards, while the behavior is defined for unsigned types.) > The > > C++ STL is a guide here: they use size_t and foo::size_type for very good > > reasons. > > > > (Namely, you're iterating though database_forms, which is of size size_t, but > > you're counting with int, which may lead to undefined overflow; this would > throw > > off your metrics, since you don't use these for offsets, but you hopefully see > > the issue) > > Done. Though implicit conversion to int in UMA_HISTOGRAM_COUNTS isn't defined > too. You could use a checked_cast (from safe_numerics.h) if you'd like to be really thorough. https://codereview.chromium.org/1207373002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1207373002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27681: + name="PasswordManager.KeychainMigration.NumChromeOwnedFailedPasswords"> nit: Maybe "NumChromeOwnedInaccessiblePasswords" would be a clearer name, if this isn't strictly a subset of "NumFailedPasswords"? The current names make it seem like this should be a strict subset. https://codereview.chromium.org/1207373002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27693: + in the Chrome Password Manager database which values in the Keychain are not nit: "which values" -> "for which the corresponding values" https://codereview.chromium.org/1207373002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27702: + in the Chrome Password Manager database to be migrated. nit: Please clarify that there is no partial migration, so this is the total number of passwords in the DB.
https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/1207373002/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:1004: int chrome_owned_locked_forms_count = 0; On 2015/06/29 23:10:52, Ilya Sherman wrote: > On 2015/06/29 14:12:44, vasilii wrote: > > On 2015/06/26 16:44:35, Ryan Sleevi (slow through 7-15 wrote: > > > STYLE: (usually a SECURITY BUG, but here it just seems 'danger') > > > > > > https://www.chromium.org/developers/coding-style#TOC-Types > > > Use size_t for object and allocation sizes, object counts, array and pointer > > > offsets, vector indices, and so on. The signed types are incorrect and > unsafe > > > for these purposes. (Integer overflow behavior for signed types is undefined > > in > > > the C and C++ standards, while the behavior is defined for unsigned types.) > > The > > > C++ STL is a guide here: they use size_t and foo::size_type for very good > > > reasons. > > > > > > (Namely, you're iterating though database_forms, which is of size size_t, > but > > > you're counting with int, which may lead to undefined overflow; this would > > throw > > > off your metrics, since you don't use these for offsets, but you hopefully > see > > > the issue) > > > > Done. Though implicit conversion to int in UMA_HISTOGRAM_COUNTS isn't defined > > too. > > You could use a checked_cast (from safe_numerics.h) if you'd like to be really > thorough. It contains CHECK(), and I don't want to have a crash here. I think Histogram::Add does exactly what I need. https://codereview.chromium.org/1207373002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1207373002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27681: + name="PasswordManager.KeychainMigration.NumChromeOwnedFailedPasswords"> On 2015/06/29 23:10:52, Ilya Sherman wrote: > nit: Maybe "NumChromeOwnedInaccessiblePasswords" would be a clearer name, if > this isn't strictly a subset of "NumFailedPasswords"? The current names make it > seem like this should be a strict subset. Done. https://codereview.chromium.org/1207373002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27693: + in the Chrome Password Manager database which values in the Keychain are not On 2015/06/29 23:10:52, Ilya Sherman wrote: > nit: "which values" -> "for which the corresponding values" Done. https://codereview.chromium.org/1207373002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27702: + in the Chrome Password Manager database to be migrated. On 2015/06/29 23:10:52, Ilya Sherman wrote: > nit: Please clarify that there is no partial migration, so this is the total > number of passwords in the DB. Done.
The CQ bit was checked by vasilii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stuartmorgan@chromium.org, rsleevi@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1207373002/#ps60001 (title: "Ilya's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1207373002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0b57b117fdabefd8584a99064822d50deea6d52a Cr-Commit-Position: refs/heads/master@{#336756} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
