Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
562833003:
Make PasswordSyncableServiceTest.* more readable. (Closed)
DescriptionMake PasswordSyncableServiceTest.* more readable. Add -Wexit-time-destructors warning for the password manager.
Committed: https://crrev.com/badbcb815fb8cfe56fdc19e207a1775b13c83c23
Cr-Commit-Position: refs/heads/master@{#295103}
Patch Set 1 #
Total comments: 6
Patch Set 2 : no static initializers #
Total comments: 4
Patch Set 3 : no flag #Messages
Total messages: 14 (2 generated)
vasilii@chromium.org changed reviewers: + isherman@chromium.org, mkwst@chromium.org
Hi guys, I address here Mike's request to make the tests really readable ;-)
https://codereview.chromium.org/562833003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/562833003/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service_unittest.cc:51: const GURL kFederationUrl = GURL("https://fb.com/federation_url"); These all introduce static initializers. Please use const char arrays for string constants, and convert them to objects within the tests. You can define helper functions for the conversions, if you like. https://codereview.chromium.org/562833003/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service_unittest.cc:56: const autofill::PasswordForm::Type kType = autofill::PasswordForm::TYPE_LAST; nit: If the type doesn't matter, perhaps name this "kArbitraryType"
https://codereview.chromium.org/562833003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/562833003/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service_unittest.cc:51: const GURL kFederationUrl = GURL("https://fb.com/federation_url"); On 2014/09/11 19:08:48, Ilya Sherman wrote: > These all introduce static initializers. Please use const char arrays for > string constants, and convert them to objects within the tests. You can define > helper functions for the conversions, if you like. Done. Though this rule is already violated in components_unittests. https://codereview.chromium.org/562833003/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service_unittest.cc:56: const autofill::PasswordForm::Type kType = autofill::PasswordForm::TYPE_LAST; On 2014/09/11 19:08:48, Ilya Sherman wrote: > nit: If the type doesn't matter, perhaps name this "kArbitraryType" Done.
LGTM, for what it's worth. You'll still need Ilya's stamp to land it. Thanks for following up on this!
https://codereview.chromium.org/562833003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/562833003/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service_unittest.cc:51: const GURL kFederationUrl = GURL("https://fb.com/federation_url"); On 2014/09/12 08:43:22, vasilii wrote: > On 2014/09/11 19:08:48, Ilya Sherman wrote: > > These all introduce static initializers. Please use const char arrays for > > string constants, and convert them to objects within the tests. You can > define > > helper functions for the conversions, if you like. > > Done. Though this rule is already violated in components_unittests. Hmm, that's surprising. I'm pretty sure that static initializers are disallowed everywhere. If we could use C++11, we'd mark these as "constexpr"; but other than that, the style guide seems pretty clear. Please let me know if I'm overlooking some guidance somewhere, and I'll update my internal style guide mappings :) https://codereview.chromium.org/562833003/diff/20001/components/password_mana... File components/password_manager.gypi (right): https://codereview.chromium.org/562833003/diff/20001/components/password_mana... components/password_manager.gypi:78: 'enable_wexit_time_destructors': 1, Please remove this, throughout.
https://codereview.chromium.org/562833003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_syncable_service_unittest.cc (right): https://codereview.chromium.org/562833003/diff/1/components/password_manager/... components/password_manager/core/browser/password_syncable_service_unittest.cc:51: const GURL kFederationUrl = GURL("https://fb.com/federation_url"); On 2014/09/12 21:33:29, Ilya Sherman wrote: > On 2014/09/12 08:43:22, vasilii wrote: > > On 2014/09/11 19:08:48, Ilya Sherman wrote: > > > These all introduce static initializers. Please use const char arrays for > > > string constants, and convert them to objects within the tests. You can > > define > > > helper functions for the conversions, if you like. > > > > Done. Though this rule is already violated in components_unittests. > > Hmm, that's surprising. I'm pretty sure that static initializers are disallowed > everywhere. If we could use C++11, we'd mark these as "constexpr"; but other > than that, the style guide seems pretty clear. Please let me know if I'm > overlooking some guidance somewhere, and I'll update my internal style guide > mappings :) You can check my statement by turning on the compiler warning. https://codereview.chromium.org/562833003/diff/20001/components/password_mana... File components/password_manager.gypi (right): https://codereview.chromium.org/562833003/diff/20001/components/password_mana... components/password_manager.gypi:78: 'enable_wexit_time_destructors': 1, On 2014/09/12 21:33:29, Ilya Sherman wrote: > Please remove this, throughout. If you say that the static initializers are prohibited, it should be enforced somehow. This check is already on for many chrome targets.
https://codereview.chromium.org/562833003/diff/20001/components/password_mana... File components/password_manager.gypi (right): https://codereview.chromium.org/562833003/diff/20001/components/password_mana... components/password_manager.gypi:78: 'enable_wexit_time_destructors': 1, On 2014/09/15 08:45:38, vasilii wrote: > On 2014/09/12 21:33:29, Ilya Sherman wrote: > > Please remove this, throughout. > > If you say that the static initializers are prohibited, it should be enforced > somehow. This check is already on for many chrome targets. Sorry, I read this boolean backward, so I thought this was disabling the warning rather than enabling it. Makes more sense now that I'm reading it correctly :) However, it seems quite odd to add the warning here, rather than in a higher-level file. This seems like the sort of setting that should be set globally, or not at all. I'd prefer that you not add this flag in this CL; and follow up by checking with someone familiar with the build system -- say, thakis or brettw -- and get their thoughts on the best path forward.
https://codereview.chromium.org/562833003/diff/20001/components/password_mana... File components/password_manager.gypi (right): https://codereview.chromium.org/562833003/diff/20001/components/password_mana... components/password_manager.gypi:78: 'enable_wexit_time_destructors': 1, On 2014/09/15 17:21:47, Ilya Sherman wrote: > On 2014/09/15 08:45:38, vasilii wrote: > > On 2014/09/12 21:33:29, Ilya Sherman wrote: > > > Please remove this, throughout. > > > > If you say that the static initializers are prohibited, it should be enforced > > somehow. This check is already on for many chrome targets. > > Sorry, I read this boolean backward, so I thought this was disabling the warning > rather than enabling it. Makes more sense now that I'm reading it correctly :) > > However, it seems quite odd to add the warning here, rather than in a > higher-level file. This seems like the sort of setting that should be set > globally, or not at all. I'd prefer that you not add this flag in this CL; and > follow up by checking with someone familiar with the build system -- say, thakis > or brettw -- and get their thoughts on the best path forward. Done.
LGTM, thanks.
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/562833003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as c25826789a6f948ac466f7f83dc3a2b2d9ab6645
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/badbcb815fb8cfe56fdc19e207a1775b13c83c23 Cr-Commit-Position: refs/heads/master@{#295103} |
