Changing policy while Chrome is running should refresh preferences without relaunching

Changing policy while Chrome is running should refresh preferences without relaunching

Includes windows-only Group Policy watcher that triggers policy refresh.

BUG=45324
TEST=--gtest_filter=PrefValueStoreTest.* and manual

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

[danno, 2010-07-29 08:53:38]

Please review, thanks!

[Mattias Nissler (ping if slow), 2010-07-29 20:37:24]

Here is your review!

http://codereview.chromium.org/2858060/diff/45013/59001
File chrome/browser/configuration_policy_provider.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59001#newcode79
chrome/browser/configuration_policy_provider.cc:79: type,
I believe it's possible to write NotificationType::POLICY_CHANGED here and rely
on the implicit conversion.

http://codereview.chromium.org/2858060/diff/45013/59004
File chrome/browser/configuration_policy_provider_win_unittest.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59004#newcode120
chrome/browser/configuration_policy_provider_win_unittest.cc:120: MessageLoop
loop_;
Please put a comment on why you need a to declare a message loop that you don't
access.

http://codereview.chromium.org/2858060/diff/45013/59005
File chrome/browser/pref_service.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59005#newcode841
chrome/browser/pref_service.cc:841: FireObservers(current->data());
I believe this only fires the observers in case a value change is detected. We
should also fire if the value doesn't change, but only the managed bit changes.
Sync needs that information to set up and tear down infrastructure for syncing
the affected preference.

http://codereview.chromium.org/2858060/diff/45013/59005#newcode855
chrome/browser/pref_service.cc:855:
&PrefService::FireObserversForRefreshedManagedPrefs);
I was talking with Pam about this today. Might there be a more generic way to
have PrefValueStore notify PrefService (or other subscribers) about changed keys
(and the source they used to come from)? That way, PrefValueStore could just
listen for the POLICY_CHANGED notification itself without the need to trigger it
from here for the sole reason of being able to pass a callback pointer. Also,
policy and extensions could probably share the mechanism to trigger PREF_CHANGED
notifications.

http://codereview.chromium.org/2858060/diff/45013/59005#newcode863
chrome/browser/pref_service.cc:863: callback));
It is a bit obscure why you need to switch to the UI thread here. Maybe you can
add a comment that we may only modify the PrefValueStore from that thread?

http://codereview.chromium.org/2858060/diff/45013/59006
File chrome/browser/pref_service.h (right):

http://codereview.chromium.org/2858060/diff/45013/59006#newcode238
chrome/browser/pref_service.h:238: const NotificationDetails& details);
Please move this to the private section.

http://codereview.chromium.org/2858060/diff/45013/59006#newcode266
chrome/browser/pref_service.h:266: // this method is called asynchrounously as a
callback from another thread.
s/asynchrounously/asynchronously/

http://codereview.chromium.org/2858060/diff/45013/59007
File chrome/browser/pref_value_store.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59007#newcode5
chrome/browser/pref_value_store.cc:5: #include "chrome/browser/chrome_thread.h"
pref_value_store.h already #includes chrome_thread.h

http://codereview.chromium.org/2858060/diff/45013/59007#newcode8
chrome/browser/pref_value_store.cc:8: #include
"chrome/common/notification_type.h"
I cannot see where you need these.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode136
chrome/browser/pref_value_store.cc:136: namespace {
I'm not 100% sure what the style guide says, but I'd expect this block at the
top of the file.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode140
chrome/browser/pref_value_store.cc:140: static void
GetUniqueDictionaryPaths(DictionaryValue* dict,
Can we put a const here?

http://codereview.chromium.org/2858060/diff/45013/59007#newcode146
chrome/browser/pref_value_store.cc:146: while (current != end) {
Why not use a for loop? I appreciate the effort to try and enhance readability,
but I think it's better with a plain for loop, since you find the ++current
where you look for it.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode166
chrome/browser/pref_value_store.cc:166: DictionaryValue* after_dict,
Is there a reason why these two are not const?

http://codereview.chromium.org/2858060/diff/45013/59007#newcode169
chrome/browser/pref_value_store.cc:169: // the unique set of full of value paths
that are leaf nodes.
I don't quite understand this sentence. Full of value paths?

Also, we'll run into problems with this approach once we actually have
dictionary-valued preferences controlled through policy. If you only look at
leaf nodes, you won't catch the paths of these.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode196
chrome/browser/pref_value_store.cc:196: CHECK(after_dict->Get(*current_after,
&after_value));
In an interview, I'd ask what these two calls do to the running time complexity
of your algorithm :) If you recursively traverse both dictionaries instead of
building the key sets, you avoid the O(log N) factor you get here ;)

http://codereview.chromium.org/2858060/diff/45013/59007#newcode196
chrome/browser/pref_value_store.cc:196: CHECK(after_dict->Get(*current_after,
&after_value));
You should also use GetWithoutPathExpansion() here.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode204
chrome/browser/pref_value_store.cc:204: }
After reading this, I think this code really belongs into base/values.h. Maybe
as DictionaryValue member functions called:

