Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7029007:
Implement EvictOriginData in QuotaManager. (Closed)
|
Created:
9 years, 7 months ago by Dai Mikurube (NOT FULLTIME) Modified:
9 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, tzik Visibility:
Public. |
DescriptionImplement EvictOriginData in QuotaManager.
BUG=61676
TEST=QuotaManagerTest.EvictOriginData
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85750
Patch Set 1 #Patch Set 2 : Updated. #
Total comments: 8
Patch Set 3 : Extracted only EvictOriginData (f.k.a. DeleteOriginData). #
Total comments: 8
Patch Set 4 : Added a. test function. (Not working well, and not reflected the comments.) #
Total comments: 12
Patch Set 5 : Fixed, updated the test, and reflected the comments. #
Total comments: 8
Patch Set 6 : Reflected the comments. #
Total comments: 10
Patch Set 7 : Reflected the comments. #
Total comments: 2
Patch Set 8 : Renamed On... to Did... #
Messages
Total messages: 19 (0 generated)
Extracted QuotaManager functions from http://codereview.chromium.org/7002024/.
Sorry for having confused you, I basically wanted to make the tasks splittable / put a better distinction between evictor implementation and QM-side implementation. http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.h#ne... webkit/quota/quota_manager.h:189: DeleteOriginDataCallback* callback) OVERRIDE; style-nit: Can you put derived methods together without empty lines? http://dev.chromium.org/developers/coding-style says: "When you implement an interface, group those functions in your header file in one section, clearly labeled as implementing that interface. Be sure to preserve the order of the virtual base class, and do not add comments or whitespace between them, as the reader can refer to the virtual base class for more information."
http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.cc#n... webkit/quota/quota_manager.cc:734: num_deleted_clients_ = 0; you might be able to do this with one data member instead of two... pending_deletion_requests_ ++ prior to calling client->DeleteOriginData() -- here http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.cc#n... webkit/quota/quota_manager.cc:752: GetUsageAndQuotaForEviction(callback); Seems strange to call this method for the last condition... an origin is already being evicted and you can't do more than one at a time. It looks like the only caller you expect to have of this method is the Evictor class and it won't issue a second call with the first still pending. If that's the case maybe DCHECK() that last condition is hit. Btw, it's not clear to me why we need a seperate Evictor class that is tightly coupled with this class. Seems like the logic of the Evictor could be baked straight into this class, assuming there's not a lot to that logic. http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.h#ne... webkit/quota/quota_manager.h:50: virtual void DeleteOriginData( Delete sounds more generic than this really is given the signature of the callback, maybe EvictOriginData(...)
I think http://codereview.chromium.org/7029009/ is the newer one of this. On 2011/05/17 00:19:45, michaeln wrote: > http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.cc > File webkit/quota/quota_manager.cc (right): > > http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.cc#n... > webkit/quota/quota_manager.cc:734: num_deleted_clients_ = 0; > you might be able to do this with one data member instead of two... > > pending_deletion_requests_ > ++ prior to calling client->DeleteOriginData() > -- here > > http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.cc#n... > webkit/quota/quota_manager.cc:752: GetUsageAndQuotaForEviction(callback); I believe the QM code in this patch is very tentative one... > Seems strange to call this method for the last condition... an origin is already > being evicted and you can't do more than one at a time. > > It looks like the only caller you expect to have of this method is the Evictor > class and it won't issue a second call with the first still pending. If that's > the case maybe DCHECK() that last condition is hit. > > Btw, it's not clear to me why we need a seperate Evictor class that is tightly > coupled with this class. Seems like the logic of the Evictor could be baked > straight into this class, assuming there's not a lot to that logic. We may end up having them together in QM. Or the Evictor will have some modest amount of code for the eviction control code and we could separately test the eviction logic with a smaller set of code. > http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.h > File webkit/quota/quota_manager.h (right): > > http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.h#ne... > webkit/quota/quota_manager.h:50: virtual void DeleteOriginData( > Delete sounds more generic than this really is given the signature of the > callback, maybe EvictOriginData(...) +1
Separated GetUsageAndQuotaForEviction out into http://codereview.chromium.org/7039006/. Now changes on eviction are : * http://codereview.chromium.org/7029009/ (interface; landed) * http://codereview.chromium.org/7002024/ (QuotaTemporaryStorageEvictor) * http://codereview.chromium.org/7029007/ (EvictOriginData; this) * http://codereview.chromium.org/7039006/ (GetUsageAndQuotaForEviction) http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.cc#n... webkit/quota/quota_manager.cc:734: num_deleted_clients_ = 0; On 2011/05/17 00:19:45, michaeln wrote: > you might be able to do this with one data member instead of two... > > pending_deletion_requests_ > ++ prior to calling client->DeleteOriginData() > -- here We can do that, but I think these two variables help us debugging. http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.cc#n... webkit/quota/quota_manager.cc:752: GetUsageAndQuotaForEviction(callback); On 2011/05/17 00:19:45, michaeln wrote: > Seems strange to call this method for the last condition... an origin is already > being evicted and you can't do more than one at a time. > > It looks like the only caller you expect to have of this method is the Evictor > class and it won't issue a second call with the first still pending. If that's > the case maybe DCHECK() that last condition is hit. > > Btw, it's not clear to me why we need a seperate Evictor class that is tightly > coupled with this class. Seems like the logic of the Evictor could be baked > straight into this class, assuming there's not a lot to that logic. > I'm separating and re-constructing the change. Let's discuss it later. http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.h#ne... webkit/quota/quota_manager.h:50: virtual void DeleteOriginData( On 2011/05/17 00:19:45, michaeln wrote: > Delete sounds more generic than this really is given the signature of the > callback, maybe EvictOriginData(...) Done. http://codereview.chromium.org/7029007/diff/4/webkit/quota/quota_manager.h#ne... webkit/quota/quota_manager.h:189: DeleteOriginDataCallback* callback) OVERRIDE; On 2011/05/16 13:48:01, kinuko wrote: > style-nit: Can you put derived methods together without empty lines? > > http://dev.chromium.org/developers/coding-style says: > "When you implement an interface, group those functions in your header file in > one section, clearly labeled as implementing that interface. Be sure to preserve > the order of the virtual base class, and do not add comments or whitespace > between them, as the reader can refer to the virtual base class for more > information." Done.
Some earlier comments http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:742: EvictOriginDataCallback* callback) { This method won't take the callback parameter for non-testing code right? http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:756: // TODO(dmikurube): Get usage, quota and physical space and then callback. This comment is obsoleted? http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:768: LazyInitialize(); Eviction must run after temporary storage initialization has finished, so I think we can replace this with DCHECK(database_.get()) ? http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:772: if (origin.is_empty() || num_clients == 0 || num_deleted_clients_ > 0) { As michael pointed out please DCHECK on num_deleted_clients_ or on whatever with which you can ensure we're not re-entering this method. If you're not going to make a separate class to track DeleteOriginData progress you'll want to keep the given EvictOriginDataCallback as a QM member variable (to dispatch the callback later), so probably you can DCHECK on the callback I think.
http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:759: delete evict_origin_data_callback_; is the callback leaked if the class is destructed prior to completion? http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:767: LOG(ERROR) << "QM::DeleteOriginData"; Please fixup the LOG(ERROR) usage throughout. It's not an error to have this method called. http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:768: DCHECK(io_thread_->BelongsToCurrentThread()); Since LazyInitialize has this DCHECK, consider removing it from this method http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:785: evict_origin_data_callback_ = callback; maybe move this assignment up out of the loop http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:211: int num_deleted_clients_; consider using the term 'evict' in some form in all of these data members http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager_u... File webkit/quota/quota_manager_unittest.cc (right): http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager_u... webkit/quota/quota_manager_unittest.cc:148: &QuotaManagerTest::DidDelete)); DidEvict?
Thanks. http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:742: EvictOriginDataCallback* callback) { On 2011/05/17 09:48:55, kinuko wrote: > This method won't take the callback parameter for non-testing code right? Done. http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:756: // TODO(dmikurube): Get usage, quota and physical space and then callback. On 2011/05/17 09:48:55, kinuko wrote: > This comment is obsoleted? Done. http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:768: LazyInitialize(); On 2011/05/17 09:48:55, kinuko wrote: > Eviction must run after temporary storage initialization has finished, so I > think we can replace this with DCHECK(database_.get()) ? Done. http://codereview.chromium.org/7029007/diff/7001/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:772: if (origin.is_empty() || num_clients == 0 || num_deleted_clients_ > 0) { On 2011/05/17 09:48:55, kinuko wrote: > As michael pointed out please DCHECK on num_deleted_clients_ or on whatever with > which you can ensure we're not re-entering this method. > If you're not going to make a separate class to track DeleteOriginData progress > you'll want to keep the given EvictOriginDataCallback as a QM member variable > (to dispatch the callback later), so probably you can DCHECK on the callback I > think. Done. I don't understand the second paragraph... Which callback? What is DCHECK'd on the callback? http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:759: } On 2011/05/17 19:30:01, michaeln wrote: > is the callback leaked if the class is destructed prior to completion? Done. http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:767: DCHECK(io_thread_->BelongsToCurrentThread()); On 2011/05/17 19:30:01, michaeln wrote: > Please fixup the LOG(ERROR) usage throughout. It's not an error to have this > method called. Yeah, I'll remove them finally for other changes. They're just local messages for debugging. I was uploading the code just to share the current status... http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:768: LazyInitialize(); On 2011/05/17 19:30:01, michaeln wrote: > Since LazyInitialize has this DCHECK, consider removing it from this method LazyInitialize here was replaced with DCHECK(database_.get()) by Kinuko-san's comment. http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:785: p->DeleteOriginData(origin, kStorageTypeTemporary, NewRunnableMethod( On 2011/05/17 19:30:01, michaeln wrote: > maybe move this assignment up out of the loop Done. http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:211: int num_deleted_clients_; On 2011/05/17 19:30:01, michaeln wrote: > consider using the term 'evict' in some form in all of these data members Done. http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager_u... File webkit/quota/quota_manager_unittest.cc (right): http://codereview.chromium.org/7029007/diff/7004/webkit/quota/quota_manager_u... webkit/quota/quota_manager_unittest.cc:148: &QuotaManagerTest::DidDelete)); On 2011/05/17 19:30:01, michaeln wrote: > DidEvict? Exactly. Done.
http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:807: DCHECK(num_evicted_clients_ == 0); num_evicted_clients_ == 0 wouldn't guarantee we're not entering this method while there're in-flight tasks. DCHECK(num_eviction_requested_clients_) would be better? http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:213: EvictOriginDataCallback* evict_origin_data_callback_; could this be simply scoped_ptr? The current code doesn't initialize this to zero anywhere or doesn't check if the callback is already set when we assign a new callback, and it worries me a bit. http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager_u... File webkit/quota/quota_manager_unittest.cc (right): http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager_u... webkit/quota/quota_manager_unittest.cc:135: void DeleteOriginData(QuotaClient* client, Could we rename this to DeleteClientOriginData and DidDeleteClientData to avoid confusion?
http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:807: DCHECK(num_evicted_clients_ == 0); On 2011/05/18 04:45:12, kinuko wrote: > num_evicted_clients_ == 0 wouldn't guarantee we're not entering this method > while there're in-flight tasks. > > DCHECK(num_eviction_requested_clients_) would be better? Do you mean DCHECK(num_eviction_requested_clients_ == 0) ? Done with it. http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:213: EvictOriginDataCallback* evict_origin_data_callback_; On 2011/05/18 04:45:12, kinuko wrote: > could this be simply scoped_ptr? > > The current code doesn't initialize this to zero anywhere or doesn't check if > the callback is already set when we assign a new callback, and it worries me a > bit. I guess scoped_ptr is better. Done though I didn't understand the error case you worries... http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager_u... File webkit/quota/quota_manager_unittest.cc (right): http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager_u... webkit/quota/quota_manager_unittest.cc:135: void DeleteOriginData(QuotaClient* client, On 2011/05/18 04:45:12, kinuko wrote: > Could we rename this to DeleteClientOriginData and DidDeleteClientData to avoid > confusion? Done.
http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:213: EvictOriginDataCallback* evict_origin_data_callback_; > I guess scoped_ptr is better. Done though I didn't understand the error case > you worries... not a good idea to have wild pointers or dangling pointers lingering around in long-lived classes... long live scoped_ptr<> !!
http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/3007/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:213: EvictOriginDataCallback* evict_origin_data_callback_; On 2011/05/18 05:24:36, michaeln wrote: > > I guess scoped_ptr is better. Done though I didn't understand the error case > > you worries... > > not a good idea to have wild pointers or dangling pointers lingering around in > long-lived classes... long live scoped_ptr<> !! Thanks. Yes, I agree with that and modified it to scoped_ptr<>. :)
some more nits. http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:18: #include "webkit/quota/quota_client.h" nit: this is already included in quota_manager.h http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:793: Here can we call DeleteOriginFromDatabase (if there were no errors)? http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:795: evict_origin_data_callback_.reset(NULL); reset() will do (no need to specify NULL) http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:806: DCHECK(type == kStorageTypeTemporary) ? http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:822: (*p)->DeleteOriginData(origin, kStorageTypeTemporary, callback_factory_. s/kStorageTypeTemporary/type/ ?
http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:18: #include "webkit/quota/quota_client.h" On 2011/05/18 05:48:35, kinuko wrote: > nit: this is already included in quota_manager.h Done. http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:793: On 2011/05/18 05:48:35, kinuko wrote: > Here can we call DeleteOriginFromDatabase (if there were no errors)? How about add it later? (Since it looks like not tested, and it requires some parameters from EvictOriginData. Extracting them into a class looks better to do that.) http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:795: evict_origin_data_callback_.reset(NULL); On 2011/05/18 05:48:35, kinuko wrote: > reset() will do (no need to specify NULL) Done. http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:806: On 2011/05/18 05:48:35, kinuko wrote: > DCHECK(type == kStorageTypeTemporary) ? Done. http://codereview.chromium.org/7029007/diff/10006/webkit/quota/quota_manager.... webkit/quota/quota_manager.cc:822: (*p)->DeleteOriginData(origin, kStorageTypeTemporary, callback_factory_. On 2011/05/18 05:48:35, kinuko wrote: > s/kStorageTypeTemporary/type/ ? Done.
lgtm if tests look happy. http://codereview.chromium.org/7029007/diff/8003/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/8003/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:183: void OnOriginDataEvicted(QuotaStatusCode status); nit: I may want to rename this to DidEvictOriginData as this class's all the other callback methods are named like DidXxx
http://codereview.chromium.org/7029007/diff/8003/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7029007/diff/8003/webkit/quota/quota_manager.h... webkit/quota/quota_manager.h:183: void OnOriginDataEvicted(QuotaStatusCode status); On 2011/05/18 06:21:28, kinuko wrote: > nit: I may want to rename this to DidEvictOriginData as this class's all the > other callback methods are named like DidXxx Renamed it.
LGTM
Thanks. Checked the box.
Change committed as 85750 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
