Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10825415:
Added code to persist downloaded cloud policy to disk. (Closed)
|
Created:
8 years, 4 months ago by Andrew T Wilson (Slow) Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded code to persist downloaded cloud policy to disk.
Updated UserCloudPolicyStore to actually persist and restore policy from a disk
file, and added new tests to exercise this functionality.
BUG=141123
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153114
Patch Set 1 #
Total comments: 19
Patch Set 2 : Review feedback addressed #Patch Set 3 : Added support for Clear(). #Patch Set 4 : Removed accidentally added merge files. #Patch Set 5 : Removed unneeded #include #
Total comments: 30
Patch Set 6 : Change to use PostTaskAndReplyWithResult #Patch Set 7 : Removed debugging statements that are unneeded. #
Total comments: 16
Patch Set 8 : More review feedback addressed. #
Messages
Total messages: 15 (0 generated)
PTAL https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... File chrome/browser/policy/user_cloud_policy_store.cc (left): https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... chrome/browser/policy/user_cloud_policy_store.cc:35: weak_factory_.InvalidateWeakPtrs(); Per my email, note that I no longer call InvalidateWeakPtrs() here - I just let whatever previous Store() operation was in progress run to completion. Let me know if you think this is a problem (and what use case the Invalidate() call was addressing) and I'll re-examine this.
https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... File chrome/browser/policy/user_cloud_policy_store.cc (left): https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... chrome/browser/policy/user_cloud_policy_store.cc:35: weak_factory_.InvalidateWeakPtrs(); On 2012/08/17 06:26:13, Andrew T Wilson wrote: > Per my email, note that I no longer call InvalidateWeakPtrs() here - I just let > whatever previous Store() operation was in progress run to completion. Let me > know if you think this is a problem (and what use case the Invalidate() call was > addressing) and I'll re-examine this. As pointed out on the mail thread, it's actually required for properly syncing updates. https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... File chrome/browser/policy/user_cloud_policy_store.cc (right): https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... chrome/browser/policy/user_cloud_policy_store.cc:31: no newline here I think? https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... chrome/browser/policy/user_cloud_policy_store.cc:106: remove extra blank line https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... chrome/browser/policy/user_cloud_policy_store.cc:143: DVLOG(1) << "Policy validation succeeded - storing to disk."; might just replace the two DVLOGs with a single one logging the status? https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... File chrome/browser/policy/user_cloud_policy_store.h (right): https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... chrome/browser/policy/user_cloud_policy_store.h:15: #include "chrome/browser/policy/user_policy_disk_cache.h" This is a part of the chrome OS user policy legacy that should eventually go away. It uses a wrapper protobuf that is no longer need: Just storing PolicyFetchResponse is enough as it contains everything we need, and has the additional benefit that all data contained in it can be protected by a signature. Thus, you may want to reconsider using UserPolicyDiskCache. https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... chrome/browser/policy/user_cloud_policy_store.h:40: remove extra blank line https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... File chrome/browser/policy/user_cloud_policy_store_unittest.cc (right): https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... chrome/browser/policy/user_cloud_policy_store_unittest.cc:160: VerifyPolicyMap(store_.get()); shouldn't this fail now that show home button is false? https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... File chrome/browser/policy/user_policy_signin_service.cc (right): https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... chrome/browser/policy/user_policy_signin_service.cc:134: manager_->cloud_policy_service()->store()->is_initialized()) { Isn't this racy? What if the store just hasn't initialized yet?
On 2012/08/20 15:29:30, Mattias Nissler wrote: > https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... > File chrome/browser/policy/user_cloud_policy_store.cc (left): > > https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... > chrome/browser/policy/user_cloud_policy_store.cc:35: > weak_factory_.InvalidateWeakPtrs(); > On 2012/08/17 06:26:13, Andrew T Wilson wrote: > > Per my email, note that I no longer call InvalidateWeakPtrs() here - I just > let > > whatever previous Store() operation was in progress run to completion. Let me > > know if you think this is a problem (and what use case the Invalidate() call > was > > addressing) and I'll re-examine this. > > As pointed out on the mail thread, it's actually required for properly syncing > updates. > My understanding from the mail thread is that it's only required for pyautos on cros, to deal with flakiness because CloudPolicyService::RefreshPolicy() can trigger callbacks based on an previously-pending Store() call. I was considering omitting this from the non-cros store code since we don't run the pyauto tests on non-cros stores, but I think it's not great to have the code behave differently on different platforms. I'll add back in the invalidate call and update the documentation and fix up my unit tests (which currently rely on being able to call Store() twice in a row and have them both executed). It really feels like this isn't the right behavior for this API though, and I'd like to suggest that ultimately we address this in the pyauto tests themselves (by making multiple refresh calls) rather than force these APIs to abort operations since that can, in theory, leave us without valid policy.
http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... File chrome/browser/policy/user_cloud_policy_store.h (right): http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... chrome/browser/policy/user_cloud_policy_store.h:15: #include "chrome/browser/policy/user_policy_disk_cache.h" On 2012/08/20 15:29:30, Mattias Nissler wrote: > Thus, you may want to reconsider using UserPolicyDiskCache. OK, I'll roll my own version that doesn't use the wrapper protobuf. http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... File chrome/browser/policy/user_cloud_policy_store_unittest.cc (right): http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... chrome/browser/policy/user_cloud_policy_store_unittest.cc:160: VerifyPolicyMap(store_.get()); On 2012/08/20 15:29:30, Mattias Nissler wrote: > shouldn't this fail now that show home button is false? No, because the first policy I store is the one with the false showhomebutton(), *then* I overwrite it with the regular policy (the one with showhomebutton=true). http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_pol... File chrome/browser/policy/user_policy_signin_service.cc (right): http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_pol... chrome/browser/policy/user_policy_signin_service.cc:134: manager_->cloud_policy_service()->store()->is_initialized()) { It is indeed racy. The solution is to move the registration code into UserCloudPolicyManager, per the TODO. I didn't want to bother trying to address the race condition in this code, only to have to re-address it a different way when I move it to UCPM. The race condition is OK for now, since there are basically only 3 situations: a) there are no downloaded policies yet - in this case, it seems that we always initialize the store before the token DB finishes loading (since they share the same thread). Obviously this is fragile, but it's good enough until we address the TODO. b) There are stored policies, and they fail to validate c) There are stored policies, and they don't fail to validate - in that case, when the store finishes initializing, the CloudPolicyService will register the client using the DM token from the policy. So the only case that fails currently is b), which I've mentioned in the comment. I think this is fine for now, but obviously the TODO must be addressed in short order/before launch.
On 2012/08/20 18:03:56, Andrew T Wilson wrote: > On 2012/08/20 15:29:30, Mattias Nissler wrote: > > > https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... > > File chrome/browser/policy/user_cloud_policy_store.cc (left): > > > > > https://chromiumcodereview.appspot.com/10825415/diff/1/chrome/browser/policy/... > > chrome/browser/policy/user_cloud_policy_store.cc:35: > > weak_factory_.InvalidateWeakPtrs(); > > On 2012/08/17 06:26:13, Andrew T Wilson wrote: > > > Per my email, note that I no longer call InvalidateWeakPtrs() here - I just > > let > > > whatever previous Store() operation was in progress run to completion. Let > me > > > know if you think this is a problem (and what use case the Invalidate() call > > was > > > addressing) and I'll re-examine this. > > > > As pointed out on the mail thread, it's actually required for properly syncing > > updates. > > > My understanding from the mail thread is that it's only required for pyautos on > cros, to deal with flakiness because CloudPolicyService::RefreshPolicy() can > trigger callbacks based on an previously-pending Store() call. > > I was considering omitting this from the non-cros store code since we don't run > the pyauto tests on non-cros stores, but I think it's not great to have the code > behave differently on different platforms. I'll add back in the invalidate call > and update the documentation and fix up my unit tests (which currently rely on > being able to call Store() twice in a row and have them both executed). > > It really feels like this isn't the right behavior for this API though, and I'd > like to suggest that ultimately we address this in the pyauto tests themselves > (by making multiple refresh calls) rather than force these APIs to abort > operations since that can, in theory, leave us without valid policy. (Adding Joao to this discussion, since it was him who originally introduced the RefreshPolicy semantics and thus might have an opinion.) I can relate to your point that we shouldn't cancel operations, since that may cause us losing policy data. I don't agree to the conclusion that we shouldn't have a proper way to be notified of a policy refresh having completed properly though. One other use case for this is Tango push notifications on policy updates. We'll have to signal to the Ticl that we have successfully processed the invalidation, so it knows what version is current. For that, it's essential again to have a reliable signal on policy refresh completion. So let's instead think about a better way to associate completed operations with to original request. In particular, let's see whether we can pass a callback to Load() that will get invoked once the operation completes, so we don't have to cancel pending operations any longer.
http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... File chrome/browser/policy/user_cloud_policy_store_unittest.cc (right): http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... chrome/browser/policy/user_cloud_policy_store_unittest.cc:160: VerifyPolicyMap(store_.get()); On 2012/08/20 18:04:37, Andrew T Wilson wrote: > On 2012/08/20 15:29:30, Mattias Nissler wrote: > > shouldn't this fail now that show home button is false? > No, because the first policy I store is the one with the false showhomebutton(), > *then* I overwrite it with the regular policy (the one with > showhomebutton=true). True, I had missed that. Makes sense then. http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_pol... File chrome/browser/policy/user_policy_signin_service.cc (right): http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_pol... chrome/browser/policy/user_policy_signin_service.cc:134: manager_->cloud_policy_service()->store()->is_initialized()) { On 2012/08/20 18:04:37, Andrew T Wilson wrote: > It is indeed racy. The solution is to move the registration code into > UserCloudPolicyManager, per the TODO. I didn't want to bother trying to address > the race condition in this code, only to have to re-address it a different way > when I move it to UCPM. > > The race condition is OK for now, since there are basically only 3 situations: > > a) there are no downloaded policies yet - in this case, it seems that we always > initialize the store before the token DB finishes loading (since they share the > same thread). Obviously this is fragile, but it's good enough until we address > the TODO. > b) There are stored policies, and they fail to validate > c) There are stored policies, and they don't fail to validate - in that case, > when the store finishes initializing, the CloudPolicyService will register the > client using the DM token from the policy. > > So the only case that fails currently is b), which I've mentioned in the > comment. I think this is fine for now, but obviously the TODO must be addressed > in short order/before launch. Fair enough. Also mention a) in the comment?
PTAL http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... File chrome/browser/policy/user_cloud_policy_store.cc (right): http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... chrome/browser/policy/user_cloud_policy_store.cc:31: On 2012/08/20 15:29:30, Mattias Nissler wrote: > no newline here I think? Done. http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... chrome/browser/policy/user_cloud_policy_store.cc:106: On 2012/08/20 15:29:30, Mattias Nissler wrote: > remove extra blank line Done. http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... chrome/browser/policy/user_cloud_policy_store.cc:143: DVLOG(1) << "Policy validation succeeded - storing to disk."; On 2012/08/20 15:29:30, Mattias Nissler wrote: > might just replace the two DVLOGs with a single one logging the status? Done. http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... File chrome/browser/policy/user_cloud_policy_store.h (right): http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_clo... chrome/browser/policy/user_cloud_policy_store.h:40: On 2012/08/20 15:29:30, Mattias Nissler wrote: > remove extra blank line Done. http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_pol... File chrome/browser/policy/user_policy_signin_service.cc (right): http://codereview.chromium.org/10825415/diff/1/chrome/browser/policy/user_pol... chrome/browser/policy/user_policy_signin_service.cc:134: manager_->cloud_policy_service()->store()->is_initialized()) { On 2012/08/21 10:45:16, Mattias Nissler wrote: > On 2012/08/20 18:04:37, Andrew T Wilson wrote: > > It is indeed racy. The solution is to move the registration code into > > UserCloudPolicyManager, per the TODO. I didn't want to bother trying to > address > > the race condition in this code, only to have to re-address it a different way > > when I move it to UCPM. > > > > The race condition is OK for now, since there are basically only 3 situations: > > > > a) there are no downloaded policies yet - in this case, it seems that we > always > > initialize the store before the token DB finishes loading (since they share > the > > same thread). Obviously this is fragile, but it's good enough until we address > > the TODO. > > b) There are stored policies, and they fail to validate > > c) There are stored policies, and they don't fail to validate - in that case, > > when the store finishes initializing, the CloudPolicyService will register the > > client using the DM token from the policy. > > > > So the only case that fails currently is b), which I've mentioned in the > > comment. I think this is fine for now, but obviously the TODO must be > addressed > > in short order/before launch. > > Fair enough. Also mention a) in the comment? Done.
Thanks for doing the invalidations to keep the RefreshPolicies() semantics! RefreshPolicies() is quite handy in tests (and not just pyautos, we are moving those to browser_tests) to make sure the updated policies have been loaded. Those tests flaked often before we enforced the rule that after RefreshPolicies() returns then all current policies have already been loaded. It did indeed happen that a previous load/store request was spuriously notifying a subsequent RefreshPolicies(), leading to test failures. A couple of other comments inline, otherwise looks good to me. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... File chrome/browser/policy/user_cloud_policy_store.cc (right): http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:8: #include "base/file_path.h" Nit: already in .h http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:55: return; policy_.reset() before returning here? http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:114: static void PolicyLoadedFromDisk( Nit: namespace { ... } instead of static? http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:115: UserCloudPolicyStore* store, Shouldn't this be a WeakPtr? http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:136: policy_backing_file)); Two suggestions, feel free to ignore :-) 1) Instead of making PolicyBackingFile a refcounted object, you can just make it a "regular" object and pass as base::Owned() to the 2nd bind. 2) PolicyLoadedFromDisk() can be a method of UserCloudPolicyStore, and the |this| object can be passed as a WeakPtr. Bind() takes care of testing the WeakPtr before dispatching. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:182: static void DeleteFileOnFileThread(const FilePath& path) { Nit: namespace { ... } instead of static? http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:189: base::Bind(&DeleteFileOnFileThread, backing_file_path_)); Why not just bind file_util::Delete()? http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:236: policy_backing_file, *validator->policy())); Same here: |policy_backing_file| can be passed with base::Owned() so that it is deleted once the task is dropped (whether it executed before or not). http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:247: path = path.Append(kPolicyCacheFile); Suggestion: FilePath path = profile->GetPath().Append(kPolicyDir).Append(kPolicyCacheFile); http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... File chrome/browser/policy/user_cloud_policy_store.h (right): http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.h:25: explicit UserCloudPolicyStore(Profile* profile, Nit: no need to be explicit
http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... File chrome/browser/policy/user_cloud_policy_store.cc (right): http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:28: : public base::RefCountedThreadSafe<PolicyBackingFile> { I don't see the need for making this a RefCountedThreadSafe class. Two static functions and PostTaskAndReplyWithResult should work? The old UserPolicyDiskCache is from the times when we didn't have a modern callback system that works nice with weak ptrs. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:45: DLOG(WARNING) << "Failed to read policy data from " Make this and the one below a LOG(WARNING)? That'd help diagnose issues in release builds, which we've struggled with on Chrome OS due to insufficient logging a number of times. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:140: bool error, scoped_ptr<em::PolicyFetchResponse> cloud_policy) { each parameter on separate line. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:189: base::Bind(&DeleteFileOnFileThread, backing_file_path_)); On 2012/08/22 08:07:00, Joao da Silva wrote: > Why not just bind file_util::Delete()? That should work if you wrap it in a base::IgnoreResult.
PTAL http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... File chrome/browser/policy/user_cloud_policy_store.cc (right): http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:8: #include "base/file_path.h" On 2012/08/22 08:07:00, Joao da Silva wrote: > Nit: already in .h Done. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:28: : public base::RefCountedThreadSafe<PolicyBackingFile> { On 2012/08/22 08:21:23, Mattias Nissler wrote: > I don't see the need for making this a RefCountedThreadSafe class. Two static > functions and PostTaskAndReplyWithResult should work? The old > UserPolicyDiskCache is from the times when we didn't have a modern callback > system that works nice with weak ptrs. Hah, I knew about PostTaskAndReply but not PostTaskAndReplyWithResult. OK, I'll give it a shot. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:45: DLOG(WARNING) << "Failed to read policy data from " On 2012/08/22 08:21:23, Mattias Nissler wrote: > Make this and the one below a LOG(WARNING)? That'd help diagnose issues in > release builds, which we've struggled with on Chrome OS due to insufficient > logging a number of times. Hmmm. I thought we (chromium) were trying to avoid LOG() since it bloats the executable? http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:55: return; On 2012/08/22 08:07:00, Joao da Silva wrote: > policy_.reset() before returning here? No longer necessary after latest refactoring. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:114: static void PolicyLoadedFromDisk( On 2012/08/22 08:07:00, Joao da Silva wrote: > Nit: namespace { ... } instead of static? Removed. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:115: UserCloudPolicyStore* store, On 2012/08/22 08:07:00, Joao da Silva wrote: > Shouldn't this be a WeakPtr? Interesting - I thought that the WeakPtr->regular ptr translation was handled by the calling template code, so all I needed to do was check for null? If not, I wonder how this compiles - *somebody* must be converting between the types. In any case, it's gone now. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:136: policy_backing_file)); On 2012/08/22 08:07:00, Joao da Silva wrote: > Two suggestions, feel free to ignore :-) > > > 2) PolicyLoadedFromDisk() can be a method of UserCloudPolicyStore, and the > |this| object can be passed as a WeakPtr. Bind() takes care of testing the > WeakPtr before dispatching. The reason I did not do this is because I did not want to expose PolicyBackingFile in the header file (which means I'd have to keep it outside the anonymous namespace). I'll look at this again as part of the suggestion to use PostTaskAndReplyWithResult(). http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:140: bool error, scoped_ptr<em::PolicyFetchResponse> cloud_policy) { On 2012/08/22 08:21:23, Mattias Nissler wrote: > each parameter on separate line. Done. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:182: static void DeleteFileOnFileThread(const FilePath& path) { On 2012/08/22 08:07:00, Joao da Silva wrote: > Nit: namespace { ... } instead of static? Removed in favor of base::IgnoreResult http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:189: base::Bind(&DeleteFileOnFileThread, backing_file_path_)); On 2012/08/22 08:07:00, Joao da Silva wrote: > Why not just bind file_util::Delete()? I could not get it to compile - I thought it was because file_util::Delete() is not a void() function. I guess I could have thrown some casting at it. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:189: base::Bind(&DeleteFileOnFileThread, backing_file_path_)); On 2012/08/22 08:21:23, Mattias Nissler wrote: > On 2012/08/22 08:07:00, Joao da Silva wrote: > > Why not just bind file_util::Delete()? > > That should work if you wrap it in a base::IgnoreResult. Ah, there you go. Thanks. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:236: policy_backing_file, *validator->policy())); On 2012/08/22 08:07:00, Joao da Silva wrote: > Same here: |policy_backing_file| can be passed with base::Owned() so that it is > deleted once the task is dropped (whether it executed before or not). Thanks for the tips. I haven't used the new bind stuff much so I'm still learning the best practices. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:247: path = path.Append(kPolicyCacheFile); On 2012/08/22 08:07:00, Joao da Silva wrote: > Suggestion: > > FilePath path = profile->GetPath().Append(kPolicyDir).Append(kPolicyCacheFile); Done. And, tragically, it comes to 81 characters or otherwise it'd fit on one line :) http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... File chrome/browser/policy/user_cloud_policy_store.h (right): http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.h:25: explicit UserCloudPolicyStore(Profile* profile, On 2012/08/22 08:07:00, Joao da Silva wrote: > Nit: no need to be explicit Done.
LGTM with nits. http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... File chrome/browser/policy/user_cloud_policy_store.cc (right): http://codereview.chromium.org/10825415/diff/10002/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:45: DLOG(WARNING) << "Failed to read policy data from " On 2012/08/22 23:39:30, Andrew T Wilson wrote: > On 2012/08/22 08:21:23, Mattias Nissler wrote: > > Make this and the one below a LOG(WARNING)? That'd help diagnose issues in > > release builds, which we've struggled with on Chrome OS due to insufficient > > logging a number of times. > > Hmmm. I thought we (chromium) were trying to avoid LOG() since it bloats the > executable? So why do we have crash reporting code then given that it adds bytes to the executable? :) I agree with the general idea of avoiding logs, but I think these would be actually helpful in the wild. http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... File chrome/browser/policy/user_cloud_policy_store.cc (right): http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:30: LOAD_RESULT_LOAD_ERROR add trailing comma http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:49: // error() will return true if an error was encountered. comment is outdated http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:53: // If the backing file did not exist, just return. nit: s/did/does/ http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:125: DLOG(WARNING) << "Error reading policy from disk"; nit: reconcile with the LOG in the helper function? i.e. all LOGging either there or here? http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... File chrome/browser/policy/user_cloud_policy_store.h (right): http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.h:26: const FilePath& policy_file); indentation.
LGTM http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... File chrome/browser/policy/user_cloud_policy_store.cc (right): http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:35: enterprise_management::PolicyFetchResponse policy; Nit: em:: http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:38: } // namespace policy Nit: two spaces before // Also, you can keep the policy namespace open here, since it's opened again after the anonymous namespace. http://codereview.chromium.org/10825415/diff/22003/chrome/browser/policy/user... chrome/browser/policy/user_cloud_policy_store.cc:74: DVLOG(1) << "Storing policy to " << path.AsUTF8Unsafe(); First time I see path.AsUTF8Unsafe(), I think path.value() is more common.
https://chromiumcodereview.appspot.com/10825415/diff/10002/chrome/browser/pol... File chrome/browser/policy/user_cloud_policy_store.cc (right): https://chromiumcodereview.appspot.com/10825415/diff/10002/chrome/browser/pol... chrome/browser/policy/user_cloud_policy_store.cc:45: DLOG(WARNING) << "Failed to read policy data from " On 2012/08/23 09:28:34, Mattias Nissler wrote: > On 2012/08/22 23:39:30, Andrew T Wilson wrote: > > On 2012/08/22 08:21:23, Mattias Nissler wrote: > > > Make this and the one below a LOG(WARNING)? That'd help diagnose issues in > > > release builds, which we've struggled with on Chrome OS due to insufficient > > > logging a number of times. > > > > Hmmm. I thought we (chromium) were trying to avoid LOG() since it bloats the > > executable? > > So why do we have crash reporting code then given that it adds bytes to the > executable? :) I agree with the general idea of avoiding logs, but I think these > would be actually helpful in the wild. Done. https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... File chrome/browser/policy/user_cloud_policy_store.cc (right): https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... chrome/browser/policy/user_cloud_policy_store.cc:30: LOAD_RESULT_LOAD_ERROR On 2012/08/23 09:28:35, Mattias Nissler wrote: > add trailing comma Done, although the style guide seems to be inconsistent about this point. https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... chrome/browser/policy/user_cloud_policy_store.cc:35: enterprise_management::PolicyFetchResponse policy; On 2012/08/23 16:23:46, Joao da Silva wrote: > Nit: em:: Done. https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... chrome/browser/policy/user_cloud_policy_store.cc:38: } // namespace policy On 2012/08/23 16:23:46, Joao da Silva wrote: > Nit: two spaces before // > > Also, you can keep the policy namespace open here, since it's opened again after > the anonymous namespace. Done. https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... chrome/browser/policy/user_cloud_policy_store.cc:49: // error() will return true if an error was encountered. On 2012/08/23 09:28:35, Mattias Nissler wrote: > comment is outdated Done. https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... chrome/browser/policy/user_cloud_policy_store.cc:53: // If the backing file did not exist, just return. On 2012/08/23 09:28:35, Mattias Nissler wrote: > nit: s/did/does/ Done. https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... chrome/browser/policy/user_cloud_policy_store.cc:74: DVLOG(1) << "Storing policy to " << path.AsUTF8Unsafe(); On 2012/08/23 16:23:46, Joao da Silva wrote: > First time I see path.AsUTF8Unsafe(), I think path.value() is more common. Done. https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... chrome/browser/policy/user_cloud_policy_store.cc:125: DLOG(WARNING) << "Error reading policy from disk"; On 2012/08/23 09:28:35, Mattias Nissler wrote: > nit: reconcile with the LOG in the helper function? i.e. all LOGging either > there or here? OK, removed from here in favor of there. https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... File chrome/browser/policy/user_cloud_policy_store.h (right): https://chromiumcodereview.appspot.com/10825415/diff/22003/chrome/browser/pol... chrome/browser/policy/user_cloud_policy_store.h:26: const FilePath& policy_file); On 2012/08/23 09:28:35, Mattias Nissler wrote: > indentation. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/atwilson@chromium.org/10825415/28008
Change committed as 153114 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