void DictionaryValue::GetAllPaths(std::set<std::wstring>* paths)
bool DictionaryValue::CompareAndFindUniquePaths(const DictionaryValue* other,
std::set<std::wstring>* paths)

My guess is that implementing this in one recursive pass will also simplify to
collect paths to different inner nodes.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode249
chrome/browser/pref_value_store.cc:249: LOG(ERROR) << L"refresh of recommended
policy failed: PrefReadError = "
Any reason you need wchar literals here? Same below.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode255
chrome/browser/pref_value_store.cc:255: }
I believe readability of this function could be greatly increased if you negated
the checks and used early returns. Also note that you're leaking the pref stores
in the error case, I suggest you put them into scoped_ptr_ and call release()
when passing them on.

http://codereview.chromium.org/2858060/diff/45013/59008
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/2858060/diff/45013/59008#newcode37
chrome/browser/pref_value_store.h:37: class PrefValueStore : public
base::RefCountedThreadSafe<PrefValueStore> {
If you make this RefCountedThreadSafe, you should also go through the existing
member functions and mark any that are unsafe to call from arbitrary threads (I
presume there are some, but didn't check).

http://codereview.chromium.org/2858060/diff/45013/59008#newcode118
chrome/browser/pref_value_store.h:118: // new, unitialized managed and
recommended PrefStores in
s/unitialized/uninitialzed/

http://codereview.chromium.org/2858060/diff/45013/59008#newcode126
chrome/browser/pref_value_store.h:126: AfterRefreshCallback callback);
If we had a generic notification mechanism, we could get rid of the callback
parameter here (and you wouldn't need to pass it through the whole PostTask()
chain).

http://codereview.chromium.org/2858060/diff/45013/59008#newcode162
chrome/browser/pref_value_store.h:162: // Called during policy refresh to do the
ReadPrefs on the FILE thread
nit: missing period.

http://codereview.chromium.org/2858060/diff/45013/59009
File chrome/browser/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59009#newcode67
chrome/browser/pref_value_store_unittest.cc:67: MessageLoop loop_;
AFAIK, the style guide says that data members should be declared after
functions. Can you fix that?

http://codereview.chromium.org/2858060/diff/45013/59009#newcode183
chrome/browser/pref_value_store_unittest.cc:183: virtual void TearDown() {
If you have a MessageLoop in your test, it's IMHO a good idea to put a call to
RunAllPending() here. I've seen a few instances of leaks that were caused
because it was missing.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode442
chrome/browser/pref_value_store_unittest.cc:442: namespace {
Move this to the top of the file?

http://codereview.chromium.org/2858060/diff/45013/59009#newcode452
chrome/browser/pref_value_store_unittest.cc:452: stream << L"(";
Again, why do we need wchar_t constants here?

http://codereview.chromium.org/2858060/diff/45013/59009#newcode513
chrome/browser/pref_value_store_unittest.cc:513:
ConfigurationPolicyPrefStore::CreateRecommendedPolicyPrefStore(),
Shouldn't you be using DummyPrefStores here?

http://codereview.chromium.org/2858060/diff/45013/59009#newcode517
chrome/browser/pref_value_store_unittest.cc:517: loop_.RunAllPending();
I'd expect the callback to be called at this point and with an empty set of
changed paths. You configured the callback to expect three. Does the EXPECT_TRUE
inside the callback actually work?

http://codereview.chromium.org/2858060/diff/45013/59009#newcode518
chrome/browser/pref_value_store_unittest.cc:518: callback.WaitForCompletion();
There's no point in waiting here. We're single-threaded, so the callback won't
happen if it hasn't already, even if you wait for it. If it didn't you'll be
waiting indefinitely and stalling the whole test executable. I think a simple
boolean completed flag in the callback class will do.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode525
chrome/browser/pref_value_store_unittest.cc:525:
ConfigurationPolicyPrefStore::CreateManagedPolicyPrefStore());
Why not a DummyPrefStore? More instances below.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode538
chrome/browser/pref_value_store_unittest.cc:538:
&TestPolicyRefreshCallback::DoCallback));
Shouldn't the EXPECT_TRUE in the callback code fail? "tabs.max_tabs" and
"this.pref.recommended_value_only" didn't change after all.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode540
chrome/browser/pref_value_store_unittest.cc:540: // Test properties that have
been removed from the managed store
nit: period.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode548
chrome/browser/pref_value_store_unittest.cc:548:
&TestPolicyRefreshCallback::DoCallback));
Didn't you remove the homepage pref as a result of the first call to
RefreshPolicyPrefsCompletion() above? If I understand the flow correctly, we
don't have any changed prefs here.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode569
chrome/browser/pref_value_store_unittest.cc:569:
new_managed_store->prefs()->Set(L"segment1", dictionary);
Why not just use prefs()->SetString(L"segment1.segment2", L"value")?

