Move shell_util's RegistryEntry class into a separate file.

Move shell_util's RegistryEntry class into a separate file.

Makes shell_util more modular and breaks it up a bit. RegistryEntry is a
generally useful API.

BUG=419972
Committed: https://crrev.com/3bfee5e3676510bf8348bb46334a7dbea4a0a181
Cr-Commit-Position: refs/heads/master@{#362923}

[Matt Giuca, 2014-10-03 04:55:07]

mgiuca@chromium.org changed reviewers:
+ gab@chromium.org

[Matt Giuca, 2014-10-03 04:55:07]

Hi Gabriel,

Here's the RegistryEntry refactor that we discussed briefly in the previous CL,
and which I provided a rationale for at http://crbug.com/419972.

This isn't urgent; we can discuss later (I see you are going on vacation for 3
weeks, no need to rush). I'm not blocking on it and if it doesn't go ahead,
that's fine.

[Will Harris, 2014-10-03 05:13:45]

wfh@chromium.org changed reviewers:
+ wfh@chromium.org

[Will Harris, 2014-10-03 05:13:45]

comments from the peanut gallery

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.cc (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:42: items->AddCreateRegKeyWorkItem(root,
key_path_, WorkItem::kWow64Default);
Will this class ever be used to access 64-bit or 32-bit hives - using
KEY_WOW64_64KEY or KEY_WOW64_32KEY?  If so, this should be a parameter to the
constructor?

[Matt Giuca, 2014-10-03 10:09:39]

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.cc (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:42: items->AddCreateRegKeyWorkItem(root,
key_path_, WorkItem::kWow64Default);
No idea --- note that I'm just moving this code here (not writing it) and I
don't think it's a good idea to modify said code in the same CL.

If you think it should be altered or made more general in some way, please
submit a follow-up or precursor patch. (If you don't want to wait for this CL,
just make a patch against the existing RegistryEntry class in shell_util.cc and
I will merge your change in.)

[grt (UTC plus 2), 2014-10-03 14:31:06]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2014-10-03 14:31:07]

nice refactor. please add a unittest.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.cc (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:16: is_string_(true),
      type_(REG_SZ),
      value_(reinterpret_cast<const uint8_t*>(value.c_str()),
             reinterpret_cast<const uint8_t*>(value.c_str()) +
                 (value.size() + 1) * sizeof(base::string16::value_type)))

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:26: is_string_(true),
as above

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:36: is_string_(false),
      type_(REG_DWORD),
      value_(reinterpret_cast<const uint8_t*>(&value),
             reinterpret_cast<const uint8_t*>(&value) + sizeof(value))

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:43: if (is_string_) {
  items->AddSetRegValueWorkItem(root_, key_path_, WorkItem::kWow64Default,
name_, type_, value_, true);

looks like this method doesn't exist in the WorkItem family. adding it is a
bunch of copy-n-paste.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:79: if (is_string_) {
since this has string-specific handling, you could do something like:
  if (type_ == REG_SZ) {
    ...same as before...
  } else {
    std::vector<uint8_t> buffer(value_.size());
    DWORD data_size = buffer.size();
    DWORD data_type = REG_NONE;
    LONG result = key.ReadValue(name_.c_str(), &buffer[0], &data_size,
                                &data_type);
    found = (result == ERROR_SUCCESS || result == ERROR_MORE_DATA);
    if (found && data_size == value_.size() && data_type == type_)
      correct_value = buffer == value_;
  }

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.h (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.h:80: base::string16 value_;     // string
value (useful if is_string_ = true)
this class would be more flexible if is_string_, value_, and int_value_ were
replaced with:
  DWORD type_;
  std::vector<uint8_t> value_;
see my comments in the implementation for how these would be used.

i would also give the class a single public ctor along these lines:
  RegistryEntry(const base::string16& key_path,
                const base::string16& name,
                DWORD type,
                const uint8_t* data,
                size_t data_size);
and then a set of builders like:
  static scoped_ptr<RegistryEntry> CreateString(const base::string16& key_path,
                                                const base::string16& name,
                                                const base::string16& value);
that simply new up an instance, calling the ctor as appropriate,

[gab, 2014-10-03 15:40:03]

Overall idea looks good to me. I'll leave you in the hands of grt for this
review since I won't be able to finish it before leaving.

Two things:
 1) One concern I have is that I actually liked that "not everybody could create
RegistryEntry's" forcing users of the API to use the appropriate patterns. I
guess your comment at the top of the extracted class is sufficient to cover
this, but I'm torn.
Ideally this would be moved into a components/ and its usage would thus be
explicitly controlled through DEPS.

 2) Another very nice refactor that could/should follow this is to move all of
the registry specific code out of shell_util.cc into shell_registry_util.cc (and
perhaps similarly for shell_shortcut_util.cc) as this file is way convoluted
with unrelated code.

Cheers and thanks for working on this!
Gab

[Matt Giuca, 2015-01-20 02:11:37]

Patchset #3 (id:40001) has been deleted

[Matt Giuca, 2015-01-20 02:41:17]

Hi all,

I am finally getting back around to this CL. Sorry it has taken so long, but I
got busy and then merge conflicts happened.

So, I have considered all of the comments on here, but I am going to be firm
about *NOT* doing them in this CL. I want this submitted ASAP: if someone
modifies the code while this CL is in flight, I will have to manually deal with
the merge conflicts, and it runs the risk that I will make a mistake. This
happened several times in the last few months and ultimately I decided the only
way to be sure I didn't make any mistakes was to recreate the CL from scratch. I
don't want to have to do this again.

It is inappropriate to be making semantic changes at the same time as a big
move. Let's suppose that I make a mistake or do something someone disagrees
with, and they bisect down to find the cause of the issue, and find that it was
done in this humongous CL which nobody can properly read the diff of. They will
not be very happy. Therefore I don't want to add unit tests, change constructors
or the internal representation of RegistryEntry, or any other suggested changes
here. The code that is being moved should not be changed more than the minimum
necessary.

gab:
> One concern I have is that I actually liked that "not everybody could create
> RegistryEntry's" forcing users of the API to use the appropriate patterns. I
> guess your comment at the top of the extracted class is sufficient to cover
> this, but I'm torn.
> Ideally this would be moved into a components/ and its usage would thus be
> explicitly controlled through DEPS.

We discussed this a long time ago. As I argued back then, I don't see the
dilemma. RegistryEntry is a class that abstracts over Windows' registry APIs and
allows clients to declaratively set up new registry entries. It is a very nice
abstraction, and it makes no sense to force all users of the class to be static
methods inside the class itself, just because you don't trust people to use it
correctly.

You could apply this logic to virtually any class: "I wrote class X, and I don't
trust anybody to use class X correctly; therefore I am making its constructors
private, and anybody who uses it needs to add themselves as a static method
inside X." That is an abuse of OO patterns and leads to giant monolithic classes
(like RegistryEntry before this CL), and confusion about responsibility.

Anyway, it's your call if you want this CL, but this is my proposed solution to
the monolithic nature of RegistryEntry.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.cc (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:16: is_string_(true),
On 2014/10/03 14:31:07, grt wrote:
>       type_(REG_SZ),
>       value_(reinterpret_cast<const uint8_t*>(value.c_str()),
>              reinterpret_cast<const uint8_t*>(value.c_str()) +
>                  (value.size() + 1) * sizeof(base::string16::value_type)))

Acknowledged.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:26: is_string_(true),
On 2014/10/03 14:31:07, grt wrote:
> as above

Acknowledged.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:36: is_string_(false),
On 2014/10/03 14:31:07, grt wrote:
>       type_(REG_DWORD),
>       value_(reinterpret_cast<const uint8_t*>(&value),
>              reinterpret_cast<const uint8_t*>(&value) + sizeof(value))

Acknowledged.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:43: if (is_string_) {
Again, should not be changed here.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:79: if (is_string_) {
Again, not here.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.h (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.h:80: base::string16 value_;     // string
value (useful if is_string_ = true)
OK, I totally agree that this is better, but it is not appropriate in this CL.
It is never a good idea to mix semantic changes with large amounts of code being
moved around. I will take this into consideration after landing the big one (or
you can make that change if you like).

[grt (UTC plus 2), 2015-01-20 20:05:32]

I'm fine with the is_string staying until a subsequent cleanup, but the Wow64
thing needs to be addressed. Thanks.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.cc (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:42: items->AddCreateRegKeyWorkItem(root,
key_path_, WorkItem::kWow64Default);
On 2014/10/03 10:09:39, Matt Giuca wrote:
> No idea --- note that I'm just moving this code here (not writing it) and I
> don't think it's a good idea to modify said code in the same CL.
> 
> If you think it should be altered or made more general in some way, please
> submit a follow-up or precursor patch. (If you don't want to wait for this CL,
> just make a patch against the existing RegistryEntry class in shell_util.cc
and
> I will merge your change in.)

This CL takes a private class and makes it public. This may require small
changes to make it appropriate for general consumption. For RegistryEntry to be
generally useful, it must take a "REGSAM wow64_access" param in its ctors that
it passes down to the work items in place of WorkItem::kWow64Default and that it
uses in StatusInRegistryUnderRoot to probe the registry. kWow64Default was
correct for all uses in shell_util.cc, but it isn't necessarily correct for all
potential consumers of RegistryEntry once it's publicly available. The point of
that extra parameter is to force consumers to make a conscious choice. Leaving
it for later opens up the possibility of RegistryEntry consumers doing the wrong
thing because they thought the API was somehow safe and protected them from the
reality of the registry.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.h (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.h:80: base::string16 value_;     // string
value (useful if is_string_ = true)
On 2015/01/20 02:41:17, Matt Giuca wrote:
> OK, I totally agree that this is better, but it is not appropriate in this CL.
> It is never a good idea to mix semantic changes with large amounts of code
being
> moved around.

Not to nitpick, but I proposed an implementation change rather than a semantic
change. That aside, tests should prove that such changes are safe. It'd be nice
to have at least rudimentary unit test for this class now that it's public.
Would you add one?

[Matt Giuca, 2015-01-20 23:16:35]

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.cc (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:42: items->AddCreateRegKeyWorkItem(root,
key_path_, WorkItem::kWow64Default);
OK, if you really feel strongly about this, then I will add this parameter in a
precursor CL. I feel equally strongly about not changing the interface in the
same CL as moving it.

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.h (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.h:80: base::string16 value_;     // string
value (useful if is_string_ = true)
On 2015/01/20 20:05:31, grt wrote:
> Not to nitpick, but I proposed an implementation change rather than a semantic
> change.

semantics != interface. I meant a semantic change as opposed to a syntactic
change (i.e., anything which changes the meaning of the program, as opposed to
the formatting and arrangement).

> That aside, tests should prove that such changes are safe. It'd be nice
> to have at least rudimentary unit test for this class now that it's public.
> Would you add one?

OK I will add a test case.

[grt (UTC plus 2), 2015-01-20 23:30:56]

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.cc (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.cc:42: items->AddCreateRegKeyWorkItem(root,
key_path_, WorkItem::kWow64Default);
On 2015/01/20 23:16:35, Matt Giuca wrote:
> OK, if you really feel strongly about this, then I will add this parameter in
a
> precursor CL. I feel equally strongly about not changing the interface in the
> same CL as moving it.

I think it's a waste of both of our time to do it in a separate CL. The
parameter wasn't needed when it was a private class since no callers needed it.
By making the class public, it really needs to be correct for any caller who
tries to use it.

If it helps you sleep at night, feel free to tell yourself that an OWNER asked
you to defy your own sense of what is right. :-)

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
File chrome/installer/util/registry_entry.h (right):

https://codereview.chromium.org/623903002/diff/1/chrome/installer/util/regist...
chrome/installer/util/registry_entry.h:80: base::string16 value_;     // string
value (useful if is_string_ = true)
On 2015/01/20 23:16:35, Matt Giuca wrote:
> On 2015/01/20 20:05:31, grt wrote:
> > Not to nitpick, but I proposed an implementation change rather than a
semantic
> > change.
> 
> semantics != interface. I meant a semantic change as opposed to a syntactic
> change (i.e., anything which changes the meaning of the program, as opposed to
> the formatting and arrangement).

I don't think my proposal changed the meaning or behavior of the class from the
caller's point of view. It change the implementation. Callers wouldn't care.
This is why regression tests are awesome. Such changes are easy to make and
validate.

> > That aside, tests should prove that such changes are safe. It'd be nice
> > to have at least rudimentary unit test for this class now that it's public.
> > Would you add one?
> 
> OK I will add a test case.

Thanks!

[gab, 2015-01-21 21:20:01]

I appreciate your urge to minimize churn with this code move.

In that spirit, how about we do this in two steps to minimize churn and make for
nicer diffs:
 1) Make all the changes to make for a public RegistryEntry interface in
shell_util.cc while keeping the RegistryEntry class there.
 2) Move the RegistryEntry class outside shell_util.cc

I think the diff tool will have much less trouble coping with (1) then with
(1)+(2) at once: as-is right now, although I trust you, it's pretty much
impossible to proof-read this code move (which also contains some extra
(non-move-only) changes to the public API of RegistryEntry which, as you
mentioned yourself, should be avoided in a code move).

In (2) you can then play with "git cl upload --similarity=XX" until you find
what makes it realize it's a copy+diff from shell_util.cc which will highlight
only the bits that changed in the copy because of the API changes + Greg's
request.


One last thing, having everything under static calls in RegistryEntry at least
had the advantage of grouping all registry-list-building methods. It'd be nice
to consider moving all of those (and the other registry APIs) to
shell_registry_util.cc or something.


Thanks,
Gab

[gab, 2015-11-16 16:58:24]

On 2015/01/21 21:20:01, gab wrote:
> I appreciate your urge to minimize churn with this code move.
> 
> In that spirit, how about we do this in two steps to minimize churn and make
for
> nicer diffs:
>  1) Make all the changes to make for a public RegistryEntry interface in
> shell_util.cc while keeping the RegistryEntry class there.
>  2) Move the RegistryEntry class outside shell_util.cc
> 
> I think the diff tool will have much less trouble coping with (1) then with
> (1)+(2) at once: as-is right now, although I trust you, it's pretty much
> impossible to proof-read this code move (which also contains some extra
> (non-move-only) changes to the public API of RegistryEntry which, as you
> mentioned yourself, should be avoided in a code move).
> 
> In (2) you can then play with "git cl upload --similarity=XX" until you find
> what makes it realize it's a copy+diff from shell_util.cc which will highlight
> only the bits that changed in the copy because of the API changes + Greg's
> request.
> 
> 
> One last thing, having everything under static calls in RegistryEntry at least
> had the advantage of grouping all registry-list-building methods. It'd be nice
> to consider moving all of those (and the other registry APIs) to
> shell_registry_util.cc or something.
> 
> 
> Thanks,
> Gab

Hey Matt, ping to see whether you still intend to complete this? I like the
refactoring with the additional suggestions above to make the move easy to
review.

Cheers,
Gab

[Matt Giuca, 2015-11-25 03:37:04]

On 2015/11/16 16:58:24, gab wrote:
> Hey Matt, ping to see whether you still intend to complete this? I like the
> refactoring with the additional suggestions above to make the move easy to
> review.

Hi Gab,

Sorry for silence on this. TBH this fell off my radar many months ago. Other
things came up and this refactor was no longer something I needed.

I suspect the whole thing will have to be done again from the start (since the
changes can't be merged). I stopped keeping this branch up-to-date months ago
after I got a giant merge conflict. Well I seem to have some spare cycles now so
I will try your suggestion from #14. But if this takes too much effort, I likely
won't have the cycles to complete it (note that this is NOT a priority for me).

[Matt Giuca, 2015-11-25 06:11:18]

On 2015/11/25 03:37:04, Matt Giuca wrote:
> On 2015/11/16 16:58:24, gab wrote:
> > Hey Matt, ping to see whether you still intend to complete this? I like the
> > refactoring with the additional suggestions above to make the move easy to
> > review.
> 
> Hi Gab,
> 
> Sorry for silence on this. TBH this fell off my radar many months ago. Other
> things came up and this refactor was no longer something I needed.
> 
> I suspect the whole thing will have to be done again from the start (since the
> changes can't be merged). I stopped keeping this branch up-to-date months ago
> after I got a giant merge conflict. Well I seem to have some spare cycles now
so
> I will try your suggestion from #14. But if this takes too much effort, I
likely
> won't have the cycles to complete it (note that this is NOT a priority for
me).

First part: https://codereview.chromium.org/1472273003/

[Matt Giuca, 2015-12-02 02:42:03]

On 2015/11/25 06:11:18, Matt Giuca wrote:
> On 2015/11/25 03:37:04, Matt Giuca wrote:
> > On 2015/11/16 16:58:24, gab wrote:
> > > Hey Matt, ping to see whether you still intend to complete this? I like
the
> > > refactoring with the additional suggestions above to make the move easy to
> > > review.
> > 
> > Hi Gab,
> > 
> > Sorry for silence on this. TBH this fell off my radar many months ago. Other
> > things came up and this refactor was no longer something I needed.
> > 
> > I suspect the whole thing will have to be done again from the start (since
the
> > changes can't be merged). I stopped keeping this branch up-to-date months
ago
> > after I got a giant merge conflict. Well I seem to have some spare cycles
now
> so
> > I will try your suggestion from #14. But if this takes too much effort, I
> likely
> > won't have the cycles to complete it (note that this is NOT a priority for
> me).
> 
> First part: https://codereview.chromium.org/1472273003/

Second part: https://codereview.chromium.org/1486493002
Third part: https://codereview.chromium.org/1486513002

[Matt Giuca, 2015-12-02 02:46:21]

Description was changed from

==========
Split up shell_util's RegistryEntry class into a separate file.

This makes it possible to use RegistryEntry to add new keys and values
to the Windows registry without having to add new static methods to
RegistryEntry itself.

1. Made the RegistryEntry constructors public.

2. Moved ApplicationInfo and all static methods out of RegistryEntry.
(These didn't really belong in RegistryEntry; they were just there so
they could use its constructor.)

3. Moved the RegistryEntry class from shell_util.cc to its own file.

BUG=419972
==========

to

==========
Move shell_util's RegistryEntry class into a separate file.

Makes shell_util more modular and breaks it up a bit. RegistryEntry is a
generally useful API.

BUG=419972
==========

[Matt Giuca, 2015-12-02 02:46:33]

Description was changed from

==========
Move shell_util's RegistryEntry class into a separate file.

Makes shell_util more modular and breaks it up a bit. RegistryEntry is a
generally useful API.

BUG=419972
==========

to

==========
Move shell_util's RegistryEntry class into a separate file.

Makes shell_util more modular and breaks it up a bit. RegistryEntry is a
generally useful API.

BUG=419972
==========

[Matt Giuca, 2015-12-02 02:54:24]

Now I have done all the changes I want to do (thanks for the many reviews,
Gabriel!) except for moving the code out into separate files.

Which brings me back to this original CL. I've uploaded PS 5 which is now *just*
moving the RegistryEntry code to registry_entry.h. After that, I will move the
other code that came out of RegistryEntry to a different file (just to further
break up shell_util).

Now grt@, you said in #11 and #13 that you wanted a few more changes to happen.
Adding some extra parameters, and tests. As I was saying above, I think that
those should be done separately and it is just much easier to think about this
and do code reviews once it is in its own file. Honestly, these requests were
what prevented me from finishing this off last time. So I say we just get this
move out of the way then worry about cleaning up and testing this API.

[Matt Giuca, 2015-12-02 02:54:41]

https://codereview.chromium.org/623903002/diff/80001/chrome/installer/util/re...
File chrome/installer/util/registry_entry.cc (right):

https://codereview.chromium.org/623903002/diff/80001/chrome/installer/util/re...
chrome/installer/util/registry_entry.cc:85: base::win::RegKey key(root,
key_path_.c_str(), KEY_QUERY_VALUE);
Note: This was the only line that changed during the move (added "base::win::"
instead of copying the "using base::win::RegKey" line from shell_util.cc).

[grt (UTC plus 2), 2015-12-02 18:04:20]

nice. i'm fine with leaving the tweaks and tests for another CL. thanks for the
cleanups.

[grt (UTC plus 2), 2015-12-02 18:04:30]

On 2015/12/02 18:04:20, grt wrote:
> nice. i'm fine with leaving the tweaks and tests for another CL. thanks for
the
> cleanups.

lgtm

[gab, 2015-12-02 20:58:18]

lgtm, though I'm surprised the new files are not considered 'A+' (new file
copied from previous one instead of 'A' which just means add).

Perhaps try git cl upload --similarity=25 (or something lower than the default
50?).

Thanks!
Gab

[Will Harris, 2015-12-02 21:00:39]

What was decided for the wow64 stuff here? Will this be exposed in a future CL,
or just added if/when we ever need it?

[gab, 2015-12-02 21:31:11]

On 2015/12/02 21:00:39, Will Harris wrote:
> What was decided for the wow64 stuff here? Will this be exposed in a future
CL,
> or just added if/when we ever need it?

This CL is essentially just meant to cleanup and move some much intertwined code
to its own place.

While this move may open up an opportunity to also clean up and add to the API,
I don't think it's mandatory to it happening.

Therefore, to answer your question, it wasn't discussed, but I don't think it
should block this CL (unless you think making this API public without this
functionality is a show stopper?).

[Will Harris, 2015-12-02 21:32:30]

no, not a showstopper. if/when we need it, we can add it.

lgtm

[Matt Giuca, 2015-12-02 22:58:04]

I don't think similarity works that way. --similarity=X means "if you see a new
file, search the entire code base for a file that at least X% of matches X% of
this new file". Since shell_util.cc is so huge, registry_entry.h and
registry_entry.cc don't look anything like it.

I tried --similarity=10 and got no match. Then I calculated that these files
represent about 3% of shell_util.cc so I tried with --similarity=3 and got a
match on a completely random file that made the diff completely
incomprehensible.

So I'm just deleting those patch sets to avoid confusion.

TL;DR, no we can't have a nice diff.

[Matt Giuca, 2015-12-02 22:58:14]

Patchset #7 (id:120001) has been deleted

[Matt Giuca, 2015-12-02 22:58:19]

Patchset #6 (id:100001) has been deleted

[Matt Giuca, 2015-12-02 22:59:52]

On 2015/12/02 21:00:39, Will Harris wrote:
> What was decided for the wow64 stuff here? Will this be exposed in a future
CL,
> or just added if/when we ever need it?

I think it makes sense to expose it even if not strictly needed. As grt said in
#11, "The point of that extra parameter is to force consumers to make a
conscious choice. Leaving it for later opens up the possibility of RegistryEntry
consumers doing the wrong thing because they thought the API was somehow safe
and protected them from the reality of the registry."

So I'm conscious of that but I also don't want to block this refactor on it.

[Matt Giuca, 2015-12-02 23:00:02]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-12-02 23:01:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/623903002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/623903002/80001

[commit-bot: I haz the power, 2015-12-02 23:19:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 23:19:11]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Matt Giuca, 2015-12-03 06:18:35]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-12-03 06:18:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/623903002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/623903002/80001

[commit-bot: I haz the power, 2015-12-03 06:55:12]

Description was changed from

==========
Move shell_util's RegistryEntry class into a separate file.

Makes shell_util more modular and breaks it up a bit. RegistryEntry is a
generally useful API.

BUG=419972
==========

to

==========
Move shell_util's RegistryEntry class into a separate file.

Makes shell_util more modular and breaks it up a bit. RegistryEntry is a
generally useful API.

BUG=419972
==========

[commit-bot: I haz the power, 2015-12-03 06:55:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-03 06:56:16]

Description was changed from

==========
Move shell_util's RegistryEntry class into a separate file.

Makes shell_util more modular and breaks it up a bit. RegistryEntry is a
generally useful API.

BUG=419972
==========

to

==========
Move shell_util's RegistryEntry class into a separate file.

Makes shell_util more modular and breaks it up a bit. RegistryEntry is a
generally useful API.

BUG=419972

Committed: https://crrev.com/3bfee5e3676510bf8348bb46334a7dbea4a0a181
Cr-Commit-Position: refs/heads/master@{#362923}
==========

[commit-bot: I haz the power, 2015-12-03 06:56:18]

Patchset 5 (id:??) landed as
https://crrev.com/3bfee5e3676510bf8348bb46334a7dbea4a0a181
Cr-Commit-Position: refs/heads/master@{#362923}

[gab, 2015-12-03 15:20:12]

On 2015/12/02 22:59:52, Matt Giuca wrote:
> On 2015/12/02 21:00:39, Will Harris wrote:
> > What was decided for the wow64 stuff here? Will this be exposed in a future
> CL,
> > or just added if/when we ever need it?
> 
> I think it makes sense to expose it even if not strictly needed. As grt said
in
> #11, "The point of that extra parameter is to force consumers to make a
> conscious choice. Leaving it for later opens up the possibility of
RegistryEntry
> consumers doing the wrong thing because they thought the API was somehow safe
> and protected them from the reality of the registry."
> 
> So I'm conscious of that but I also don't want to block this refactor on it.

SGTM, thanks!