Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
205813002:
Separate storage for protected preferences into Protected Preferences file. (Closed)
|
Created:
6 years, 9 months ago by erikwright (departed) Modified:
6 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@pp4_profile_pref_store Visibility:
Public. |
DescriptionSeparate storage for protected preferences into Protected Preferences file.
Introduces ProtectedPrefStore which delegates to two different backing stores to handle unprotected and protected values.
NOTRY=True
BUG=349158
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260387
Patch Set 1 #Patch Set 2 : Review comments from previous mega-CL. #Patch Set 3 : Pull some changes into an earlier CL. #Patch Set 4 : Pre-review. #
Total comments: 44
Patch Set 5 : Enhanced tests for ProtectedPrefStore. #Patch Set 6 : Review comments. #Patch Set 7 : Pull out some changes into other CLs. #
Total comments: 31
Patch Set 8 : Review comments. #Patch Set 9 : Fix copyright. #
Total comments: 10
Patch Set 10 : Increased test coverage. #Patch Set 11 : Add comments to test. #Patch Set 12 : Make it compile and pass. #
Total comments: 2
Patch Set 13 : Rename TeeObserver, remove use of GMock. #Patch Set 14 : Integrate improvement to PrefStoreObserverMock. #Patch Set 15 : Rebase. #Patch Set 16 : Fix a compile error on clang. #Patch Set 17 : Fix MigrateValues test. #Patch Set 18 : Eliminate spurious metrics. #Patch Set 19 : Revert code that breaks tests, commit what works. #
Total comments: 23
Messages
Total messages: 57 (0 generated)
robert: PTAL. Still pending some tests.
Looks great, some comments and such below https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_filter.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_filter.cc:105: if (source->GetValue(it->first, &source_value)) if this fails, can we skip the rest of this loop iteration? https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_filter.cc:107: it->second->EnforceAndReport(&temp_dictionary, transaction.get()); this line needs a comment stating that it validates the setting and can remove it from the dict if it fails to validate (that isn't clear even from the documentation on EnforceAndReport). https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_filter.cc:121: // run. ".. in a future run when MigrateValues() is called again." https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_filter.h (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_filter.h:90: // |source| is trusted according to this filter's PrefHashStore. This will also unconditionally remove tracked preferences from |source| will it not? https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:64: virtual std::string GetSuperMac() const OVERRIDE { nit: double space https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:68: virtual void SetSuperMac(const std::string& super_mac) OVERRIDE { please update hash_store_contents.h with a description of what a "super mac" is. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:72: virtual bool GetVersion(int* version) const OVERRIDE { nit: double space https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:106: scoped_ptr<int> version_; I wish HashStoreContents had const int kInvalidVersion = -1; which would remove the need for using a NULL-valued scoped_ptr to serve as an "I don't have a version" marker. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:284: protected_pref_names.insert(tracking_configuration_[i].name); I find the following easier to read: if (is_protected) { protected_configuration.push_back(tracking_configuration_[i]); protected_pref_names.insert(tracking_configuration_[i].name); } else { unprotected_configuration.push_back(tracking_configuration_[i]); } https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:352: // immediately migrated to two files on load. Suggestion: "This will write out to a single combined file which will be immediately migrated into two files on load." https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:389: reporting_ids_count_))); Can the above be replaced by: scoped_ptr<PrefFilter> pref_filter; if (kPlatformSupportsPreferenceTracking) { pref_filter.reset(new PrefHashFilter( GetPrefHashStoreImpl().PassAs<PrefHashStore>(), tracking_configuration_, reporting_ids_count_)) } return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_), io_task_runner, pref_filter); https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.h:94: // Creates a single-file PrefStore as was used in M34 and earlier. Used for nit: Used only for.. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.h:113: const size_t reporting_ids_count_; Please add a comment describing that this is used for metrics. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:352: TEST_F(ProfilePrefStoreManagerTest, UnprotectedToProtectedWithoutTrust) { Can we add a test that simulates partial migration form UnprotectedToProtected (i.e. MigrateValues() was called earlier but failed to commit the changes to the source PersistentPrefStore) https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/protected_pref_store.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.cc:15: // an event for reporting to the external ReadErrorDelegate if necessary. Please explicitly state that this class is not thread safe. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.cc:98: } Can you replace lines 88-98 with: if (failed_sub_initializations_ + successful_sub_initializations_ == 2) { if (!outer_->on_initialization_.is_null()) outer_->on_initialization_.Run(); FOR_EACH_OBSERVER( PrefStore::Observer, outer_->observers_, OnInitializationCompleted(successful_sub_initializations_ == 2)); } https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.cc:169: read_error : protected_pref_store_->GetReadError(); nit: indent https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/protected_pref_store.h (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.h:66: // observers of the combined store. Also document how this invokes the on_initialization callback. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.h:67: class TeeObserver : public PrefStore::Observer { Naming is hard. NotificationAggregator maybe? Maybe not, your name is fine too. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.h:69: explicit TeeObserver(ProtectedPrefStore* outer); Should pass just the |observers_| list and the |on_initialization_| closure here instead of the whole outer class. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/protected_pref_store_unittest.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store_unittest.cc:68: TEST_F(ProtectedPrefStoreTest, StoreValues) { Can you add a test where either or both of the backing pref stores fail to initialize?
PTAL. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_filter.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_filter.cc:105: if (source->GetValue(it->first, &source_value)) On 2014/03/25 03:05:12, robertshield wrote: > if this fails, can we skip the rest of this loop iteration? No. We want to check if NULL is what was expected, for reporting purposes. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_filter.cc:107: it->second->EnforceAndReport(&temp_dictionary, transaction.get()); On 2014/03/25 03:05:12, robertshield wrote: > this line needs a comment stating that it validates the setting and can remove > it from the dict if it fails to validate (that isn't clear even from the > documentation on EnforceAndReport). Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_filter.cc:121: // run. On 2014/03/25 03:05:12, robertshield wrote: > ".. in a future run when MigrateValues() is called again." Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_filter.h (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_filter.h:90: // |source| is trusted according to this filter's PrefHashStore. On 2014/03/25 03:05:12, robertshield wrote: > This will also unconditionally remove tracked preferences from |source| will it > not? Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:64: virtual std::string GetSuperMac() const OVERRIDE { On 2014/03/25 03:05:12, robertshield wrote: > nit: double space Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:68: virtual void SetSuperMac(const std::string& super_mac) OVERRIDE { On 2014/03/25 03:05:12, robertshield wrote: > please update hash_store_contents.h with a description of what a "super mac" is. Done here: https://codereview.chromium.org/205563009/ https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:72: virtual bool GetVersion(int* version) const OVERRIDE { On 2014/03/25 03:05:12, robertshield wrote: > nit: double space Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:106: scoped_ptr<int> version_; On 2014/03/25 03:05:12, robertshield wrote: > I wish HashStoreContents had > > const int kInvalidVersion = -1; > > which would remove the need for using a NULL-valued scoped_ptr to serve as an "I > don't have a version" marker. The version number will go away in a little while. This could be a bool and an int. Do you like that better? https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:284: protected_pref_names.insert(tracking_configuration_[i].name); On 2014/03/25 03:05:12, robertshield wrote: > I find the following easier to read: > > if (is_protected) { > protected_configuration.push_back(tracking_configuration_[i]); > protected_pref_names.insert(tracking_configuration_[i].name); > } else { > unprotected_configuration.push_back(tracking_configuration_[i]); > } > > Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:352: // immediately migrated to two files on load. On 2014/03/25 03:05:12, robertshield wrote: > Suggestion: "This will write out to a single combined file which will be > immediately migrated into two files on load." Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:389: reporting_ids_count_))); On 2014/03/25 03:05:12, robertshield wrote: > Can the above be replaced by: > > scoped_ptr<PrefFilter> pref_filter; > if (kPlatformSupportsPreferenceTracking) { > pref_filter.reset(new PrefHashFilter( > GetPrefHashStoreImpl().PassAs<PrefHashStore>(), > tracking_configuration_, reporting_ids_count_)) > } > > return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_), > io_task_runner, pref_filter); Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.h:94: // Creates a single-file PrefStore as was used in M34 and earlier. Used for On 2014/03/25 03:05:12, robertshield wrote: > nit: Used only for.. Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.h:113: const size_t reporting_ids_count_; On 2014/03/25 03:05:12, robertshield wrote: > Please add a comment describing that this is used for metrics. Added to the constructor in the prior CL: https://codereview.chromium.org/185543017/diff2/250001:270001/chrome/browser/... is that sufficient? https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:352: TEST_F(ProfilePrefStoreManagerTest, UnprotectedToProtectedWithoutTrust) { On 2014/03/25 03:05:12, robertshield wrote: > Can we add a test that simulates partial migration form UnprotectedToProtected > (i.e. MigrateValues() was called earlier but failed to commit the changes to the > source PersistentPrefStore) Yes. That's a bit trickier so I'm going to upload the rest and think about how to do it best. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/protected_pref_store.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.cc:15: // an event for reporting to the external ReadErrorDelegate if necessary. On 2014/03/25 03:05:12, robertshield wrote: > Please explicitly state that this class is not thread safe. Luckily the class is no longer needed. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.cc:98: } On 2014/03/25 03:05:12, robertshield wrote: > Can you replace lines 88-98 with: > > if (failed_sub_initializations_ + successful_sub_initializations_ == 2) { > if (!outer_->on_initialization_.is_null()) > outer_->on_initialization_.Run(); > FOR_EACH_OBSERVER( > PrefStore::Observer, > outer_->observers_, > OnInitializationCompleted(successful_sub_initializations_ == 2)); > } Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.cc:169: read_error : protected_pref_store_->GetReadError(); On 2014/03/25 03:05:12, robertshield wrote: > nit: indent Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/protected_pref_store.h (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.h:66: // observers of the combined store. On 2014/03/25 03:05:12, robertshield wrote: > Also document how this invokes the on_initialization callback. Done. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.h:67: class TeeObserver : public PrefStore::Observer { On 2014/03/25 03:05:12, robertshield wrote: > Naming is hard. NotificationAggregator maybe? Maybe not, your name is fine too. In Unix there is a utility named tee that takes input from one thing and outputs it to two things. Although this is the reverse, it was my naming inspiration. I don't mind changing it. There will be a reviewer from Munich and I'll get them to weigh in before I go to the trouble. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.h:69: explicit TeeObserver(ProtectedPrefStore* outer); On 2014/03/25 03:05:12, robertshield wrote: > Should pass just the |observers_| list and the |on_initialization_| closure here > instead of the whole outer class. There are three members of the outer class that are accessed by the inner class. Two of them are only accessed by the inner class (read_error_delegate_ and on_initialization_). observers_ is accessed by three methods of the outer class (AddObserver, RemoveObserver, and HasObservers). Forwarding those three methods to the inner class is possible but cumbersome and also turns the Observer implementation into something more than an observer implementation. So we are left with the fact that there _will_ be shared state (i.e., at a minimum, TeeObserver must have a pointer to observers_ as well as the two other values, which it could be the owner of). I don't really mind, but truth be told I think that the current design is correct and good. The inner class is part of the implementation of the outer class, it's OK and normal for them to share state. I'll upload another patchset with things the way you suggested, and you/someone in Munich can compare the two approaches. https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/protected_pref_store_unittest.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store_unittest.cc:68: TEST_F(ProtectedPrefStoreTest, StoreValues) { On 2014/03/25 03:05:12, robertshield wrote: > Can you add a test where either or both of the backing pref stores fail to > initialize? Done.
https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/protected_pref_store.h (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/protected_pref_store.h:69: explicit TeeObserver(ProtectedPrefStore* outer); On 2014/03/25 20:28:26, erikwright wrote: > On 2014/03/25 03:05:12, robertshield wrote: > > Should pass just the |observers_| list and the |on_initialization_| closure > here > > instead of the whole outer class. > > There are three members of the outer class that are accessed by the inner class. > Two of them are only accessed by the inner class (read_error_delegate_ and > on_initialization_). observers_ is accessed by three methods of the outer class > (AddObserver, RemoveObserver, and HasObservers). Forwarding those three methods > to the inner class is possible but cumbersome and also turns the Observer > implementation into something more than an observer implementation. > Also, in the current patchset we now access outer_->GetReadError() from the inner class. So we are not going to avoid having a pointer to the outer class. With that in mind, I think the current design is now definitively better. An alternative would be to have only a pointer to outer_, and simply forward all of the calls that access outer_'s private state to private methods of outer_. But that doesn't seem different or better to me. > So we are left with the fact that there _will_ be shared state (i.e., at a > minimum, TeeObserver must have a pointer to observers_ as well as the two other > values, which it could be the owner of). > > I don't really mind, but truth be told I think that the current design is > correct and good. The inner class is part of the implementation of the outer > class, it's OK and normal for them to share state. > > I'll upload another patchset with things the way you suggested, and you/someone > in Munich can compare the two approaches.
bauerb: PTAL.
https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/50001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:106: scoped_ptr<int> version_; On 2014/03/25 20:28:26, erikwright wrote: > On 2014/03/25 03:05:12, robertshield wrote: > > I wish HashStoreContents had > > > > const int kInvalidVersion = -1; > > > > which would remove the need for using a NULL-valued scoped_ptr to serve as an > "I > > don't have a version" marker. > > The version number will go away in a little while. > > This could be a bool and an int. Do you like that better? FWIW, I would prefer a negative value for "no version" as well (I was going to write a comment to that effect, before I found this one). https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter.h (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.h:22: class PersistentPrefStore; Nit: Move before PrefService https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:36: : static_cast<base::DictionaryValue*>(NULL)) { Is the static_cast because of the ?: operator? I would probably initialize |dictionary_| to NULL here in any case, and in the constructor body conditionally assign the other value to it. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:47: super_mac_ = ""; In general, using std::string() instead of "" saves a handful of instructions. In this case, you can just call .clear(). https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:52: return dictionary_; I would be a bit happier if we had an explicit coercion to bool here (for example, by negating it twice, i.e. `!!dictionary_`). I think I remember a bug where a value was truncated to a char (because there is no type that has only one bit), which meant that a non-zero value where the least-significant byte was zero would end up being interpreted as false. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:140: const bool ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking = Hm, couldn't you make this a #define and then #ifdef out all the code that's unnecessary if the platform doesn't support preference tracking? https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:108: "", Use std::string() instead of "". https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:134: } Nit: empty line afterwards. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/protected_pref_store.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.cc:21: if (failed_sub_initializations_ + successful_sub_initializations_ == 2) { You could negate the condition and early-return. That would also fit in a bit more with the style of the comment (which talks about what happens if initialization is *not* finished yet). https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/protected_pref_store.h (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:68: class TeeObserver : public PrefStore::Observer { Hmm... do you even need this class? The outer class could just as well directly observe its pref stores. Then you wouldn't need to reach into the |outer_| object like you do now. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:78: uint8 failed_sub_initializations_; Nit: The style guide says to only use unsigned types if you really need them (because you store bit patterns, or need defined overflow behavior), and in general use plain ints for small integral types. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:97: scoped_refptr<PersistentPrefStore> protected_pref_store_; It's a bit strange that the ProtectedPrefStore has a |protected_pref_store_| ;-) Should this class maybe be called "UnifiedPrefStore" or "MergedPrefStore" or something?
Two quick responses. I'll work on the rest now. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/protected_pref_store.h (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:68: class TeeObserver : public PrefStore::Observer { On 2014/03/26 15:05:50, Bernhard Bauer wrote: > Hmm... do you even need this class? The outer class could just as well directly > observe its pref stores. Then you wouldn't need to reach into the |outer_| > object like you do now. In my personal opinion it breaks encapsulation by exposing the PrefStoreObserver interface to the client of ProtectedPrefStore. I believe that the current design is a model for how a class can implement multiple interfaces while enforcing encapsulation. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:97: scoped_refptr<PersistentPrefStore> protected_pref_store_; On 2014/03/26 15:05:50, Bernhard Bauer wrote: > It's a bit strange that the ProtectedPrefStore has a |protected_pref_store_| ;-) > Should this class maybe be called "UnifiedPrefStore" or "MergedPrefStore" or > something? We all agree on this. It's a bit more than a Unified or Merged pref store because it has logic specific to protection (i.e., it doesn't solve any generalized problem of aggregating two stores). PartiallyProtectedPrefStore? No. How about SegregatedPrefStore? That's really the problem this solves - segregating a subset of prefs into a separate store. The reason they are segregated (that they are protected) is not so important.
https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/protected_pref_store.h (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:68: class TeeObserver : public PrefStore::Observer { On 2014/03/26 15:17:14, erikwright wrote: > On 2014/03/26 15:05:50, Bernhard Bauer wrote: > > Hmm... do you even need this class? The outer class could just as well > directly > > observe its pref stores. Then you wouldn't need to reach into the |outer_| > > object like you do now. > > In my personal opinion it breaks encapsulation by exposing the PrefStoreObserver > interface to the client of ProtectedPrefStore. Hm, I would have been okay with that. Clients do see implementation details of this class already (like private methods), they just can't access them. I don't think it's the end of the world if a client could now in theory register this class as an observer of a different pref store, because there would be no reason to do so. If you really don't want to expose the PrefStore::Observer interface, you could make a private subclass of this class that implements it (with a factory method in this class). > I believe that the current design is a model for how a class can implement > multiple interfaces while enforcing encapsulation. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:97: scoped_refptr<PersistentPrefStore> protected_pref_store_; On 2014/03/26 15:17:14, erikwright wrote: > On 2014/03/26 15:05:50, Bernhard Bauer wrote: > > It's a bit strange that the ProtectedPrefStore has a |protected_pref_store_| > ;-) > > Should this class maybe be called "UnifiedPrefStore" or "MergedPrefStore" or > > something? > > We all agree on this. > > It's a bit more than a Unified or Merged pref store because it has logic > specific to protection (i.e., it doesn't solve any generalized problem of > aggregating two stores). > > PartiallyProtectedPrefStore? No. > > How about SegregatedPrefStore? That's really the problem this solves - > segregating a subset of prefs into a separate store. The reason they are > segregated (that they are protected) is not so important. SGTM.
https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:52: return dictionary_; On 2014/03/26 15:05:50, Bernhard Bauer wrote: > I would be a bit happier if we had an explicit coercion to bool here (for > example, by negating it twice, i.e. `!!dictionary_`). I think I remember a bug > where a value was truncated to a char (because there is no type that has only > one bit), which meant that a non-zero value where the least-significant byte was > zero would end up being interpreted as false. This is explicitly supported by scoped_ptr, and covered by its unit tests. I don't mind following your suggestion if you feel strongly about it, but since other reviewers have specifically asked me to use implicit coercion to boolean in the past I suspect it will be unlikely to catch on. Of course, it would be a problem if this were safe with scoped_ptr but not with raw pointers, in the chance that a future CL changed the type of dictionary_. I read up a bit about it, and couldn't find any reference to the potential for harm here. I did find the following (search for "Boolean conversions"): http://en.cppreference.com/w/cpp/language/implicit_cast It explicitly defines the behaviour when a pointer is cast to bool.
https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter.h (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.h:22: class PersistentPrefStore; On 2014/03/26 15:05:50, Bernhard Bauer wrote: > Nit: Move before PrefService Done. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:36: : static_cast<base::DictionaryValue*>(NULL)) { On 2014/03/26 15:05:50, Bernhard Bauer wrote: > Is the static_cast because of the ?: operator? > > I would probably initialize |dictionary_| to NULL here in any case, and in the > constructor body conditionally assign the other value to it. Done. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:47: super_mac_ = ""; On 2014/03/26 15:05:50, Bernhard Bauer wrote: > In general, using std::string() instead of "" saves a handful of instructions. > In this case, you can just call .clear(). Done. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:140: const bool ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking = On 2014/03/26 15:05:50, Bernhard Bauer wrote: > Hm, couldn't you make this a #define and then #ifdef out all the code that's > unnecessary if the platform doesn't support preference tracking? It's also referenced in chrome_pref_service_factory.cc, so you would need to expose that #define in the header and reference it there too (or use both a #define and a boolean). (1) This will be universally supported shortly (the follow up to this CL will resolve the issues blocking ChromeOS and Android). (2) I don't think it will have any measurable impact on performance or code size. I'm pretty sure 80% of the stuff that would be hidden would still be referenceable. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:108: "", On 2014/03/26 15:05:50, Bernhard Bauer wrote: > Use std::string() instead of "". Done. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:134: } On 2014/03/26 15:05:50, Bernhard Bauer wrote: > Nit: empty line afterwards. Done. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/protected_pref_store.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.cc:21: if (failed_sub_initializations_ + successful_sub_initializations_ == 2) { On 2014/03/26 15:05:50, Bernhard Bauer wrote: > You could negate the condition and early-return. That would also fit in a bit > more with the style of the comment (which talks about what happens if > initialization is *not* finished yet). Done. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/protected_pref_store.h (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:68: class TeeObserver : public PrefStore::Observer { On 2014/03/26 16:41:11, Bernhard Bauer wrote: > On 2014/03/26 15:17:14, erikwright wrote: > > On 2014/03/26 15:05:50, Bernhard Bauer wrote: > > > Hmm... do you even need this class? The outer class could just as well > > directly > > > observe its pref stores. Then you wouldn't need to reach into the |outer_| > > > object like you do now. > > > > In my personal opinion it breaks encapsulation by exposing the > PrefStoreObserver > > interface to the client of ProtectedPrefStore. > > Hm, I would have been okay with that. Clients do see implementation details of > this class already (like private methods), they just can't access them. I don't > think it's the end of the world if a client could now in theory register this > class as an observer of a different pref store, because there would be no reason > to do so. I don't really agree with the reasoning. I find the subclass + factory method approach to be the least desirable of everything proposed. If you want me to make ProtectedPrefStore implement PrefStore::Observer directly I will. But I would like to know what the specific objection is to the current design. > > If you really don't want to expose the PrefStore::Observer interface, you could > make a private subclass of this class that implements it (with a factory method > in this class). > > > I believe that the current design is a model for how a class can implement > > multiple interfaces while enforcing encapsulation. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:78: uint8 failed_sub_initializations_; On 2014/03/26 15:05:50, Bernhard Bauer wrote: > Nit: The style guide says to only use unsigned types if you really need them > (because you store bit patterns, or need defined overflow behavior), and in > general use plain ints for small integral types. Done. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/protected_pref_store_unittest.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store_unittest.cc:5: #include "chrome/browser/prefs/tracked/protected_pref_store.cc" Ha. Can't believe I wrote it, not too surprised y'all missed it. But I bet gab@ would have caught it. grt@ too.
looking good, few more comments below https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:191: EXPECT_EQ((int)pref_file_contents.length(), why is this using a C-style cast? https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:387: // Accessing the value of the previously proteted pref didn't trigger its move protected https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:397: ExpectStringValueEquals(kProtectedAtomic, kGoodbyeWorld); It's not clear to me how this test checks that the migration back to the unprotected preferences file occurs. Please could you add a comment explaining how it does that. https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:400: // Add test coverage for when Reset events should and should not be recorded. Please make this a TODO if you're not planning on doing it in this CL https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/segregated_pref_store.cc (right): https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:19: // told about initialization. Why is it safe to drop these events? (I ask mostly out of ignorance as I don't know what consumers of these events do with them)
https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/segregated_pref_store.cc (right): https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:19: // told about initialization. On 2014/03/26 21:41:54, robertshield wrote: > Why is it safe to drop these events? (I ask mostly out of ignorance as I don't > know what consumers of these events do with them) The model is as follows: void X::whatever() { store_ = ...?; // At this point, no data is loaded. store_.ReadPrefsAsync(); // At this point, data is loading, but I have no idea what it is ... } // implementation of PrefStore::Observer void X::OnInitializationCompleted() { // OK! Data is loaded. Now I can inspect it to get the values that came from disk. store_.Get(kMyValue, &some_value_); // Now I know what the value of kMyValue is... If someone ever changes it, I'll get a notification! } void X::OnValueChanged(string key) { // Alright, now I can refresh my copy of kMyValue ... } So, basically, if I never heard that the data is loaded, then a change in the value is not relevant to me.
PTAL. https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:191: EXPECT_EQ((int)pref_file_contents.length(), On 2014/03/26 21:41:54, robertshield wrote: > why is this using a C-style cast? Done. https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:387: // Accessing the value of the previously proteted pref didn't trigger its move On 2014/03/26 21:41:54, robertshield wrote: > protected Done. https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:397: ExpectStringValueEquals(kProtectedAtomic, kGoodbyeWorld); On 2014/03/26 21:41:54, robertshield wrote: > It's not clear to me how this test checks that the migration back to the > unprotected preferences file occurs. Please could you add a comment explaining > how it does that. Clarified the purpose of the test with a comment at the top. https://codereview.chromium.org/205813002/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:400: // Add test coverage for when Reset events should and should not be recorded. On 2014/03/26 21:41:54, robertshield wrote: > Please make this a TODO if you're not planning on doing it in this CL Done.
LGTM, just one nit below: https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:52: return dictionary_; On 2014/03/26 18:37:15, erikwright wrote: > On 2014/03/26 15:05:50, Bernhard Bauer wrote: > > I would be a bit happier if we had an explicit coercion to bool here (for > > example, by negating it twice, i.e. `!!dictionary_`). I think I remember a bug > > where a value was truncated to a char (because there is no type that has only > > one bit), which meant that a non-zero value where the least-significant byte > was > > zero would end up being interpreted as false. > > This is explicitly supported by scoped_ptr, and covered by its unit tests. I > don't mind following your suggestion if you feel strongly about it, but since > other reviewers have specifically asked me to use implicit coercion to boolean > in the past I suspect it will be unlikely to catch on. Oh, right, it's a scoped_ptr. Yeah, those are safe. > Of course, it would be a problem if this were safe with scoped_ptr but not with > raw pointers, in the chance that a future CL changed the type of dictionary_. I > read up a bit about it, and couldn't find any reference to the potential for > harm here. I did find the following (search for "Boolean conversions"): > > http://en.cppreference.com/w/cpp/language/implicit_cast > > It explicitly defines the behaviour when a pointer is cast to bool. What I was thinking of was http://bugs.mysql.com/bug.php?id=64884 (which made it to some tech news sites at the time). Maybe that bug is because of C though and C++ has sane implicit conversion to bool. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:140: const bool ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking = On 2014/03/26 21:08:12, erikwright wrote: > On 2014/03/26 15:05:50, Bernhard Bauer wrote: > > Hm, couldn't you make this a #define and then #ifdef out all the code that's > > unnecessary if the platform doesn't support preference tracking? > > It's also referenced in chrome_pref_service_factory.cc, so you would need to > expose that #define in the header and reference it there too (or use both a > #define and a boolean). > > (1) This will be universally supported shortly (the follow up to this CL will > resolve the issues blocking ChromeOS and Android). > (2) I don't think it will have any measurable impact on performance or code > size. I'm pretty sure 80% of the stuff that would be hidden would still be > referenceable. Oh, ok. If it will be removed soon anyway, it's fine. https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/protected_pref_store.h (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:68: class TeeObserver : public PrefStore::Observer { On 2014/03/26 21:08:12, erikwright wrote: > On 2014/03/26 16:41:11, Bernhard Bauer wrote: > > On 2014/03/26 15:17:14, erikwright wrote: > > > On 2014/03/26 15:05:50, Bernhard Bauer wrote: > > > > Hmm... do you even need this class? The outer class could just as well > > > directly > > > > observe its pref stores. Then you wouldn't need to reach into the |outer_| > > > > object like you do now. > > > > > > In my personal opinion it breaks encapsulation by exposing the > > PrefStoreObserver > > > interface to the client of ProtectedPrefStore. > > > > Hm, I would have been okay with that. Clients do see implementation details of > > this class already (like private methods), they just can't access them. I > don't > > think it's the end of the world if a client could now in theory register this > > class as an observer of a different pref store, because there would be no > reason > > to do so. > > I don't really agree with the reasoning. > > I find the subclass + factory method approach to be the least desirable of > everything proposed. > > If you want me to make ProtectedPrefStore implement PrefStore::Observer directly > I will. But I would like to know what the specific objection is to the current > design. Some problems with the current design: * You have non-zero overhead due to the additional |outer_| member * You need to reach into |outer_| and access private members * As previously discussed on the review, the class name isn't very intuitive Now, granted, none of these are big issues, it's just that exposing the PrefStore::Observer interface to clients isn't a big issue to me either. In fact, we have a lot of instances where a class subscribes itself as an observer (e.g. almost all classes implement NotificationObserver), and if that class is public, it will expose the observer interface to its clients. All of that being said, if you want to keep the current approach, I won't block it; I just wanted to put all the options on the table. > > > > If you really don't want to expose the PrefStore::Observer interface, you > could > > make a private subclass of this class that implements it (with a factory > method > > in this class). > > > > > I believe that the current design is a model for how a class can implement > > > multiple interfaces while enforcing encapsulation. > https://codereview.chromium.org/205813002/diff/500001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/205813002/diff/500001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:132: pref_service_factory.Create(profile_pref_registry_).get()) This expression is a bit big. Could you extract e.g. the value returned from pref_service_factory.Create() to a local variable?
lgtm
FYI. Waiting on a review from mnissler on https://codereview.chromium.org/210063003/ . https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:52: return dictionary_; On 2014/03/27 15:10:45, Bernhard Bauer wrote: > On 2014/03/26 18:37:15, erikwright wrote: > > On 2014/03/26 15:05:50, Bernhard Bauer wrote: > > > I would be a bit happier if we had an explicit coercion to bool here (for > > > example, by negating it twice, i.e. `!!dictionary_`). I think I remember a > bug > > > where a value was truncated to a char (because there is no type that has > only > > > one bit), which meant that a non-zero value where the least-significant byte > > was > > > zero would end up being interpreted as false. > > > > This is explicitly supported by scoped_ptr, and covered by its unit tests. I > > don't mind following your suggestion if you feel strongly about it, but since > > other reviewers have specifically asked me to use implicit coercion to boolean > > in the past I suspect it will be unlikely to catch on. > > Oh, right, it's a scoped_ptr. Yeah, those are safe. > > > Of course, it would be a problem if this were safe with scoped_ptr but not > with > > raw pointers, in the chance that a future CL changed the type of dictionary_. > I > > read up a bit about it, and couldn't find any reference to the potential for > > harm here. I did find the following (search for "Boolean conversions"): > > > > http://en.cppreference.com/w/cpp/language/implicit_cast > > > > It explicitly defines the behaviour when a pointer is cast to bool. > > What I was thinking of was http://bugs.mysql.com/bug.php?id=64884 (which made it > to some tech news sites at the time). Maybe that bug is because of C though and > C++ has sane implicit conversion to bool. Ah, right. Yeah, that would be an improvement due to a native bool type with explicit conversion semantics. Thanks for sharing that! https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/protected_pref_store.h (right): https://codereview.chromium.org/205813002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/protected_pref_store.h:68: class TeeObserver : public PrefStore::Observer { On 2014/03/27 15:10:45, Bernhard Bauer wrote: > On 2014/03/26 21:08:12, erikwright wrote: > > On 2014/03/26 16:41:11, Bernhard Bauer wrote: > > > On 2014/03/26 15:17:14, erikwright wrote: > > > > On 2014/03/26 15:05:50, Bernhard Bauer wrote: > > > > > Hmm... do you even need this class? The outer class could just as well > > > > directly > > > > > observe its pref stores. Then you wouldn't need to reach into the > |outer_| > > > > > object like you do now. > > > > > > > > In my personal opinion it breaks encapsulation by exposing the > > > PrefStoreObserver > > > > interface to the client of ProtectedPrefStore. > > > > > > Hm, I would have been okay with that. Clients do see implementation details > of > > > this class already (like private methods), they just can't access them. I > > don't > > > think it's the end of the world if a client could now in theory register > this > > > class as an observer of a different pref store, because there would be no > > reason > > > to do so. > > > > I don't really agree with the reasoning. > > > > I find the subclass + factory method approach to be the least desirable of > > everything proposed. > > > > If you want me to make ProtectedPrefStore implement PrefStore::Observer > directly > > I will. But I would like to know what the specific objection is to the current > > design. > > Some problems with the current design: > * You have non-zero overhead due to the additional |outer_| member You mean the extra dereference? I suppose so, in theory, but I don't think that's relevant here. > * You need to reach into |outer_| and access private members There will always be the exact same amount of logic sharing state - either in two nested classes or joined together in a single class. Although there are other ways of sharing the state (introducing a third class/type to hold the shared state, sharing pointers/copying the values of the shared state, ...) I don't believe they are justified here. Sharing the state and using a nested class helps to decompose the logic of this class into two related but clearly delineated parts, while also presenting a robust, provably safe, and clear public API. > * As previously discussed on the review, the class name isn't very intuitive We agreed to change this to AggregatingObserver, which narrowly beat out UpsideDownTeeObserver and EetObserver. > Now, granted, none of these are big issues, it's just that exposing the > PrefStore::Observer interface to clients isn't a big issue to me either. In > fact, we have a lot of instances where a class subscribes itself as an observer > (e.g. almost all classes implement NotificationObserver), and if that class is > public, it will expose the observer interface to its clients. I agree that it is common, but I think that we can also agree that the presence of a pattern in the Chromium codebase does not, in and of itself, indicate that it is a good pattern or without tradeoffs. I believe that the approach demonstrated in this CL represents an improvement over that pattern. > All of that being said, if you want to keep the current approach, I won't block > it; I just wanted to put all the options on the table. Thank you for the discussion and the flexibility. > > > If you really don't want to expose the PrefStore::Observer interface, you > > could > > > make a private subclass of this class that implements it (with a factory > > method > > > in this class). > > > > > > > I believe that the current design is a model for how a class can implement > > > > multiple interfaces while enforcing encapsulation. > > > https://codereview.chromium.org/205813002/diff/500001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/205813002/diff/500001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:132: pref_service_factory.Create(profile_pref_registry_).get()) On 2014/03/27 15:10:45, Bernhard Bauer wrote: > This expression is a bit big. Could you extract e.g. the value returned from > pref_service_factory.Create() to a local variable? Done.
jochen: PTAL for chrome_constants.{cc,h}
lgtm
The CQ bit was checked by erikwright@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/205813002/560001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by erikwright@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/205813002/560001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was unchecked by erikwright@chromium.org
The CQ bit was checked by erikwright@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/205813002/560001
The CQ bit was checked by erikwright@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/205813002/580001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by erikwright@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/205813002/600001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by erikwright@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/205813002/620001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by erikwright@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/205813002/640001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by erikwright@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/205813002/640001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by erikwright@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/205813002/640001
Message was sent while issue was closed.
Change committed as 260387
Message was sent while issue was closed.
Very nice :)! Some comments below, mostly nits, one question about the correctness of MigrateValues(). Cheers, Gab https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.cc:98: pref_hash_store_->BeginTransaction(); Is pref_hash_store_ expected to be the underlying hash store for |source| or for |destination| (I'm guessing |source| since this is how it's used in this method). I think a meta-comment in the interface is necessary to clarify this. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.cc:110: // value from |temp_dictionary|. nit: Remove extra space at beginning of this line. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.cc:125: transaction.reset(); This reset() won't actually force a CommitPendingWrite() to the underlying storage (right?), isn't that a problem? If pref_hash_store_ is also the underlying store for |source| then maybe this is okay as it becomes irrelevant after |destination| was flushed? In fact does the explicit reset() even matter? https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter_unittest.cc:314: // |mock_pref_hash_store|. This comment is wrong, what about: // Resets |pref_hash_filter_| with a PrefHashFilter that uses a MockPrefHashStore. The raw pointer to the MockPrefHashStore (owned by the PrefHashFilter) is stored in |mock_pref_hash_store_| for testing. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter_unittest.cc:891: PrefHashStoreTransaction::UNCHANGED); To have better test coverage, one of these would be CHANGED and expected to be reset below. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter_unittest.cc:928: ASSERT_TRUE(destination->GetValue(kAtomicPref3, NULL)); Why not check the value of kAtomicPref3 in this case? https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:161: // It's a bit of a coincidence that this (and ClearResetTime) work(s). The :( -- to avoid all of this madness how about we use PrefService to Set/Get (we can't do this directly at the time of Set() since the PrefService doesn't exist yet, but we could post a task to have an anonymous method kick in to do this properly when the message loop starts running). Feels cleaner than depending so tightly on the implementation for this to work. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:168: // "protected" it will be read from the protected store prefentially to the s/prefentially/preferentially https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/segregated_pref_store.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:22: return; Shouldn't we keep this notification around to send it after we send the initialization ping? i.e. 1) Store 1 is initialized 2) A value is changed in store 1 3) Store 2 is initialized 4) We notify observers of initialization Shouldn't we then 5) Notify observers of the value changed in (2)? Is it even possible for (2) to occur before (3) or should this return also be NOTREACHED()? https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:38: rm empty line https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:155: SegregatedPrefStore::StoreForKey(const std::string& key) const { Method name fits on previous line, wrap params instead of method name. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:175: default_pref_store_->SetValue(key, migrated_value.release()); Why not just inline value->DeepCopy() here? https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:178: selected_pref_store_->CommitPendingWrite(); This commit isn't strictly speaking necessary right? i.e., the only important thing is that the |default_pref_store| be flushed first (not that the |selected_pref_store| be flushed immediately after). Should we remove it? https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/segregated_pref_store.h (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.h:30: // before observers of the combined store are notified. What's "the combined store", this SegregatedPrefStore?
Message was sent while issue was closed.
https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.cc:98: pref_hash_store_->BeginTransaction(); On 2014/04/01 18:55:06, gab wrote: > Is pref_hash_store_ expected to be the underlying hash store for |source| or for > |destination| (I'm guessing |source| since this is how it's used in this > method). I think a meta-comment in the interface is necessary to clarify this. Ah, now I understand (from seeing how it's actually used in https://codereview.chromium.org/218583003/). This PrefHashFilter is actually a throw away PrefHashFilter using the protected configuration and |pref_hash_store_| is a disposable copy of |source|'s PrefHashStore. I think the API comment should still be clarified to mention this, this method is easier to reason about if the reader can assume |pref_hash_store_| isn't the underlying store for |source| nor |destination|. (also note that this means that flushing the |transaction| below is irrelevant IMO)
Message was sent while issue was closed.
Some responses. The changes will be uploaded in a separate CL. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.cc:98: pref_hash_store_->BeginTransaction(); On 2014/04/01 18:55:06, gab wrote: > Is pref_hash_store_ expected to be the underlying hash store for |source| or for > |destination| (I'm guessing |source| since this is how it's used in this > method). I think a meta-comment in the interface is necessary to clarify this. The method comment for MigrateValues says, in part: "Values are migrated if they are protected according to this filter's configuration, the corresponding key has no value in |destination|, and the value in |source| is trusted according to this filter's PrefHashStore." Is that sufficient? https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.cc:125: transaction.reset(); On 2014/04/01 18:55:06, gab wrote: > This reset() won't actually force a CommitPendingWrite() to the underlying > storage (right?), isn't that a problem? > > If pref_hash_store_ is also the underlying store for |source| then maybe this is > okay as it becomes irrelevant after |destination| was flushed? In fact does the > explicit reset() even matter? It doesn't matter in practice, not the least because the hash store that is used is actually a copy of the original hash store (see ProfilePrefStoreManager). But if it were hypothetically going to have an impact on anything, it would presumably be most appropriate to persist changes _after_ persisting the destination and before persisting the source (on the assumption that this could hypothetically be colocated with source). Basically, I think it's appropriate for us to explicitly decide where to end the transaction, and this location seems like the least surprising. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:161: // It's a bit of a coincidence that this (and ClearResetTime) work(s). The On 2014/04/01 18:55:06, gab wrote: > :( -- to avoid all of this madness how about we use PrefService to Set/Get (we > can't do this directly at the time of Set() since the PrefService doesn't exist > yet, but we could post a task to have an anonymous method kick in to do this > properly when the message loop starts running). > > Feels cleaner than depending so tightly on the implementation for this to work. The current implementation is cleaner, easier to understand, and has less moving parts than what you propose. If I were really worried I would just expose the name of the pref from PrefHashFilter and then add it to the list of selected prefs that I pass to the segregated pref store. Then I would know for certain that it was stored in the right place. But it's not really tightly coupled to anything too far removed. Remember that it is _this_ class (ProfilePrefStoreManager) that is responsible for doing the configuration - so if the behaviour changes, this class will be part of the change. Finally, there are unit tests that prevent unintended regressions of this functionality. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/segregated_pref_store.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:22: return; On 2014/04/01 18:55:06, gab wrote: > Shouldn't we keep this notification around to send it after we send the > initialization ping? > > i.e. > 1) Store 1 is initialized > 2) A value is changed in store 1 > 3) Store 2 is initialized > 4) We notify observers of initialization > > Shouldn't we then 5) Notify observers of the value changed in (2)? > > Is it even possible for (2) to occur before (3) or should this return also be > NOTREACHED()? Yes it is possible for (2) to occur before (3). For example, the PrefHashFilter attached to Store 1 might change a value in Store 1. Please see previous discussion with Robert about this. There is no reason to inform clients of a change in this store when they never heard of the original value. The value that is present when they first hear about the store _is_ the original value, as far as they are concerned. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:178: selected_pref_store_->CommitPendingWrite(); On 2014/04/01 18:55:06, gab wrote: > This commit isn't strictly speaking necessary right? > > i.e., the only important thing is that the |default_pref_store| be flushed first > (not that the |selected_pref_store| be flushed immediately after). > > Should we remove it? I suppose it's OK. If someone calls CommitPendingWrite on the SegregatedPrefStore we will commit each of the underlying stores, which will have the affect that the caller expected.
Message was sent while issue was closed.
https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.cc:98: pref_hash_store_->BeginTransaction(); On 2014/04/01 19:46:08, erikwright wrote: > On 2014/04/01 18:55:06, gab wrote: > > Is pref_hash_store_ expected to be the underlying hash store for |source| or > for > > |destination| (I'm guessing |source| since this is how it's used in this > > method). I think a meta-comment in the interface is necessary to clarify this. > > The method comment for MigrateValues says, in part: > > "Values are migrated if they are protected according to this filter's > configuration, the corresponding key has no value in |destination|, and the > value in |source| is trusted according to this filter's PrefHashStore." > > Is that sufficient? Perhaps adding something like: "This PrefHashFilter shouldn't be tied to |source| nor |destination|." ? I don't really like this approach overall, but it's the easiest short term solution. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:161: // It's a bit of a coincidence that this (and ClearResetTime) work(s). The On 2014/04/01 19:46:08, erikwright wrote: > On 2014/04/01 18:55:06, gab wrote: > > :( -- to avoid all of this madness how about we use PrefService to Set/Get (we > > can't do this directly at the time of Set() since the PrefService doesn't > exist > > yet, but we could post a task to have an anonymous method kick in to do this > > properly when the message loop starts running). > > > > Feels cleaner than depending so tightly on the implementation for this to > work. > > The current implementation is cleaner, easier to understand, and has less moving > parts than what you propose. > > If I were really worried I would just expose the name of the pref from > PrefHashFilter and then add it to the list of selected prefs that I pass to the > segregated pref store. Then I would know for certain that it was stored in the > right place. > > But it's not really tightly coupled to anything too far removed. Remember that > it is _this_ class (ProfilePrefStoreManager) that is responsible for doing the > configuration - so if the behaviour changes, this class will be part of the > change. > > Finally, there are unit tests that prevent unintended regressions of this > functionality. Okay, makes sense, I agree with you now that I understand the SegregatedPrefStore (I hadn't reviewed that change yet when I wrote this comment originally). This comment might sound more alarming than it needs to be, but that's okay. https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/segregated_pref_store.cc (right): https://codereview.chromium.org/205813002/diff/640001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/segregated_pref_store.cc:22: return; On 2014/04/01 19:46:08, erikwright wrote: > On 2014/04/01 18:55:06, gab wrote: > > Shouldn't we keep this notification around to send it after we send the > > initialization ping? > > > > i.e. > > 1) Store 1 is initialized > > 2) A value is changed in store 1 > > 3) Store 2 is initialized > > 4) We notify observers of initialization > > > > Shouldn't we then 5) Notify observers of the value changed in (2)? > > > > Is it even possible for (2) to occur before (3) or should this return also be > > NOTREACHED()? > > Yes it is possible for (2) to occur before (3). For example, the PrefHashFilter > attached to Store 1 might change a value in Store 1. I was talking about (2) being a "real" change. PrefHashFilter's changes aren't "real" (i.e. they don't generate OnValueChanged notifications). I don't think that's possible (since no external user is made aware of the store before they are both initialized) -- and thus I think this should be a NOTREACHED(). > > Please see previous discussion with Robert about this. > > There is no reason to inform clients of a change in this store when they never > heard of the original value. The value that is present when they first hear > about the store _is_ the original value, as far as they are concerned. |
