Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2027063002:
Introduce UserHiveVisitor for performing operations on local users' registry hives. (Closed)
|
Created:
4 years, 6 months ago by grt (UTC plus 2) Modified:
4 years, 6 months ago Reviewers:
gab CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce UserHiveVisitor for performing operations on local users' registry hives.
BUG=none
R=gab@chromium.org
Committed: https://crrev.com/05cc7e91866921b919e7f2909df6ae2f4d1735d4
Cr-Commit-Position: refs/heads/master@{#398026}
Patch Set 1 #Patch Set 2 : added missing :: #
Total comments: 4
Patch Set 3 : tweaks #
Total comments: 15
Patch Set 4 : gab review #Patch Set 5 : gab review #
Messages
Total messages: 32 (11 generated)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2027063002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2027063002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2027063002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2027063002/20001
PTAL
The diff is fairly hard to review since it wasn't moved as-is. Could you make a CL that makes the changes you want inline in uninstall.cc (i.e. make it generic but keep single use case) and then a follow-up CL that moves the code as-is? A few notes as I went below but I stopped short because of the above. https://codereview.chromium.org/2027063002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor.cc (right): https://codereview.chromium.org/2027063002/diff/40001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.cc:131: result = key.ReadValue(kProfileImagePathValue, &image_path); I'd expect keeping L"ProfileImagePath" inline here to result in an implicit constexpr but I might be wrong.. if so, I prefer inline. https://codereview.chromium.org/2027063002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor.h (right): https://codereview.chromium.org/2027063002/diff/40001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.h:23: using HiveVisitor = base::Callback<bool(const wchar_t*, base::win::RegKey*)>; Name parameters here and use their names in comment above.
https://codereview.chromium.org/2027063002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor.cc (right): https://codereview.chromium.org/2027063002/diff/40001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.cc:131: result = key.ReadValue(kProfileImagePathValue, &image_path); On 2016/06/01 14:34:44, gab wrote: > I'd expect keeping L"ProfileImagePath" inline here to result in an implicit > constexpr but I might be wrong.. if so, I prefer inline. Done. https://codereview.chromium.org/2027063002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor.h (right): https://codereview.chromium.org/2027063002/diff/40001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.h:23: using HiveVisitor = base::Callback<bool(const wchar_t*, base::win::RegKey*)>; On 2016/06/01 14:34:44, gab wrote: > Name parameters here and use their names in comment above. Done.
On 2016/06/01 14:34:44, gab wrote: > The diff is fairly hard to review since it wasn't moved as-is. Could you make a > CL that makes the changes you want inline in uninstall.cc (i.e. make it generic > but keep single use case) and then a follow-up CL that moves the code as-is? Making it generic w/ a single use in uninstall.cc would still yield a large red blob that was removed and a large green blob that was added. The difference would be that they'd be in the same file rather than in two files. Am I misunderstanding the request?
On 2016/06/01 14:41:59, grt (slow) wrote: > On 2016/06/01 14:34:44, gab wrote: > > The diff is fairly hard to review since it wasn't moved as-is. Could you make > a > > CL that makes the changes you want inline in uninstall.cc (i.e. make it > generic > > but keep single use case) and then a follow-up CL that moves the code as-is? > > Making it generic w/ a single use in uninstall.cc would still yield a large red > blob that was removed and a large green blob that was added. The difference > would be that they'd be in the same file rather than in two files. Am I > misunderstanding the request? If you put the "generic" method just below UninstallActiveSetupEntries() I think the diff would show just what was changed in the actual hive iteration code?
On 2016/06/01 15:22:42, gab wrote: > On 2016/06/01 14:41:59, grt (slow) wrote: > > On 2016/06/01 14:34:44, gab wrote: > > > The diff is fairly hard to review since it wasn't moved as-is. Could you > make > > a > > > CL that makes the changes you want inline in uninstall.cc (i.e. make it > > generic > > > but keep single use case) and then a follow-up CL that moves the code as-is? > > > > Making it generic w/ a single use in uninstall.cc would still yield a large > red > > blob that was removed and a large green blob that was added. The difference > > would be that they'd be in the same file rather than in two files. Am I > > misunderstanding the request? > > If you put the "generic" method just below UninstallActiveSetupEntries() I think > the diff would show just what was changed in the actual hive iteration code? My simple attempt to put the new impl next to the old one didn't yield an interesting diff on rietveld. It isn't really all that much code. Could you review it as-is? Perhaps open the old and new in two browser windows that are side-by side?
lg, few comments https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/uninstall.cc:775: std::vector<base::string16> paths = {active_setup_path, StringPiece16 would avoid copies here and would be fine since this is synchronous or string16* https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor.cc (right): https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.cc:81: LONG result = ::RegUnLoadKey(HKEY_LOCAL_MACHINE, subkey_name_.c_str()); Should we only do the work in this destructor if valid()? (this is at least what the previous code used to do in its own way) https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.cc:83: return; I'd prefer to handle failure here to keep the flow similar to when the function doesn't end right after. https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.cc:129: base::string16 image_path; |image_path| isn't required if hive is already mounted (i.e. this can go after 137-144) https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor_unittest.cc (right): https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor_unittest.cc:29: return !max_count_ ? true : (sids_visited_.size() < max_count_); So |max_count_| is always 0 or 1. Make it a bool? https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor_unittest.cc:55: EXPECT_LT(0U, visitor.sids_visited().size()); Inequalities are more readable with constant on RHS IMO.
Thanks. PTAL. https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/uninstall.cc:775: std::vector<base::string16> paths = {active_setup_path, On 2016/06/01 16:17:03, gab wrote: > StringPiece16 would avoid copies here and would be fine since this is > synchronous or string16* Used const base::string16* since c_str() is needed (this doesn't exist for StringPiece). https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor.cc (right): https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.cc:81: LONG result = ::RegUnLoadKey(HKEY_LOCAL_MACHINE, subkey_name_.c_str()); On 2016/06/01 16:17:03, gab wrote: > Should we only do the work in this destructor if valid()? > > (this is at least what the previous code used to do in its own way) No. It's possible for RegLoadKey on line 59 to succeed (in which case subkey_name_ is non-empty as per the comment on line 40-41) but Open on line 69 to fail. https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.cc:83: return; On 2016/06/01 16:17:03, gab wrote: > I'd prefer to handle failure here to keep the flow similar to when the function > doesn't end right after. Done (I think?). https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.cc:129: base::string16 image_path; On 2016/06/01 16:17:03, gab wrote: > |image_path| isn't required if hive is already mounted (i.e. this can go after > 137-144) Done. https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor_unittest.cc (right): https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor_unittest.cc:55: EXPECT_LT(0U, visitor.sids_visited().size()); On 2016/06/01 16:17:04, gab wrote: > Inequalities are more readable with constant on RHS IMO. Okay. I had done it this way for consistency with EXPECT_EQ and EXPECT_NE, which are documented as taking the "expected" value on the LHS.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2027063002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2027063002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/01 19:23:51, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. ping!
lgtm w/ comments (sorry for delay)
Can you run this test manually just to be sure:
1) Install Chrome system-level
2) Login to another Windows profile
3) Confirm that HKCU\Software\Wow6432Node\Microsoft\Active Setup\...\{8A...} is
set
4) Logout of that Windows profile
5) Uninstall Chrome
6) Log back in to that user profile and confirm that
HKCU\Software\Wow6432Node\Microsoft\Active Setup\...\{8A...} is gone.
(Wow6432Node might not be required if 64-bit build -- makes me think that this
is probably a problem for many registry keys when we auto-migrate people to
64-bit installs?)
https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/user_hive_visitor.cc (right):
https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/user_hive_visitor.cc:81: LONG result =
::RegUnLoadKey(HKEY_LOCAL_MACHINE, subkey_name_.c_str());
On 2016/06/01 18:00:47, grt (slow) wrote:
> On 2016/06/01 16:17:03, gab wrote:
> > Should we only do the work in this destructor if valid()?
> >
> > (this is at least what the previous code used to do in its own way)
>
> No. It's possible for RegLoadKey on line 59 to succeed (in which case
> subkey_name_ is non-empty as per the comment on line 40-41) but Open on line
69
> to fail.
Ah I see, hadn't noticed that |subkey_name_| being set specifically after
RegLoadKey had that purpose. Can you document that (suggest both at the move
site on 66 and early return here on 79).
Another option (think I prefer that): don't use a local variable for the name,
put it directly in |subkey_name_| but clear() it on error with a comment. That
plus comment on variable saying it's not set if not loaded should make it clear
(and not require a comment here on 79 also).
https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/...
File chrome/installer/setup/user_hive_visitor_unittest.cc (right):
https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/...
chrome/installer/setup/user_hive_visitor_unittest.cc:55: EXPECT_LT(0U,
visitor.sids_visited().size());
On 2016/06/01 18:00:47, grt (slow) wrote:
> On 2016/06/01 16:17:04, gab wrote:
> > Inequalities are more readable with constant on RHS IMO.
>
> Okay. I had done it this way for consistency with EXPECT_EQ and EXPECT_NE,
which
> are documented as taking the "expected" value on the LHS.
Right but reading inequalities backward is so weird that I prefer the
inconsistency.
Alternatively since this is unsigned you could EXPECT_NE(0U, ...size());
Thanks. Will do the manual test before landing. https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor.cc (right): https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor.cc:81: LONG result = ::RegUnLoadKey(HKEY_LOCAL_MACHINE, subkey_name_.c_str()); On 2016/06/03 15:13:08, gab wrote: > On 2016/06/01 18:00:47, grt (slow) wrote: > > On 2016/06/01 16:17:03, gab wrote: > > > Should we only do the work in this destructor if valid()? > > > > > > (this is at least what the previous code used to do in its own way) > > > > No. It's possible for RegLoadKey on line 59 to succeed (in which case > > subkey_name_ is non-empty as per the comment on line 40-41) but Open on line > 69 > > to fail. > > Ah I see, hadn't noticed that |subkey_name_| being set specifically after > RegLoadKey had that purpose. Can you document that (suggest both at the move > site on 66 and early return here on 79). > > Another option (think I prefer that): don't use a local variable for the name, > put it directly in |subkey_name_| but clear() it on error with a comment. That > plus comment on variable saying it's not set if not loaded should make it clear > (and not require a comment here on 79 also). Done. https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/user_hive_visitor_unittest.cc (right): https://codereview.chromium.org/2027063002/diff/60001/chrome/installer/setup/... chrome/installer/setup/user_hive_visitor_unittest.cc:55: EXPECT_LT(0U, visitor.sids_visited().size()); On 2016/06/03 15:13:08, gab wrote: > On 2016/06/01 18:00:47, grt (slow) wrote: > > On 2016/06/01 16:17:04, gab wrote: > > > Inequalities are more readable with constant on RHS IMO. > > > > Okay. I had done it this way for consistency with EXPECT_EQ and EXPECT_NE, > which > > are documented as taking the "expected" value on the LHS. > > Right but reading inequalities backward is so weird that I prefer the > inconsistency. I agreed with your logic and changed it as you requested. My remark was just explaining the original code, not pushing back on the request. :-)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2027063002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2027063002/#ps100001 (title: "gab review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2027063002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Introduce UserHiveVisitor for performing operations on local users' registry hives. BUG=none R=gab@chromium.org ========== to ========== Introduce UserHiveVisitor for performing operations on local users' registry hives. BUG=none R=gab@chromium.org Committed: https://crrev.com/05cc7e91866921b919e7f2909df6ae2f4d1735d4 Cr-Commit-Position: refs/heads/master@{#398026} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/05cc7e91866921b919e7f2909df6ae2f4d1735d4 Cr-Commit-Position: refs/heads/master@{#398026} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