http://codereview.chromium.org/2858060/diff/45013/59009#newcode582
chrome/browser/pref_value_store_unittest.cc:582: DummyPrefStore* pref_store_1 =
new DummyPrefStore();
Why do you need a reference to the DummyPrefStore()? An inline new
DummyPrefStore() below should suffice. Also in the two blocks below.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode626
chrome/browser/pref_value_store_unittest.cc:626: loop_.RunAllPending();
Have you checked whether the Refresh sequences actually run interleaved? I'd
expect the loop processes its tasks in FIFO order, so it should work, but I'm
not too sure. Maybe put a comment detailing the expected call order?

Again, I'd expect the no changed paths and the EXPECT_TRUE in the callback to
fail for the callback1.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode629
chrome/browser/pref_value_store_unittest.cc:629: callback3.WaitForCompletion();
Waiting for completion not necessary. See comment above.

http://codereview.chromium.org/2858060/diff/45013/59010
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/2858060/diff/45013/59010#newcode1103
chrome/common/notification_type.h:1103: // This notification is sent whenever
the administrator changes policy
Nit: period.

You might also want to mention that the details parameter is not used.

[danno, 2010-08-03 16:18:04]

Addressed feedback. Note that this CL now requires
http://codereview.chromium.org/3035045 applied first to work.

http://codereview.chromium.org/2858060/diff/45013/59001
File chrome/browser/configuration_policy_provider.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59001#newcode79
chrome/browser/configuration_policy_provider.cc:79: type,
On 2010/07/29 20:37:24, mnissler wrote:
> I believe it's possible to write NotificationType::POLICY_CHANGED here and
rely
> on the implicit conversion.

Done.

http://codereview.chromium.org/2858060/diff/45013/59004
File chrome/browser/configuration_policy_provider_win_unittest.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59004#newcode120
chrome/browser/configuration_policy_provider_win_unittest.cc:120: MessageLoop
loop_;
On 2010/07/29 20:37:24, mnissler wrote:
> Please put a comment on why you need a to declare a message loop that you
don't
> access.

Done.

