Added refreshing of elevation policy on first run of new version for all user...

Added refreshing of elevation policy on first run of new version for all users.

BUG=65973
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68951

[Sigurður Ásgeirsson, 2010-12-11 01:03:58]

lgtm with a couple of nits.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util.cc
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:317: // This may access to InternetRegistry
instead of real one. However this is
access to -> access

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:319: base::win::RegKey
hkcu(HKEY_CURRENT_USER, kRegistryPath, KEY_ALL_ACCESS);
is KEY_WRITE sufficient here?

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:335: // Write after refreshing because if two
thread get here simultaneously
two thread -> two threads

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:336: // better to refresh twice than pass one
thread without refreshing.
maybe rephrase to 
it's better to refresh twice, than to miss once
?

[Vitaly Buka (NO REVIEWS), 2010-12-11 01:17:44]

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util.cc
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:317: // This may access to InternetRegistry
instead of real one. However this is
On 2010/12/11 01:03:58, Ruðrugis wrote:
> access to -> access

Done.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:319: base::win::RegKey
hkcu(HKEY_CURRENT_USER, kRegistryPath, KEY_ALL_ACCESS);
On 2010/12/11 01:03:58, Ruðrugis wrote:
> is KEY_WRITE sufficient here?

KEY_WRITE does not allow read value.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:335: // Write after refreshing because if two
thread get here simultaneously
On 2010/12/11 01:03:58, Ruðrugis wrote:
> two thread -> two threads

Done.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:336: // better to refresh twice than pass one
thread without refreshing.
On 2010/12/11 01:03:58, Ruðrugis wrote:
> maybe rephrase to 
> it's better to refresh twice, than to miss once
> ?

Done.

[Jói, 2010-12-11 02:23:14]

LGTM with nits

http://codereview.chromium.org/5720004/diff/1/ceee/ie/broker/broker_rpc_clien...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/5720004/diff/1/ceee/ie/broker/broker_rpc_clien...
ceee/ie/broker/broker_rpc_client.cc:9: #include <atlbase.h>
I think we normally put one blank line after all the system headers.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/broker/broker_rpc_clien...
ceee/ie/broker/broker_rpc_client.cc:14: #include "broker_lib.h"  // NOLINT
our convention has been to put pathless includes like these two at the bottom of
the include list with a blank line before them (i.e. as a separate section)

http://codereview.chromium.org/5720004/diff/1/ceee/ie/broker/broker_rpc_clien...
ceee/ie/broker/broker_rpc_client.cc:214: LOG_IF(ERROR, FAILED(hr)) << "Failed to
create broker. " << com::LogHr(hr);
nit: suggest moving inside the if (FAILED(hr)) conditional (adding braces, and
changing to LOG(ERROR)) so that you don't have to repeat the condition, less
chance of errors in maintenance that way.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util.cc
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:319: base::win::RegKey
hkcu(HKEY_CURRENT_USER, kRegistryPath, KEY_ALL_ACCESS);
On 2010/12/11 01:17:44, Vitaly Buka wrote:
> On 2010/12/11 01:03:58, Ruðrugis wrote:
> > is KEY_WRITE sufficient here?
> 
> KEY_WRITE does not allow read value.

How about KEY_WRITE | KEY_QUERY_VALUE ?

[Vitaly Buka (NO REVIEWS), 2010-12-11 08:26:24]

http://codereview.chromium.org/5720004/diff/1/ceee/ie/broker/broker_rpc_clien...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/5720004/diff/1/ceee/ie/broker/broker_rpc_clien...
ceee/ie/broker/broker_rpc_client.cc:9: #include <atlbase.h>
On 2010/12/11 02:23:14, Jói wrote:
> I think we normally put one blank line after all the system headers.

Done.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/broker/broker_rpc_clien...
ceee/ie/broker/broker_rpc_client.cc:14: #include "broker_lib.h"  // NOLINT
On 2010/12/11 02:23:14, Jói wrote:
> our convention has been to put pathless includes like these two at the bottom
of
> the include list with a blank line before them (i.e. as a separate section)

Done.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/broker/broker_rpc_clien...
ceee/ie/broker/broker_rpc_client.cc:214: LOG_IF(ERROR, FAILED(hr)) << "Failed to
create broker. " << com::LogHr(hr);
On 2010/12/11 02:23:14, Jói wrote:
> nit: suggest moving inside the if (FAILED(hr)) conditional (adding braces, and
> changing to LOG(ERROR)) so that you don't have to repeat the condition, less
> chance of errors in maintenance that way.

