Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
881413003:
Make SDCH dictionaries persistent across browser restart. (Closed)
|
Created:
5 years, 10 months ago by Randy Smith (Not in Mondays) Modified:
3 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Elly Fong-Jones Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake SDCH dictionaries persistent across browser restart.
This CL also adds a generic persistence mechanism for //net, separate
from Chrome user preferences, that loads asynchronously on startup (i.e.
doesn't block startup).
BUG=374915
R=mmenke@chromium.org
R=ellyjones@chromium.org
R=bauerb@chromium.org
Patch Set 1 #
Total comments: 36
Patch Set 2 : Incorporated Bernhard's comments. #Patch Set 3 : Incorporated Matt's comments. #
Total comments: 28
Patch Set 4 : Incorporated Matt's comments, shifted to WriteablePrefStore, clean up observation and fleshed out e… #Patch Set 5 : Incorporated all comments. #Patch Set 6 : Preference registration and UMA. #Patch Set 7 : Incorporated Matt's suggestion and removed SdchDictionaryFetcher::Data. #
Total comments: 28
Patch Set 8 : First round of persistence tests. #
Messages
Total messages: 20 (1 generated)
I'd like to get some early design level feedback on this approach. This CL isn't done, but plenty enough is there to see what I'm trying to do. Matt: Is this something like what you had in mind in our conversation? Bernhard: I picked your name a bit at random as someone who's worked with JsonPrefStore previously. I was hoping you could give me feedback as to whether there's a better way to do what I'm trying to do with our infrastructure. My requirements are to setup a persistent, non-user visible, general store that will be accessed on the IO thread that portions of the network stack can use for storing data that needs to persist across browser restarts. Historically this type of info has been dumped in user prefs, but that's bad, both because it isn't a user pref, and because the more that's in user prefs, the more startup is slowed down. This file is read asynchronously, and hence doesn't slow down startup (much). Elly: This is currently primarily FYI, though I'll want a real review from you before landing. A couple of notes: * I tried to use just the minimum machinery from base/prefs that was useful for my purposes; I may have used too much, or I may have left something vital out. * One piece of machinery from base/prefs that would have been useful but that came with too much baggage to simply grab was registration of keys with collision, to guarantee namespace disjointness (possibly also with default setting). I'm planning to cons up something simple in this space before landing the CL; it's not needed for just one subsystem, but should be in place before a second subsystem is added.
Using a JSONPrefStore for asynchronous persisted storage is... unconventional, but works out pretty neatly! :) I don't think there are issues with the general approach. That being said, now you get all my other nits :D https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:16: // Notes on the persistent prefs store. Yes? :) https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:145: new base::DictionaryValue).Pass()); This is aligned weirdly. The four-space indent is really only with respect to the indentation on the previous line, not with respect to aligned parameters (*either* you align parameters, *or* you indent by four spaces). As an alternative, if you clang-format this, I will accept whatever the output is :) Also, you can use make_scoped_ptr, which is a bit shorter. Also also, if you create a brand new scoped_ptr, you don't need .Pass (you only need it for an existing scoped_ptr, where you want to *pass* ownership). https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:146: store->RemoveValue(kPreferenceName); This isn't necessary if you set a value right afterwards. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:225: scoped_ptr<SdchDictionaryFetcher::Data> extra_data, Where is this used? https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:408: // data comes from disk, and hence can't be trusted. What does it mean that the data can't be trusted? Are we actually concerned about an attacker who can modify local files? (Usually we aren't). Or does that mean something else? https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h#newcod... net/sdch/sdch_owner.h:117: PersistentPrefStore* persistent_pref_store_; Can't this be a scoped_refptr? https://codereview.chromium.org/881413003/diff/1/net/url_request/sdch_diction... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/881413003/diff/1/net/url_request/sdch_diction... net/url_request/sdch_dictionary_fetcher.cc:45: while (!fetch_queue_.empty()) { Maybe extract this into a method EmptyQueue()?
On 2015/01/29 17:42:24, Bernhard Bauer wrote: > Using a JSONPrefStore for asynchronous persisted storage is... unconventional, > but works out pretty neatly! :) I don't think there are issues with the general > approach. That being said, now you get all my other nits :D Awesome! That's a load off my mind. Part of me's reluctant to ask, lest it mean I catch the bullet I just dodged, but what's the conventional solution for async persisted storage? (I will incorporate your nits, it's just not my focus this instant. Thank you for all the feedback.) > > https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc > File net/sdch/sdch_owner.cc (right): > > https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... > net/sdch/sdch_owner.cc:16: // Notes on the persistent prefs store. > Yes? :) > > https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... > net/sdch/sdch_owner.cc:145: new base::DictionaryValue).Pass()); > This is aligned weirdly. The four-space indent is really only with respect to > the indentation on the previous line, not with respect to aligned parameters > (*either* you align parameters, *or* you indent by four spaces). As an > alternative, if you clang-format this, I will accept whatever the output is :) > > Also, you can use make_scoped_ptr, which is a bit shorter. > > Also also, if you create a brand new scoped_ptr, you don't need .Pass (you only > need it for an existing scoped_ptr, where you want to *pass* ownership). > > https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... > net/sdch/sdch_owner.cc:146: store->RemoveValue(kPreferenceName); > This isn't necessary if you set a value right afterwards. > > https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... > net/sdch/sdch_owner.cc:225: scoped_ptr<SdchDictionaryFetcher::Data> extra_data, > Where is this used? > > https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... > net/sdch/sdch_owner.cc:408: // data comes from disk, and hence can't be trusted. > What does it mean that the data can't be trusted? Are we actually concerned > about an attacker who can modify local files? (Usually we aren't). Or does that > mean something else? > > https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h > File net/sdch/sdch_owner.h (right): > > https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h#newcod... > net/sdch/sdch_owner.h:117: PersistentPrefStore* persistent_pref_store_; > Can't this be a scoped_refptr? > > https://codereview.chromium.org/881413003/diff/1/net/url_request/sdch_diction... > File net/url_request/sdch_dictionary_fetcher.cc (right): > > https://codereview.chromium.org/881413003/diff/1/net/url_request/sdch_diction... > net/url_request/sdch_dictionary_fetcher.cc:45: while (!fetch_queue_.empty()) { > Maybe extract this into a method EmptyQueue()?
On 2015/01/29 18:31:33, rdsmith wrote: > On 2015/01/29 17:42:24, Bernhard Bauer wrote: > > Using a JSONPrefStore for asynchronous persisted storage is... unconventional, > > but works out pretty neatly! :) I don't think there are issues with the > general > > approach. That being said, now you get all my other nits :D > > Awesome! That's a load off my mind. > > Part of me's reluctant to ask, lest it mean I catch the bullet I just dodged, > but what's the conventional solution for async persisted storage? TBH, I'm not completely sure... A SQLite database would probably do it, for example, and we use those for a lot of things (Autofill, OAuth tokens, Sync data, etc). Another alternative would be LevelDB.
> TBH, I'm not completely sure... A SQLite database would probably > do it, for example, and we use those for a lot of things > (Autofill, OAuth tokens, Sync data, etc). Another alternative would > be LevelDB. Ok, I think I'm good with a pref store, then. Part of the issue for this has been that the easy (but not right) thing has historically been to stick into the user prefs, and this makes transitioning away from that model easier. Thanks very much for the comments! https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:16: // Notes on the persistent prefs store. On 2015/01/29 17:42:23, Bernhard Bauer wrote: > Yes? :) Sorry, moved the notes that used to be here to later in the file :-}. Removed. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:145: new base::DictionaryValue).Pass()); On 2015/01/29 17:42:23, Bernhard Bauer wrote: > This is aligned weirdly. The four-space indent is really only with respect to > the indentation on the previous line, not with respect to aligned parameters > (*either* you align parameters, *or* you indent by four spaces). As an > alternative, if you clang-format this, I will accept whatever the output is :) Huh; I thought I had (net/ now requires it at a warning level for uploads). I will (and upload) before replying to your comments. > Also, you can use make_scoped_ptr, which is a bit shorter. Right, forgot about that. Thank you. > Also also, if you create a brand new scoped_ptr, you don't need .Pass (you only > need it for an existing scoped_ptr, where you want to *pass* ownership). Ah, I didn't know that! That will make some code cleaner. Thank you. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:146: store->RemoveValue(kPreferenceName); On 2015/01/29 17:42:23, Bernhard Bauer wrote: > This isn't necessary if you set a value right afterwards. Ah, cool. The comments in WriteablePrefStore weren't clear as to whether set replaced or not (to me, at least). Done. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:225: scoped_ptr<SdchDictionaryFetcher::Data> extra_data, On 2015/01/29 17:42:23, Bernhard Bauer wrote: > Where is this used? This has to do with wanting to get early comments on the direction of the CL before finishing it. The code that I intended to add in the future is simple enough, though, so I just added it; if the dictionary was persisted, we also persist the usage info. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:408: // data comes from disk, and hence can't be trusted. On 2015/01/29 17:42:23, Bernhard Bauer wrote: > What does it mean that the data can't be trusted? Are we actually concerned > about an attacker who can modify local files? (Usually we aren't). Or does that > mean something else? I'm not concerned about it from a security POV, but I am from a stability POV. I don't want disk corruption to result in Chrome crashes (or something else ugly) from which the user can't recover because the corruption is persistent. I've rephrased the comment to try to make that more clear. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h#newcod... net/sdch/sdch_owner.h:117: PersistentPrefStore* persistent_pref_store_; On 2015/01/29 17:42:23, Bernhard Bauer wrote: > Can't this be a scoped_refptr? Hmmm. My orientation (as backed up by bludgeoning by senior members of the network stack, e.g. willchan@) is to avoid relying on ref-counting as much as possible, and instead have clear ownership semantics. If I was building the infrastructure I need from the ground up I wouldn't use refcounting. given that PrefStore is RC, I'm using "scoped_refptr<>" to indicate "owned by" and a pure pointer (and comments in the constructor) to indicate "not owned by; caller has responsibility for maintaining ownership". If you feel strongly, I can convert to scoped_refptr<>, but I do have a rationale for not using it. https://codereview.chromium.org/881413003/diff/1/net/url_request/sdch_diction... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/881413003/diff/1/net/url_request/sdch_diction... net/url_request/sdch_dictionary_fetcher.cc:45: while (!fetch_queue_.empty()) { On 2015/01/29 17:42:24, Bernhard Bauer wrote: > Maybe extract this into a method EmptyQueue()? I hit the code duplication with a slightly bigger hammer for smaller code size; let me know if you object.
I haven't done a full review, but I think this looks pretty good. https://codereview.chromium.org/881413003/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/881413003/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:429: // Setup a persistent store for use by the network stack on the IO thread. Setup -> Set up (Setup is a noun, Set up a verb) https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:110: const static int kVersion = 1; statics not needed, since we're in an anonymous namespace https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:145: new base::DictionaryValue).Pass()); I don't think you need the Pass() here, when you create a new dictionary. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:250: // stale dictionaries if needed to make space. Don't use "we" in comments. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:399: // persistent store in this case. Hrm...Do we fail if the file is corrupt, or just if it can't be opened, or what? Would like to see this figured out sooner rather than later. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:461: !dict_it.IsAtEnd(); dict_it.Advance()) { Maybe use a nifty new range loop? https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:462: GURL dict_url; This doesn't seem to be initialized https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h#newcod... net/sdch/sdch_owner.h:102: std::map<std::string, DictionaryInfo> local_dictionary_info_; While you're here, should include <map> https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h#newcod... net/sdch/sdch_owner.h:116: scoped_refptr<ValueMapPrefStore> temporary_pref_store_; I think "in_memory_pref_sort_" is more accurate - temporary could be interpreted to mean it's always just until persistent_pref_store_ is ready, but that's not always the case. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h#newcod... net/sdch/sdch_owner.h:117: PersistentPrefStore* persistent_pref_store_; On 2015/01/29 17:42:23, Bernhard Bauer wrote: > Can't this be a scoped_refptr? The general bias of the network stack team is to avoid scoped_refptrs unless necessary, since they make shutdown and dependencies pretty much impossible to reason about.
Awesome! That makes me much more comfortable that I can finish this for M42. Matt, I've responded to your comments, but no need for you to do another review now--I've got a couple of rounds of stuff to do (handling errors on read-in, writing tests) before I'll be ready for detailed review. The one possible exception is that you indicated some concern around the error handling plan and said you wanted that worked out earlier rather than later, so I did a sketch of my plan below; feel free to review that comment now if you wish (matching code not yet written). https://codereview.chromium.org/881413003/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/881413003/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:429: // Setup a persistent store for use by the network stack on the IO thread. On 2015/01/30 20:18:40, mmenke wrote: > Setup -> Set up (Setup is a noun, Set up a verb) Done. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:110: const static int kVersion = 1; On 2015/01/30 20:18:41, mmenke wrote: > statics not needed, since we're in an anonymous namespace Done. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:145: new base::DictionaryValue).Pass()); On 2015/01/30 20:18:41, mmenke wrote: > I don't think you need the Pass() here, when you create a new dictionary. Done. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:250: // stale dictionaries if needed to make space. On 2015/01/30 20:18:41, mmenke wrote: > Don't use "we" in comments. Slowly breaking myself of the habit :-{; done. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:399: // persistent store in this case. On 2015/01/30 20:18:41, mmenke wrote: > Hrm...Do we fail if the file is corrupt, or just if it can't be opened, or what? > Would like to see this figured out sooner rather than later. If you're asking about the implementation of JsonPrefStore, it looks to me as if the only time in which it signals a lack of success is if the directory containing the specified file doesn't exist. There can be errors returned even if the initialization counts as successful; if the error has to do with file corruption, the file is moved off to the side and no initialization is done (but presumably the later writing to disk will work). There are some other errors that I would think would result in failure (and hence should probably not signal initialization succeeded) such as not being able to read the file (probably can't write it either, and it's not moved) and the file being locked. I think what this means is that I need to handle errors by explicitly checking the error code. I'll bucket the error code according to whether it indicates that we will be able to write the file to disk or not; if it suggests we won't, I won't switch to the persistent pref store (i.e. we we won't persist this information at the next browser restart) and otherwise I will, but I won't worry about whether the data is read in or not (since if it isn't, the remedial action is to fault in the dictionaries again through user action). Sound good? https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:461: !dict_it.IsAtEnd(); dict_it.Advance()) { On 2015/01/30 20:18:41, mmenke wrote: > Maybe use a nifty new range loop? So I suspect I'm missing something (I find the new range loops really cool, but a bit of black magic--maybe there's some specific signature on an iterator that allows them to be used?) but I don't think that'd work with this class. Specifically, I think base::DictonaryValue::Iterator is conceptually an iterator in that it allows iterating through dictionary values, but it's not a C++ iterator; at least, there's no begin() method that can initialize it. Just for grins and giggles I tried the first syntax that came to mind for doing this ( for (base::DictionaryValue::Iterator dict_it: *dictionary_set) { ) and got a "no viable 'begin' functionality available", which fit what I thought from reading the class decl. So I don't think I can use a range loop. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:462: GURL dict_url; On 2015/01/30 20:18:41, mmenke wrote: > This doesn't seem to be initialized Whoops, true that. Fixed. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h#newcod... net/sdch/sdch_owner.h:102: std::map<std::string, DictionaryInfo> local_dictionary_info_; On 2015/01/30 20:18:41, mmenke wrote: > While you're here, should include <map> Done. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h#newcod... net/sdch/sdch_owner.h:116: scoped_refptr<ValueMapPrefStore> temporary_pref_store_; On 2015/01/30 20:18:41, mmenke wrote: > I think "in_memory_pref_sort_" is more accurate - temporary could be interpreted > to mean it's always just until persistent_pref_store_ is ready, but that's not > always the case. Done. https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.h#newcod... net/sdch/sdch_owner.h:117: PersistentPrefStore* persistent_pref_store_; On 2015/01/30 20:18:41, mmenke wrote: > On 2015/01/29 17:42:23, Bernhard Bauer wrote: > > Can't this be a scoped_refptr? > > The general bias of the network stack team is to avoid scoped_refptrs unless > necessary, since they make shutdown and dependencies pretty much impossible to > reason about. Acknowledged.
https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/1/net/sdch/sdch_owner.cc#newco... net/sdch/sdch_owner.cc:461: !dict_it.IsAtEnd(); dict_it.Advance()) { On 2015/01/31 02:53:29, rdsmith wrote: > On 2015/01/30 20:18:41, mmenke wrote: > > Maybe use a nifty new range loop? > > So I suspect I'm missing something (I find the new range loops really cool, but > a bit of black magic--maybe there's some specific signature on an iterator that > allows them to be used?) but I don't think that'd work with this class. > Specifically, I think base::DictonaryValue::Iterator is conceptually an iterator > in that it allows iterating through dictionary values, but it's not a C++ > iterator; at least, there's no begin() method that can initialize it. Just for > grins and giggles I tried the first syntax that came to mind for doing this ( > > for (base::DictionaryValue::Iterator dict_it: *dictionary_set) { > > ) > > and got a "no viable 'begin' functionality available", which fit what I thought > from reading the class decl. So I don't think I can use a range loop. Yeah, a C++11-style for-loop only works with classes that have begin() and end() methods which return suitable iterators (or that have std::begin() and std::end() overloaded, I think); see http://en.cppreference.com/w/cpp/language/range-for. https://codereview.chromium.org/881413003/diff/1/net/url_request/sdch_diction... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/881413003/diff/1/net/url_request/sdch_diction... net/url_request/sdch_dictionary_fetcher.cc:45: while (!fetch_queue_.empty()) { On 2015/01/30 20:11:10, rdsmith wrote: > On 2015/01/29 17:42:24, Bernhard Bauer wrote: > > Maybe extract this into a method EmptyQueue()? > > I hit the code duplication with a slightly bigger hammer for smaller code size; > let me know if you object. Acknowledged. https://codereview.chromium.org/881413003/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/881413003/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl_io_data.cc:887: base::Bind(&ProfileImplIOData::FlushPrefsToDisk, factory_.GetWeakPtr())); JsonPrefStore has an internal timer that will commit pending writes (also every ten seconds). The explicit method is only for cases where you want to commit *now* (like right before shutdown). In any case, using a weak pointer here is probably not necessary, as the timer is a member, so it will cancel itself when this object is destroyed. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:112: bool success = store->GetMutableValue(kPreferenceName, &result); Mutating the value returned by GetMutableValue will *not* trigger the mechanism that notifies PrefStore observers and automatically commits changes, as we have no way of knowing when the mutation happens. If you do that, you need to trigger the notifications manually, by calling ReportValueChanged(). What we usually do is to wrap accesses to the mutable value in a stack-allocated object that will do that on destruction (e.g. ScopedUserPrefUpdate). https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:319: FetchData* sdch_fetch_data = static_cast<FetchData*>(extra_data.get()); An alternative to passing this as SdchDictionaryFetcher::Data and then having to cast it back to FetchData would be to bind the data in a callback instead. That means that instead of initializing the |fetcher_| with the SdchOwner::OnDictionaryFetched() callback and passing the data directly to ScheduleReload(), you would pass the callback already bound to the data.
https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:70: const int kInitialHoursSinceLastUsed = 23; May be beyond the scoped of this CL, but this seems like a hack. I'd suggest just checking "last_used" and replace this with "kHoursFreshIfNeverUsed", rather than storing bogus last used values. Alternatives, could have an expires or eviction time instead of last used. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:201: DCHECK(!persistent_pref_store_); include base/logging.h https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:319: FetchData* sdch_fetch_data = static_cast<FetchData*>(extra_data.get()); On 2015/02/02 14:39:46, Bernhard Bauer wrote: > An alternative to passing this as SdchDictionaryFetcher::Data and then having to > cast it back to FetchData would be to bind the data in a callback instead. That > means that instead of initializing the |fetcher_| with the > SdchOwner::OnDictionaryFetched() callback and passing the data directly to > ScheduleReload(), you would pass the callback already bound to the data. +1 to binding it...Actually, could make the prototype: SdchOwner::OnDictionaryFetched( base::Time last_used, int use_count, const GURL& dictionary_url, const std::string& dictionary_text, const net::BoundNetLog& net_log); And always bind the first two values. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:352: it->second.use_count++; optional: I'm not a big fan of storing things twice. May want to just get rid of local_dictionary_info_ in favor of always using the pref dictionary. Admittedly, it's heavier weight, and doesn't save much memory - but my concern is the more copies of data we have, the more chance there is for it to end up inconsistent, due to a bug. Of course, with the pref, there's always the question "What if the dictionary was removed from the pref store". Anyhow, up to you. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:360: it->second.last_used.ToDoubleT()); Shouldn't you be updating the dictionary fields you added at the bottom of the above function? https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:463: // TODO(rdsmith): Handle version upgrades. Note: I'm not sure we care enough to do this. Just losing the old index is not the end of the world, and the added complexity of additions tests, and remembering to remove the upgrade code at some point, doesn't seem worth the effort to me. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:475: return false; Should we return early here? Currently, we schedule reloads of all dictionaries before the first malformed entry in the dictionary, and then ignore the rest, even if they're also well-formed. I think we should be consistent, unless there's some reason to believe that all entries after the first broken one will also be broken in ways we may not be able to notice. Either load all well-formed entries, regardless of the presence of a malformed one, or load non in the presence of a malformed entry. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:482: double last_used = 0.0; No need to initialize this or use_count. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.h#ne... net/sdch/sdch_owner.h:81: // Indexed by server hash. Should define server hash. I'm assuming it's the server's hash for the dictionary, rather than a hash of the server name. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.h#ne... net/sdch/sdch_owner.h:98: bool LoadPersistedDictionaries(const base::DictionaryValue& persisted_info); These needs a lot more detail, and maybe a rename, because of how it works - scheduling fetches, rather than loading them into a pref store directly isn't terribly intuitive.
New version uploaded. This is everything besides new tests; PTAL. A couple of notes: * Either at this point or after the next PS (depending on how much progress I make on tests this afternoon) Elly (cc'd) is going to take over as I'm going on vacation. I don't believe one can change owners of a CL, so she'll be uploading a new CL with pointers; watch for that change. * Matt: I didn't see your response to my proposal as to how to handle errors. That proposal is now implemented--let me know whether or not it satisfies your concerns. * There are two changes in this PS (other than comment responses): UMA and PreferenceRegistration. * UMA: My primary goal was to have UMA that let us know if this was actually working in the field. I was going to include "dictionary loaded by persistence mechanism used" but I decided that just adding a new dictionary fate (added because of persistence) was good enough. I've also added a histogram to cover the distribution of inter-usage intervals for dictionaries. I have two goals for this histogram: a) Get a sense of how long dictionaries are taking up space without being used, b) get an accurate count of how many times total dictionaries are used (to compare against error frequencies). * Preference registration. This isn't directly needed for this CL (it's to avoid name collisions between different users of the net persistence store, and with this CL there's only one user), but I was reluctant to commit the general mechanism without a template to avoid namespace collisions. I thought about using PrefsRegistry (& sub-classes), but they provide setting of default values, and I couldn't figure out how to make that work with JsonPrefStore (which just overwrites the preferences in the persistent pref store with what's read in from the file, and doesn't make any distinction between observers to allow some special observer to get notified before the others on initialization complete). I could have modified JsonPrefStore, but I decided that was beyond the scope of this CL. So: Small hammer. Let me know what you think. https://codereview.chromium.org/881413003/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/881413003/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl_io_data.cc:887: base::Bind(&ProfileImplIOData::FlushPrefsToDisk, factory_.GetWeakPtr())); On 2015/02/02 14:39:46, Bernhard Bauer wrote: > JsonPrefStore has an internal timer that will commit pending writes (also every > ten seconds). The explicit method is only for cases where you want to commit > *now* (like right before shutdown). > > In any case, using a weak pointer here is probably not necessary, as the timer > is a member, so it will cancel itself when this object is destroyed. That makes my life simpler! Much excess stuff removed. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:70: const int kInitialHoursSinceLastUsed = 23; On 2015/02/02 16:35:28, mmenke wrote: > May be beyond the scoped of this CL, but this seems like a hack. No argument, but ... > I'd suggest just checking "last_used" and replace this with > "kHoursFreshIfNeverUsed", rather than storing bogus last used values. > Alternatives, could have an expires or eviction time instead of last used. ... I'm not understanding the alternative. Do you mean leaving last_used null if it hasn't been used, and handling that explicitly? If we do that, we lose the information about when the dictionary was added, which seems important (at least for the shape of algorithm I currently have). Either hours fresh if never used is greater than the stale threshold (in which case we can thrash if there are two dictionaries that both want to be loaded) or it's less than the stale threshold (in which case if a dictionary is never used, it'll never be evicted). Doing an expiration time could work. I'm not thrilled by it--I'd rather keep data in storage and policy in the code--but I don't have a strong argument against it. Let me understand your first alternative, and if that doesn't work, I"ll do expiration time. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:112: bool success = store->GetMutableValue(kPreferenceName, &result); On 2015/02/02 14:39:46, Bernhard Bauer wrote: > Mutating the value returned by GetMutableValue will *not* trigger the mechanism > that notifies PrefStore observers and automatically commits changes, as we have > no way of knowing when the mutation happens. If you do that, you need to trigger > the notifications manually, by calling ReportValueChanged(). What we usually do > is to wrap accesses to the mutable value in a stack-allocated object that will > do that on destruction (e.g. ScopedUserPrefUpdate). Ah! Thank you for the heads up. I can't use ScopedUserPrefUpdate (it uses PrefService and requires the UI thread), and I decided that this wasn't worth my writing a wrapper class for base::DictionaryValue* that would combine the functionality of DictionaryValue and ScopedUserPrefUpdate, so I just wrote a simple ScopedPrefNotifier class and paired it appropriately with calls to getting mutable values from the preference store. Let me know what you think. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:201: DCHECK(!persistent_pref_store_); On 2015/02/02 16:35:28, mmenke wrote: > include base/logging.h Done. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:319: FetchData* sdch_fetch_data = static_cast<FetchData*>(extra_data.get()); On 2015/02/02 14:39:46, Bernhard Bauer wrote: > An alternative to passing this as SdchDictionaryFetcher::Data and then having to > cast it back to FetchData would be to bind the data in a callback instead. That > means that instead of initializing the |fetcher_| with the > SdchOwner::OnDictionaryFetched() callback and passing the data directly to > ScheduleReload(), you would pass the callback already bound to the data. Good point; that's an excellent idea that cleans up some extra code. Thank you. Done. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:319: FetchData* sdch_fetch_data = static_cast<FetchData*>(extra_data.get()); On 2015/02/02 16:35:28, mmenke wrote: > On 2015/02/02 14:39:46, Bernhard Bauer wrote: > > An alternative to passing this as SdchDictionaryFetcher::Data and then having > to > > cast it back to FetchData would be to bind the data in a callback instead. > That > > means that instead of initializing the |fetcher_| with the > > SdchOwner::OnDictionaryFetched() callback and passing the data directly to > > ScheduleReload(), you would pass the callback already bound to the data. > > +1 to binding it...Actually, could make the prototype: > > SdchOwner::OnDictionaryFetched( > base::Time last_used, > int use_count, > const GURL& dictionary_url, > const std::string& dictionary_text, > const net::BoundNetLog& net_log); > > And always bind the first two values. Yep. This makes several things much simpler. Thank you. Done. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:352: it->second.use_count++; On 2015/02/02 16:35:28, mmenke wrote: > optional: I'm not a big fan of storing things twice. May want to just get rid > of local_dictionary_info_ in favor of always using the pref dictionary. > Admittedly, it's heavier weight, and doesn't save much memory - but my concern > is the more copies of data we have, the more chance there is for it to end up > inconsistent, due to a bug. > > Of course, with the pref, there's always the question "What if the dictionary > was removed from the pref store". Anyhow, up to you. Yeah, I had the same thought--just hadn't gotten to it yet. Done. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:360: it->second.last_used.ToDoubleT()); On 2015/02/02 16:35:28, mmenke wrote: > Shouldn't you be updating the dictionary fields you added at the bottom of the > above function? I presume you mean "shouldn't you be using the constants for those fields that you've carefully setup rather than magic strings"? Yep, I should. Done. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:463: // TODO(rdsmith): Handle version upgrades. On 2015/02/02 16:35:28, mmenke wrote: > Note: I'm not sure we care enough to do this. Just losing the old index is not > the end of the world, and the added complexity of additions tests, and > remembering to remove the upgrade code at some point, doesn't seem worth the > effort to me. Good point. I wasn't planning to do any work (besides writing the TODO :-}) in this CL, but you're right, I'll put a stake in the ground saying we throw away data on version upgrades. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:475: return false; On 2015/02/02 16:35:28, mmenke wrote: > Should we return early here? Currently, we schedule reloads of all dictionaries > before the first malformed entry in the dictionary, and then ignore the rest, > even if they're also well-formed. > > I think we should be consistent, unless there's some reason to believe that all > entries after the first broken one will also be broken in ways we may not be > able to notice. Either load all well-formed entries, regardless of the presence > of a malformed one, or load non in the presence of a malformed entry. Good point. Done. https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:482: double last_used = 0.0; On 2015/02/02 16:35:28, mmenke wrote: > No need to initialize this or use_count. Hmmm. That seems like it's relying on the interface contract of DictionaryValue (or on ASAN & etc. to find the uninitialized memory read), which is information that's pretty distant from this location in the code, so it makes me a little uncomfortable. I'll change it, but it makes me a bit uncomfortable (though the code breaks if that interface contract isn't followed, and if we don't initialize, ASAN & etc. might catch that breakage, so you could make an argument in favor of not initializing. Still, it feels weird :-}.) https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.h#ne... net/sdch/sdch_owner.h:81: // Indexed by server hash. On 2015/02/02 16:35:28, mmenke wrote: > Should define server hash. I'm assuming it's the server's hash for the > dictionary, rather than a hash of the server name. Done (though it will be moot). https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.h#ne... net/sdch/sdch_owner.h:98: bool LoadPersistedDictionaries(const base::DictionaryValue& persisted_info); On 2015/02/02 16:35:28, mmenke wrote: > These needs a lot more detail, and maybe a rename, because of how it works - > scheduling fetches, rather than loading them into a pref store directly isn't > terribly intuitive. I've rewritten the comment and renamed the function; let me know what you think.
https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:70: const int kInitialHoursSinceLastUsed = 23; On 2015/02/04 19:29:03, rdsmith wrote: > On 2015/02/02 16:35:28, mmenke wrote: > > May be beyond the scoped of this CL, but this seems like a hack. > > No argument, but ... > > > I'd suggest just checking "last_used" and replace this with > > "kHoursFreshIfNeverUsed", rather than storing bogus last used values. > > Alternatives, could have an expires or eviction time instead of last used. > > ... I'm not understanding the alternative. Do you mean leaving last_used null > if it hasn't been used, and handling that explicitly? If we do that, we lose > the information about when the dictionary was added, which seems important (at > least for the shape of algorithm I currently have). Either hours fresh if never > used is greater than the stale threshold (in which case we can thrash if there > are two dictionaries that both want to be loaded) or it's less than the stale > threshold (in which case if a dictionary is never used, it'll never be evicted). We're already recording the use_count, so I was thinking we store time loaded as last_used, and have something like: base::TimeDelta time_since_last_used = Now() - last_used; if (time_since_last_used < kFreshnessLifetime || (use_count == 0 && time_since_last_used < kNeverUsedFreshnessLiftime)) { // Evict here. } Anyhow, if you prefer what you have over my suggestions, feel free to keep it as-is, and maybe add a comment about it, or maybe rename time since last used to something more accurate (If perhaps more painfully verbose. time_to_use_for_freshness_calculations?)
On 2015/02/04 19:53:46, mmenke wrote: > https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc > File net/sdch/sdch_owner.cc (right): > > https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... > net/sdch/sdch_owner.cc:70: const int kInitialHoursSinceLastUsed = 23; > On 2015/02/04 19:29:03, rdsmith wrote: > > On 2015/02/02 16:35:28, mmenke wrote: > > > May be beyond the scoped of this CL, but this seems like a hack. > > > > No argument, but ... > > > > > I'd suggest just checking "last_used" and replace this with > > > "kHoursFreshIfNeverUsed", rather than storing bogus last used values. > > > Alternatives, could have an expires or eviction time instead of last used. > > > > ... I'm not understanding the alternative. Do you mean leaving last_used null > > if it hasn't been used, and handling that explicitly? If we do that, we lose > > the information about when the dictionary was added, which seems important (at > > least for the shape of algorithm I currently have). Either hours fresh if > never > > used is greater than the stale threshold (in which case we can thrash if there > > are two dictionaries that both want to be loaded) or it's less than the stale > > threshold (in which case if a dictionary is never used, it'll never be > evicted). > > We're already recording the use_count, so I was thinking we store time loaded as > last_used, and have something like: > > base::TimeDelta time_since_last_used = Now() - last_used; > > if (time_since_last_used < kFreshnessLifetime || (use_count == 0 && > time_since_last_used < kNeverUsedFreshnessLiftime)) { > // Evict here. > } > > Anyhow, if you prefer what you have over my suggestions, feel free to keep it > as-is, and maybe add a comment about it, or maybe rename time since last used to > something more accurate (If perhaps more painfully verbose. > time_to_use_for_freshness_calculations?) Ah, got it! No, that makes a lot of sense. Done in the last patch set (also fixed a glitch Elly noticed).
https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:434: DCHECK(preference_names.insert(net::SdchOwner::PreferenceName()).second); DCHECKs with side effects just seem like a bad idea to me. And I'm extremely skeptical about expecting people will remember to update this if they add a new preference to this store. Also, there's already PrefRegistry...If we want to do this, should we be using that instead? The way they work is the object that cares about the preference does the registering, I believe. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:55: enum PersistenceDrop { I don't know what a PersistenceDrop is. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:72: // Dictionaries that haven't ever been used only stay fresh for one hour. nit: haven't ever -> have never? https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:109: const char kPreferenceName[] = "SDCH"; optional: Rather than "name" suggest using "key" for these. Think it's a bit more precise. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:168: GURL Url() const; optional: Hrm...Url() looks weird on its own to me. I'd prefix these with Get() as a result, but up to you - I can't really say why it looks weird, but the others look less so. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:416: DCHECK(avail_bytes >= dictionary_text.size()); DCHECK_GE? https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:46: static std::string PreferenceName(); Should we just make this a static const char[]? Or get rid of it, per my PrefRegistry suggestion.
Quick response to one comment. https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:434: DCHECK(preference_names.insert(net::SdchOwner::PreferenceName()).second); On 2015/02/04 21:14:39, mmenke wrote: > DCHECKs with side effects just seem like a bad idea to me. Well, given that if the DCHECK is compiled out, there won't be any point to doing this, a DCHECK with a side effect seemed like it pretty precisely encapsulated my intent. I could make it explicit with an #ifndef NDEBUG if you want, but I'm not sure I see the point. > And I'm extremely > skeptical about expecting people will remember to update this if they add a new > preference to this store. Why is this any worse than pref registry? The registration function still needs to be called from some central place (e.g. IOThread::IOThread, search for SetDefaultPrefValue). > Also, there's already PrefRegistry...If we want to do this, should we be using > that instead? The way they work is the object that cares about the preference > does the registering, I believe. So I considered PrefRegistry but rejected it for reasons I sketched out in the main comment on my last post; the quick summary is that PrefRegistry also offers default value setting, which I don't see a way to integrate cleanly with JsonPrefStore without changes to JsonPrefStore, which seemed like too much mission creeep.
https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:434: DCHECK(preference_names.insert(net::SdchOwner::PreferenceName()).second); On 2015/02/04 21:20:24, rdsmith wrote: > On 2015/02/04 21:14:39, mmenke wrote: > > DCHECKs with side effects just seem like a bad idea to me. > > Well, given that if the DCHECK is compiled out, there won't be any point to > doing this, a DCHECK with a side effect seemed like it pretty precisely > encapsulated my intent. I could make it explicit with an #ifndef NDEBUG if you > want, but I'm not sure I see the point. > > > And I'm extremely > > skeptical about expecting people will remember to update this if they add a > new > > preference to this store. > > Why is this any worse than pref registry? The registration function still needs > to be called from some central place (e.g. IOThread::IOThread, search for > SetDefaultPrefValue). Only one reason. Suppose people see what you're doing, and want to copy it... I think they're more likely than not to completely miss this line. On the other hand, if you pass in the registry to your constructor (along with the pref store) than they're more likely to notice it. I see both as a very flake-prone (non-robust? way of trying to prevent regressions, so my opinion is that either: 1) Breakages here aren't a big concern. Or 2) They are a real concern, and this method is too easy to miss, so is not robust enough to protect against them. If this is a big concern, we could make a wrapper around the PrefStore, I suppose, where when you register for a message, you get a unique object to access it, but that seems like overkill. > > Also, there's already PrefRegistry...If we want to do this, should we be using > > that instead? The way they work is the object that cares about the preference > > does the registering, I believe. > > So I considered PrefRegistry but rejected it for reasons I sketched out in the > main comment on my last post; the quick summary is that PrefRegistry also offers > default value setting, which I don't see a way to integrate cleanly with > JsonPrefStore without changes to JsonPrefStore, which seemed like too much > mission creeep.
https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/40001/net/sdch/sdch_owner.cc#n... net/sdch/sdch_owner.cc:112: bool success = store->GetMutableValue(kPreferenceName, &result); On 2015/02/04 19:29:03, rdsmith wrote: > On 2015/02/02 14:39:46, Bernhard Bauer wrote: > > Mutating the value returned by GetMutableValue will *not* trigger the > mechanism > > that notifies PrefStore observers and automatically commits changes, as we > have > > no way of knowing when the mutation happens. If you do that, you need to > trigger > > the notifications manually, by calling ReportValueChanged(). What we usually > do > > is to wrap accesses to the mutable value in a stack-allocated object that will > > do that on destruction (e.g. ScopedUserPrefUpdate). > > Ah! Thank you for the heads up. I can't use ScopedUserPrefUpdate (it uses > PrefService and requires the UI thread), and I decided that this wasn't worth my > writing a wrapper class for base::DictionaryValue* that would combine the > functionality of DictionaryValue and ScopedUserPrefUpdate, so I just wrote a > simple ScopedPrefNotifier class and paired it appropriately with calls to > getting mutable values from the preference store. Let me know what you think. Yes, that's what I meant actually (I just mentioned ScopedUserPrefUpdate as an example of the pattern). I should have been clearer. https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:434: DCHECK(preference_names.insert(net::SdchOwner::PreferenceName()).second); On 2015/02/04 21:20:24, rdsmith wrote: > On 2015/02/04 21:14:39, mmenke wrote: > > DCHECKs with side effects just seem like a bad idea to me. > > Well, given that if the DCHECK is compiled out, there won't be any point to > doing this, a DCHECK with a side effect seemed like it pretty precisely > encapsulated my intent. I could make it explicit with an #ifndef NDEBUG if you > want, but I'm not sure I see the point. > > > And I'm extremely > > skeptical about expecting people will remember to update this if they add a > new > > preference to this store. > > Why is this any worse than pref registry? The registration function still needs > to be called from some central place (e.g. IOThread::IOThread, search for > SetDefaultPrefValue). The difference is that a PrefService requires a preference to be registered beforehand, so if someone forgets it, they will get a DCHECK when trying to access the preference. PrefStore does not have that requirement. > > Also, there's already PrefRegistry...If we want to do this, should we be using > > that instead? The way they work is the object that cares about the preference > > does the registering, I believe. > > So I considered PrefRegistry but rejected it for reasons I sketched out in the > main comment on my last post; the quick summary is that PrefRegistry also offers > default value setting, which I don't see a way to integrate cleanly with > JsonPrefStore without changes to JsonPrefStore, which seemed like too much > mission creeep. > Yeah, PrefRegistry is desigend for use with a full PrefService and not just a PrefStore. Really, if we want to use PrefRegistry, we could just directly use a PrefService and load it asynchronously. We would still have to deal with storing in-memory data before it is loaded, but we could probably do that with _another_ PrefService that has an in-memory user pref store... The alternative would be to just roll our own registration mechanism, and ensure accesses to the contents of the PrefStore always go through that (which we could then possibly even combine with the ScopedPrefNotifier...). But TBH, I'm not sure collisions are even going to be that likely, assuming that there are only going to be handful of clients and people use sensible preference keys. https://codereview.chromium.org/881413003/diff/120001/chrome/common/chrome_co... File chrome/common/chrome_constants.cc (right): https://codereview.chromium.org/881413003/diff/120001/chrome/common/chrome_co... chrome/common/chrome_constants.cc:159: FPL("NetworkPersistentState"); Nit: We write out filenames with spaces in between, https://crbug.com/35210 notwithstanding ;-) https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:37: DICTIONARY_FATE_ADD_RESPONSE_TRIGGERRED = 5, Nit: "triggered" with a single "r". Also in some comments below. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:230: // when the object goes out of scope. The Nit: Dangling "The" https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:416: DCHECK(avail_bytes >= dictionary_text.size()); DCHECK_GE for nicer error messages on failure. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:596: // This object can stop waiting on (== observing) the external preference Super-nit: Double equal signs are only used in code, prose uses single. https://codereview.chromium.org/881413003/diff/120001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/881413003/diff/120001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:273: std::vector<DictionaryAdditions> dictionary_additions; This should end with an underscore.
On 2015/02/04 21:34:32, mmenke wrote: > https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... > chrome/browser/profiles/profile_impl_io_data.cc:434: > DCHECK(preference_names.insert(net::SdchOwner::PreferenceName()).second); > On 2015/02/04 21:20:24, rdsmith wrote: > > On 2015/02/04 21:14:39, mmenke wrote: > > > DCHECKs with side effects just seem like a bad idea to me. > > > > Well, given that if the DCHECK is compiled out, there won't be any point to > > doing this, a DCHECK with a side effect seemed like it pretty precisely > > encapsulated my intent. I could make it explicit with an #ifndef NDEBUG if > you > > want, but I'm not sure I see the point. > > > > > And I'm extremely > > > skeptical about expecting people will remember to update this if they add a > > new > > > preference to this store. > > > > Why is this any worse than pref registry? The registration function still > needs > > to be called from some central place (e.g. IOThread::IOThread, search for > > SetDefaultPrefValue). > > Only one reason. Suppose people see what you're doing, and want to copy it... > I think they're more likely than not to completely miss this line. On the other > hand, if you pass in the registry to your constructor (along with the pref > store) than they're more likely to notice it. > > I see both as a very flake-prone (non-robust? way of trying to prevent > regressions, so my opinion is that either: > 1) Breakages here aren't a big concern. > Or > 2) They are a real concern, and this method is too easy to miss, so is not > robust enough to protect against them. > > If this is a big concern, we could make a wrapper around the PrefStore, I > suppose, where when you register for a message, you get a unique object to > access it, but that seems like overkill. > > > > Also, there's already PrefRegistry...If we want to do this, should we be > using > > > that instead? The way they work is the object that cares about the > preference > > > does the registering, I believe. > > > > So I considered PrefRegistry but rejected it for reasons I sketched out in the > > main comment on my last post; the quick summary is that PrefRegistry also > offers > > default value setting, which I don't see a way to integrate cleanly with > > JsonPrefStore without changes to JsonPrefStore, which seemed like too much > > mission creeep. Matt and I talked offline, and our conclusion was that this registration functionality isn't worth doing, at least at the moment. There will likely be few customers for this inside of the net stack, and they're all likely to be doing creation of the persistence values on the fly (which means that the obvious failure mode for registration, which is doing a "Get" and getting a null value, won't happen). So we'll skip the registration functionality.
On 2015/02/04 21:48:06, rdsmith wrote: > On 2015/02/04 21:34:32, mmenke wrote: > > > https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... > > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > > > > https://codereview.chromium.org/881413003/diff/120001/chrome/browser/profiles... > > chrome/browser/profiles/profile_impl_io_data.cc:434: > > DCHECK(preference_names.insert(net::SdchOwner::PreferenceName()).second); > > On 2015/02/04 21:20:24, rdsmith wrote: > > > On 2015/02/04 21:14:39, mmenke wrote: > > > > DCHECKs with side effects just seem like a bad idea to me. > > > > > > Well, given that if the DCHECK is compiled out, there won't be any point to > > > doing this, a DCHECK with a side effect seemed like it pretty precisely > > > encapsulated my intent. I could make it explicit with an #ifndef NDEBUG if > > you > > > want, but I'm not sure I see the point. > > > > > > > And I'm extremely > > > > skeptical about expecting people will remember to update this if they add > a > > > new > > > > preference to this store. > > > > > > Why is this any worse than pref registry? The registration function still > > needs > > > to be called from some central place (e.g. IOThread::IOThread, search for > > > SetDefaultPrefValue). > > > > Only one reason. Suppose people see what you're doing, and want to copy it... > > > I think they're more likely than not to completely miss this line. On the > other > > hand, if you pass in the registry to your constructor (along with the pref > > store) than they're more likely to notice it. > > > > I see both as a very flake-prone (non-robust? way of trying to prevent > > regressions, so my opinion is that either: > > 1) Breakages here aren't a big concern. > > Or > > 2) They are a real concern, and this method is too easy to miss, so is not > > robust enough to protect against them. > > > > If this is a big concern, we could make a wrapper around the PrefStore, I > > suppose, where when you register for a message, you get a unique object to > > access it, but that seems like overkill. > > > > > > Also, there's already PrefRegistry...If we want to do this, should we be > > using > > > > that instead? The way they work is the object that cares about the > > preference > > > > does the registering, I believe. > > > > > > So I considered PrefRegistry but rejected it for reasons I sketched out in > the > > > main comment on my last post; the quick summary is that PrefRegistry also > > offers > > > default value setting, which I don't see a way to integrate cleanly with > > > JsonPrefStore without changes to JsonPrefStore, which seemed like too much > > > mission creeep. > > Matt and I talked offline, and our conclusion was that this registration > functionality isn't worth doing, at least at the moment. There will likely be > few customers for this inside of the net stack, and they're all likely to be > doing creation of the persistence values on the fly (which means that the > obvious failure mode for registration, which is doing a "Get" and getting a null > value, won't happen). So we'll skip the registration functionality. OK.
bauerb, mmenke: I've uploaded a new version with these changes at https://codereview.chromium.org/901303002, please review there :) https://codereview.chromium.org/881413003/diff/120001/chrome/common/chrome_co... File chrome/common/chrome_constants.cc (right): https://codereview.chromium.org/881413003/diff/120001/chrome/common/chrome_co... chrome/common/chrome_constants.cc:159: FPL("NetworkPersistentState"); On 2015/02/04 21:48:00, Bernhard Bauer wrote: > Nit: We write out filenames with spaces in between, https://crbug.com/35210 > notwithstanding ;-) Done. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc File net/sdch/sdch_owner.cc (right): https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:37: DICTIONARY_FATE_ADD_RESPONSE_TRIGGERRED = 5, On 2015/02/04 21:48:00, Bernhard Bauer wrote: > Nit: "triggered" with a single "r". Also in some comments below. Done. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:55: enum PersistenceDrop { On 2015/02/04 21:14:39, mmenke wrote: > I don't know what a PersistenceDrop is. Done. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:72: // Dictionaries that haven't ever been used only stay fresh for one hour. On 2015/02/04 21:14:39, mmenke wrote: > nit: haven't ever -> have never? Done. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:109: const char kPreferenceName[] = "SDCH"; On 2015/02/04 21:14:39, mmenke wrote: > optional: Rather than "name" suggest using "key" for these. Think it's a bit > more precise. Done. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:168: GURL Url() const; On 2015/02/04 21:14:39, mmenke wrote: > optional: Hrm...Url() looks weird on its own to me. I'd prefix these with > Get() as a result, but up to you - I can't really say why it looks weird, but > the others look less so. I changed this to load all these fields when the iterator advances, then make these into primitive accessors. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:230: // when the object goes out of scope. The On 2015/02/04 21:48:00, Bernhard Bauer wrote: > Nit: Dangling "The" Done. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:416: DCHECK(avail_bytes >= dictionary_text.size()); On 2015/02/04 21:14:39, mmenke wrote: > DCHECK_GE? Done. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:416: DCHECK(avail_bytes >= dictionary_text.size()); On 2015/02/04 21:48:00, Bernhard Bauer wrote: > DCHECK_GE for nicer error messages on failure. Done. https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.cc#... net/sdch/sdch_owner.cc:596: // This object can stop waiting on (== observing) the external preference On 2015/02/04 21:48:00, Bernhard Bauer wrote: > Super-nit: Double equal signs are only used in code, prose uses single. Changed to "i.e." https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.h File net/sdch/sdch_owner.h (right): https://codereview.chromium.org/881413003/diff/120001/net/sdch/sdch_owner.h#n... net/sdch/sdch_owner.h:46: static std::string PreferenceName(); On 2015/02/04 21:14:39, mmenke wrote: > Should we just make this a static const char[]? Or get rid of it, per my > PrefRegistry suggestion. Done. https://codereview.chromium.org/881413003/diff/120001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher_unittest.cc (right): https://codereview.chromium.org/881413003/diff/120001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher_unittest.cc:273: std::vector<DictionaryAdditions> dictionary_additions; On 2015/02/04 21:48:01, Bernhard Bauer wrote: > This should end with an underscore. Done.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