http://codereview.chromium.org/2858060/diff/45013/59005
File chrome/browser/pref_service.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59005#newcode841
chrome/browser/pref_service.cc:841: FireObservers(current->data());
No, only the FireObserversIfChanged does this, and it expects to be passed the
previous value. The managed bit already gets handled correctly, because a change
from manage to unmanaged and back means that a pref path is added or removed,
which sends a notification.
On 2010/07/29 20:37:24, mnissler wrote:
> I believe this only fires the observers in case a value change is detected. We
> should also fire if the value doesn't change, but only the managed bit
changes.
> Sync needs that information to set up and tear down infrastructure for syncing
> the affected preference.

http://codereview.chromium.org/2858060/diff/45013/59005#newcode855
chrome/browser/pref_service.cc:855:
&PrefService::FireObserversForRefreshedManagedPrefs);
I suspect you're right. However, would you be satisfied if it were in another
CL? It is a more comprehensive change.
On 2010/07/29 20:37:24, mnissler wrote:
> I was talking with Pam about this today. Might there be a more generic way to
> have PrefValueStore notify PrefService (or other subscribers) about changed
keys
> (and the source they used to come from)? That way, PrefValueStore could just
> listen for the POLICY_CHANGED notification itself without the need to trigger
it
> from here for the sole reason of being able to pass a callback pointer. Also,
> policy and extensions could probably share the mechanism to trigger
PREF_CHANGED
> notifications.

http://codereview.chromium.org/2858060/diff/45013/59005#newcode863
chrome/browser/pref_service.cc:863: callback));
On 2010/07/29 20:37:24, mnissler wrote:
> It is a bit obscure why you need to switch to the UI thread here. Maybe you
can
> add a comment that we may only modify the PrefValueStore from that thread?

Done.

http://codereview.chromium.org/2858060/diff/45013/59006
File chrome/browser/pref_service.h (right):

http://codereview.chromium.org/2858060/diff/45013/59006#newcode238
chrome/browser/pref_service.h:238: const NotificationDetails& details);
On 2010/07/29 20:37:24, mnissler wrote:
> Please move this to the private section.

Done.

http://codereview.chromium.org/2858060/diff/45013/59006#newcode266
chrome/browser/pref_service.h:266: // this method is called asynchrounously as a
callback from another thread.
On 2010/07/29 20:37:24, mnissler wrote:
> s/asynchrounously/asynchronously/

Done.

http://codereview.chromium.org/2858060/diff/45013/59007
File chrome/browser/pref_value_store.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59007#newcode5
chrome/browser/pref_value_store.cc:5: #include "chrome/browser/chrome_thread.h"
On 2010/07/29 20:37:24, mnissler wrote:
> pref_value_store.h already #includes chrome_thread.h

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode8
chrome/browser/pref_value_store.cc:8: #include
"chrome/common/notification_type.h"
On 2010/07/29 20:37:24, mnissler wrote:
> I cannot see where you need these.

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode136
chrome/browser/pref_value_store.cc:136: namespace {
On 2010/07/29 20:37:24, mnissler wrote:
> I'm not 100% sure what the style guide says, but I'd expect this block at the
> top of the file.

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode140
chrome/browser/pref_value_store.cc:140: static void
GetUniqueDictionaryPaths(DictionaryValue* dict,
On 2010/07/29 20:37:24, mnissler wrote:
> Can we put a const here?

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode146
chrome/browser/pref_value_store.cc:146: while (current != end) {
If it makes you happy, Done. :-)
On 2010/07/29 20:37:24, mnissler wrote:
> Why not use a for loop? I appreciate the effort to try and enhance
readability,
> but I think it's better with a plain for loop, since you find the ++current
> where you look for it.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode166
chrome/browser/pref_value_store.cc:166: DictionaryValue* after_dict,
On 2010/07/29 20:37:24, mnissler wrote:
> Is there a reason why these two are not const?

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode169
chrome/browser/pref_value_store.cc:169: // the unique set of full of value paths
that are leaf nodes.
On 2010/07/29 20:37:24, mnissler wrote:
> I don't quite understand this sentence. Full of value paths?
> 
> Also, we'll run into problems with this approach once we actually have
> dictionary-valued preferences controlled through policy. If you only look at
> leaf nodes, you won't catch the paths of these.

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode196
chrome/browser/pref_value_store.cc:196: CHECK(after_dict->Get(*current_after,
&after_value));
On 2010/07/29 20:37:24, mnissler wrote:
> You should also use GetWithoutPathExpansion() here.

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode196
chrome/browser/pref_value_store.cc:196: CHECK(after_dict->Get(*current_after,
&after_value));
On 2010/07/29 20:37:24, mnissler wrote:
> In an interview, I'd ask what these two calls do to the running time
complexity
> of your algorithm :) If you recursively traverse both dictionaries instead of
> building the key sets, you avoid the O(log N) factor you get here ;)

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode204
chrome/browser/pref_value_store.cc:204: }
On 2010/07/29 20:37:24, mnissler wrote:
> After reading this, I think this code really belongs into base/values.h. Maybe
> as DictionaryValue member functions called:
> 
> void DictionaryValue::GetAllPaths(std::set<std::wstring>* paths)
> bool DictionaryValue::CompareAndFindUniquePaths(const DictionaryValue* other,
> std::set<std::wstring>* paths)
> 
> My guess is that implementing this in one recursive pass will also simplify to
> collect paths to different inner nodes.

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode249
chrome/browser/pref_value_store.cc:249: LOG(ERROR) << L"refresh of recommended
policy failed: PrefReadError = "
On 2010/07/29 20:37:24, mnissler wrote:
> Any reason you need wchar literals here? Same below.

