Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1173343009:
LevelDB storage for data reduction proxy to store data usage stats. (Closed)
|
Created:
5 years, 6 months ago by Not at Google. Contact bengr Modified:
5 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLevelDB storage for data reduction proxy to store data usage stats.
* DataStore is an interface for a key-value store, with
DataStoreImpl providing a LevelDB based implementation.
* Data usage for an interval of time is stored in DataUsageBucket.
* DataUsageStore uses DataStore to store data
usage in a circular array. Array is stored in key-value store by
converting array indexes to a unique key.
* Ownership: DBDataOwner owns all objects that live on the DB sequenced task
runner.
DBDataOwner is created on UI task runner, but thereafter used and destroyed
on DB task runner. DBDataOwner is owned by DataReductionProxyService which
lives on UI task runner.
* Threading: All DB operations occur on DB task runner. DataReductionProxyService
will posts tasks to DBDataOwner on the DB task runner.
BUG=482442
Committed: https://crrev.com/9a02cfe87e75d1c4996b87fb3ec9d4e1fc9b8d6b
Cr-Commit-Position: refs/heads/master@{#337968}
Patch Set 1 #Patch Set 2 : Readd DEPS to fix history #Patch Set 3 : Comments #
Total comments: 1
Patch Set 4 : Fix tests #
Total comments: 54
Patch Set 5 : Made code more robust against LevelDB failures. #Patch Set 6 : Minor formatting #Patch Set 7 : Use ref counting for DataUsageStorageHelper. #Patch Set 8 : Remove unused weak factory #Patch Set 9 : Misc changes #Patch Set 10 : Change ERROR to MISC_ERROR to fix compile. #Patch Set 11 : Remove DCHECKs. #
Total comments: 40
Patch Set 12 : Addressed cmumford's comments. #Patch Set 13 : Minor formatting #Patch Set 14 : Fix DataUsageBucketWrapper ref counting #Patch Set 15 : Use FILE_PATH_LITERAL #Patch Set 16 : Add level DB dep to BUILD.gn #Patch Set 17 : Add leveldb dep to correct BUILD.gn #Patch Set 18 : Use raw pointers for arguments instead of ref counting #
Total comments: 74
Patch Set 19 : Use scoped ptr for data usage store. #Patch Set 20 : Remove friend from impl #
Total comments: 30
Patch Set 21 : Temp patch before splitting into multiple cls. #Patch Set 22 : Smaller subset of original cl. #Patch Set 23 : Add missing include #
Total comments: 16
Patch Set 24 : Use SequenceChecker. Drop content/public/ dependency. #
Total comments: 16
Patch Set 25 : Remove unused includes #Patch Set 26 : Rename classes to remove DataReductionProxy prefix. #
Total comments: 18
Patch Set 27 : Fix Build.gn #Patch Set 28 : Rename DBDataStore to DataStore #Patch Set 29 : Minor formatting #Patch Set 30 : Minor format #Patch Set 31 : db_data_store.proto -> data_store.proto #Patch Set 32 : Make data_usage_store_ private in unittest #Patch Set 33 : Fix unittests #
Total comments: 32
Patch Set 34 : Addressed comments #Patch Set 35 : Remove wrong const #Messages
Total messages: 41 (6 generated)
kundaji@chromium.org changed reviewers: + bengr@chromium.org, cmumford@chromium.org, jeremyim@chromium.org
kundaji@chromium.org changed reviewers: + thestig@chromium.org
thestig: chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h chrome/browser/profiles/profile_impl_io_data.cc bengr,jeremyim: chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h components/data_reduction_proxy.gypi components/data_reduction_proxy/core/browser/BUILD.gn components/data_reduction_proxy/core/browser/DEPS components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h components/data_reduction_proxy/core/browser/data_reduction_proxy_store.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc components/data_reduction_proxy/core/browser/data_usage_storage_helper.h components/data_reduction_proxy/proto/BUILD.gn components/data_reduction_proxy/proto/store.proto cmumford: (Add leveldb to DEPS) components/data_reduction_proxy/core/browser/DEPS https://codereview.chromium.org/1173343009/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/DEPS (left): https://codereview.chromium.org/1173343009/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/DEPS:3: "+third_party/leveldatabase/src/helpers/memenv", Old snapshot is incorrect. This file is being added in this cl.
chrome/ lgtm since it's just passing a bit more info down.
A bunch of comments, but a few memory bugs, so marking as not lgtm. Also, since file I/O failures happen for legit reasons (full disks, etc.) having DCHECK's which break on errors technically are not valid. I suggest keeping them in for db corruption (maybe), or other program errors. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h (right): https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:21: enum Status { OK, NOT_FOUND, ERROR }; I recommend I/O Error as well as corruption error codes (both leveldb things). I/O Errors will happen with greater frequency (like a virus scanner having a file open, etc.). Then maybe use ERROR as a "catch all" for other error types. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:24: DCHECK(thread_checker_.CalledOnValidThread()); Nit: Suggest adding DCHECK_EQ(!db_); to catch a double open. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:27: options.create_if_missing = true; Recommend adding: options.paranoid_checks = true; options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue; https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:30: leveldb::Status status = leveldb::DB::Open(options, db_name, &dbptr); Add UMA stats to record db open result. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:33: LOG(FATAL) << "Failed to open Data Reduction Proxy DB: " I wouldn't make this FATAL. Failing to open a db will happen IRL. Also, you need to handle corrupted db's in one of two ways: 1) delete the db directory, or 2) call leveldb::RepairDB (which may drop some data). https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:35: } So this function returns void so there's no way for the caller to know it failed right? There's also no public method in DataReductionProxyStore so caller has no way to know if open was successful before calling Get/Put. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:38: DataReductionProxyStore::Status LevelDbToDRPStoreStatus( Put LevelDbToDRPStoreStatus in an anonymous namespace (recommended), or make a static method. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:53: leveldb::ReadOptions read_options; Recommend adding: read_options.verify_checksums https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:55: return LevelDbToDRPStoreStatus(db_->Get(read_options, slice, &value)); db_ isn't checked for nullptr before dereferencing. If db can't be opened then this will result in a crash right? https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:62: scoped_ptr<leveldb::WriteBatch> batch(new leveldb::WriteBatch()); Why allocate on the heap? WriteBatch is small, consider: WriteBatch batch; https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:70: return LevelDbToDRPStoreStatus(db_->Write(write_options, batch.get())); You should check the returned status (also above in Get). If corrupt (yes, this can happen with a successfully opened db), then you should probably close, and recover (see comment above). https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:73: DataReductionProxyStoreImpl::~DataReductionProxyStoreImpl() { What about adding DCHECK(thread_checker_.CalledOnValidThread()); to verify it's being deleted on the correct thread? https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:33: } // namespace https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:52: std::string DbKeyForBucketIndex(int index) { Move to anonymout namespace. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:52: std::string DbKeyForBucketIndex(int index) { Do you really intend to index into this string? This is an (to me at least) and unexpected way to construct the key. Maybe you intended to concatenate the two values? You should also add some DCHECK's to assert the index >= 0 && < strlen(kBucketKeyPrefix) if you intend to stick with this approach. Edit: I see you have that below. Not sure if it should be moved or duplicated? https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:58: DCHECK(current->bucket() == nullptr); DCHECK_EQ https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:67: current_bucket_index_ = atoi(current_index_string.c_str()); Is there a way you can use MessageLite::ParseFromString so that your serialize/deserialize methods use the same implementation? https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:69: current_bucket_index_ = 0; If not one of these two errors then current_bucket_index_ may still be -1, and then DbKeyForBucketIndex will have a buffer underflow right? https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:97: current->bucket()->last_updated_timestamp() == 0) Q: Why the second check for == 0? https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:104: scoped_ptr<std::map<std::string, std::string>> buckets_to_save( No need for buckets_to_save to be on the heap. Will be much simpler as a stack object. (and can change DataReductionProxyStore::Put to receive a reference instead of a ptr.) https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:108: scoped_ptr<DataUsageBucket> bucket(new DataUsageBucket()); Simplify to: AddBucketToMap(buckets_to_save.get(), new DataUsageBucket()); and then remove the braces. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:115: DCHECK(status == DataReductionProxyStore::Status::OK); Recommend removing DCHECK and simply returning the Status to the caller. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:127: map->insert(std::pair<std::string, std::string>(bucket_key, bucket_value)); This will write an empty value if unable to serialize. Probably don't want that? https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:131: if (current_bucket_index_ == kNumDataUsageBuckets) { Style nit: braces not required for one line. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:136: base::Time BucketLowerBoundary(base::Time time) { Move to anonymous namespace. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:139: exploded.minute = (exploded.minute / kDataUsageBucketLengthMins) * Consider: exploded.minute -= exploded.minute % kDataUsageBucketLengthMins; https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:147: DCHECK(kMinsInHour % kDataUsageBucketLengthMins == 0); This should be static_assert.
Thanks for the detailed review cmumford@. Made the code more robust against errors from level DB. PTAL. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h (right): https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:21: enum Status { OK, NOT_FOUND, ERROR }; On 2015/06/19 20:17:19, cmumford wrote: > I recommend I/O Error as well as corruption error codes (both leveldb things). > I/O Errors will happen with greater frequency (like a virus scanner having a > file open, etc.). Then maybe use ERROR as a "catch all" for other error types. Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:24: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/06/19 20:17:20, cmumford wrote: > Nit: Suggest adding DCHECK_EQ(!db_); to catch a double open. Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:27: options.create_if_missing = true; On 2015/06/19 20:17:19, cmumford wrote: > Recommend adding: > > options.paranoid_checks = true; > options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue; Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:30: leveldb::Status status = leveldb::DB::Open(options, db_name, &dbptr); On 2015/06/19 20:17:19, cmumford wrote: > Add UMA stats to record db open result. Done. There isn't a good way to convert leveldb::Status to an integer, so using DataReductionProxyStore::Status to record UMA. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:33: LOG(FATAL) << "Failed to open Data Reduction Proxy DB: " On 2015/06/19 20:17:19, cmumford wrote: > I wouldn't make this FATAL. Failing to open a db will happen IRL. > > Also, you need to handle corrupted db's in one of two ways: 1) delete the db > directory, or 2) call leveldb::RepairDB (which may drop some data). Done. RecreateDB below. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:35: } On 2015/06/19 20:17:20, cmumford wrote: > So this function returns void so there's no way for the caller to know it failed > right? There's also no public method in DataReductionProxyStore so caller has no > way to know if open was successful before calling Get/Put. Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:38: DataReductionProxyStore::Status LevelDbToDRPStoreStatus( On 2015/06/19 20:17:19, cmumford wrote: > Put LevelDbToDRPStoreStatus in an anonymous namespace (recommended), or make a > static method. Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:53: leveldb::ReadOptions read_options; On 2015/06/19 20:17:19, cmumford wrote: > Recommend adding: > > read_options.verify_checksums Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:55: return LevelDbToDRPStoreStatus(db_->Get(read_options, slice, &value)); On 2015/06/19 20:17:19, cmumford wrote: > db_ isn't checked for nullptr before dereferencing. If db can't be opened then > this will result in a crash right? Added DCHECK in Get and Put. Clients of this class will call IsValid() before calling any methods. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:62: scoped_ptr<leveldb::WriteBatch> batch(new leveldb::WriteBatch()); On 2015/06/19 20:17:19, cmumford wrote: > Why allocate on the heap? WriteBatch is small, consider: > > WriteBatch batch; Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:70: return LevelDbToDRPStoreStatus(db_->Write(write_options, batch.get())); On 2015/06/19 20:17:20, cmumford wrote: > You should check the returned status (also above in Get). If corrupt (yes, this > can happen with a successfully opened db), then you should probably close, and > recover (see comment above). Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:73: DataReductionProxyStoreImpl::~DataReductionProxyStoreImpl() { On 2015/06/19 20:17:19, cmumford wrote: > What about adding DCHECK(thread_checker_.CalledOnValidThread()); to verify it's > being deleted on the correct thread? Ref counted object. Could be destroyed on any thread. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:33: } On 2015/06/19 20:17:20, cmumford wrote: > // namespace Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:52: std::string DbKeyForBucketIndex(int index) { On 2015/06/19 20:17:20, cmumford wrote: > Move to anonymout namespace. Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:52: std::string DbKeyForBucketIndex(int index) { On 2015/06/19 20:17:20, cmumford wrote: > Do you really intend to index into this string? This is an (to me at least) and > unexpected way to construct the key. Maybe you intended to concatenate the two > values? > > You should also add some DCHECK's to assert the index >= 0 && < > strlen(kBucketKeyPrefix) if you intend to stick with this approach. > > Edit: I see you have that below. Not sure if it should be moved or duplicated? Good catch. Meant to append. Fixed. Added DCHECK here as well. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:58: DCHECK(current->bucket() == nullptr); On 2015/06/19 20:17:20, cmumford wrote: > DCHECK_EQ Using DCHECK_EQ requires using static_cast<DataUsageBucket*>(nullptr), so prefer DCHECK. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:67: current_bucket_index_ = atoi(current_index_string.c_str()); On 2015/06/19 20:17:20, cmumford wrote: > Is there a way you can use MessageLite::ParseFromString so that your > serialize/deserialize methods use the same implementation? Deserializing DataUsageBucket does use ParseFromString. Since the index is an integer, I parse it directly instead of using a proto. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:69: current_bucket_index_ = 0; On 2015/06/19 20:17:20, cmumford wrote: > If not one of these two errors then current_bucket_index_ may still be -1, and > then DbKeyForBucketIndex will have a buffer underflow right? Fixed. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:97: current->bucket()->last_updated_timestamp() == 0) On 2015/06/19 20:17:21, cmumford wrote: > Q: Why the second check for == 0? Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:104: scoped_ptr<std::map<std::string, std::string>> buckets_to_save( On 2015/06/19 20:17:20, cmumford wrote: > No need for buckets_to_save to be on the heap. Will be much simpler as a stack > object. (and can change DataReductionProxyStore::Put to receive a reference > instead of a ptr.) Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:108: scoped_ptr<DataUsageBucket> bucket(new DataUsageBucket()); On 2015/06/19 20:17:20, cmumford wrote: > Simplify to: > > AddBucketToMap(buckets_to_save.get(), new DataUsageBucket()); > > and then remove the braces. Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:115: DCHECK(status == DataReductionProxyStore::Status::OK); On 2015/06/19 20:17:20, cmumford wrote: > Recommend removing DCHECK and simply returning the Status to the caller. This class will only be used by DataReductionProxyCompressionStats. No point surfacing errors there. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:127: map->insert(std::pair<std::string, std::string>(bucket_key, bucket_value)); On 2015/06/19 20:17:20, cmumford wrote: > This will write an empty value if unable to serialize. Probably don't want that? This is intended. Empty data is better than outdated data. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:131: if (current_bucket_index_ == kNumDataUsageBuckets) { On 2015/06/19 20:17:20, cmumford wrote: > Style nit: braces not required for one line. Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:136: base::Time BucketLowerBoundary(base::Time time) { On 2015/06/19 20:17:20, cmumford wrote: > Move to anonymous namespace. Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:139: exploded.minute = (exploded.minute / kDataUsageBucketLengthMins) * On 2015/06/19 20:17:20, cmumford wrote: > Consider: > > exploded.minute -= exploded.minute % kDataUsageBucketLengthMins; Done. https://codereview.chromium.org/1173343009/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:147: DCHECK(kMinsInHour % kDataUsageBucketLengthMins == 0); On 2015/06/19 20:17:20, cmumford wrote: > This should be static_assert. Done.
Run "git cl lint" too. It will catch a few other minor issues in this patch. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.cc (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.cc:18: bool DummyDataReductionProxyStore::IsValid() { IsValid() const { https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.cc:24: std::string& value) { Can Get() be const, or does it need to modify the class? https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.cc:33: DummyDataReductionProxyStore::DummyDataReductionProxyStore(){}; Remove trailing semicolon here (and below). https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:21: enum Status { OK, NOT_FOUND, CORRUPTED, IO_ERROR, MISC_ERROR, STATUS_MAX }; Add a warning comment above this enum definition to warn that values are used in UMA, and are not to be changed, and only appended (to the end). https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:29: virtual Status Get(const std::string& key, std::string& value) = 0; "out" parameters need to be pointers. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:55: const std::map<std::string, std::string>& map) override; Need to #include <string>. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:70: if (status.IsCorruption()) { Nit: no braces needed for single-line scope. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:89: leveldb::Status status = db_->Write(write_options, &batch); So I'm still worried about a null ptr dereference of db_ in this class. In the field you *will* experience failures to open a database, which will currently leave db_ NULL. This class relies solely on it's owner (DataUsageStorageHelper) to not call Get/Put/etc. if the db couldn't be opened by first calling IsValid(). I think that's fragile. I'd feel safer with these methods checking db_. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:109: LevelDbToDRPStoreStatus(leveldb::DB::Open(options, db_name, &dbptr)); Here, once profile_path_ has been converted to a base::FilePath you should consider using AsUTF8Unsafe() instead of value() to get the file path. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:113: if (status != OK) This is the most likely place for corruption to occur. Do you want to recreate here? https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:48: const std::string profile_path_; In chrome/browser/profiles/profile_impl_io_data.cc, where InitDataReductionProxySettings() is called you have a FilePath, but you call value() to convert to a string. Then, when used in DataReductionProxyStoreImpl::RecreateDB(), it's converted back into a FilePath. Just keep it a base::FilePath everywhere - you only need to convert to a string in DataReductionProxyStoreImpl::OpenDB. Basically, keep it a FilePath as long as possible. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:11: #include <stdlib.h> Need to #include <string>, <map>, <utility>, and <algorithm> https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:39: std::stringstream ss; Consider using base::StringPrintf instead of std::stringstream which is barely used in Chrome. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:87: DCHECK(current_bucket_index_ >= 0 && Nit: You could convert to two DCHECK_GE and DCHECK_LT calls. This is nice when one fails - you'll get a much better error message. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:170: base::Time new_last_updated_timestamp) { Can this be const? https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.h (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:21: class DataUsageBucketWrapper I'm wondering if this class is needed. It's hard to tell because StoreCurrentDataUsageBucket and LoadCurrentDataUsageBucket are unused, but another common pattern is just to pass ownership of the DataUseBucket (via the closure) to the function in the other thread (which then assumes ownership of it). https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:24: DataUsageBucketWrapper(DataUsageBucket* bucket); explicit https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:45: DataUsageStorageHelper(scoped_refptr<DataReductionProxyStore> db); explicit https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:48: // called atleast once before any calls to |StoreCurrentDataUsageBucket|. nit: typo
https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.cc (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.cc:18: bool DummyDataReductionProxyStore::IsValid() { On 2015/06/24 18:48:59, cmumford wrote: > IsValid() const { Removed method based on comments elsewhere. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.cc:24: std::string& value) { On 2015/06/24 18:48:59, cmumford wrote: > Can Get() be const, or does it need to modify the class? No. We call RecreateDB() in Get() in case of corruption. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.cc:33: DummyDataReductionProxyStore::DummyDataReductionProxyStore(){}; On 2015/06/24 18:48:59, cmumford wrote: > Remove trailing semicolon here (and below). Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:21: enum Status { OK, NOT_FOUND, CORRUPTED, IO_ERROR, MISC_ERROR, STATUS_MAX }; On 2015/06/24 18:48:59, cmumford wrote: > Add a warning comment above this enum definition to warn that values are used in > UMA, and are not to be changed, and only appended (to the end). Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:29: virtual Status Get(const std::string& key, std::string& value) = 0; On 2015/06/24 18:48:59, cmumford wrote: > "out" parameters need to be pointers. Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:55: const std::map<std::string, std::string>& map) override; On 2015/06/24 18:48:59, cmumford wrote: > Need to #include <string>. Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:70: if (status.IsCorruption()) { On 2015/06/24 18:48:59, cmumford wrote: > Nit: no braces needed for single-line scope. Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:89: leveldb::Status status = db_->Write(write_options, &batch); On 2015/06/24 18:48:59, cmumford wrote: > So I'm still worried about a null ptr dereference of db_ in this class. In the > field you *will* experience failures to open a database, which will currently > leave db_ NULL. This class relies solely on it's owner (DataUsageStorageHelper) > to not call Get/Put/etc. if the db couldn't be opened by first calling > IsValid(). I think that's fragile. I'd feel safer with these methods checking > db_. Done. Removed the IsValid() function since there is no reason for both callers and this method db_ nullptr check. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:109: LevelDbToDRPStoreStatus(leveldb::DB::Open(options, db_name, &dbptr)); On 2015/06/24 18:48:59, cmumford wrote: > Here, once profile_path_ has been converted to a base::FilePath you should > consider using AsUTF8Unsafe() instead of value() to get the file path. Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:113: if (status != OK) On 2015/06/24 18:48:59, cmumford wrote: > This is the most likely place for corruption to occur. Do you want to recreate > here? The current code achieves the same. Keeping OpenDB and RecreateDB code simple makes reuse easier(Called from Get and Put). https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:48: const std::string profile_path_; On 2015/06/24 18:48:59, cmumford wrote: > In chrome/browser/profiles/profile_impl_io_data.cc, where > InitDataReductionProxySettings() is called you have a FilePath, but you call > value() to convert to a string. Then, when used in > DataReductionProxyStoreImpl::RecreateDB(), it's converted back into a FilePath. > Just keep it a base::FilePath everywhere - you only need to convert to a string > in DataReductionProxyStoreImpl::OpenDB. > > Basically, keep it a FilePath as long as possible. Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:11: #include <stdlib.h> On 2015/06/24 18:48:59, cmumford wrote: > Need to #include <string>, <map>, <utility>, and <algorithm> Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:39: std::stringstream ss; On 2015/06/24 18:48:59, cmumford wrote: > Consider using base::StringPrintf instead of std::stringstream which is barely > used in Chrome. Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:87: DCHECK(current_bucket_index_ >= 0 && On 2015/06/24 18:48:59, cmumford wrote: > Nit: You could convert to two DCHECK_GE and DCHECK_LT calls. This is nice when > one fails - you'll get a much better error message. Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:170: base::Time new_last_updated_timestamp) { On 2015/06/24 18:49:00, cmumford wrote: > Can this be const? Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.h (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:21: class DataUsageBucketWrapper On 2015/06/24 18:49:00, cmumford wrote: > I'm wondering if this class is needed. It's hard to tell because > StoreCurrentDataUsageBucket and LoadCurrentDataUsageBucket are unused, but > another common pattern is just to pass ownership of the DataUseBucket (via the > closure) to the function in the other thread (which then assumes ownership of > it). We will be using PostTaskAndReply so need a ref counted wrapper which will be a shared argument to task and reply. Not sure this can be achieved with scoped_ptr. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:24: DataUsageBucketWrapper(DataUsageBucket* bucket); On 2015/06/24 18:49:00, cmumford wrote: > explicit Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:45: DataUsageStorageHelper(scoped_refptr<DataReductionProxyStore> db); On 2015/06/24 18:49:00, cmumford wrote: > explicit Done. https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:48: // called atleast once before any calls to |StoreCurrentDataUsageBucket|. On 2015/06/24 18:49:00, cmumford wrote: > nit: typo Done.
lgtm FYI: you're still missing owners on: android_webview/browser/aw_browser_context.cc tools/metrics/histograms/histograms.xml
kundaji@chromium.org changed reviewers: + isherman@chromium.org, sgurun@chromium.org
Thanks cmumford@! Adding sgurun@ and isherman@ for new dependencies. sgurun: android_webview/browser/aw_browser_context.cc isherman: tools/metrics/histograms/histograms.xml
michaeln@chromium.org changed reviewers: + michaeln@chromium.org
https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.h (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:21: class DataUsageBucketWrapper On 2015/06/24 23:54:42, kundaji wrote: > On 2015/06/24 18:49:00, cmumford wrote: > > I'm wondering if this class is needed. It's hard to tell because > > StoreCurrentDataUsageBucket and LoadCurrentDataUsageBucket are unused, but > > another common pattern is just to pass ownership of the DataUseBucket (via the > > closure) to the function in the other thread (which then assumes ownership of > > it). > > We will be using PostTaskAndReply so need a ref counted wrapper which will be a > shared argument to task and reply. Not sure this can be achieved with > scoped_ptr. drive by: sure you can, for example... RegistrationList* registrations = new RegistrationList; std::vector<ResourceList>* resource_lists = new std::vector<ResourceList>; PostTaskAndReplyWithResult( database_task_manager_->GetTaskRunner(), FROM_HERE, base::Bind(&ServiceWorkerDatabase::GetRegistrationsForOrigin, base::Unretained(database_.get()), origin, base::Unretained(registrations), base::Unretained(resource_lists)), base::Bind(&ServiceWorkerStorage::DidGetRegistrations, weak_factory_.GetWeakPtr(), callback, base::Owned(registrations), base::Owned(resource_lists), origin));
On 2015/06/25 19:56:26, michaeln wrote: > https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... > File components/data_reduction_proxy/core/browser/data_usage_storage_helper.h > (right): > > https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:21: > class DataUsageBucketWrapper > On 2015/06/24 23:54:42, kundaji wrote: > > On 2015/06/24 18:49:00, cmumford wrote: > > > I'm wondering if this class is needed. It's hard to tell because > > > StoreCurrentDataUsageBucket and LoadCurrentDataUsageBucket are unused, but > > > another common pattern is just to pass ownership of the DataUseBucket (via > the > > > closure) to the function in the other thread (which then assumes ownership > of > > > it). > > > > We will be using PostTaskAndReply so need a ref counted wrapper which will be > a > > shared argument to task and reply. Not sure this can be achieved with > > scoped_ptr. > > drive by: sure you can, for example... > > RegistrationList* registrations = new RegistrationList; > std::vector<ResourceList>* resource_lists = new std::vector<ResourceList>; > PostTaskAndReplyWithResult( > database_task_manager_->GetTaskRunner(), FROM_HERE, > base::Bind(&ServiceWorkerDatabase::GetRegistrationsForOrigin, > base::Unretained(database_.get()), origin, > base::Unretained(registrations), > base::Unretained(resource_lists)), > base::Bind(&ServiceWorkerStorage::DidGetRegistrations, > weak_factory_.GetWeakPtr(), callback, > base::Owned(registrations), base::Owned(resource_lists), > origin)); aw lgtm, with the node that at some point of time this feature will be enabled for webview and we will probably need to collect and store data usage data similar to chrome. but then there is a lot of time for that.
histograms lgtm
https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.h (right): https://codereview.chromium.org/1173343009/diff/200001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:21: class DataUsageBucketWrapper On 2015/06/25 19:56:26, michaeln wrote: > On 2015/06/24 23:54:42, kundaji wrote: > > On 2015/06/24 18:49:00, cmumford wrote: > > > I'm wondering if this class is needed. It's hard to tell because > > > StoreCurrentDataUsageBucket and LoadCurrentDataUsageBucket are unused, but > > > another common pattern is just to pass ownership of the DataUseBucket (via > the > > > closure) to the function in the other thread (which then assumes ownership > of > > > it). > > > > We will be using PostTaskAndReply so need a ref counted wrapper which will be > a > > shared argument to task and reply. Not sure this can be achieved with > > scoped_ptr. > > drive by: sure you can, for example... > > RegistrationList* registrations = new RegistrationList; > std::vector<ResourceList>* resource_lists = new std::vector<ResourceList>; > PostTaskAndReplyWithResult( > database_task_manager_->GetTaskRunner(), FROM_HERE, > base::Bind(&ServiceWorkerDatabase::GetRegistrationsForOrigin, > base::Unretained(database_.get()), origin, > base::Unretained(registrations), > base::Unretained(resource_lists)), > base::Bind(&ServiceWorkerStorage::DidGetRegistrations, > weak_factory_.GetWeakPtr(), callback, > base::Owned(registrations), base::Owned(resource_lists), > origin)); Great! Had not looked at base::Owned before. Thanks for pointing me to it. I now understand what cmumford was suggesting. Removed the wrapper and changed to use pointers.
https://codereview.chromium.org/1173343009/diff/340001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc (right): https://codereview.chromium.org/1173343009/diff/340001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:194: scoped_refptr<data_reduction_proxy::DataReductionProxyStoreImpl> store( So the store is created on UI and initialzed on DB? Can DB outlive UI? Also, our UI-to-IO thread hopping code in in DRPService. It might make sense to put our UI-to-DB posts there too. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:309: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner, #include single_thread_task_runner.h https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:24: class SequencedTaskRunner; You should forward declare the SingleThreadTaskRunner here instead, though I'm not sure why you changed from a SequencedTaskRunner. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:39: // Constructs a data reduction proxy delayed pref service object using Please update this comment. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:11: #include "base/sequenced_task_runner.h" single_thread_task_runner https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:73: // Constructs compression stats with a dummy store. This should not be called What is a dummy store? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:16: // Interface for a permanent key value store used by the Data Reduction Proxy key value -> key/value https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:19: class DataReductionProxyStore Why not just have no-op implementations instead of pure virtuals? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:30: virtual Status Get(const std::string& key, std::string* value) = 0; Can this method be const? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:46: Add another blank line. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:48: class DummyDataReductionProxyStore : public DataReductionProxyStore { I don't think the dummy is needed. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:34: else You can delete this line. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:14: #include "base/memory/weak_ptr.h" Why is this needed? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:45: Status OpenDB(); Please add comments for these methods and separate with a blank line. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:28: const int kMinsInHour = 60; kMinutesInHour https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:32: const int kDataUsageBucketLengthMins = 15; kDataUsageBucketLengthInMinutes https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:33: static_assert(kMinsInHour % kDataUsageBucketLengthMins == 0, kDataUsageBucketLengthMins should also be > 0. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:105: DCHECK(current); No need to DCHECK the pointer. You'll know quickly enough. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:120: for (int i = 0; i < num_buckets_since_last_saved; i++) { ++i
Drive by comments, cmumford asked me to look at one thing, but then i looked at more. Not nearly a full review, but enough to make some comments. I don't think this is ready for prime time just yet. The method to the madness of the refcounting in here doesn't seem right. Does anything in here really need to be refcounted? Given that the 'store' is effectively owned by the compressionStats instance, why is refcounting needed at all. Please use the SequencedWorkerPool instead (unless there's some compelling reason not to). https://codereview.chromium.org/1173343009/diff/340001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc (right): https://codereview.chromium.org/1173343009/diff/340001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:210: data_reduction_proxy::DataReductionProxySettings:: Lots of symbols in this method are prefixed with data_reduction_proxy:: which makes it less readable. Looks like most of it boils down to constructing and initializing a DataReductionProxyService instance. Would it make sense to push more of this complicatd setup code into the data_reduction_proxy namespace? Or maybe use some using statements to help with readability? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:316: data_usage_store_(new DataUsageStorageHelper(store)), Why the double wrapping of refcounted objects where the sole refholder of the helper is |this| and the sole refholder of the store is the helper? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:48: scoped_refptr<DataReductionProxyStore> store, can these be passed by reference? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:49: const base::TimeDelta& delay); no real reason to pass TimeDeltas by reference (they just has a single int64 data member) https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:78: scoped_refptr<base::SingleThreadTaskRunner> db_task_runner, We have a preference for using the SequencedWorkerPool instead of a specific named thread. Is there a reason to use a specific thread in this case? If not, probably should massage the CL to use a SWP sequence instead. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:19: class DataReductionProxyStore On 2015/06/25 23:22:55, bengr wrote: > Why not just have no-op implementations instead of pure virtuals? Is the abstract base class really needed at all? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:97: DataReductionProxyStoreImpl::~DataReductionProxyStoreImpl() { What ensures this refcounted object isn't deleted on the UI or IO thread? It will close the db_ in here which involves file io which we don't want to do on the UI or IO threads. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:107: std::string db_name = profile_path_.Append(kDBName).AsUTF8Unsafe(); What about incognito? Is this used for incognito profiles where we don't want to write anything to disk? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:43: friend class base::RefCountedThreadSafe<DataReductionProxyStore>; drive by: is this friend decl really needed? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:68: void DataUsageStorageHelper::LoadCurrentDataUsageBucket( returning a DataUsageBucket might make more sense for this method https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:96: DCHECK(current->ParseFromString(bucket_as_string)); why is |current| a parameter to this method since its only used in a DCHECK? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:103: const DataUsageBucket* current) { const ref might make more sense for this arg https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:121: AddBucketToMap(new DataUsageBucket(), &buckets_to_save); does this leak buckets? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:147: const DataUsageBucket* bucket, const ref might make more sense for this argument
This patch is kind of large, it may be helpful to break it down into smaller parts and build up incrementally to a functioning whole.
Thanks for the reviews! @michaeln: DataReductionProxyStore is a common store for the component. Will eventually be used by more than compression stats. There isn't a good owner for the store, which is why I used ref counting. All of the other classes in data_reduction_proxy either live on the IO or UI thread. All IO thread objects hang off of DRPIOData while UI objects hang off of DRPService. SequencedWorkerPool seems heavier weight than SequencedTaskRunner. We don't need any of the functionality provided by SWP, so not sure if there is a point in moving to that. Also that is beyond the scope of this cl. Regarding size of cl, I already tried to pick a small part of a bigger initiative. The bulk of the change is in DataUsageStorageHelper and DataReductionProxyStore. The rest of it is only hooking stuff up. I could separate out these classes into a different cl, but it will lose context of their usage. https://codereview.chromium.org/1173343009/diff/340001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc (right): https://codereview.chromium.org/1173343009/diff/340001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:194: scoped_refptr<data_reduction_proxy::DataReductionProxyStoreImpl> store( On 2015/06/25 23:22:54, bengr wrote: > So the store is created on UI and initialzed on DB? Can DB outlive UI? > > Also, our UI-to-IO thread hopping code in in DRPService. It might make sense to > put our UI-to-DB posts there too. UI thread outlives DB thread. For DB thread hops we don't use weak pointers so a similar structure does not get anything functionally. We could do this for stylistic reasons, but DRPService seems like the wrong place. I could add a new class -- DRPDBService say -- which will post to DB thread. However, currently compression stats and here are the only places such posts would happen. How about waiting until there is a usecase outside of compression stats? https://codereview.chromium.org/1173343009/diff/340001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:210: data_reduction_proxy::DataReductionProxySettings:: On 2015/06/26 20:15:00, michaeln wrote: > Lots of symbols in this method are prefixed with data_reduction_proxy:: which > makes it less readable. Looks like most of it boils down to constructing and > initializing a DataReductionProxyService instance. Would it make sense to push > more of this complicatd setup code into the data_reduction_proxy namespace? Or > maybe use some using statements to help with readability? The data_reduction_proxy code has actively moved away from "using". Could look at moving this code, but out of scope for this cl. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:309: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner, On 2015/06/25 23:22:54, bengr wrote: > #include single_thread_task_runner.h Reverted. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:316: data_usage_store_(new DataUsageStorageHelper(store)), On 2015/06/26 20:15:00, michaeln wrote: > Why the double wrapping of refcounted objects where the sole refholder of the > helper is |this| and the sole refholder of the store is the helper? DataReductionProxyStore will have uses outside this class. Changed data_usage_store_ to use scoped_ptr. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:24: class SequencedTaskRunner; On 2015/06/25 23:22:54, bengr wrote: > You should forward declare the SingleThreadTaskRunner here instead, though I'm > not sure why you changed from a SequencedTaskRunner. Reverted to SequencedTaskRunner. Had initially changed it to allow calling BelondsToCurrentThread, but not required for current implementation. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:39: // Constructs a data reduction proxy delayed pref service object using On 2015/06/25 23:22:54, bengr wrote: > Please update this comment. Done. Will add more details when I add code to collect detailed data usage. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:48: scoped_refptr<DataReductionProxyStore> store, On 2015/06/26 20:15:00, michaeln wrote: > can these be passed by reference? DataReductionProxyStore is meant to be a common store for all of the data reduction proxy component. Current, it is only used from here, but there are expected to be uses from elsewhere, so ref counting seems to make more sense. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:49: const base::TimeDelta& delay); On 2015/06/26 20:15:00, michaeln wrote: > no real reason to pass TimeDeltas by reference (they just has a single int64 > data member) Existing code. Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:11: #include "base/sequenced_task_runner.h" On 2015/06/25 23:22:54, bengr wrote: > single_thread_task_runner Reverted https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:73: // Constructs compression stats with a dummy store. This should not be called On 2015/06/25 23:22:54, bengr wrote: > What is a dummy store? Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:78: scoped_refptr<base::SingleThreadTaskRunner> db_task_runner, On 2015/06/26 20:15:01, michaeln wrote: > We have a preference for using the SequencedWorkerPool instead of a specific > named thread. Is there a reason to use a specific thread in this case? If not, > probably should massage the CL to use a SWP sequence instead. Reverted back to using SequencedTaskRunner. This code base has been using STR all through. Not sure there is an advantage to moving to the heavier weight SWP. Can you please clarify why you suggesting this? Also, seems outside the scope of this cl? https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:16: // Interface for a permanent key value store used by the Data Reduction Proxy On 2015/06/25 23:22:55, bengr wrote: > key value -> key/value Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:19: class DataReductionProxyStore On 2015/06/25 23:22:55, bengr wrote: > Why not just have no-op implementations instead of pure virtuals? Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:19: class DataReductionProxyStore On 2015/06/26 20:15:01, michaeln wrote: > On 2015/06/25 23:22:55, bengr wrote: > > Why not just have no-op implementations instead of pure virtuals? > > Is the abstract base class really needed at all? Removed. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:30: virtual Status Get(const std::string& key, std::string* value) = 0; On 2015/06/25 23:22:55, bengr wrote: > Can this method be const? No. Get from level db could return CORRUPTION in which case we recreate DB. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:46: On 2015/06/25 23:22:55, bengr wrote: > Add another blank line. Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:48: class DummyDataReductionProxyStore : public DataReductionProxyStore { On 2015/06/25 23:22:55, bengr wrote: > I don't think the dummy is needed. Done https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:34: else On 2015/06/25 23:22:55, bengr wrote: > You can delete this line. Having the 'else' will prevent you from adding to the Status enum without updating this method. Otherwise any new entry in the enum will cause this method to always return MISC_ERROR. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:97: DataReductionProxyStoreImpl::~DataReductionProxyStoreImpl() { On 2015/06/26 20:15:01, michaeln wrote: > What ensures this refcounted object isn't deleted on the UI or IO thread? It > will close the db_ in here which involves file io which we don't want to do on > the UI or IO threads. Added check for valid thread. Users of this class will ensure that destruction is on the correct thread. Currently, DRP_compression_stats releases DataUsageStorageHelper on DB thread, which triggers the destruction of this class. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:107: std::string db_name = profile_path_.Append(kDBName).AsUTF8Unsafe(); On 2015/06/26 20:15:01, michaeln wrote: > What about incognito? Is this used for incognito profiles where we don't want to > write anything to disk? No, data_reduction_proxy component does not run for incognito. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:14: #include "base/memory/weak_ptr.h" On 2015/06/25 23:22:55, bengr wrote: > Why is this needed? Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:43: friend class base::RefCountedThreadSafe<DataReductionProxyStore>; On 2015/06/26 20:15:01, michaeln wrote: > drive by: is this friend decl really needed? Removed here. Declaration in DataReductionProxyStore is sufficient. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:45: Status OpenDB(); On 2015/06/25 23:22:55, bengr wrote: > Please add comments for these methods and separate with a blank line. Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:28: const int kMinsInHour = 60; On 2015/06/25 23:22:55, bengr wrote: > kMinutesInHour Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:32: const int kDataUsageBucketLengthMins = 15; On 2015/06/25 23:22:55, bengr wrote: > kDataUsageBucketLengthInMinutes Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:33: static_assert(kMinsInHour % kDataUsageBucketLengthMins == 0, On 2015/06/25 23:22:55, bengr wrote: > kDataUsageBucketLengthMins should also be > 0. Done. Although someone wanting to specify a negative value here will not be stopped by a static assert:) https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:68: void DataUsageStorageHelper::LoadCurrentDataUsageBucket( On 2015/06/26 20:15:01, michaeln wrote: > returning a DataUsageBucket might make more sense for this method Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:96: DCHECK(current->ParseFromString(bucket_as_string)); On 2015/06/26 20:15:01, michaeln wrote: > why is |current| a parameter to this method since its only used in a DCHECK? ParseFromString(..) populates |current|. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:103: const DataUsageBucket* current) { On 2015/06/26 20:15:01, michaeln wrote: > const ref might make more sense for this arg Called from a different thread. So const DataUsageBucket* or const scoped_refptr<>& seem to make more sense. Otherwise the other thread will have to own it. With either DataUsageBucket* or const scoped_refptr<>&, ref counting avoids having to explicitly deal with ownership. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:105: DCHECK(current); On 2015/06/25 23:22:55, bengr wrote: > No need to DCHECK the pointer. You'll know quickly enough. This is more to set expectations for someone reading the code. For example, someone might come along and put a null check here. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:120: for (int i = 0; i < num_buckets_since_last_saved; i++) { On 2015/06/25 23:22:55, bengr wrote: > ++i Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:121: AddBucketToMap(new DataUsageBucket(), &buckets_to_save); On 2015/06/26 20:15:01, michaeln wrote: > does this leak buckets? Good catch. Fixed. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:147: const DataUsageBucket* bucket, On 2015/06/26 20:15:01, michaeln wrote: > const ref might make more sense for this argument StoreCurrentDataUsageBucket will be called from different thread which in turn calls this. Not sure const reference to entity on another thread makes sense.
https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:316: data_usage_store_(new DataUsageStorageHelper(store)), On 2015/06/29 20:53:26, kundaji wrote: > On 2015/06/26 20:15:00, michaeln wrote: > > Why the double wrapping of refcounted objects where the sole refholder of the > > helper is |this| and the sole refholder of the store is the helper? > > DataReductionProxyStore will have uses outside this class. Changed > data_usage_store_ to use scoped_ptr. Note that calls to |data_usage_store_| will be posted to db_task_runner_. I used ref counted to ensure that data_usage_store_ will be around when the task executes. So perhaps I should go back to ref counting here. Or is base::Owned preferable?
michaeln@: I am changing this cl to get rid of ref counting. Instead DataReductionProxyService will own a object (D_R_P_DBService) which will live on the DB thread and support weak pointers. All tasks will be posted using weak pointers. Discussed this approach in person with bengr@ and jeremyim@. Will update when this version is uploaded.
> I am changing this cl to get rid of ref counting. sgtm! https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:48: scoped_refptr<DataReductionProxyStore> store, On 2015/06/29 20:53:26, kundaji wrote: > On 2015/06/26 20:15:00, michaeln wrote: > > can these be passed by reference? > > DataReductionProxyStore is meant to be a common store for all of the data > reduction proxy component. Current, it is only used from here, but there are > expected to be uses from elsewhere, so ref counting seems to make more sense. i meant pass the scoper by reference: const scoped_refptr<T>& look_ma_no_refcount_churn https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:96: DCHECK(current->ParseFromString(bucket_as_string)); On 2015/06/29 20:53:27, kundaji wrote: > On 2015/06/26 20:15:01, michaeln wrote: > > why is |current| a parameter to this method since its only used in a DCHECK? > > ParseFromString(..) populates |current|. DCHECKS are compiled out of release builds https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:103: const DataUsageBucket* current) { On 2015/06/29 20:53:28, kundaji wrote: > On 2015/06/26 20:15:01, michaeln wrote: > > const ref might make more sense for this arg > > Called from a different thread. So const DataUsageBucket* or const > scoped_refptr<>& seem to make more sense. Otherwise the other thread will have > to own it. With either DataUsageBucket* or const scoped_refptr<>&, ref counting > avoids having to explicitly deal with ownership. Not sure const T* makes any more or less sense then const T&? DataUsageBucket is not a refcounted class so the discusion of refcounting doesn't seem to make sense here. https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:206: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB)); Please use the default blocking pool setup by the BrowserThread class for these new tasks that do fileio. We'd like to eventually remove the named DB and FILE threads and to use the blocking pool for the existing callers of those threads. Since you're adding new code, it'd be good to use the blocking pool straight away, unless there's a reason not to. pool = BrowserThread::GetBlockingPool(); runner = pool->GetSequencedTaskRunnerWithShutdownBehavior( pool->GetSequenceToken(), SKIP_ON_SHUTDOWN); There's nothing heavy weight about doing so.
https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h (right): https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:8: #include "base/files/file_path.h" remove include, forward declare base::FilePath https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:201: DataReductionProxyChromeSettingsFactory::GetForBrowserContext(profile_) nit: leave -> on the previous line since git cl format isn't enforced in this directory https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:53: const base::TimeDelta delay); Don't need const keyword for delay https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:96: void InitOnDBThread(); Does this method exist? https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:73: // Constructs compression stats with a dummy store which does nothing. Load Constructs compression stats with a noop |DataReductionProxyStore|; load and store calls do nothing. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:158: DCHECK(thread_checker_.CalledOnValidThread()); Not necessary, since it's a private method called from a public method which already does this check https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:175: DCHECK(thread_checker_.CalledOnValidThread()); DCHECK unnecessary https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.h (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:24: class DataUsageStorageHelper { rename to DataReductionProxyUsageStore or DataUsageStore (depending on whether you plan on collecting non DRP data) https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:26: explicit DataUsageStorageHelper(scoped_refptr<DataReductionProxyStore> db); const scoped_refptr& https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:40: void StoreCurrentDataUsageBucket(const DataUsageBucket* current_bucket); const DataUsageBucket& ? https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper_unittest.cc:102: LOG(WARNING) << "######" << iter->first << ":" << iter->second; I'm guessing this line will be removed before checkin? https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/proto/store.proto (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/proto/store.proto:13: // Contains data usage for an interval of time. Does it make sense for a bucket to contain it's time information? That would allow the values to be changed in the future. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/proto/store.proto:14: message DataUsageBucket { What happened to tag 1?
Also, +1 on splitting this CL into 2 parts. =)
Thanks for the reviews! I changed this cl to get rid of ref counting for the new objects. Now all the DB based objects are owned by DBService. DBService will be owned by DataReductionProxyService, but I have split this up into a different cl. For this cl, the new classes have not yet been hooked up into DRPService. For reference, 2nd part of this cl is here: https://codereview.chromium.org/1221663009 Thanks! https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:48: scoped_refptr<DataReductionProxyStore> store, On 2015/06/29 23:19:30, michaeln wrote: > On 2015/06/29 20:53:26, kundaji wrote: > > On 2015/06/26 20:15:00, michaeln wrote: > > > can these be passed by reference? > > > > DataReductionProxyStore is meant to be a common store for all of the data > > reduction proxy component. Current, it is only used from here, but there are > > expected to be uses from elsewhere, so ref counting seems to make more sense. > > i meant pass the scoper by reference: > const scoped_refptr<T>& look_ma_no_refcount_churn Ah! Fixed. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:34: else On 2015/06/29 20:53:27, kundaji wrote: > On 2015/06/25 23:22:55, bengr wrote: > > You can delete this line. > > Having the 'else' will prevent you from adding to the Status enum without > updating this method. Otherwise any new entry in the enum will cause this method > to always return MISC_ERROR. Ignore above comment. Done. https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:96: DCHECK(current->ParseFromString(bucket_as_string)); On 2015/06/29 23:19:30, michaeln wrote: > On 2015/06/29 20:53:27, kundaji wrote: > > On 2015/06/26 20:15:01, michaeln wrote: > > > why is |current| a parameter to this method since its only used in a DCHECK? > > > > ParseFromString(..) populates |current|. > > DCHECKS are compiled out of release builds Doh! Thanks:) https://codereview.chromium.org/1173343009/diff/340001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:103: const DataUsageBucket* current) { On 2015/06/29 23:19:30, michaeln wrote: > On 2015/06/29 20:53:28, kundaji wrote: > > On 2015/06/26 20:15:01, michaeln wrote: > > > const ref might make more sense for this arg > > > > Called from a different thread. So const DataUsageBucket* or const > > scoped_refptr<>& seem to make more sense. Otherwise the other thread will > have > > to own it. With either DataUsageBucket* or const scoped_refptr<>&, ref > counting > > avoids having to explicitly deal with ownership. > > Not sure const T* makes any more or less sense then const T&? DataUsageBucket is > not a refcounted class so the discusion of refcounting doesn't seem to make > sense here. Changed to const ref. I meant "T*" seemed easier than "const T&" since "T*" allows pointer to be bound to callback. Wasn't sure where the object would live if I used "const T&". Now I have a DBService which can hold this reference on the same thread which allows using "const T&". https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h (right): https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:8: #include "base/files/file_path.h" On 2015/06/30 01:59:18, jeremyim wrote: > remove include, forward declare base::FilePath Done. https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:201: DataReductionProxyChromeSettingsFactory::GetForBrowserContext(profile_) On 2015/06/30 01:59:18, jeremyim wrote: > nit: leave -> on the previous line since git cl format isn't enforced in this > directory "->" goes back to new line everytime I run git cl format. Seems better to leave as is rather than manually changing back each time? https://codereview.chromium.org/1173343009/diff/380001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:206: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB)); On 2015/06/29 23:19:30, michaeln wrote: > Please use the default blocking pool setup by the BrowserThread class for these > new tasks that do fileio. We'd like to eventually remove the named DB and FILE > threads and to use the blocking pool for the existing callers of those threads. > Since you're adding new code, it'd be good to use the blocking pool straight > away, unless there's a reason not to. > > pool = BrowserThread::GetBlockingPool(); > runner = pool->GetSequencedTaskRunnerWithShutdownBehavior( > pool->GetSequenceToken(), SKIP_ON_SHUTDOWN); > > There's nothing heavy weight about doing so. Done. Thanks for the explanation. I had misunderstood your suggestion. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:53: const base::TimeDelta delay); On 2015/06/30 01:59:18, jeremyim wrote: > Don't need const keyword for delay Done. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:96: void InitOnDBThread(); On 2015/06/30 01:59:18, jeremyim wrote: > Does this method exist? Removed. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:73: // Constructs compression stats with a dummy store which does nothing. Load On 2015/06/30 01:59:18, jeremyim wrote: > Constructs compression stats with a noop |DataReductionProxyStore|; load and > store calls do nothing. Done. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:158: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/06/30 01:59:19, jeremyim wrote: > Not necessary, since it's a private method called from a public method which > already does this check Done. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.cc:175: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/06/30 01:59:18, jeremyim wrote: > DCHECK unnecessary Done. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper.h (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:24: class DataUsageStorageHelper { On 2015/06/30 01:59:19, jeremyim wrote: > rename to DataReductionProxyUsageStore or DataUsageStore (depending on whether > you plan on collecting non DRP data) Done. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:26: explicit DataUsageStorageHelper(scoped_refptr<DataReductionProxyStore> db); On 2015/06/30 01:59:19, jeremyim wrote: > const scoped_refptr& No longer using ref ptr. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper.h:40: void StoreCurrentDataUsageBucket(const DataUsageBucket* current_bucket); On 2015/06/30 01:59:19, jeremyim wrote: > const DataUsageBucket& ? Done. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_storage_helper_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_storage_helper_unittest.cc:102: LOG(WARNING) << "######" << iter->first << ":" << iter->second; On 2015/06/30 01:59:19, jeremyim wrote: > I'm guessing this line will be removed before checkin? Yep, removed. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/proto/store.proto (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/proto/store.proto:13: // Contains data usage for an interval of time. On 2015/06/30 01:59:19, jeremyim wrote: > Does it make sense for a bucket to contain it's time information? That would > allow the values to be changed in the future. You can use last_updated_timestamp to compute the time range. https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/proto/store.proto:14: message DataUsageBucket { On 2015/06/30 01:59:19, jeremyim wrote: > What happened to tag 1? Fixed.
https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/proto/store.proto (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/proto/store.proto:13: // Contains data usage for an interval of time. On 2015/07/01 17:13:49, kundaji wrote: > On 2015/06/30 01:59:19, jeremyim wrote: > > Does it make sense for a bucket to contain it's time information? That would > > allow the values to be changed in the future. > > You can use last_updated_timestamp to compute the time range. The only issue is that a computed time range depends on the computation params which are hard coded (i.e. if the bucket contained its own time range, the params in code could change). https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/DEPS (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/DEPS:3: "+content/public/browser/browser_thread.h", I believe this is a layering violation. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:50: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); I believe these should all be task_runner->RunTasksOnCurrentThread() - i.e. DRPStoreImpl should not aware of what the actual backing of the taskrunner is. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:14: #include "base/threading/thread_checker.h" Unused https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.h (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:15: #include "base/threading/thread_checker.h" Unused. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_service.cc (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/db_service.cc:23: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); Use task_runner, not the named thread pool. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_service.h (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/db_service.h:10: #include "base/threading/thread_checker.h" Unused. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/db_service.h:26: class DBService { DataReductionProxyDBData?
https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... File components/data_reduction_proxy/proto/store.proto (right): https://codereview.chromium.org/1173343009/diff/380001/components/data_reduct... components/data_reduction_proxy/proto/store.proto:13: // Contains data usage for an interval of time. On 2015/07/01 21:31:01, jeremyim wrote: > On 2015/07/01 17:13:49, kundaji wrote: > > On 2015/06/30 01:59:19, jeremyim wrote: > > > Does it make sense for a bucket to contain it's time information? That would > > > allow the values to be changed in the future. > > > > You can use last_updated_timestamp to compute the time range. > > The only issue is that a computed time range depends on the computation params > which are hard coded (i.e. if the bucket contained its own time range, the > params in code could change). We spent some time debating the smallest bucket size so don't expect this to change. We should be able to fields in case this changes and still be backwards compatible. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/DEPS (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/DEPS:3: "+content/public/browser/browser_thread.h", On 2015/07/01 21:31:01, jeremyim wrote: > I believe this is a layering violation. Good point. Fixed. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:50: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2015/07/01 21:31:01, jeremyim wrote: > I believe these should all be task_runner->RunTasksOnCurrentThread() - i.e. > DRPStoreImpl should not aware of what the actual backing of the taskrunner is. Found DCHECK(sequence_checker_.CalledOnValidSequencedThread()). "task_runner->RunTasksOnCurrentThread()" requires task runner to be passed in, so sequence checker seems preferable. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:14: #include "base/threading/thread_checker.h" On 2015/07/01 21:31:01, jeremyim wrote: > Unused Done. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.h (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:15: #include "base/threading/thread_checker.h" On 2015/07/01 21:31:01, jeremyim wrote: > Unused. Done. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_service.cc (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/db_service.cc:23: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2015/07/01 21:31:01, jeremyim wrote: > Use task_runner, not the named thread pool. Used sequence_checker. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_service.h (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/db_service.h:10: #include "base/threading/thread_checker.h" On 2015/07/01 21:31:01, jeremyim wrote: > Unused. Done. https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/db_service.h:26: class DBService { On 2015/07/01 21:31:02, jeremyim wrote: > DataReductionProxyDBData? Personally prefer just DBData without the prefix. Do you have a preference on prefix vs no prefix? Using "DataReductionProxy" prefix on everything makes things pretty hard to read. Especially when we also tag on "data_reduction_proxy::" namespace.
https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_service.h (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/db_service.h:26: class DBService { On 2015/07/01 23:17:38, kundaji wrote: > On 2015/07/01 21:31:02, jeremyim wrote: > > DataReductionProxyDBData? > > Personally prefer just DBData without the prefix. Do you have a preference on > prefix vs no prefix? > > Using "DataReductionProxy" prefix on everything makes things pretty hard to > read. Especially when we also tag on "data_reduction_proxy::" namespace. For better our worse, we prefix our filenames + classes (especially when the names would be fairly generic otherwise). https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:12: #include "base/memory/ref_counted.h" Unneeded. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:11: #include "base/threading/sequenced_worker_pool.h" Unused. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:22: class WriteBatch; A bunch of unused forward declares. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:26: class DataUsageBucket; Unused? https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.h (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:14: #include "base/memory/weak_ptr.h" Unused counted/ptr* includes https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:6: #include "base/memory/ref_counted.h" Unused https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:58: scoped_ptr<DataUsageStore> data_usage_store_; Shouldn't this be at line 68/69? Does this need to be part of DataUsageStoreTest?
Thanks for the review! https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_service.h (right): https://codereview.chromium.org/1173343009/diff/440001/components/data_reduct... components/data_reduction_proxy/core/browser/db_service.h:26: class DBService { On 2015/07/06 20:08:42, jeremyim wrote: > On 2015/07/01 23:17:38, kundaji wrote: > > On 2015/07/01 21:31:02, jeremyim wrote: > > > DataReductionProxyDBData? > > > > Personally prefer just DBData without the prefix. Do you have a preference on > > prefix vs no prefix? > > > > Using "DataReductionProxy" prefix on everything makes things pretty hard to > > read. Especially when we also tag on "data_reduction_proxy::" namespace. > > For better our worse, we prefix our filenames + classes (especially when the > names would be fairly generic otherwise). sleevi@ had brought our naming convention up in a prior review and now michaeln@ pointed out the same issue. Sent an email to the team. Will wait to see if there is a consensus before I get LGTMs. If not, I'll rename to DataReductionProxyDBData. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store.h:12: #include "base/memory/ref_counted.h" On 2015/07/06 20:08:42, jeremyim wrote: > Unneeded. Done. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.cc:11: #include "base/threading/sequenced_worker_pool.h" On 2015/07/06 20:08:43, jeremyim wrote: > Unused. Done. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:22: class WriteBatch; On 2015/07/06 20:08:43, jeremyim wrote: > A bunch of unused forward declares. Done. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_store_impl.h:26: class DataUsageBucket; On 2015/07/06 20:08:43, jeremyim wrote: > Unused? Done. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.h (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:14: #include "base/memory/weak_ptr.h" On 2015/07/06 20:08:43, jeremyim wrote: > Unused counted/ptr* includes Done. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:6: #include "base/memory/ref_counted.h" On 2015/07/06 20:08:43, jeremyim wrote: > Unused Done. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:58: scoped_ptr<DataUsageStore> data_usage_store_; On 2015/07/06 20:08:43, jeremyim wrote: > Shouldn't this be at line 68/69? Does this need to be part of > DataUsageStoreTest? Moved to line 69. Not sure what the alternative is to having this be part of the test?
Ping. Can you please take a look?
Some minor comments about renaming/sorting of forward declarations. https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:58: scoped_ptr<DataUsageStore> data_usage_store_; On 2015/07/06 21:02:23, kundaji wrote: > On 2015/07/06 20:08:43, jeremyim wrote: > > Shouldn't this be at line 68/69? Does this need to be part of > > DataUsageStoreTest? > > Moved to line 69. > > Not sure what the alternative is to having this be part of the test? It would be moving the creation of the data store inside of each test. https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy.gypi (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy.gypi:56: 'data_reduction_proxy/core/browser/db_data_owner.h', Fix sorting https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/BUILD.gn (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/BUILD.gn:46: "data_reduction_proxy_store.h", Fix file renames here https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.cc (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.cc:22: #include "components/data_reduction_proxy/core/browser/db_data_store.h" At the top? https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.h (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:17: class DataUsageBucket; Sorting https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:16: class TestDataReductionProxyStore : public data_reduction_proxy::DBDataStore { Rename https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_data_owner.h (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_owner.h:17: class DBDataStore; Sorting https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_data_store.h (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_store.h:17: // support a permanent store. |DataReductionProxyStoreImpl| provides a LevelDB Rename in comment https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_store.h:19: class DBDataStore { Just DataStore? https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_data_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_store_impl.h:23: // Implementation of DataReductionProxyStore using LevelDB. Rename in comment
https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/460001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:58: scoped_ptr<DataUsageStore> data_usage_store_; On 2015/07/08 18:21:45, jeremyim wrote: > On 2015/07/06 21:02:23, kundaji wrote: > > On 2015/07/06 20:08:43, jeremyim wrote: > > > Shouldn't this be at line 68/69? Does this need to be part of > > > DataUsageStoreTest? > > > > Moved to line 69. > > > > Not sure what the alternative is to having this be part of the test? > > It would be moving the creation of the data store inside of each test. Seems better to do it in SetUp than in every test? https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy.gypi (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy.gypi:56: 'data_reduction_proxy/core/browser/db_data_owner.h', On 2015/07/08 18:21:46, jeremyim wrote: > Fix sorting Done. https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/BUILD.gn (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/BUILD.gn:46: "data_reduction_proxy_store.h", On 2015/07/08 18:21:46, jeremyim wrote: > Fix file renames here Done. https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.cc (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.cc:22: #include "components/data_reduction_proxy/core/browser/db_data_store.h" On 2015/07/08 18:21:46, jeremyim wrote: > At the top? Done. https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.h (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:17: class DataUsageBucket; On 2015/07/08 18:21:46, jeremyim wrote: > Sorting Done. https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:16: class TestDataReductionProxyStore : public data_reduction_proxy::DBDataStore { On 2015/07/08 18:21:46, jeremyim wrote: > Rename Done. https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_data_owner.h (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_owner.h:17: class DBDataStore; On 2015/07/08 18:21:46, jeremyim wrote: > Sorting Done. https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_data_store.h (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_store.h:17: // support a permanent store. |DataReductionProxyStoreImpl| provides a LevelDB On 2015/07/08 18:21:46, jeremyim wrote: > Rename in comment Done. https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_store.h:19: class DBDataStore { On 2015/07/08 18:21:46, jeremyim wrote: > Just DataStore? Done. https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_data_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/500001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_store_impl.h:23: // Implementation of DataReductionProxyStore using LevelDB. On 2015/07/08 18:21:46, jeremyim wrote: > Rename in comment Done.
lgtm % comment nits https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_store.h (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store.h:18: // implementation. nit: Don't think we need the comment about |DataStoreImpl|. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store.h:28: // Initializes the store on DB Thread. nit: Initializes the store on the DB sequenced task runner. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store_impl.h:29: void InitializeOnDBThread() override; nit: Comment "Overrides of DataStore" https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store_impl.h:36: // Opens the underlying Level DB for read and write. nit: s/Level DB/LevelDB/ to be consistent https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store_impl.h:40: // return |CORRUPTED| status. nit: "This method is called if any DB call returns a |CORRUPTED| status." https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store_impl.h:42: nit: Comment for db_ and profile_path_ https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.h (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:33: // the current data usage bucket in the DB if they are for the same interval. nit: "in the DataStore" or "in |db_|" https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:43: // |DataReductionProxyStore| and add to the map. nit: |DataStore| or |db_| https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:54: // The index of the last bucket persisted in the DB. |DataUsageBucket| is nit: "persisted in |db_|" https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:55: // stored in the DB as a circular array. This index points to the array nit: "stored in |db_|" https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:59: base::SequenceChecker sequence_checker_; nit: move sequence_checker_ below current_bucket_last_updated_ https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:61: // The time when the current bucket was last written to DB. This field is nit: "DB" -> "|db_|" https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:15: // In memory hash map implemention of |DataReductionProxyStore| for testing. nit: |DataStore| https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:16: class TestDataStore : public data_reduction_proxy::DataStore { FYI: You could also have put TestDataStore in an anonymous namespace inside the data_reduction_proxy namespace to avoid using the fully qualified DataStore class here. It doesn't matter too much since you don't have fully qualified names elsewhere in this class. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:66: DataUsageStore* data_usage_store() { return data_usage_store_.get(); } nit: const on data_usage_store() and store() (and probably the two int/int64 methods) https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_data_owner.h (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_owner.h:30: comments on methods
https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_store.h (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store.h:18: // implementation. On 2015/07/08 22:17:56, jeremyim wrote: > nit: Don't think we need the comment about |DataStoreImpl|. Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store.h:28: // Initializes the store on DB Thread. On 2015/07/08 22:17:56, jeremyim wrote: > nit: Initializes the store on the DB sequenced task runner. Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_store_impl.h (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store_impl.h:29: void InitializeOnDBThread() override; On 2015/07/08 22:17:56, jeremyim wrote: > nit: Comment "Overrides of DataStore" Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store_impl.h:36: // Opens the underlying Level DB for read and write. On 2015/07/08 22:17:56, jeremyim wrote: > nit: s/Level DB/LevelDB/ to be consistent Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store_impl.h:40: // return |CORRUPTED| status. On 2015/07/08 22:17:56, jeremyim wrote: > nit: "This method is called if any DB call returns a |CORRUPTED| status." Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_store_impl.h:42: On 2015/07/08 22:17:56, jeremyim wrote: > nit: Comment for db_ and profile_path_ Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store.h (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:33: // the current data usage bucket in the DB if they are for the same interval. On 2015/07/08 22:17:57, jeremyim wrote: > nit: "in the DataStore" or "in |db_|" Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:43: // |DataReductionProxyStore| and add to the map. On 2015/07/08 22:17:56, jeremyim wrote: > nit: |DataStore| or |db_| Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:54: // The index of the last bucket persisted in the DB. |DataUsageBucket| is On 2015/07/08 22:17:57, jeremyim wrote: > nit: "persisted in |db_|" Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:55: // stored in the DB as a circular array. This index points to the array On 2015/07/08 22:17:57, jeremyim wrote: > nit: "stored in |db_|" Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:59: base::SequenceChecker sequence_checker_; On 2015/07/08 22:17:56, jeremyim wrote: > nit: move sequence_checker_ below current_bucket_last_updated_ Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store.h:61: // The time when the current bucket was last written to DB. This field is On 2015/07/08 22:17:57, jeremyim wrote: > nit: "DB" -> "|db_|" Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:15: // In memory hash map implemention of |DataReductionProxyStore| for testing. On 2015/07/08 22:17:57, jeremyim wrote: > nit: |DataStore| Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:16: class TestDataStore : public data_reduction_proxy::DataStore { On 2015/07/08 22:17:57, jeremyim wrote: > FYI: You could also have put TestDataStore in an anonymous namespace inside the > data_reduction_proxy namespace to avoid using the fully qualified DataStore > class here. It doesn't matter too much since you don't have fully qualified > names elsewhere in this class. Acknowledged. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc:66: DataUsageStore* data_usage_store() { return data_usage_store_.get(); } On 2015/07/08 22:17:57, jeremyim wrote: > nit: const on data_usage_store() and store() (and probably the two int/int64 > methods) Done. https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... File components/data_reduction_proxy/core/browser/db_data_owner.h (right): https://codereview.chromium.org/1173343009/diff/630001/components/data_reduct... components/data_reduction_proxy/core/browser/db_data_owner.h:30: On 2015/07/08 22:17:57, jeremyim wrote: > comments on methods Done.
The CQ bit was checked by kundaji@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, cmumford@chromium.org, sgurun@chromium.org, isherman@chromium.org, jeremyim@chromium.org Link to the patchset: https://codereview.chromium.org/1173343009/#ps670001 (title: "Remove wrong const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1173343009/670001
Message was sent while issue was closed.
Committed patchset #35 (id:670001)
Message was sent while issue was closed.
Patchset 35 (id:??) landed as https://crrev.com/9a02cfe87e75d1c4996b87fb3ec9d4e1fc9b8d6b Cr-Commit-Position: refs/heads/master@{#337968} |