Done.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util.cc
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:319: base::win::RegKey
hkcu(HKEY_CURRENT_USER, kRegistryPath, KEY_ALL_ACCESS);
Than may be KEY_READ | KEY_WRITE?

KEY_READ | KEY_WRITE is almost the same as KEY_ALL_ACCESS. But KEY_ALL_ACCESS
fits to 80 chars :-) We never had problems with KEY_ALL_ACCESS on my "previouse
project".

With KEY_WRITE | KEY_QUERY_VALUE you just omit KEY_ENUMERATE_SUB_KEYS
KEY_NOTIFY. I don't expect that registry key will be configured to provide
everything except them.


On 2010/12/11 02:23:14, Jói wrote:
> On 2010/12/11 01:17:44, Vitaly Buka wrote:
> > On 2010/12/11 01:03:58, Ruðrugis wrote:
> > > is KEY_WRITE sufficient here?
> > 
> > KEY_WRITE does not allow read value.
> 
> How about KEY_WRITE | KEY_QUERY_VALUE ?

[Jói, 2010-12-11 21:30:45]

LGTM once the access flags issue is addressed.

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util.cc
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:319: base::win::RegKey
hkcu(HKEY_CURRENT_USER, kRegistryPath, KEY_ALL_ACCESS);
On 2010/12/11 08:26:24, Vitaly Buka wrote:
> Than may be KEY_READ | KEY_WRITE?
> 
> KEY_READ | KEY_WRITE is almost the same as KEY_ALL_ACCESS. But KEY_ALL_ACCESS
> fits to 80 chars :-) We never had problems with KEY_ALL_ACCESS on my
"previouse
> project".
> 
> With KEY_WRITE | KEY_QUERY_VALUE you just omit KEY_ENUMERATE_SUB_KEYS
> KEY_NOTIFY. I don't expect that registry key will be configured to provide
> everything except them.
> 
> 
> On 2010/12/11 02:23:14, Jói wrote:
> > On 2010/12/11 01:17:44, Vitaly Buka wrote:
> > > On 2010/12/11 01:03:58, Ruðrugis wrote:
> > > > is KEY_WRITE sufficient here?
> > > 
> > > KEY_WRITE does not allow read value.
> > 
> > How about KEY_WRITE | KEY_QUERY_VALUE ?
> 

I agree it's unlikely, but as a general principle you're better off requesting
the minimum access that you need. You lose no functionality that way, and there
might be a computer or two out there that have some weird configuration or
security software that you might just stay compatible with.

[Vitaly Buka (NO REVIEWS), 2010-12-11 23:58:14]

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util.cc
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/5720004/diff/1/ceee/ie/common/ceee_module_util...
ceee/ie/common/ceee_module_util.cc:319: base::win::RegKey
hkcu(HKEY_CURRENT_USER, kRegistryPath, KEY_ALL_ACCESS);
On 2010/12/11 21:30:45, Jói wrote:
> On 2010/12/11 08:26:24, Vitaly Buka wrote:
> > Than may be KEY_READ | KEY_WRITE?
> > 
> > KEY_READ | KEY_WRITE is almost the same as KEY_ALL_ACCESS. But
KEY_ALL_ACCESS
> > fits to 80 chars :-) We never had problems with KEY_ALL_ACCESS on my
> "previouse
> > project".
> > 
> > With KEY_WRITE | KEY_QUERY_VALUE you just omit KEY_ENUMERATE_SUB_KEYS
> > KEY_NOTIFY. I don't expect that registry key will be configured to provide
> > everything except them.
> > 
> > 
> > On 2010/12/11 02:23:14, Jói wrote:
> > > On 2010/12/11 01:17:44, Vitaly Buka wrote:
> > > > On 2010/12/11 01:03:58, Ruðrugis wrote:
> > > > > is KEY_WRITE sufficient here?
> > > > 
> > > > KEY_WRITE does not allow read value.
> > > 
> > > How about KEY_WRITE | KEY_QUERY_VALUE ?
> > 
> 
> I agree it's unlikely, but as a general principle you're better off requesting
> the minimum access that you need. You lose no functionality that way, and
there
> might be a computer or two out there that have some weird configuration or
> security software that you might just stay compatible with.

Done.

[mad-corp, 2010-12-13 14:12:45]

LGTM2 with onw minor nit...

BYE
MAD

http://codereview.chromium.org/5720004/diff/19002/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/5720004/diff/19002/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:219: *broker = broker_tmp.Detach();
DCHECK(broker != NULL) before using unknown pointer from caller.