Done.

http://codereview.chromium.org/2858060/diff/45013/59007#newcode255
chrome/browser/pref_value_store.cc:255: }
On 2010/07/29 20:37:24, mnissler wrote:
> I believe readability of this function could be greatly increased if you
negated
> the checks and used early returns. Also note that you're leaking the pref
stores
> in the error case, I suggest you put them into scoped_ptr_ and call release()
> when passing them on.

Done.

http://codereview.chromium.org/2858060/diff/45013/59008
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/2858060/diff/45013/59008#newcode37
chrome/browser/pref_value_store.h:37: class PrefValueStore : public
base::RefCountedThreadSafe<PrefValueStore> {
On 2010/07/29 20:37:24, mnissler wrote:
> If you make this RefCountedThreadSafe, you should also go through the existing
> member functions and mark any that are unsafe to call from arbitrary threads
(I
> presume there are some, but didn't check).

Done.

http://codereview.chromium.org/2858060/diff/45013/59008#newcode118
chrome/browser/pref_value_store.h:118: // new, unitialized managed and
recommended PrefStores in
On 2010/07/29 20:37:24, mnissler wrote:
> s/unitialized/uninitialzed/

Done.

http://codereview.chromium.org/2858060/diff/45013/59008#newcode126
chrome/browser/pref_value_store.h:126: AfterRefreshCallback callback);
As before, I agree, but I would like to keep that refactor/cleanup separate from
this change.
On 2010/07/29 20:37:24, mnissler wrote:
> If we had a generic notification mechanism, we could get rid of the callback
> parameter here (and you wouldn't need to pass it through the whole PostTask()
> chain).

http://codereview.chromium.org/2858060/diff/45013/59008#newcode162
chrome/browser/pref_value_store.h:162: // Called during policy refresh to do the
ReadPrefs on the FILE thread
On 2010/07/29 20:37:24, mnissler wrote:
> nit: missing period.

Done.

http://codereview.chromium.org/2858060/diff/45013/59009
File chrome/browser/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/2858060/diff/45013/59009#newcode67
chrome/browser/pref_value_store_unittest.cc:67: MessageLoop loop_;
On 2010/07/29 20:37:24, mnissler wrote:
> AFAIK, the style guide says that data members should be declared after
> functions. Can you fix that?

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode183
chrome/browser/pref_value_store_unittest.cc:183: virtual void TearDown() {
On 2010/07/29 20:37:24, mnissler wrote:
> If you have a MessageLoop in your test, it's IMHO a good idea to put a call to
> RunAllPending() here. I've seen a few instances of leaks that were caused
> because it was missing.

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode442
chrome/browser/pref_value_store_unittest.cc:442: namespace {
On 2010/07/29 20:37:24, mnissler wrote:
> Move this to the top of the file?

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode452
chrome/browser/pref_value_store_unittest.cc:452: stream << L"(";
On 2010/07/29 20:37:24, mnissler wrote:
> Again, why do we need wchar_t constants here?

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode513
chrome/browser/pref_value_store_unittest.cc:513:
ConfigurationPolicyPrefStore::CreateRecommendedPolicyPrefStore(),
On 2010/07/29 20:37:24, mnissler wrote:
> Shouldn't you be using DummyPrefStores here?

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode517
chrome/browser/pref_value_store_unittest.cc:517: loop_.RunAllPending();
pref_value_store_ is initialized by PrefValueStoreTest to have values in both
it's managed and recommended store. By replacing them with dummy stores, all of
the paths of the prefs originally managed and recommended stores should change.
Added as comment
2010/07/29 20:37:24, mnissler wrote:
> I'd expect the callback to be called at this point and with an empty set of
> changed paths. You configured the callback to expect three. Does the
EXPECT_TRUE
> inside the callback actually work?

http://codereview.chromium.org/2858060/diff/45013/59009#newcode518
chrome/browser/pref_value_store_unittest.cc:518: callback.WaitForCompletion();
On 2010/07/29 20:37:24, mnissler wrote:
> There's no point in waiting here. We're single-threaded, so the callback won't
> happen if it hasn't already, even if you wait for it. If it didn't you'll be
> waiting indefinitely and stalling the whole test executable. I think a simple
> boolean completed flag in the callback class will do.

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode525
chrome/browser/pref_value_store_unittest.cc:525:
ConfigurationPolicyPrefStore::CreateManagedPolicyPrefStore());
On 2010/07/29 20:37:24, mnissler wrote:
> Why not a DummyPrefStore? More instances below.

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode538
chrome/browser/pref_value_store_unittest.cc:538:
&TestPolicyRefreshCallback::DoCallback));
On 2010/07/29 20:37:24, mnissler wrote:
> Shouldn't the EXPECT_TRUE in the callback code fail? "tabs.max_tabs" and
> "this.pref.recommended_value_only" didn't change after all.

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode540
chrome/browser/pref_value_store_unittest.cc:540: // Test properties that have
been removed from the managed store
On 2010/07/29 20:37:24, mnissler wrote:
> nit: period.

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode548
chrome/browser/pref_value_store_unittest.cc:548:
&TestPolicyRefreshCallback::DoCallback));
Nope. The value of homepage was changed, but it wasn't removed.
On 2010/07/29 20:37:24, mnissler wrote:
> Didn't you remove the homepage pref as a result of the first call to
> RefreshPolicyPrefsCompletion() above? If I understand the flow correctly, we
> don't have any changed prefs here.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode569
chrome/browser/pref_value_store_unittest.cc:569:
new_managed_store->prefs()->Set(L"segment1", dictionary);
On 2010/07/29 20:37:24, mnissler wrote:
> Why not just use prefs()->SetString(L"segment1.segment2", L"value")?

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode582
chrome/browser/pref_value_store_unittest.cc:582: DummyPrefStore* pref_store_1 =
new DummyPrefStore();
On 2010/07/29 20:37:24, mnissler wrote:
> Why do you need a reference to the DummyPrefStore()? An inline new
> DummyPrefStore() below should suffice. Also in the two blocks below.

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode626
chrome/browser/pref_value_store_unittest.cc:626: loop_.RunAllPending();
On 2010/07/29 20:37:24, mnissler wrote:
> Have you checked whether the Refresh sequences actually run interleaved? I'd
> expect the loop processes its tasks in FIFO order, so it should work, but I'm
> not too sure. Maybe put a comment detailing the expected call order?
> 
> Again, I'd expect the no changed paths and the EXPECT_TRUE in the callback to
> fail for the callback1.

