Silence logging at uninstall if the ClientState key doesn't exist.

Silence logging at uninstall if the ClientState key doesn't exist.

BUG=none

Committed: https://crrev.com/06983b3f5218f38b9de952b47b64ae9ea92f67e7
Cr-Commit-Position: refs/heads/master@{#293829}

[grt (UTC plus 2), 2014-09-08 15:45:11]

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

[grt (UTC plus 2), 2014-09-08 15:45:11]

This gets rid of the annoying

[0908/082503:ERROR:product.cc(126)] Failed to Open or Write MSI valueto client
state key. error: 2

messages in valid uninstall logs.

[gab, 2014-09-08 16:29:58]

lgtm w/ nit

Question: Why would the ClientState key not exist at uninstall? Is this called
after it's deleted?

https://codereview.chromium.org/551783003/diff/1/chrome/installer/util/produc...
File chrome/installer/util/product.cc (right):

https://codereview.chromium.org/551783003/diff/1/chrome/installer/util/produc...
chrome/installer/util/product.cc:126: if (result == ERROR_SUCCESS || result ==
ERROR_FILE_NOT_FOUND)
nit: I'd prefer handling the error in scope and having the regular (non-scoped)
flow be the expected flow, i.e.:

----------------------------------------------------------------------
if (result != ERROR_SUCESS && result != ERROR_FILE_NOT_FOUND) {
  LOG(ERROR) << ...;
  return false;
}

return true;
----------------------------------------------------------------------

makes it such that any other code can be added after handling the error without
having to flip the logic.

[grt (UTC plus 2), 2014-09-08 16:45:26]

On 2014/09/08 16:29:58, gab wrote:
> Question: Why would the ClientState key not exist at uninstall? Is this called
> after it's deleted?

In a Google Chrome build, ClientState will still be there. In a Chromium build,
Clients and ClientState are both the generic "Software\Chromium" key, which is
deleted early on.

[grt (UTC plus 2), 2014-09-08 16:47:56]

https://codereview.chromium.org/551783003/diff/1/chrome/installer/util/produc...
File chrome/installer/util/product.cc (right):

https://codereview.chromium.org/551783003/diff/1/chrome/installer/util/produc...
chrome/installer/util/product.cc:126: if (result == ERROR_SUCCESS || result ==
ERROR_FILE_NOT_FOUND)
On 2014/09/08 16:29:58, gab wrote:
> nit: I'd prefer handling the error in scope and having the regular
(non-scoped)
> flow be the expected flow, i.e.:
> 
> ----------------------------------------------------------------------
> if (result != ERROR_SUCESS && result != ERROR_FILE_NOT_FOUND) {
>   LOG(ERROR) << ...;
>   return false;
> }
> 
> return true;
> ----------------------------------------------------------------------
> 
> makes it such that any other code can be added after handling the error
without
> having to flip the logic.

yeah. my urge to avoid braces made me do it this way. it's prettier. i can't
imagine this function having more code after this. how strongly do you feel
about this?

[gab, 2014-09-08 16:58:41]

https://codereview.chromium.org/551783003/diff/1/chrome/installer/util/produc...
File chrome/installer/util/product.cc (right):

https://codereview.chromium.org/551783003/diff/1/chrome/installer/util/produc...
chrome/installer/util/product.cc:126: if (result == ERROR_SUCCESS || result ==
ERROR_FILE_NOT_FOUND)
On 2014/09/08 16:47:56, grt wrote:
> On 2014/09/08 16:29:58, gab wrote:
> > nit: I'd prefer handling the error in scope and having the regular
> (non-scoped)
> > flow be the expected flow, i.e.:
> > 
> > ----------------------------------------------------------------------
> > if (result != ERROR_SUCESS && result != ERROR_FILE_NOT_FOUND) {
> >   LOG(ERROR) << ...;
> >   return false;
> > }
> > 
> > return true;
> > ----------------------------------------------------------------------
> > 
> > makes it such that any other code can be added after handling the error
> without
> > having to flip the logic.
> 
> yeah. my urge to avoid braces made me do it this way. it's prettier. i can't
> imagine this function having more code after this. how strongly do you feel
> about this?

Mildly. I don't think having to add braces matters, but I do find it weird for
the success condition to be in the  middle while the end of the method is the
error case.

[grt (UTC plus 2), 2014-09-08 17:03:29]

Flipped!

https://codereview.chromium.org/551783003/diff/1/chrome/installer/util/produc...
File chrome/installer/util/product.cc (right):

https://codereview.chromium.org/551783003/diff/1/chrome/installer/util/produc...
chrome/installer/util/product.cc:126: if (result == ERROR_SUCCESS || result ==
ERROR_FILE_NOT_FOUND)
On 2014/09/08 16:58:41, gab wrote:
> On 2014/09/08 16:47:56, grt wrote:
> > On 2014/09/08 16:29:58, gab wrote:
> > > nit: I'd prefer handling the error in scope and having the regular
> > (non-scoped)
> > > flow be the expected flow, i.e.:
> > > 
> > > ----------------------------------------------------------------------
> > > if (result != ERROR_SUCESS && result != ERROR_FILE_NOT_FOUND) {
> > >   LOG(ERROR) << ...;
> > >   return false;
> > > }
> > > 
> > > return true;
> > > ----------------------------------------------------------------------
> > > 
> > > makes it such that any other code can be added after handling the error
> > without
> > > having to flip the logic.
> > 
> > yeah. my urge to avoid braces made me do it this way. it's prettier. i can't
> > imagine this function having more code after this. how strongly do you feel
> > about this?
> 
> Mildly. I don't think having to add braces matters, but I do find it weird for
> the success condition to be in the  middle while the end of the method is the
> error case.

Done.

[grt (UTC plus 2), 2014-09-08 17:03:33]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-09-08 17:19:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grt@chromium.org/551783003/20001

[commit-bot: I haz the power, 2014-09-08 22:10:14]

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

[commit-bot: I haz the power, 2014-09-08 22:10:15]

Exceeded time limit waiting for builds to trigger.

[gab, 2014-09-08 22:36:49]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-09-08 22:38:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grt@chromium.org/551783003/20001

[commit-bot: I haz the power, 2014-09-09 01:23:28]

Committed patchset #2 (id:20001) as 0fe0769c6b2cb9013edf776db4eb185b9df7c41a

[commit-bot: I haz the power, 2014-09-10 03:49:33]

Patchset 2 (id:??) landed as
https://crrev.com/06983b3f5218f38b9de952b47b64ae9ea92f67e7
Cr-Commit-Position: refs/heads/master@{#293829}