Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2150543002:
OSCrypt supports encryption with KWallet (Closed)
|
Created:
4 years, 5 months ago by cfroussios Modified:
4 years, 4 months ago Reviewers:
Lei Zhang CC:
chromium-reviews, danakj Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOSCrypt supports encryption with KWallet
Implemented KeyStorageLinux for KWallet.
BUG=602624
Committed: https://crrev.com/2e6729a44f5a07c939209bce99f803dc90c5966f
Cr-Commit-Position: refs/heads/master@{#407746}
Patch Set 1 #Patch Set 2 : Give me an overview #Patch Set 3 : forward localised product name #Patch Set 4 : nits #Patch Set 5 : fixed build #
Total comments: 44
Patch Set 6 : feedback #
Total comments: 4
Patch Set 7 : feedback #Patch Set 8 : feedback #
Total comments: 2
Messages
Total messages: 59 (38 generated)
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
cfroussios@chromium.org changed reviewers: + thestig@chromium.org
Hi! This is the implementation with kwallet for os_crypt. Can you review? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
No serious concerns, 1000 nits below: https://codereview.chromium.org/2150543002/diff/80001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/2150543002/diff/80001/components/components_t... components/components_tests.gyp:1592: 'os_crypt/key_storage_kwallet_unittest.cc', alphabetical order please https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:17: const char kFolderName[] = "Chrome Keys"; Any chance we can share these with the libsecret impl? https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:28: : desktop_env_(desktop_env), handle_(0), app_name_(std::move(app_name)) {} Should |handle_| be initialized to -1? Is 0 a valid value? https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:58: if (result == InitResult::TEMPORARY_FAIL) if (result == InitResult::TEMPORARY_FAIL && kwallet_dbus_->StartKWalletd()) result = InitWallet(); https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.h (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.h:17: explicit KeyStorageKWallet(base::nix::DesktopEnvironment desktop_env, nit explicit https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.h:18: std::string app_name); Just pass by ref and set app_name_? https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet_unittest.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet_unittest.cc:116: // Tests are an unofficial build, so only Chromium will be used. Umm, I think there exists an internal waterfall that runs tests for official builds. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet_unittest.cc:138: EXPECT_EQ("butter", password); Can we do EXPECT_STREQ(), and just drop |password|? Ditto on line 152 and more below, and mmm, butter. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:21: base::LazyInstance<std::string> g_product_name_ = LAZY_INSTANCE_INITIALIZER; Sorry if I missed it earlier, but g_foo doesn't need the trailing underscore since it's not a class member. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... File components/os_crypt/kwallet_dbus.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus.cc:361: if (!response.get()) { No need for .get() here, ditto on line 390, and 414. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus.cc:362: LOG(ERROR) << "Error contacting << " << kwalletd_name_ did you want the extra "<< " in the string? Ditto on line 391 and 415. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... File components/os_crypt/kwallet_dbus.h (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus.h:106: // Writes a password to the current folder. "Write" to be consistent with all the comments above. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus.h:115: // Returns "" if no such password exists. Clear |password_ptr| if no such password exists. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... File components/os_crypt/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:286: if (i != int_1) You put int_1 on the right side, but moved str_N to the left side below. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:819: EXPECT_EQ("password", password); Can you use EXPECT_STREQ() here? https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:855: EXPECT_EQ(true, success); EXPECT_TRUE(success) https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:869: EXPECT_EQ(false, success); EXPECT_FALSE
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2150543002/diff/80001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/2150543002/diff/80001/components/components_t... components/components_tests.gyp:1592: 'os_crypt/key_storage_kwallet_unittest.cc', On 2016/07/20 01:08:26, Lei Zhang wrote: > alphabetical order please Done. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:17: const char kFolderName[] = "Chrome Keys"; On 2016/07/20 01:08:26, Lei Zhang wrote: > Any chance we can share these with the libsecret impl? I moved them to KeyStorageLinux, mostly because these are also visible to the user and should be consistent. Conceptually, reusing those doesn't mean much, since KWallet and Libsecret have diffent structures. Libsecret doesn't even have folders and doesn't rely on the name for lookup. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:28: : desktop_env_(desktop_env), handle_(0), app_name_(std::move(app_name)) {} On 2016/07/20 01:08:26, Lei Zhang wrote: > Should |handle_| be initialized to -1? Is 0 a valid value? Both are invalid values, but let's just use -1, since that is what we check for in line 99. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:58: if (result == InitResult::TEMPORARY_FAIL) On 2016/07/20 01:08:26, Lei Zhang wrote: > if (result == InitResult::TEMPORARY_FAIL && kwallet_dbus_->StartKWalletd()) > result = InitWallet(); I am not a big fan of conditions where the order of execution matters. I tend to not pay attention, and I am not the only one. In this case, StartKWalletd is costly (response times vary, up to several seconds) and we explicitly avoid calling it when unnecessary. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.h (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.h:17: explicit KeyStorageKWallet(base::nix::DesktopEnvironment desktop_env, On 2016/07/20 01:08:26, Lei Zhang wrote: > nit explicit Done. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.h:18: std::string app_name); On 2016/07/20 01:08:26, Lei Zhang wrote: > Just pass by ref and set app_name_? For constructors, this is supposed to perform better on average than passing by reference. Not that this is likely to ever become hot. Is that a pattern we don't care for in Chrome? https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet_unittest.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet_unittest.cc:116: // Tests are an unofficial build, so only Chromium will be used. On 2016/07/20 01:08:26, Lei Zhang wrote: > Umm, I think there exists an internal waterfall that runs tests for official > builds. Done. I added a comment about why I'm not pulling the values from KeyStorageLinux itself. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet_unittest.cc:138: EXPECT_EQ("butter", password); On 2016/07/20 01:08:26, Lei Zhang wrote: > Can we do EXPECT_STREQ(), and just drop |password|? Ditto on line 152 and more > below, and mmm, butter. Butter from peanuts. GetKey() doesn't return a C string. |password| was just redundant. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:21: base::LazyInstance<std::string> g_product_name_ = LAZY_INSTANCE_INITIALIZER; On 2016/07/20 01:08:26, Lei Zhang wrote: > Sorry if I missed it earlier, but g_foo doesn't need the trailing underscore > since it's not a class member. Done. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... File components/os_crypt/kwallet_dbus.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus.cc:361: if (!response.get()) { On 2016/07/20 01:08:26, Lei Zhang wrote: > No need for .get() here, ditto on line 390, and 414. Done. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus.cc:362: LOG(ERROR) << "Error contacting << " << kwalletd_name_ On 2016/07/20 01:08:26, Lei Zhang wrote: > did you want the extra "<< " in the string? Ditto on line 391 and 415. Done. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... File components/os_crypt/kwallet_dbus.h (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus.h:106: // Writes a password to the current folder. On 2016/07/20 01:08:26, Lei Zhang wrote: > "Write" to be consistent with all the comments above. Done. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus.h:115: // Returns "" if no such password exists. On 2016/07/20 01:08:26, Lei Zhang wrote: > Clear |password_ptr| if no such password exists. Done. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... File components/os_crypt/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:286: if (i != int_1) On 2016/07/20 01:08:26, Lei Zhang wrote: > You put int_1 on the right side, but moved str_N to the left side below. Done. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:819: EXPECT_EQ("password", password); On 2016/07/20 01:08:26, Lei Zhang wrote: > Can you use EXPECT_STREQ() here? EXPECT_STREQ is for C strings. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:855: EXPECT_EQ(true, success); On 2016/07/20 01:08:26, Lei Zhang wrote: > EXPECT_TRUE(success) Done. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:869: EXPECT_EQ(false, success); On 2016/07/20 01:08:26, Lei Zhang wrote: > EXPECT_FALSE Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, with a few more nitty bits. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:58: if (result == InitResult::TEMPORARY_FAIL) On 2016/07/20 15:22:46, cfroussios wrote: > On 2016/07/20 01:08:26, Lei Zhang wrote: > > if (result == InitResult::TEMPORARY_FAIL && kwallet_dbus_->StartKWalletd()) > > result = InitWallet(); > > I am not a big fan of conditions where the order of execution matters. I tend to > not pay attention, and I am not the only one. In this case, StartKWalletd is > costly (response times vary, up to several seconds) and we explicitly avoid > calling it when unnecessary. But ordering of execution in if statements do matter and the left to right eval ordering is a fact of life you have to live with. Writing it as a single if statement doesn't change the behavior. As an exaggeration, what if we had 4 conditions to check? Which should we use? if (foo) if (bar) if (qux) if (baz) DoIt(); or if (foo && bar && qux && baz) DoIt(); https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.h (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.h:18: std::string app_name); On 2016/07/20 15:22:46, cfroussios wrote: > On 2016/07/20 01:08:26, Lei Zhang wrote: > > Just pass by ref and set app_name_? > > For constructors, this is supposed to perform better on average than passing by > reference. Not that this is likely to ever become hot. Is that a pattern we > don't care for in Chrome? Well, this is definitely not hot code, and I think (please double check) if you want to use move semantics, you do "std::string&&" but the Chromium code base has 2 instances of that, so in general, we are not quite there yet. Plus, I like |g_product_name| being set once and staying the same. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... File components/os_crypt/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:819: EXPECT_EQ("password", password); On 2016/07/20 15:22:47, cfroussios wrote: > On 2016/07/20 01:08:26, Lei Zhang wrote: > > Can you use EXPECT_STREQ() here? > > EXPECT_STREQ is for C strings. It is indeed, and this is shorter than EXPECT_STREQ("password", password.c_str()); https://codereview.chromium.org/2150543002/diff/120001/components/os_crypt/ke... File components/os_crypt/key_storage_kwallet_unittest.cc (right): https://codereview.chromium.org/2150543002/diff/120001/components/os_crypt/ke... components/os_crypt/key_storage_kwallet_unittest.cc:119: const std::string expected_folder_name_ = "Chrome Keys"; Do these have to be class members? Can this be const char kFoo[] and sit around line 30? https://codereview.chromium.org/2150543002/diff/120001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/2150543002/diff/120001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:38: static const char* const kFolderName; Can these be static const char kFoo[] ?
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:58: if (result == InitResult::TEMPORARY_FAIL) On 2016/07/20 19:15:07, Lei Zhang wrote: > On 2016/07/20 15:22:46, cfroussios wrote: > > On 2016/07/20 01:08:26, Lei Zhang wrote: > > > if (result == InitResult::TEMPORARY_FAIL && kwallet_dbus_->StartKWalletd()) > > > result = InitWallet(); > > > > I am not a big fan of conditions where the order of execution matters. I tend > to > > not pay attention, and I am not the only one. In this case, StartKWalletd is > > costly (response times vary, up to several seconds) and we explicitly avoid > > calling it when unnecessary. > > But ordering of execution in if statements do matter and the left to right eval > ordering is a fact of life you have to live with. Writing it as a single if > statement doesn't change the behavior. > > As an exaggeration, what if we had 4 conditions to check? Which should we use? > > if (foo) > if (bar) > if (qux) > if (baz) > DoIt(); > > or > > if (foo && bar && qux && baz) > DoIt(); There might have been a miscommunication. I didn't say the behavior changes. My objection is about readability. Logical operators draw attention to the logic of the condition, whereas ifs draw attention to the flow of execution. Every time I've seen code in Chrome where right eval matters, they've also added a comment to note that. That's because one might reorder a condition without thinking much about it. But why do it in a way that requires a note, when you can have code that speaks for itself? People don't invert nested blocks expecting things to be the same. Aesthetically, in your example, too many nested ifs waste screen space. However, in my specific case, the result doesn't suffer much. I don't think there needs to be a one-size-fits-all rule here, such that we need to discuss which rule gets worse in its extreme. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.h (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.h:18: std::string app_name); On 2016/07/20 19:15:07, Lei Zhang wrote: > On 2016/07/20 15:22:46, cfroussios wrote: > > On 2016/07/20 01:08:26, Lei Zhang wrote: > > > Just pass by ref and set app_name_? > > > > For constructors, this is supposed to perform better on average than passing > by > > reference. Not that this is likely to ever become hot. Is that a pattern we > > don't care for in Chrome? > > Well, this is definitely not hot code, and I think (please double check) if you > want to use move semantics, you do "std::string&&" but the Chromium code base > has 2 instances of that, so in general, we are not quite there yet. Plus, I like > |g_product_name| being set once and staying the same. You don't need the "std::string&&" to take advantage of move semantics, because std::string already has a move constructor. The idea is that |app_name| will be constructed with the best constructor in each case: 1. If the input can be moved, it will be moved into |app_name| and then into |app_name_|. 2. If stuff can't be moved, |app_name| will be a new copy, which will be moved into |app_name_|. We would have had to create a copy anyway. I don't quite understand your comment about |g_product_name|. Are you saying that, since it won't change, we can pass pointers to it? Or that we would have to cannibalize it, thus changing it? In the way that KeyStorageKWallet is used now, it won't actually be moved. It is just a good pattern for constructors, because it works both when you can and can't move. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... File components/os_crypt/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/kwa... components/os_crypt/kwallet_dbus_unittest.cc:819: EXPECT_EQ("password", password); On 2016/07/20 19:15:07, Lei Zhang wrote: > On 2016/07/20 15:22:47, cfroussios wrote: > > On 2016/07/20 01:08:26, Lei Zhang wrote: > > > Can you use EXPECT_STREQ() here? > > > > EXPECT_STREQ is for C strings. > > It is indeed, and this is shorter than EXPECT_STREQ("password", > password.c_str()); Acknowledged. https://codereview.chromium.org/2150543002/diff/120001/components/os_crypt/ke... File components/os_crypt/key_storage_kwallet_unittest.cc (right): https://codereview.chromium.org/2150543002/diff/120001/components/os_crypt/ke... components/os_crypt/key_storage_kwallet_unittest.cc:119: const std::string expected_folder_name_ = "Chrome Keys"; On 2016/07/20 19:15:07, Lei Zhang wrote: > Do these have to be class members? Can this be const char kFoo[] and sit around > line 30? Done. https://codereview.chromium.org/2150543002/diff/120001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/2150543002/diff/120001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:38: static const char* const kFolderName; On 2016/07/20 19:15:07, Lei Zhang wrote: > Can these be static const char kFoo[] ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:58: if (result == InitResult::TEMPORARY_FAIL) On 2016/07/21 11:49:45, cfroussios wrote: > There might have been a miscommunication. I didn't say the behavior changes. My > objection is about readability. My objection is also about readability, but I just wanted to point out ordering does matter for completeness. I wasn't suggesting we change the order. > Logical operators draw attention to the logic of the condition, whereas ifs draw > attention to the flow of execution. Every time I've seen code in Chrome where > right eval matters, they've also added a comment to note that. That's because > one might reorder a condition without thinking much about it. But why do it in a > way that requires a note, when you can have code that speaks for itself? People > don't invert nested blocks expecting things to be the same. Can you provide some examples to back up your claim that everyone adds comments when the ordering matters? To counter your claim, look at https://cs.chromium.org/search/?q=GetAsDictionary%5C(.*%26%26&sq=package:chro... and see how many places first make sure base::Value::GetAsDictionary() succeeds before using the pointer passed in to it. Nobody writes a comment to explain the ordering. People don't randomly reorder the insides of conditionals either. The only time I've seen people bother to do so is when you have: if (ExpensiveConstFunc() || completely_unrelated_bool) in which case, reordering makes sense because |completely_unrelated_bool| is much faster to evaluate and it can evaluate to false faster than ExpensiveConstFunc(), where ExpensiveConstFunc() doesn't have any side effects. > Aesthetically, in your example, too many nested ifs waste screen space. However, > in my specific case, the result doesn't suffer much. I don't think there needs > to be a one-size-fits-all rule here, such that we need to discuss which rule > gets worse in its extreme. There's no one side fit all rule and my objection is not a strong one. In my experience, oftentimes nested conditionals that are not strictly required can make code harder to read, especially when the nesting goes too deep. That is why I suggested combining the two if statements, especially considering they fit on one line.
Description was changed from ========== OSCrypt supports encryption with KWallet Implemented KeyStorageLinux for KWallet. BUG=602624 ========== to ========== OSCrypt supports encryption with KWallet Implemented KeyStorageLinux for KWallet. BUG=602624 ==========
https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.h (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.h:18: std::string app_name); On 2016/07/21 11:49:46, cfroussios wrote: > On 2016/07/20 19:15:07, Lei Zhang wrote: > > On 2016/07/20 15:22:46, cfroussios wrote: > > > On 2016/07/20 01:08:26, Lei Zhang wrote: > > > > Just pass by ref and set app_name_? > > > > > > For constructors, this is supposed to perform better on average than passing > > by > > > reference. Not that this is likely to ever become hot. Is that a pattern we > > > don't care for in Chrome? > > > > Well, this is definitely not hot code, and I think (please double check) if > you > > want to use move semantics, you do "std::string&&" but the Chromium code base > > has 2 instances of that, so in general, we are not quite there yet. Plus, I > like > > |g_product_name| being set once and staying the same. > > You don't need the "std::string&&" to take advantage of move semantics, because > std::string already has a move constructor. The idea is that |app_name| will be > constructed with the best constructor in each case: > > 1. If the input can be moved, it will be moved into |app_name| and then into > |app_name_|. > 2. If stuff can't be moved, |app_name| will be a new copy, which will be moved > into |app_name_|. We would have had to create a copy anyway. +danakj to help provide guidance for the more academic portion of this discussion. In general, are we ok with having functions that pass objects by value with intention to std::move it? How do we make that intention clear? Always pass objects by const-ref is a bit engrained in my head. > I don't quite understand your comment about |g_product_name|. Are you saying > that, since it won't change, we can pass pointers to it? Or that we would have > to cannibalize it, thus changing it? In the way that KeyStorageKWallet is used > now, it won't actually be moved. It is just a good pattern for constructors, > because it works both when you can and can't move. I expect |g_product_name| to be set once by the higher level, and remain the same for the lifetime of its existence. Then it can be passed around by const-ref and be used and copied as needed. The std::move() in the KeyStorageKWallet ctor made me think this CL is trying to: - setting |g_product_name| once - clearing it as its value is being moved into KeyStorageKWallet::app_name but now I see it's not actually being moved... this is confusing. . o O (std::move() only casts, std::move() only casts)
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Hi. I think we've settled most things. I'm going to give a bit of time for danakj@ to respond. If what I'm doing is unusual, it can't hurt to be cautious and get another opinion on it. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.cc (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.cc:58: if (result == InitResult::TEMPORARY_FAIL) On 2016/07/21 21:07:39, Lei Zhang wrote: > On 2016/07/21 11:49:45, cfroussios wrote: > > There might have been a miscommunication. I didn't say the behavior changes. > My > > objection is about readability. > > My objection is also about readability, but I just wanted to point out ordering > does matter for completeness. I wasn't suggesting we change the order. > > > Logical operators draw attention to the logic of the condition, whereas ifs > draw > > attention to the flow of execution. Every time I've seen code in Chrome where > > right eval matters, they've also added a comment to note that. That's because > > one might reorder a condition without thinking much about it. But why do it in > a > > way that requires a note, when you can have code that speaks for itself? > People > > don't invert nested blocks expecting things to be the same. > > Can you provide some examples to back up your claim that everyone adds comments > when the ordering matters? To counter your claim, look at > https://cs.chromium.org/search/?q=GetAsDictionary%5C(.*%26%26&sq=package:chro... > and see how many places first make sure base::Value::GetAsDictionary() succeeds > before using the pointer passed in to it. Nobody writes a comment to explain the > ordering. > > People don't randomly reorder the insides of conditionals either. The only time > I've seen people bother to do so is when you have: > > if (ExpensiveConstFunc() || completely_unrelated_bool) > > in which case, reordering makes sense because |completely_unrelated_bool| is > much faster to evaluate and it can evaluate to false faster than > ExpensiveConstFunc(), where ExpensiveConstFunc() doesn't have any side effects. > > > Aesthetically, in your example, too many nested ifs waste screen space. > However, > > in my specific case, the result doesn't suffer much. I don't think there needs > > to be a one-size-fits-all rule here, such that we need to discuss which rule > > gets worse in its extreme. > > There's no one side fit all rule and my objection is not a strong one. In my > experience, oftentimes nested conditionals that are not strictly required can > make code harder to read, especially when the nesting goes too deep. That is why > I suggested combining the two if statements, especially considering they fit on > one line. "Every time I've seen code in Chrome" is not comparable to "everyone" in my case, I'm afraid. The easiest example is the code that this came from https://cs.chromium.org/chromium/src/chrome/browser/password_manager/native_b... I've seen it once or twice more, but I don't quickly recall how to search for it. My experience with readability comes from outside of Chrome, so if you say that this is normal in Chrome, let's go with your experience. https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... File components/os_crypt/key_storage_kwallet.h (right): https://codereview.chromium.org/2150543002/diff/80001/components/os_crypt/key... components/os_crypt/key_storage_kwallet.h:18: std::string app_name); On 2016/07/21 21:25:31, Lei Zhang wrote: > On 2016/07/21 11:49:46, cfroussios wrote: > > On 2016/07/20 19:15:07, Lei Zhang wrote: > > > On 2016/07/20 15:22:46, cfroussios wrote: > > > > On 2016/07/20 01:08:26, Lei Zhang wrote: > > > > > Just pass by ref and set app_name_? > > > > > > > > For constructors, this is supposed to perform better on average than > passing > > > by > > > > reference. Not that this is likely to ever become hot. Is that a pattern > we > > > > don't care for in Chrome? > > > > > > Well, this is definitely not hot code, and I think (please double check) if > > you > > > want to use move semantics, you do "std::string&&" but the Chromium code > base > > > has 2 instances of that, so in general, we are not quite there yet. Plus, I > > like > > > |g_product_name| being set once and staying the same. > > > > You don't need the "std::string&&" to take advantage of move semantics, > because > > std::string already has a move constructor. The idea is that |app_name| will > be > > constructed with the best constructor in each case: > > > > 1. If the input can be moved, it will be moved into |app_name| and then into > > |app_name_|. > > 2. If stuff can't be moved, |app_name| will be a new copy, which will be moved > > into |app_name_|. We would have had to create a copy anyway. > > +danakj to help provide guidance for the more academic portion of this > discussion. In general, are we ok with having functions that pass objects by > value with intention to std::move it? How do we make that intention clear? > Always pass objects by const-ref is a bit engrained in my head. > > > I don't quite understand your comment about |g_product_name|. Are you saying > > that, since it won't change, we can pass pointers to it? Or that we would have > > to cannibalize it, thus changing it? In the way that KeyStorageKWallet is used > > now, it won't actually be moved. It is just a good pattern for constructors, > > because it works both when you can and can't move. > > I expect |g_product_name| to be set once by the higher level, and remain the > same for the lifetime of its existence. Then it can be passed around by > const-ref and be used and copied as needed. The std::move() in the > KeyStorageKWallet ctor made me think this CL is trying to: > - setting |g_product_name| once > - clearing it as its value is being moved into KeyStorageKWallet::app_name > > but now I see it's not actually being moved... this is confusing. . o O > (std::move() only casts, std::move() only casts) Yes, I am only defending this pattern in general. In this particular case, it makes no real difference. We agree that |g_product_name| should not be a temporary. I tried to find documentation on the pattern, but I didn't find any specific mentions. That's a bit weird, since I only learned about it from people within Google. I'm really curious why it's not more common. If there is a pitfall I am not aware of, or if it is considered too foreign and confusing for Chrome code, I should know before slipping it past other reviews.
On 2016/07/22 11:25:25, cfroussios wrote: > The easiest example is the code that this came from > https://cs.chromium.org/chromium/src/chrome/browser/password_manager/native_b... Well, that's a more complicated statement to evaluate. If there's something tricky / subtle, then it would certainly be good to note what's going on or break the statement apart for the benefit of the reader. I just don't think our simpler case is one of them, plus you already have comment above to explain what's going on. Thus I don't think the nesting in this particular case helps.
On 2016/07/22 11:25:25, cfroussios wrote: > Hi. I think we've settled most things. I'm going to give a bit of time for > danakj@ to respond. If what I'm doing is unusual, it can't hurt to be cautious > and get another opinion on it. Please feel free to land this and not stall out. I had a discussion about passing-by-value on another CL, though in that CL, the caller always std::move()'d, whereas here, the only caller does not. It felt unusual to me mainly because we've done always pass-by-const-ref for years, so it's a bit engrained. Given this is a thing now, I'll stop being a grumpy old fart and get with the program.
Cool! Thanks!
The CQ bit was checked by cfroussios@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2150543002/#ps180001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== OSCrypt supports encryption with KWallet Implemented KeyStorageLinux for KWallet. BUG=602624 ========== to ========== OSCrypt supports encryption with KWallet Implemented KeyStorageLinux for KWallet. BUG=602624 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== OSCrypt supports encryption with KWallet Implemented KeyStorageLinux for KWallet. BUG=602624 ========== to ========== OSCrypt supports encryption with KWallet Implemented KeyStorageLinux for KWallet. BUG=602624 Committed: https://crrev.com/2e6729a44f5a07c939209bce99f803dc90c5966f Cr-Commit-Position: refs/heads/master@{#407746} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2e6729a44f5a07c939209bce99f803dc90c5966f Cr-Commit-Position: refs/heads/master@{#407746}
Message was sent while issue was closed.
https://codereview.chromium.org/2150543002/diff/180001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/2150543002/diff/180001/components/os_crypt/ke... components/os_crypt/key_storage_linux.cc:20: #if defined(OFFICIAL_BUILD) There is a minor bug here in Chromium reporting itself as Chrome, since Chromium is usually also build with IS_OFFICIAL_BUILD. GOOGLE_CHROME_BUILD works.
Message was sent while issue was closed.
I mean is_official_build. https://cs.chromium.org/chromium/src/build/config/BUILD.gn?l=249
Message was sent while issue was closed.
https://codereview.chromium.org/2150543002/diff/180001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/2150543002/diff/180001/components/os_crypt/ke... components/os_crypt/key_storage_linux.cc:20: #if defined(OFFICIAL_BUILD) On 2016/08/21 07:36:53, pdknsk wrote: > There is a minor bug here in Chromium reporting itself as Chrome, since Chromium > is usually also build with IS_OFFICIAL_BUILD. GOOGLE_CHROME_BUILD works. Thanks for noticing. Will fix.
Message was sent while issue was closed.
On 2016/08/22 17:09:50, Lei Zhang wrote: > https://codereview.chromium.org/2150543002/diff/180001/components/os_crypt/ke... > File components/os_crypt/key_storage_linux.cc (right): > > https://codereview.chromium.org/2150543002/diff/180001/components/os_crypt/ke... > components/os_crypt/key_storage_linux.cc:20: #if defined(OFFICIAL_BUILD) > On 2016/08/21 07:36:53, pdknsk wrote: > > There is a minor bug here in Chromium reporting itself as Chrome, since > Chromium > > is usually also build with IS_OFFICIAL_BUILD. GOOGLE_CHROME_BUILD works. > > Thanks for noticing. Will fix. Could we elaborate a little on which distribution is both "official" and "chromium"? My information is that Google doesn't distribute Chromium builds, and nobody else can make official builds.
Message was sent while issue was closed.
On 2016/08/22 17:09:50, Lei Zhang wrote: > Thanks for noticing. Will fix. https://codereview.chromium.org/2264163002
Message was sent while issue was closed.
On 2016/08/22 17:13:33, cfroussios wrote: > Could we elaborate a little on which distribution is both "official" and > "chromium"? My information is that Google doesn't distribute Chromium builds, > and nobody else can make official builds. There may be Chromium packagers who turn on OFFICIAL_BUILD? In any case, we shouldn't confuse OFFICIAL_BUILD and GOOGLE_CHROME_BUILD.
Message was sent while issue was closed.
On 2016/08/22 17:37:33, Lei Zhang wrote: > On 2016/08/22 17:13:33, cfroussios wrote: > > Could we elaborate a little on which distribution is both "official" and > > "chromium"? My information is that Google doesn't distribute Chromium builds, > > and nobody else can make official builds. > > There may be Chromium packagers who turn on OFFICIAL_BUILD? In any case, we > shouldn't confuse OFFICIAL_BUILD and GOOGLE_CHROME_BUILD. I have no objection to correcting this. Thanks both for addressing the issue! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