Done.

http://codereview.chromium.org/2858060/diff/45013/59009#newcode629
chrome/browser/pref_value_store_unittest.cc:629: callback3.WaitForCompletion();
On 2010/07/29 20:37:24, mnissler wrote:
> Waiting for completion not necessary. See comment above.

Done.

http://codereview.chromium.org/2858060/diff/45013/59010
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/2858060/diff/45013/59010#newcode1103
chrome/common/notification_type.h:1103: // This notification is sent whenever
the administrator changes policy
On 2010/07/29 20:37:24, mnissler wrote:
> Nit: period.
> 
> You might also want to mention that the details parameter is not used.

Done.

[markusheintz_, 2010-08-03 17:08:22]

Quick drive by:

What happened to the changes in base/value.* (introduces in patch_set 15)?
You still seem to use them in your CL.

[Mattias Nissler (ping if slow), 2010-08-04 10:07:47]

Just a couple of nits. I'm OK with doing the PrefValueStore->PrefService
notification refactoring in a separate CL.

http://codereview.chromium.org/2858060/diff/44014/64003
File chrome/browser/configuration_policy_provider_win.h (right):

http://codereview.chromium.org/2858060/diff/44014/64003#newcode11
chrome/browser/configuration_policy_provider_win.h:11: #include
"base/scoped_ptr.h"
Swap for correct lexicographic sorting.

http://codereview.chromium.org/2858060/diff/44014/64003#newcode23
chrome/browser/configuration_policy_provider_win.h:23: // Keeps watch on
Window's Group Policy notification event to trigger
Shouldn't it be Windows'?

http://codereview.chromium.org/2858060/diff/44014/64005
File chrome/browser/pref_service.cc (right):

http://codereview.chromium.org/2858060/diff/44014/64005#newcode844
chrome/browser/pref_service.cc:844:
FireObservers(UTF8ToWide(current->data()).data());
Maybe it's cleaner to convert them to wchar directly after computing them? That
would also save the conversions in the unit test. Your call.

http://codereview.chromium.org/2858060/diff/44014/64007
File chrome/browser/pref_value_store.cc (right):

http://codereview.chromium.org/2858060/diff/44014/64007#newcode160
chrome/browser/pref_value_store.cc:160: changed_paths.resize(last_insert -
changed_paths.begin());
What's the point of calling this?

http://codereview.chromium.org/2858060/diff/44014/64008
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/2858060/diff/44014/64008#newcode10
chrome/browser/pref_value_store.h:10: #include <set>
Are these needed?

http://codereview.chromium.org/2858060/diff/44014/64009
File chrome/browser/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/2858060/diff/44014/64009#newcode25
chrome/browser/pref_value_store_unittest.cc:25: : paths_(paths) {}
indent

http://codereview.chromium.org/2858060/diff/44014/64009#newcode489
chrome/browser/pref_value_store_unittest.cc:489: class TestPolicyRefreshCallback
{
I think this would be a perfect candidate for gmock's EXPECT_CALL() mechanism.
You decide :)

http://codereview.chromium.org/2858060/diff/44014/64009#newcode548
chrome/browser/pref_value_store_unittest.cc:548:
&TestPolicyRefreshCallback::DoCallback));
You should check that the callback has run here.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode559
chrome/browser/pref_value_store_unittest.cc:559:
&TestPolicyRefreshCallback::DoCallback));
and here.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode573
chrome/browser/pref_value_store_unittest.cc:573:
&TestPolicyRefreshCallback::DoCallback));
and here.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode589
chrome/browser/pref_value_store_unittest.cc:589:
&TestPolicyRefreshCallback::DoCallback));
and here.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode633
chrome/browser/pref_value_store_unittest.cc:633:
EXPECT_TRUE(callback3.HasCompleted());
So all three refresh operations detect the same changes, since they run
interlocked, right? I guess I'm OK with that, shouldn't happen much in reality,
right?

[Mattias Nissler (ping if slow), 2010-08-04 10:13:28]

LGTM if you consider my previous comments and make the trybots happy.

[danno, 2010-08-04 12:10:08]

addressed your feedback

http://codereview.chromium.org/2858060/diff/44014/64003
File chrome/browser/configuration_policy_provider_win.h (right):

http://codereview.chromium.org/2858060/diff/44014/64003#newcode11
chrome/browser/configuration_policy_provider_win.h:11: #include
"base/scoped_ptr.h"
On 2010/08/04 10:07:47, mnissler wrote:
> Swap for correct lexicographic sorting.

Done.

http://codereview.chromium.org/2858060/diff/44014/64003#newcode23
chrome/browser/configuration_policy_provider_win.h:23: // Keeps watch on
Window's Group Policy notification event to trigger
On 2010/08/04 10:07:47, mnissler wrote:
> Shouldn't it be Windows'?

Done.

http://codereview.chromium.org/2858060/diff/44014/64005
File chrome/browser/pref_service.cc (right):

http://codereview.chromium.org/2858060/diff/44014/64005#newcode844
chrome/browser/pref_service.cc:844:
FireObservers(UTF8ToWide(current->data()).data());
On 2010/08/04 10:07:47, mnissler wrote:
> Maybe it's cleaner to convert them to wchar directly after computing them?
That
> would also save the conversions in the unit test. Your call.
I am on the fence for this. trungl did a bunch of work to remove wstring from
all key/path-related APIs in Values.h/.cc, so I think anything that deals with
paths should stay a strings and the client of the API should do the conversion
if necessary.

http://codereview.chromium.org/2858060/diff/44014/64007
File chrome/browser/pref_value_store.cc (right):

http://codereview.chromium.org/2858060/diff/44014/64007#newcode160
chrome/browser/pref_value_store.cc:160: changed_paths.resize(last_insert -
changed_paths.begin());
On 2010/08/04 10:07:47, mnissler wrote:
> What's the point of calling this?
The changed_path vector is allocated with enough space to hold the result of the
merge in the "worst" case. However, merge itself doesn't change the size of the
vector. After the merge is complete, it's necessary to trim the unused items off
the end of the vector in the case that everything turned out better than the
worst case.

http://codereview.chromium.org/2858060/diff/44014/64008
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/2858060/diff/44014/64008#newcode10
chrome/browser/pref_value_store.h:10: #include <set>
On 2010/08/04 10:07:47, mnissler wrote:
> Are these needed?

Done.

http://codereview.chromium.org/2858060/diff/44014/64009
File chrome/browser/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/2858060/diff/44014/64009#newcode25
chrome/browser/pref_value_store_unittest.cc:25: : paths_(paths) {}
On 2010/08/04 10:07:47, mnissler wrote:
> indent

Done.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode489
chrome/browser/pref_value_store_unittest.cc:489: class TestPolicyRefreshCallback
{
On 2010/08/04 10:07:47, mnissler wrote:
> I think this would be a perfect candidate for gmock's EXPECT_CALL() mechanism.
> You decide :)

Done.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode548
chrome/browser/pref_value_store_unittest.cc:548:
&TestPolicyRefreshCallback::DoCallback));
On 2010/08/04 10:07:47, mnissler wrote:
> You should check that the callback has run here.

Done.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode559
chrome/browser/pref_value_store_unittest.cc:559:
&TestPolicyRefreshCallback::DoCallback));
On 2010/08/04 10:07:47, mnissler wrote:
> and here.

Done.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode573
chrome/browser/pref_value_store_unittest.cc:573:
&TestPolicyRefreshCallback::DoCallback));
On 2010/08/04 10:07:47, mnissler wrote:
> and here.

Done.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode589
chrome/browser/pref_value_store_unittest.cc:589:
&TestPolicyRefreshCallback::DoCallback));
On 2010/08/04 10:07:47, mnissler wrote:
> and here.

Done.

http://codereview.chromium.org/2858060/diff/44014/64009#newcode633
chrome/browser/pref_value_store_unittest.cc:633:
EXPECT_TRUE(callback3.HasCompleted());
They don't expect the same thing. Notice the clear of the expected path vector
after the first call.
On 2010/08/04 10:07:47, mnissler wrote:
> So all three refresh operations detect the same changes, since they run
> interlocked, right? I guess I'm OK with that, shouldn't happen much in
reality,
> right?