Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7002024:
Implement QuotaTemporaryStorageEvictor. (Closed)
|
Created:
9 years, 7 months ago by Dai Mikurube (NOT FULLTIME) Modified:
9 years, 7 months ago CC:
tzik Visibility:
Public. |
DescriptionImplement QuotaTemporaryStorageEvictor.
It's based on http://codereview.chromium.org/7029009/.
BUG=61676
TEST=QuotaTemporaryStorageEvictorTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86597
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixed. #Patch Set 3 : Removed unnecessary code. #Patch Set 4 : Catching up the latest 7003021. #
Total comments: 17
Patch Set 5 : Reflected some comments. #Patch Set 6 : Re-designed and added a sample test. #Patch Set 7 : Added a TODO to call OnQuotaManagerDestroyed. #
Total comments: 4
Patch Set 8 : Changed the flow. #Patch Set 9 : Updated. (Tests have some problems.) #Patch Set 10 : Added QuotaEvictionHandler and removed functions in QuotaManager. #Patch Set 11 : Extracted. #
Total comments: 2
Patch Set 12 : Modified to fit http://codereview.chromium.org/7029009/. #
Total comments: 6
Patch Set 13 : Cleaned-up with using Timer. (No clean-up for the unit-test.) #
Total comments: 20
Patch Set 14 : Modified the unit test. #Patch Set 15 : Reflected the comments. (Not rebased.) #Patch Set 16 : Rebased. #Patch Set 17 : Fixed and cleaned-up. #
Total comments: 20
Patch Set 18 : Fixed the test and some nit fix. #Patch Set 19 : Rebased and nit fix. #
Total comments: 16
Patch Set 20 : Reflected the comments, and added a test for available disk space. #Patch Set 21 : Reflected the comments. #
Total comments: 18
Patch Set 22 : Reflected the comments, and rebased. #
Total comments: 13
Patch Set 23 : Reflected the comments. #
Total comments: 12
Patch Set 24 : Reflected the comments. #
Total comments: 4
Patch Set 25 : Reflected the comments. #
Messages
Total messages: 49 (0 generated)
It doesn't compile and work. Uploaded it just to share outline of the design. What do you think?
http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_task.cc File webkit/quota/quota_task.cc (right): http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_task.cc#newc... webkit/quota/quota_task.cc:82: this, &QuotaThreadTask::CallRunOnTargetThread), delay_ms_); On the second thought I guess for Evictor's main job we can simply/directly call this PostDelayedTask in the Evictor code using ScopedRunnableMethodFactory? It doesn't need to carry around per-task / per-invocation info but will need to continuously work using the evictor's shared status. (while in the main method we may still need to post some quota thread tasks) http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... webkit/quota/quota_temporary_storage_evictor.cc:46: virtual void RunOnTargetThread() OVERRIDE { This method can be in the Evictor? http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... webkit/quota/quota_temporary_storage_evictor.cc:66: ++q) { Conceptually the main loop would look like that. Some random thoughts... (you may have already considered some of them): - the origin loop will need to be in a callback chain - each client will throw a task to its preferable thread to delete its data, so we'll need to collect/count callbacks to determine if one origin is completely deleted - when one origin is completely deleted we'll need to check if the remaining available space looks good to go or if we'll need to run another deletion cycle for the next origin. Actually Database::GetLRUOrigins() might not fit very well to our model? We may want to have the method that returns the least recently used one that are not in a given 'in-use' origins list? Database::GetLRUOrigin(vector<GURL> in_use_origins) http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... webkit/quota/quota_temporary_storage_evictor.h:25: virtual void Start(); does this need to be virtual?
Thanks for the comments. The patch set 2 correctly starts Evict() periodically. (With some dummy objects.) http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_task.cc File webkit/quota/quota_task.cc (right): http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_task.cc#newc... webkit/quota/quota_task.cc:82: this, &QuotaThreadTask::CallRunOnTargetThread), delay_ms_); On 2011/05/11 07:49:30, kinuko wrote: > On the second thought I guess for Evictor's main job we can simply/directly call > this PostDelayedTask in the Evictor code using ScopedRunnableMethodFactory? It > doesn't need to carry around per-task / per-invocation info but will need to > continuously work using the evictor's shared status. (while in the main method > we may still need to post some quota thread tasks) Changed it and removed QuotaDelayedThreadTask. BTW, what is the need to post quota thread tasks? http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... webkit/quota/quota_temporary_storage_evictor.cc:46: virtual void RunOnTargetThread() OVERRIDE { On 2011/05/11 07:49:30, kinuko wrote: > This method can be in the Evictor? Moved. http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... webkit/quota/quota_temporary_storage_evictor.cc:66: ++q) { On 2011/05/11 07:49:30, kinuko wrote: > Conceptually the main loop would look like that. > > Some random thoughts... (you may have already considered some of them): > - the origin loop will need to be in a callback chain > - each client will throw a task to its preferable thread to delete its data, so > we'll need to collect/count callbacks to determine if one origin is completely > deleted > - when one origin is completely deleted we'll need to check if the remaining > available space looks good to go or if we'll need to run another deletion cycle > for the next origin. > > Actually Database::GetLRUOrigins() might not fit very well to our model? > We may want to have the method that returns the least recently used one that are > not in a given 'in-use' origins list? > > Database::GetLRUOrigin(vector<GURL> in_use_origins) Yeah, it was just a conceptual code. Changed it with looking at http://codereview.chromium.org/7003021/ . http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_temporary_st... webkit/quota/quota_temporary_storage_evictor.h:25: virtual void Start(); On 2011/05/11 07:49:30, kinuko wrote: > does this need to be virtual? Exactly. Removed virtual.
Removed unnecessary code. http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_task.cc File webkit/quota/quota_task.cc (right): http://codereview.chromium.org/7002024/diff/1/webkit/quota/quota_task.cc#newc... webkit/quota/quota_task.cc:82: this, &QuotaThreadTask::CallRunOnTargetThread), delay_ms_); On 2011/05/11 11:23:50, Dai Mikurube wrote: > On 2011/05/11 07:49:30, kinuko wrote: > > On the second thought I guess for Evictor's main job we can simply/directly > call > > this PostDelayedTask in the Evictor code using ScopedRunnableMethodFactory? > It > > doesn't need to carry around per-task / per-invocation info but will need to > > continuously work using the evictor's shared status. (while in the main > method > > we may still need to post some quota thread tasks) > > Changed it and removed QuotaDelayedThreadTask. BTW, what is the need to post > quota thread tasks? Ah, failed to remove it. Removed in the patch set 3.
Just a comment for myself. http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:73: /* || not quota exceeded */) { Calling QuotaManager and callback would be required.
A few additional iterational comments. http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:476: this, database_.get(), 3000, db_thread_)); Maybe this initialization could be delayed until the InitializeTask finishes? (If it fails on some db failure we could just disable running the evictor) http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:38: NewRunnableMethod(&QuotaTemporaryStorageEvictor::Evict), delay_ms_); How would we do handle other already running client tasks for the origin? http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:47: if (false /* More deletion required? */) { This part would need some design doc. There's a rough design doc, we don't need to follow this but may worth taking another look at: https://docs.google.com/a/google.com/document/d/1EIFQDvGFo4pswyd5-v-o2tDNlhvG... In general we need to figure out how the evictor knows the current usage and how much amount of space the evictor should free. http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:70: num_clients_ = clients.size(); :) Let's start thinking how we could test this class! http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:92: MessageLoop::current()->PostTask(FROM_HERE, runnable_factory_. I think we should replace MessageLoop::current() with db_message_loop_ (here and elsewhere)? http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.h:37: int num_deleted_; just to improve readability, maybe they can be num_clients_ and num_deleted_clients or can be even xxx_for_current_origin_? http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.h:42: void OnDeleted(QuotaStatusCode status); style nit: declaration order (from what I was told before) * Typedefs and Enums * Constants (static const data members) * Constructors * Destructor * Methods, including static methods * Data Members (except static const data members)
Replying just for the last comment: http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:92: MessageLoop::current()->PostTask(FROM_HERE, runnable_factory_. On 2011/05/11 12:43:30, kinuko wrote: > I think we should replace MessageLoop::current() with db_message_loop_ (here and > elsewhere)? Actually, just replacing with db_message_loop_ caused a fatal error in (some Task?)::Run(). Run() looked requiring to be called from the "current thread"...
http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_manager.c... webkit/quota/quota_manager.cc:476: this, database_.get(), 3000, db_thread_)); On 2011/05/11 12:43:30, kinuko wrote: > Maybe this initialization could be delayed until the InitializeTask finishes? > (If it fails on some db failure we could just disable running the evictor) Hmm, added a TODO for now. http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:38: NewRunnableMethod(&QuotaTemporaryStorageEvictor::Evict), delay_ms_); On 2011/05/11 12:43:30, kinuko wrote: > How would we do handle other already running client tasks for the origin? Exactly. At first, I've added an error flag. http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:47: if (false /* More deletion required? */) { On 2011/05/11 12:43:30, kinuko wrote: > This part would need some design doc. > > There's a rough design doc, we don't need to follow this but may worth taking > another look at: > https://docs.google.com/a/google.com/document/d/1EIFQDvGFo4pswyd5-v-o2tDNlhvG... > > In general we need to figure out how the evictor knows the current usage and how > much amount of space the evictor should free. Yes, that's the point. Let's discuss later. I'll be checking the design doc. :) http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:70: num_clients_ = clients.size(); On 2011/05/11 12:43:30, kinuko wrote: > :) Let's start thinking how we could test this class! Before that, I think some method to get a list of clients from QuotaManager is required... With that, I guess we can test this class just by mocking QuotaManager. http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.h:37: int num_deleted_; On 2011/05/11 12:43:30, kinuko wrote: > just to improve readability, maybe they can be > num_clients_ and num_deleted_clients or > can be even xxx_for_current_origin_? Done. Chose num_deleted_clients. http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.h:42: void OnDeleted(QuotaStatusCode status); On 2011/05/11 12:43:30, kinuko wrote: > style nit: declaration order (from what I was told before) > * Typedefs and Enums > * Constants (static const data members) > * Constructors > * Destructor > * Methods, including static methods > * Data Members (except static const data members) Thanks. Re-ordered.
http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:92: MessageLoop::current()->PostTask(FROM_HERE, runnable_factory_. FYI: base/task.h:71-72 says // The factories are not thread safe, so always invoke on // |MessageLoop::current()|. To post the task directly to the db_thread, we cannot use ScopedRunnableMethodFactory.
http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/2002/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:92: MessageLoop::current()->PostTask(FROM_HERE, runnable_factory_. On 2011/05/12 04:12:02, Dai Mikurube wrote: > FYI: base/task.h:71-72 says > // The factories are not thread safe, so always invoke on > // |MessageLoop::current()|. > > To post the task directly to the db_thread, we cannot use > ScopedRunnableMethodFactory. :( ...I see, so it's only to post tasks to the current thread!
Updated for the new design.
(Some cosmetic (doudemoii) comments) http://codereview.chromium.org/7002024/diff/14001/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7002024/diff/14001/webkit/quota/quota_manager.... webkit/quota/quota_manager.h:131: virtual void DeleteOriginDataOnIOThread( nit: for now all the methods of QuotaManager need to be called on io thread (it's noted in the comment at the top of this file), so maybe you can drop OnIOThread from the method name. http://codereview.chromium.org/7002024/diff/14001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/14001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:24: // Constructed in the io_thread. nit: in -> on?
Changed the code flow. I know that method names are not good for now... (e.g. ...Evictor::OnDeletionCompletedOnIOThread though it is called from QM::GetUsageAndQuotaForEviction.) http://codereview.chromium.org/7002024/diff/14001/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7002024/diff/14001/webkit/quota/quota_manager.... webkit/quota/quota_manager.h:131: virtual void DeleteOriginDataOnIOThread( On 2011/05/13 05:49:04, kinuko wrote: > nit: for now all the methods of QuotaManager need to be called on io thread > (it's noted in the comment at the top of this file), so maybe you can drop > OnIOThread from the method name. Done. http://codereview.chromium.org/7002024/diff/14001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/14001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:24: // Constructed in the io_thread. On 2011/05/13 05:49:04, kinuko wrote: > nit: in -> on? Done.
Added an interface QuotaEvictionHandler and modified QuotaTemporaryStorageEvictor to use it. Removed functions from QuotaManager. (They'll be in another change.)
On 2011/05/16 12:48:16, Dai Mikurube wrote: > Added an interface QuotaEvictionHandler and modified > QuotaTemporaryStorageEvictor to use it. > > Removed functions from QuotaManager. (They'll be in another change.) Extracted functions are at http://codereview.chromium.org/7029007/.
http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:25: QuotaTemporaryStorageEvictorDeleter> { I don't understand why this is refcounted much less refcounted thread safe? Doesn't the QM live on the IO thread and this just interacts with the QM. So how do other threads come into play at all? Have you seen base/timer.h... the OneShotTimer<> and RepeatingTimer<> classes? What if the QM had a MaybeEvict method that got called on a timer.
Hi Michael, http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:25: QuotaTemporaryStorageEvictorDeleter> { On 2011/05/17 00:19:03, michaeln wrote: > I don't understand why this is refcounted much less refcounted thread safe? > Doesn't the QM live on the IO thread and this just interacts with the QM. So how > do other threads come into play at all? In the previous version, it was running on the db_thread and io_thread. This patch set currently runs only on the io_thread, but the patch set doesn't change it. The set was just for separating out QuotaTemporaryStorageEvictor. > Have you seen base/timer.h... the OneShotTimer<> and RepeatingTimer<> classes? > What if the QM had a MaybeEvict method that got called on a timer. Hmm, thanks! I'll be taking a look at them.
On 2011/05/17 05:02:13, Dai Mikurube wrote: > Hi Michael, > > http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... > File webkit/quota/quota_temporary_storage_evictor.h (right): > > http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... > webkit/quota/quota_temporary_storage_evictor.h:25: > QuotaTemporaryStorageEvictorDeleter> { > On 2011/05/17 00:19:03, michaeln wrote: > > I don't understand why this is refcounted much less refcounted thread safe? > > Doesn't the QM live on the IO thread and this just interacts with the QM. So > how > > do other threads come into play at all? > > In the previous version, it was running on the db_thread and io_thread. This > patch set currently runs only on the io_thread, but the patch set doesn't change > it. The set was just for separating out QuotaTemporaryStorageEvictor. > > > Have you seen base/timer.h... the OneShotTimer<> and RepeatingTimer<> classes? > > What if the QM had a MaybeEvict method that got called on a timer. > > Hmm, thanks! I'll be taking a look at them. I think the current PostDelayedTask is better than RepeatedTimer since we'd like to make sure that the next eviction doesn't occur before the current eviction ends. OneShotTimer might be an option for us, but it looks like having not so larger benefit than PostDelayedTask...
On 2011/05/17 05:39:03, Dai Mikurube wrote: > On 2011/05/17 05:02:13, Dai Mikurube wrote: > > Hi Michael, > > > > > http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... > > File webkit/quota/quota_temporary_storage_evictor.h (right): > > > > > http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... > > webkit/quota/quota_temporary_storage_evictor.h:25: > > QuotaTemporaryStorageEvictorDeleter> { > > On 2011/05/17 00:19:03, michaeln wrote: > > > I don't understand why this is refcounted much less refcounted thread safe? > > > Doesn't the QM live on the IO thread and this just interacts with the QM. So > > how > > > do other threads come into play at all? > > > > In the previous version, it was running on the db_thread and io_thread. This > > patch set currently runs only on the io_thread, but the patch set doesn't > change > > it. The set was just for separating out QuotaTemporaryStorageEvictor. > > > > > Have you seen base/timer.h... the OneShotTimer<> and RepeatingTimer<> > classes? > > > What if the QM had a MaybeEvict method that got called on a timer. > > > > Hmm, thanks! I'll be taking a look at them. > > I think the current PostDelayedTask is better than RepeatedTimer since we'd like > to make sure that the next eviction doesn't occur before the current eviction > ends. > > OneShotTimer might be an option for us, but it looks like having not so larger > benefit than PostDelayedTask... I looked at the timer code since I needed it for delayed transaction task for my other DB patch, and the timer looks to be a really nicely done wrapper for DelayedTask to me. If re-entrancy issue is the main concern we could simply guard the main entry point (like MayStartEviction()) like: if (timer_.IsRunning()) return; timer_.Start(time_delta, this, &QuotaManager::Evict); Then we can call MayStartEviction whenever we think we may need an eviction sometime later (I mean if we go with OneShotTimer). I think it would simplify the code a lot (while it means it'd better fit without a separate class). Wdyt?
I believe this code is largely 'under construction' since I asked you to split several changes from this one, but anyway took a look at this one as I wanted to see what's needed to be done. I think we're in a good shape now, all we have to do now is mainly chaining the things you've finished so far right? It looks like we're close now. http://codereview.chromium.org/7002024/diff/23001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/23001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:36: void QuotaTemporaryStorageEvictor::RegisterQuotaManagerOnIOThread( As we talked locally before I'd like to avoid this 'registration' stuff (regardless of whether we use a separate class or not) http://codereview.chromium.org/7002024/diff/23001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:63: this, &QuotaTemporaryStorageEvictor::Evict, true)); Any reason we don't directory call Evict here? (Just asking, you wanted to avoid to run a long-running task?) http://codereview.chromium.org/7002024/diff/23001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:85: &QuotaTemporaryStorageEvictor::CallGetUsageAndQuotaOnIOThread)); We can just chain to call mgr->GetUsageAndQuotaForEviction here? http://codereview.chromium.org/7002024/diff/23001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:119: &QuotaTemporaryStorageEvictor::CallGetUsageAndQuotaOnIOThread)); ditto
Thank you for earlier comments. I'll try using Timer later. http://codereview.chromium.org/7002024/diff/23001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/23001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:36: void QuotaTemporaryStorageEvictor::RegisterQuotaManagerOnIOThread( On 2011/05/19 03:04:13, kinuko wrote: > As we talked locally before I'd like to avoid this 'registration' stuff > (regardless of whether we use a separate class or not) Yes, I'll get rid of it. http://codereview.chromium.org/7002024/diff/23001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:63: this, &QuotaTemporaryStorageEvictor::Evict, true)); On 2011/05/19 03:04:13, kinuko wrote: > Any reason we don't directory call Evict here? (Just asking, you wanted to > avoid to run a long-running task?) It's just a remnant from the days when the Evictor was working on the db_thread at large. It'll be changed, but considering to avoid a long-running task might be an option. The same for other points.
On 2011/05/19 02:07:45, kinuko wrote: > On 2011/05/17 05:39:03, Dai Mikurube wrote: > > On 2011/05/17 05:02:13, Dai Mikurube wrote: > > > Hi Michael, > > > > > > > > > http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... > > > File webkit/quota/quota_temporary_storage_evictor.h (right): > > > > > > > > > http://codereview.chromium.org/7002024/diff/20002/webkit/quota/quota_temporar... > > > webkit/quota/quota_temporary_storage_evictor.h:25: > > > QuotaTemporaryStorageEvictorDeleter> { > > > On 2011/05/17 00:19:03, michaeln wrote: > > > > I don't understand why this is refcounted much less refcounted thread > safe? > > > > Doesn't the QM live on the IO thread and this just interacts with the QM. > So > > > how > > > > do other threads come into play at all? > > > > > > In the previous version, it was running on the db_thread and io_thread. > This > > > patch set currently runs only on the io_thread, but the patch set doesn't > > change > > > it. The set was just for separating out QuotaTemporaryStorageEvictor. > > > > > > > Have you seen base/timer.h... the OneShotTimer<> and RepeatingTimer<> > > classes? > > > > What if the QM had a MaybeEvict method that got called on a timer. > > > > > > Hmm, thanks! I'll be taking a look at them. > > > > I think the current PostDelayedTask is better than RepeatedTimer since we'd > like > > to make sure that the next eviction doesn't occur before the current eviction > > ends. > > > > OneShotTimer might be an option for us, but it looks like having not so larger > > benefit than PostDelayedTask... > > I looked at the timer code since I needed it for delayed transaction task for my > other DB patch, and the timer looks to be a really nicely done wrapper for > DelayedTask to me. If re-entrancy issue is the main concern we could simply > guard the main entry point (like MayStartEviction()) like: > > if (timer_.IsRunning()) > return; > timer_.Start(time_delta, this, &QuotaManager::Evict); > > Then we can call MayStartEviction whenever we think we may need an eviction > sometime later (I mean if we go with OneShotTimer). > I think it would simplify the code a lot (while it means it'd better fit without > a separate class). Wdyt? Excuse me, I don't get your point. By the example, you mean calling MayStartEviction() at every place where we may need an eviction (e.g. at any write operation) and stopping periodical eviction?
Cleaned-up the code with using OneShotTimer. It's not RefCounted now.
Thanks, some (next round of) earlier comments. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:18: : physical_available_space_to_be_evicted(1000 * 1000 * 500), nit: to be evicted -> to start eviction ? (from the viewpoint of evictor) Can this simply be a const var like kUsageRatioToBeEvicted? http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:20: target_available_space_(target_available_space), not used now? http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:21: delay_ms_(delay_ms), interval_ms_? (since this is used for repeated eviction) Could this be a const as well? http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:24: callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) { I guess now we can DCHECK(quota_eviction_handler) here and remove all the "if (quota_eviction_handler)" guards from the code? http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:30: void QuotaTemporaryStorageEvictor::OnQuotaManagerDestroyedOnIOThread() { Now we can happily get rid of this. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:51: repeated_eviction_ = false; why do we flip the flag here? GetLRUOrigin may return false if all of the registered origins are in use. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:77: } else if (repeated_eviction_) { Maybe if we get status != kQuotaStatusOk more than a certain number we should stop eviction? http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:96: void QuotaTemporaryStorageEvictor::Start() { Do we need this func? (I guess we could either call MayStartEviction or GetUsageAndQuotaThenEvict)
Before reply, I've modified a unit test.
Thanks. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:18: : physical_available_space_to_be_evicted(1000 * 1000 * 500), On 2011/05/19 09:27:18, kinuko wrote: > nit: to be evicted -> to start eviction ? (from the viewpoint of evictor) > > Can this simply be a const var like kUsageRatioToBeEvicted? Renamed them to: static const double kUsageRatioToStartEvictiony; const int64 kAvailableDiskSpaceToStartEviction; http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:20: target_available_space_(target_available_space), On 2011/05/19 09:27:18, kinuko wrote: > not used now? Done. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:21: delay_ms_(delay_ms), On 2011/05/19 09:27:18, kinuko wrote: > interval_ms_? (since this is used for repeated eviction) > > Could this be a const as well? My thought was that it might be changed at run-time with adding some interface like set_interval_ms[_from_next_eviction](). http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:24: callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) { On 2011/05/19 09:27:18, kinuko wrote: > I guess now we can DCHECK(quota_eviction_handler) here and remove all the "if > (quota_eviction_handler)" guards from the code? Done. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:30: void QuotaTemporaryStorageEvictor::OnQuotaManagerDestroyedOnIOThread() { On 2011/05/19 09:27:18, kinuko wrote: > Now we can happily get rid of this. Done. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:51: repeated_eviction_ = false; On 2011/05/19 09:27:18, kinuko wrote: > why do we flip the flag here? > > GetLRUOrigin may return false if all of the registered origins are in use. Oh, I wrongly edited this line. MayStartEviction(...) correctly. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:77: } else if (repeated_eviction_) { On 2011/05/19 09:27:18, kinuko wrote: > Maybe if we get status != kQuotaStatusOk more than a certain number we should > stop eviction? We should discuss error handling for the entire eviction. My current thought is: * Error in getting usage"a => stop if many errors? * Error in eviction => ignore the origin in the next time? http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:96: void QuotaTemporaryStorageEvictor::Start() { On 2011/05/19 09:27:18, kinuko wrote: > Do we need this func? (I guess we could either call MayStartEviction or > GetUsageAndQuotaThenEvict) Hmm, MayStartEviction and GetUsageAndQuotaThenEvict looked not so good name for public interface. Thinking about another name.
getting closer I think http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:21: delay_ms_(delay_ms), On 2011/05/20 02:42:24, Dai Mikurube wrote: > On 2011/05/19 09:27:18, kinuko wrote: > > interval_ms_? (since this is used for repeated eviction) > > > > Could this be a const as well? > > My thought was that it might be changed at run-time with adding some interface > like set_interval_ms[_from_next_eviction](). Ok if we change the value it makes sense. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:77: } else if (repeated_eviction_) { On 2011/05/20 02:42:24, Dai Mikurube wrote: > On 2011/05/19 09:27:18, kinuko wrote: > > Maybe if we get status != kQuotaStatusOk more than a certain number we should > > stop eviction? > > We should discuss error handling for the entire eviction. > > My current thought is: > * Error in getting usage"a => stop if many errors? > * Error in eviction => ignore the origin in the next time? sgtm. For the latter case we may also want to remove it from the eviction candidates if we get too many errors for a particular origin. If it's the oldest one we might be trying to evict the origin forever. (E.g. Like keeping set<GURL> error_origins_ in the QM and adding them to the exceptions when we call GetLRUOrigin) Wdyt? http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:35: const int64 kAvailableDiskSpaceToStartEviction; This is not static since you imagine we may give different value per instance? If so I think we usually use our usual lower_with_underscore_ naming even for constant vars. Otherwise this could be simply a static const var? http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:51: QuotaEvictionHandler* quota_eviction_handler_; Can we add a comment like "Not owned; eviction_handler owns us." ? http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:55: int64 usage() { For this I may expect the function name to be GetUsage() rather than usage() since it's not a simple getter. (I know in some other tests we/I break this naming rule but in general...) http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:69: void access_origin(const GURL& origin) { AccessOrigin ? (Since it's not a simple getter/setter) http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:73: void add_pseudo_origin(const GURL& origin, int64 usage) { ditto. http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:80: bool remove_origin(const GURL& origin) { ditto. http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:150: LOG(ERROR) << quota_eviction_handler()->usage(); These LOG(ERROR) will eventually be replaced by some EXPECT_XX? http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:169: Maybe later we could add the same set of tests using a real QM with MockStorageClient? If we could keep both tests it'd be easier to separate problems.
http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:96: void QuotaTemporaryStorageEvictor::Start() { On 2011/05/20 02:42:24, Dai Mikurube wrote: > On 2011/05/19 09:27:18, kinuko wrote: > > Do we need this func? (I guess we could either call MayStartEviction or > > GetUsageAndQuotaThenEvict) > > Hmm, MayStartEviction and GetUsageAndQuotaThenEvict looked not so good name for > public interface. Thinking about another name. First I thought GetUsageAndQuotaThenEvict could be just called Evict but it might be confusing as we also have DoEvict. Simply MayStartEviction() and StartEviction()?
Thank you. http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/28001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:96: void QuotaTemporaryStorageEvictor::Start() { On 2011/05/20 06:43:49, kinuko wrote: > On 2011/05/20 02:42:24, Dai Mikurube wrote: > > On 2011/05/19 09:27:18, kinuko wrote: > > > Do we need this func? (I guess we could either call MayStartEviction or > > > GetUsageAndQuotaThenEvict) > > > > Hmm, MayStartEviction and GetUsageAndQuotaThenEvict looked not so good name > for > > public interface. Thinking about another name. > > First I thought GetUsageAndQuotaThenEvict could be just called Evict but it > might be confusing as we also have DoEvict. > > Simply MayStartEviction() and StartEviction()? Finally, you say which should be MayStartEviction(), which should be StartEviction()? GetUsageAndQuotaThenEvict() => StartEviction() and Start() => removed? http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:35: const int64 kAvailableDiskSpaceToStartEviction; On 2011/05/20 06:41:14, kinuko wrote: > This is not static since you imagine we may give different value per instance? > If so I think we usually use our usual lower_with_underscore_ naming even for > constant vars. Otherwise this could be simply a static const var? Hmm, I guess I did two mistakes : 1. I read your previous comments just to rename it. But it looks like you didn't say so. 2. I (wrongly) found all constant variables should start with 'k' at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names . But it looks like saying "All *compile-time* constants..." by reading precisely. Are they correct? I guess we've discussed to make this variable (which contains the threshold of available disk space to start eviction) changeable by a kind of environment. http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:51: QuotaEvictionHandler* quota_eviction_handler_; On 2011/05/20 06:41:14, kinuko wrote: > Can we add a comment like "Not owned; eviction_handler owns us." ? Done. http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:55: int64 usage() { On 2011/05/20 06:41:14, kinuko wrote: > For this I may expect the function name to be GetUsage() rather than usage() > since it's not a simple getter. > > (I know in some other tests we/I break this naming rule but in general...) Done. http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:69: void access_origin(const GURL& origin) { On 2011/05/20 06:41:14, kinuko wrote: > AccessOrigin ? (Since it's not a simple getter/setter) Done. http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:73: void add_pseudo_origin(const GURL& origin, int64 usage) { On 2011/05/20 06:41:14, kinuko wrote: > ditto. Done. http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:80: bool remove_origin(const GURL& origin) { On 2011/05/20 06:41:14, kinuko wrote: > ditto. Done. http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:150: LOG(ERROR) << quota_eviction_handler()->usage(); On 2011/05/20 06:41:14, kinuko wrote: > These LOG(ERROR) will eventually be replaced by some EXPECT_XX? Of course, they're just for local execution. Just uploaded to share the current test logic. http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:169: On 2011/05/20 06:41:14, kinuko wrote: > Maybe later we could add the same set of tests using a real QM with > MockStorageClient? If we could keep both tests it'd be easier to separate > problems. I guess such test cases should be run in another test set like http://codereview.chromium.org/6905095/. They look like not unit tests.
Some nit fix and rebased. Not ready for the final review. It looks like required to add some test.
(I'll take another look at the new changes/tests later) http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:35: const int64 kAvailableDiskSpaceToStartEviction; On 2011/05/20 07:36:54, Dai Mikurube wrote: > On 2011/05/20 06:41:14, kinuko wrote: > > This is not static since you imagine we may give different value per instance? > > > If so I think we usually use our usual lower_with_underscore_ naming even for > > constant vars. Otherwise this could be simply a static const var? > > Hmm, I guess I did two mistakes : > 1. I read your previous comments just to rename it. But it looks like you > didn't say so. > 2. I (wrongly) found all constant variables should start with 'k' at > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names . > But it looks like saying "All *compile-time* constants..." by reading precisely. Yeah the detailed rule is hidden in the expandable section. > Are they correct? I think so. > I guess we've discussed to make this variable (which contains the threshold of > available disk space to start eviction) changeable by a kind of environment. Ok let me try to make the points clearer: * I think we'd better use compile-time constant for the magic number (1000 * 1000 * 500). (This is what I wanted to say in the 1st comment) * Giving the fact that we may want to change the min-available-disk-space-to-start-eviction, how about: - call the compile-time constant 'default' something, like kDefaultMinAvailableDiskSpaceToStartEviction - change the member variable available_disk_space_to_start_eviction_ (please append trailing '_') back to non-const - initialize the member variable with the constant in the constructor. Does that make sense? http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:169: On 2011/05/20 07:36:54, Dai Mikurube wrote: > On 2011/05/20 06:41:14, kinuko wrote: > > Maybe later we could add the same set of tests using a real QM with > > MockStorageClient? If we could keep both tests it'd be easier to separate > > problems. > > I guess such test cases should be run in another test set like > http://codereview.chromium.org/6905095/. They look like not unit tests. Yes of course we could add them as a new test (neither did I mean we should add them into this changeset).
http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:35: const int64 kAvailableDiskSpaceToStartEviction; On 2011/05/20 08:27:02, kinuko wrote: > On 2011/05/20 07:36:54, Dai Mikurube wrote: > > On 2011/05/20 06:41:14, kinuko wrote: > > > This is not static since you imagine we may give different value per > instance? > > > > > If so I think we usually use our usual lower_with_underscore_ naming even > for > > > constant vars. Otherwise this could be simply a static const var? > > > > Hmm, I guess I did two mistakes : > > 1. I read your previous comments just to rename it. But it looks like you > > didn't say so. > > 2. I (wrongly) found all constant variables should start with 'k' at > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names > . > > But it looks like saying "All *compile-time* constants..." by reading > precisely. > > Yeah the detailed rule is hidden in the expandable section. > > > Are they correct? > > I think so. > > > I guess we've discussed to make this variable (which contains the threshold of > > available disk space to start eviction) changeable by a kind of environment. > > Ok let me try to make the points clearer: > * I think we'd better use compile-time constant for the magic number (1000 * > 1000 * 500). (This is what I wanted to say in the 1st comment) > * Giving the fact that we may want to change the > min-available-disk-space-to-start-eviction, how about: > - call the compile-time constant 'default' something, like > kDefaultMinAvailableDiskSpaceToStartEviction > - change the member variable available_disk_space_to_start_eviction_ (please > append trailing '_') back to non-const > - initialize the member variable with the constant in the constructor. > > Does that make sense? Ah, make sense. It was just a temporary value to run it as trial (not ready for review/commit) I had to have a discussion about "what is the initial value for that?" I agree with your suggestion. I'll fix this. # I might be happy if you could avoid using pronouns...
http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/33006/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:35: const int64 kAvailableDiskSpaceToStartEviction; On 2011/05/20 09:33:07, Dai Mikurube wrote: > On 2011/05/20 08:27:02, kinuko wrote: > > On 2011/05/20 07:36:54, Dai Mikurube wrote: > > > On 2011/05/20 06:41:14, kinuko wrote: > > > > This is not static since you imagine we may give different value per > > instance? > > > > > > > If so I think we usually use our usual lower_with_underscore_ naming even > > for > > > > constant vars. Otherwise this could be simply a static const var? > > > > > > Hmm, I guess I did two mistakes : > > > 1. I read your previous comments just to rename it. But it looks like you > > > didn't say so. > > > 2. I (wrongly) found all constant variables should start with 'k' at > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names > > . > > > But it looks like saying "All *compile-time* constants..." by reading > > precisely. > > > > Yeah the detailed rule is hidden in the expandable section. > > > > > Are they correct? > > > > I think so. > > > > > I guess we've discussed to make this variable (which contains the threshold > of > > > available disk space to start eviction) changeable by a kind of environment. > > > > Ok let me try to make the points clearer: > > * I think we'd better use compile-time constant for the magic number (1000 * > > 1000 * 500). (This is what I wanted to say in the 1st comment) > > * Giving the fact that we may want to change the > > min-available-disk-space-to-start-eviction, how about: > > - call the compile-time constant 'default' something, like > > kDefaultMinAvailableDiskSpaceToStartEviction > > - change the member variable available_disk_space_to_start_eviction_ (please > > append trailing '_') back to non-const > > - initialize the member variable with the constant in the constructor. > > > > Does that make sense? > > Ah, make sense. It was just a temporary value to run it as trial (not ready for > review/commit) I had to have a discussion about "what is the initial value for > that?" > > I agree with your suggestion. I'll fix this. > > # I might be happy if you could avoid using pronouns... Will try!
http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:46: task_for_get_usage_and_quota_->Run(); don't we need to delete the task? http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:86: void AddPseudoOrigin(const GURL& origin, int64 usage) { Does this method expect the caller to call RemoveOrigin before calling this one or not? (We call RemoveOrigin before calling this in AccessOrigin() but this method (AddPseudoOrigin) also calls RemoveOrigin) How about having three distinct methods like: - AddOrigin (add the origin with its usage and insert the origin at the end of the LRU list) - RemoveOrigin (remove the origin from the origins usage list and LRU list) - UpdateOriginOrder (move the existing origin to the end of the LRU list) http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:95: if (origins_.find(origin) == origins_.end()) EXPECT_TRUE? http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:111: CancelableTask* task_for_get_usage_and_quota_; Can this task be scoped_ptr? http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:140: EXPECT_EQ(962-292, quota_eviction_handler()->GetUsage()); Please separate out these test-dependent constants (e.g. 962, 292 etc) out of the base test class (i.e. out of this QuotaTemporaryStorageEvictorTest class, which is to be used for multiple test cases). http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:214: EXPECT_EQ(962-292+280-120-150, quota_eviction_handler()->GetUsage()); EXPECT_EQ(2, num_get_usage_and_quota_for_eviction()) or something?
Thank you for the comments. http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:46: task_for_get_usage_and_quota_->Run(); On 2011/05/23 04:34:53, kinuko wrote: > don't we need to delete the task? Ah, the task should be deleted, but not here. The task runs repeatedly. It was deleted at the destructor. But scoped_ptr looks better as you say below. http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:86: void AddPseudoOrigin(const GURL& origin, int64 usage) { On 2011/05/23 04:34:53, kinuko wrote: > Does this method expect the caller to call RemoveOrigin before calling this one > or not? (We call RemoveOrigin before calling this in AccessOrigin() but this > method (AddPseudoOrigin) also calls RemoveOrigin) > > How about having three distinct methods like: > - AddOrigin (add the origin with its usage and insert the origin at the end of > the LRU list) > - RemoveOrigin (remove the origin from the origins usage list and LRU list) > - UpdateOriginOrder (move the existing origin to the end of the LRU list) Your comment includes two messages... 1) rename the methods and 2) reconstruct the methods. For 1, I agree with renaming AddPesudoOrigin. But I think AccessOrigin looks better since the method is called from test cases. The name "Access" follows the actual scenario in the test case. Web applications don't explicitly "update the order". For 2, RemoveOrigin is not required to be a public method, and it's designed just to guarantee the origin is removed. I think it can be called more than once. If you say the name RemoveOrigin is not good for that, I agree. How about EnsureOriginRemoved or like that? In addition, I modified AccessOrigin not to call RemoveOrigin. Could you describe "why" you think it's better in comments? I cannot discuss without explicit reasons. http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:95: if (origins_.find(origin) == origins_.end()) On 2011/05/23 04:34:53, kinuko wrote: > EXPECT_TRUE? As replied above, I designed this method to be called multiple times. http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:111: CancelableTask* task_for_get_usage_and_quota_; On 2011/05/23 04:34:53, kinuko wrote: > Can this task be scoped_ptr? Exactly. Done. http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:140: EXPECT_EQ(962-292, quota_eviction_handler()->GetUsage()); On 2011/05/23 04:34:53, kinuko wrote: > Please separate out these test-dependent constants (e.g. 962, 292 etc) out of > the base test class (i.e. out of this QuotaTemporaryStorageEvictorTest class, > which is to be used for multiple test cases). Done. http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:214: EXPECT_EQ(962-292+280-120-150, quota_eviction_handler()->GetUsage()); On 2011/05/23 04:34:53, kinuko wrote: > EXPECT_EQ(2, num_get_usage_and_quota_for_eviction()) or something? Done, but GetUsageAndQuotaForEviction() is called 4 times finally.
(Sorry for long comment) http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:86: void AddPseudoOrigin(const GURL& origin, int64 usage) { On 2011/05/23 05:53:37, Dai Mikurube wrote: > On 2011/05/23 04:34:53, kinuko wrote: > > Does this method expect the caller to call RemoveOrigin before calling this > one > > or not? (We call RemoveOrigin before calling this in AccessOrigin() but this > > method (AddPseudoOrigin) also calls RemoveOrigin) > > > > How about having three distinct methods like: > > - AddOrigin (add the origin with its usage and insert the origin at the end of > > the LRU list) > > - RemoveOrigin (remove the origin from the origins usage list and LRU list) > > - UpdateOriginOrder (move the existing origin to the end of the LRU list) > > Your comment includes two messages... 1) rename the methods and 2) reconstruct > the methods. > > For 1, I agree with renaming AddPesudoOrigin. But I think AccessOrigin looks > better since the method is called from test cases. The name "Access" follows > the actual scenario in the test case. Web applications don't explicitly "update > the order". Keeping 'AccessOrigin' sounds ok to me actually. Before reading what other methods do I was about to suggest renaming this 'AccessOrigin' method to 'SimulateAccessOrigin', just because when I saw 'AccessOrigin' and then saw it is calling 'RemoveOrigin' in the method I got really confused why one needs to remove the origin when it accesses the origin. Then I thought if the code was like following it would be much easier to read. void SimulateAccessOrigin(const GURL& origin) { UpdateOriginOrder(origin); } UpdateOriginOrder may not need to be a separate method as you say, but anyway this is how I started composing a comment like the previous one. A few more notes. Calling RemoveOrigin() and AddPseudoOrigin() do what UpdateOriginOrder would do, but also do something else, i.e. removing the origin usage (origins_.erase(origin)) and then re-adding the usage, and that's the subtle but big difference where I got confused. Maybe... you could have just put a short comment about that (like: 'here we remove the origin and then add it again to make sure we have the origin at the end o the LRU origin list'). Then I would have just left it as is. > For 2, RemoveOrigin is not required to be a public method, and it's designed > just to guarantee the origin is removed. I think it can be called more than > once. > > If you say the name RemoveOrigin is not good for that, I agree. How about > EnsureOriginRemoved or like that? Sounds good, this type of change would improve readability a lot. > In addition, I modified AccessOrigin not to call RemoveOrigin. In the new code AccessOrigin doesn't reorder the LRU origin list if the given origin is already in the origins_ map... is it expected? > Could you describe "why" you think it's better in comments? I cannot discuss > without explicit reasons. In the previous/current code some methods seem to be doing multiple things or doing something else than what one would expect by the method name, and which made me feel the code may not be well-defined (or I might be just mistaken something). For example in the previous/current code AddOrigin() performs not only adding a new origin but also makeing sure the given origin is at the end of LRU list. So I wanted to make what each method does clearer. If we keep the current code probably we could come up with a better name like you did for RemoveOrigin. Or maybe we could just add a comment about that. (But still maybe it'd be better named InsertOrigin or something to imply it does 'insert' like thing in an ordererd set) Btw just to make sure please do not take my word as 'do this instead of what you do' type comment. All I want is to make following things clear: - what each method is assumed to perform - if the method name implies what it does - otherwise if the code has some comments so that readers can understand what and why it's doing
Thank you for the descriptive comments. It's so helpful for me. :) http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:86: void AddPseudoOrigin(const GURL& origin, int64 usage) { On 2011/05/23 07:29:51, kinuko wrote: > On 2011/05/23 05:53:37, Dai Mikurube wrote: > > On 2011/05/23 04:34:53, kinuko wrote: > > > Does this method expect the caller to call RemoveOrigin before calling this > > one > > > or not? (We call RemoveOrigin before calling this in AccessOrigin() but > this > > > method (AddPseudoOrigin) also calls RemoveOrigin) > > > > > > How about having three distinct methods like: > > > - AddOrigin (add the origin with its usage and insert the origin at the end > of > > > the LRU list) > > > - RemoveOrigin (remove the origin from the origins usage list and LRU list) > > > - UpdateOriginOrder (move the existing origin to the end of the LRU list) > > > > Your comment includes two messages... 1) rename the methods and 2) reconstruct > > the methods. > > > > For 1, I agree with renaming AddPesudoOrigin. But I think AccessOrigin looks > > better since the method is called from test cases. The name "Access" follows > > the actual scenario in the test case. Web applications don't explicitly > "update > > the order". > > Keeping 'AccessOrigin' sounds ok to me actually. > > Before reading what other methods do I was about to suggest renaming this > 'AccessOrigin' method to 'SimulateAccessOrigin', just because when I saw > 'AccessOrigin' and then saw it is calling 'RemoveOrigin' in the method I got > really confused why one needs to remove the origin when it accesses the origin. > Then I thought if the code was like following it would be much easier to read. > > void SimulateAccessOrigin(const GURL& origin) { > UpdateOriginOrder(origin); > } > > UpdateOriginOrder may not need to be a separate method as you say, but anyway > this is how I started composing a comment like the previous one. > > A few more notes. Calling RemoveOrigin() and AddPseudoOrigin() do what > UpdateOriginOrder would do, but also do something else, i.e. removing the origin > usage (origins_.erase(origin)) and then re-adding the usage, and that's the > subtle but big difference where I got confused. > > Maybe... you could have just put a short comment about that (like: 'here we > remove the origin and then add it again to make sure we have the origin at the > end o the LRU origin list'). Then I would have just left it as is. Ok, finally, I added such comments at AccessOrigin() and AddOrigin(). > > For 2, RemoveOrigin is not required to be a public method, and it's designed > > just to guarantee the origin is removed. I think it can be called more than > > once. > > > > If you say the name RemoveOrigin is not good for that, I agree. How about > > EnsureOriginRemoved or like that? > > Sounds good, this type of change would improve readability a lot. Renamed it. > > In addition, I modified AccessOrigin not to call RemoveOrigin. > > In the new code AccessOrigin doesn't reorder the LRU origin list if the given > origin is already in the origins_ map... is it expected? Really? I think the new code reorders the list. If the given origin is not in the map, reordering has no meaning. > > Could you describe "why" you think it's better in comments? I cannot discuss > > without explicit reasons. > > In the previous/current code some methods seem to be doing multiple things or > doing something else than what one would expect by the method name, and which > made me feel the code may not be well-defined (or I might be just mistaken > something). For example in the previous/current code AddOrigin() performs not > only adding a new origin but also makeing sure the given origin is at the end of > LRU list. > > So I wanted to make what each method does clearer. If we keep the current code > probably we could come up with a better name like you did for RemoveOrigin. > > Or maybe we could just add a comment about that. (But still maybe it'd be > better named InsertOrigin or something to imply it does 'insert' like thing in > an ordererd set) Finally, just added a comment as written above. But, sorry, I could not understand the difference between 'insert' and 'add' here... > Btw just to make sure please do not take my word as 'do this instead of what you > do' type comment. Yes, thanks. What I'd like to say was: I have to imagine "what is the non-good point you say" to show another solution if no reasons are given. When I cannot imagine, I have to do just as you say... > All I want is to make following things clear: > - what each method is assumed to perform > - if the method name implies what it does > - otherwise if the code has some comments so that readers can understand what > and why it's doing Finally added comment for public methods to describe they are to 'simulate' accessing and adding. I think "Simulate-" is not required in the method names since simulating is natural since MockQuotaEvictionHandler is a Mock- class.
(long comment again, trying to be explicit) http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:86: void AddPseudoOrigin(const GURL& origin, int64 usage) { On 2011/05/23 09:50:37, Dai Mikurube wrote: > On 2011/05/23 07:29:51, kinuko wrote: > > On 2011/05/23 05:53:37, Dai Mikurube wrote: > > > On 2011/05/23 04:34:53, kinuko wrote: > > > > Does this method expect the caller to call RemoveOrigin before calling > this > > > one > > > > or not? (We call RemoveOrigin before calling this in AccessOrigin() but > > this > > > > method (AddPseudoOrigin) also calls RemoveOrigin) > > > > > > > > How about having three distinct methods like: > > > > - AddOrigin (add the origin with its usage and insert the origin at the > end > > of > > > > the LRU list) > > > > - RemoveOrigin (remove the origin from the origins usage list and LRU > list) > > > > - UpdateOriginOrder (move the existing origin to the end of the LRU list) > > > > > > Your comment includes two messages... 1) rename the methods and 2) > reconstruct > > > the methods. > > > > > > For 1, I agree with renaming AddPesudoOrigin. But I think AccessOrigin > looks > > > better since the method is called from test cases. The name "Access" > follows > > > the actual scenario in the test case. Web applications don't explicitly > > "update > > > the order". > > > > Keeping 'AccessOrigin' sounds ok to me actually. > > > > Before reading what other methods do I was about to suggest renaming this > > 'AccessOrigin' method to 'SimulateAccessOrigin', just because when I saw > > 'AccessOrigin' and then saw it is calling 'RemoveOrigin' in the method I got > > really confused why one needs to remove the origin when it accesses the > origin. > > Then I thought if the code was like following it would be much easier to read. > > > > void SimulateAccessOrigin(const GURL& origin) { > > UpdateOriginOrder(origin); > > } > > > > UpdateOriginOrder may not need to be a separate method as you say, but anyway > > this is how I started composing a comment like the previous one. > > > > A few more notes. Calling RemoveOrigin() and AddPseudoOrigin() do what > > UpdateOriginOrder would do, but also do something else, i.e. removing the > origin > > usage (origins_.erase(origin)) and then re-adding the usage, and that's the > > subtle but big difference where I got confused. > > > > Maybe... you could have just put a short comment about that (like: 'here we > > remove the origin and then add it again to make sure we have the origin at the > > end o the LRU origin list'). Then I would have just left it as is. > > Ok, finally, I added such comments at AccessOrigin() and AddOrigin(). > > > > > For 2, RemoveOrigin is not required to be a public method, and it's designed > > > just to guarantee the origin is removed. I think it can be called more than > > > once. > > > > > > If you say the name RemoveOrigin is not good for that, I agree. How about > > > EnsureOriginRemoved or like that? > > > > Sounds good, this type of change would improve readability a lot. > > Renamed it. > > > > > In addition, I modified AccessOrigin not to call RemoveOrigin. > > > > In the new code AccessOrigin doesn't reorder the LRU origin list if the given > > origin is already in the origins_ map... is it expected? > > Really? I think the new code reorders the list. If the given origin is not in > the map, reordering has no meaning. Apparently this was wrong, please nvm. > > > Could you describe "why" you think it's better in comments? I cannot > discuss > > > without explicit reasons. > > > > In the previous/current code some methods seem to be doing multiple things or > > doing something else than what one would expect by the method name, and which > > made me feel the code may not be well-defined (or I might be just mistaken > > something). For example in the previous/current code AddOrigin() performs not > > only adding a new origin but also makeing sure the given origin is at the end > of > > LRU list. > > > > So I wanted to make what each method does clearer. If we keep the current > code > > probably we could come up with a better name like you did for RemoveOrigin. > > > > Or maybe we could just add a comment about that. (But still maybe it'd be > > better named InsertOrigin or something to imply it does 'insert' like thing in > > an ordererd set) > > Finally, just added a comment as written above. But, sorry, I could not > understand the difference between 'insert' and 'add' here... > > > > Btw just to make sure please do not take my word as 'do this instead of what > you > > do' type comment. > > Yes, thanks. What I'd like to say was: I have to imagine "what is the non-good > point you say" to show another solution if no reasons are given. When I cannot > imagine, I have to do just as you say... > > > > All I want is to make following things clear: > > - what each method is assumed to perform > > - if the method name implies what it does > > - otherwise if the code has some comments so that readers can understand what > > and why it's doing > > Finally added comment for public methods to describe they are to 'simulate' > accessing and adding. I think "Simulate-" is not required in the method names > since simulating is natural since MockQuotaEvictionHandler is a Mock- class. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:87: RemoveOrigin(origin); EXPECT_TRUE? http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:145: break; Is this method meant to be a part of RepeatedEvictionTest, a part of verifier of the test? Why are we using Task/CancelableTask for this? If we use task can't we also pass the expected values when we create a new task in RepeatedEvictionTest rather than using global static vars? (With task we can bind variables that are passed as parameters when it runs the method) Or we might be able to do something like this: class MockQuotaEvictionHelper { public: virtual void GetUsageAndQuotaForEviction( GetUsageAndQuotaForEvictionCallback* callback) OVERRIDE { intermediate_usage_results_.push_back(GetUsage()); if (++current_repeat_count_ >= max_repeat_count_) set_repeated_eviction(false); // ... } void set_max_repeat_count(int max) { max_repeat_count_ = max; } // Later we could verify the results using this accessor. const std::vector<int64>& intermediate_usage_results() const { return intermediate_usage_results_; } private: int current_repeat_count_; int max_repeat_count_; std::vector<int64> intermediate_usage_results_; // ... }; Or maybe we could have a separate EvictionHandler class (like MockQuotaEvictionHandlerForRepeatedTest) only for this test case, make it derive from MockQuotaEvictionHelper and override the GetUsageAndQuotaForEviction implementation to do something you did in TaskForRepeatTestAfterGetUsageAndQuota. Or... whatever. anyways I'd like expected values for a test case to be defined in the test case code. Then we could easily add similar tests later using the same code. Can we somehow do that? http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:152: } Shouldn't we have a separate test case for the cases like this (i.e. calling AddOrigin() / AccessOrigin() between two eviction rounds etc)? I mean, could we have a test that runs one eviction round without setting set_repeated_eviction true, adds and/or accesses origin, verifies something and then calls another round of eviction? If we mix various cases I'm afraid the test code gets complicated and hard to parameterize.
http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_manager.... webkit/quota/quota_manager.h:40: virtual ~QuotaEvictionHandler() {} Does this need to be in the public interface? http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:32: void QuotaTemporaryStorageEvictor::OnEvictionCompleted( Please define the methods in the .cc file in the same order as they are declared in the .h file. I think that misordering may have contributed to why it was difficult to follow. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:29: bool repeated_eviction() const { return repeated_eviction_; } I understand what Start() means, but what does repeated_eviction mean? It doesn't seem to mean the Evictor should repeatedly consider what to evict next as that seems to be the case regardless of this flag. When and why would this bool attribute be get/set by the QuotaManager? http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:36: const int64 min_available_disk_space_to_start_eviction_; Do these belong in the public interface? http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:50: void OnEvictionCompleted(QuotaStatusCode status); I found these method names to be difficult to follow. They're mostly used as callbacks to QM methods and it's not clear which is called back from what. Are these names an improvement? void StartEvictionTimer() { timer_.Start(kEvictionInterval, &ConsiderEviction); } void ConsiderEviction() { // Get usage and disk space, then continue. mgr->GetUsageAndQuotaForEviction(callback_factory_. NewCallback(&OnGotUsageAndQuotaForEviction)) } void OnGotUsageAndQuotaForEviction( QuotaStatusCode status, int64 usage, int64 quota, int64 free_disk_space) { const double kUsageRatioToStartEviction = 0.7; const int64 kDiskSpaceToKeepFree = 1000 * 1000 * 500; if (status == kQuotaStatusOk && (usage > quota * kUsageRatioToStartEviction || available_disk_space < kDiskSpaceToKeepFree )) { // Space is getting tight. Get the least recently used origin and continue. mgr->GetLRUOrigin(kStorageTypeTemporary, callback_factory_.NewCallback( &OnGotLRUOrigin)); return; } // No action required, sleep for a while check again later. StartEvictionTimer(); } void OnGotLRUOrigin(const GURL& origin) { if (origin.is_empty()) { StartEvictionTimer(); return; } mgr->EvictOriginData(origin, kStorageTypeTemporary, callback_factory_.NewCallback(&OnEvictionComplete)); } void OnEvictionComplete(QuotaStatusCode status) { if (status != OK) { StartEvictionTimer(); return; } // We many need to get rid of more space so reconsider immediately. ConsiderEviction(); } http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:27: int64 e_size = 275; These look like they should be const values? const int64 kSizeA http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:49: task_for_get_usage_and_quota_->Run(); The indirection using a CancelableTask seems overly complicated. Maybe put that logic directly into your MockHandler instead.
Replying to Kinuko-san at first. Thank you for the detailed comments. That helped me understanding :) FYI, at least for me, frank words are easy to understand than code words. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:87: if (found != origins_.end()) On 2011/05/23 12:10:30, kinuko wrote: > EXPECT_TRUE? Done. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:145: void TaskForRepeatTestAfterGetUsageAndQuota() { Thank you for listing up our many options. They helped me to compare pros/cons of our choices so much! In my understanding, you say : * The bad point : the expected values are separated from the test case. * To be satisfied : the expected values should be declared at the test case, even if they are given to the other functions and tasks later. Are these correct? On 2011/05/23 12:10:30, kinuko wrote: > Is this method meant to be a part of RepeatedEvictionTest, a part of verifier of > the test? > > Why are we using Task/CancelableTask for this? If we use task can't we also > pass the expected values when we create a new task in RepeatedEvictionTest > rather than using global static vars? > (With task we can bind variables that are passed as parameters when it runs the > method) Finally, I chose this option because it can run some additional things, such as AddOrigin(GURL("http://www.e.com"), ...), per test case. It is required to test repeated eviction. > Or we might be able to do something like this: > > class MockQuotaEvictionHelper { > public: > virtual void GetUsageAndQuotaForEviction( > GetUsageAndQuotaForEvictionCallback* callback) OVERRIDE { > intermediate_usage_results_.push_back(GetUsage()); > if (++current_repeat_count_ >= max_repeat_count_) > set_repeated_eviction(false); > // ... > } > > void set_max_repeat_count(int max) { max_repeat_count_ = max; } > > // Later we could verify the results using this accessor. > const std::vector<int64>& intermediate_usage_results() const { > return intermediate_usage_results_; > } > > private: > int current_repeat_count_; > int max_repeat_count_; > std::vector<int64> intermediate_usage_results_; > // ... > }; I didn't choose this because it cannot run additional tasks per test case, as described above. > Or maybe we could have a separate EvictionHandler class (like > MockQuotaEvictionHandlerForRepeatedTest) only for this test case, make it derive > from MockQuotaEvictionHelper and override the GetUsageAndQuotaForEviction > implementation to do something you did in > TaskForRepeatTestAfterGetUsageAndQuota. It looked like a reasonable choice, but I didn't choose this since : * it looked a little overkilling, and * there are not so large difference from the first options, from the viewpoint of test-case separation. It requires some EXPECT_EQ (or logging values), AddOrigin() and set_repeated_eviction() in MockQuotaEvictionHandlerForRepeatedTest::GetUsageAndQuotaForEviction() which is separated from the test case "RepeatedEvictionTest". The difference from the second option is small in comparison to it's overkilling. Many options and examples help us very much to discuss the details. Thank you. I guess just "what is the bad point" and "what should be satisfied at least" are sometimes enough and helpful to understand comments than "how about that?" or "can we?". > Or... whatever. anyways I'd like expected values for a test case to be defined > in the test case code. Then we could easily add similar tests later using the > same code. Can we somehow do that? http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:152: quota_eviction_handler()->AccessOrigin(GURL("http://www.c.com")); On 2011/05/23 12:10:30, kinuko wrote: > Shouldn't we have a separate test case for the cases like this (i.e. calling > AddOrigin() / AccessOrigin() between two eviction rounds etc)? I mean, could we > have a test that runs one eviction round without setting set_repeated_eviction > true, adds and/or accesses origin, verifies something and then calls another > round of eviction? > > If we mix various cases I'm afraid the test code gets complicated and hard to > parameterize. What do you mean by "a test case" and "a test" ? SimpleEvictionTest does "a test that runs one eviction round without setting set_repeated_eviction true". RepeatedEvictionTest does "accesses origin, verifies something and then calls another round of eviction". What is the bad point you mean? BTW, I didn't choose the third option in the above comment as described there.
Thank you for the comments, Michael. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:32: void QuotaTemporaryStorageEvictor::OnEvictionCompleted( On 2011/05/23 19:53:47, michaeln wrote: > Please define the methods in the .cc file in the same order as they are declared > in the .h file. I think that misordering may have contributed to why it was > difficult to follow. Exactly. Reordered them. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:29: bool repeated_eviction() const { return repeated_eviction_; } On 2011/05/23 19:53:47, michaeln wrote: > I understand what Start() means, but what does repeated_eviction mean? It > doesn't seem to mean the Evictor should repeatedly consider what to evict next > as that seems to be the case regardless of this flag. > > When and why would this bool attribute be get/set by the QuotaManager? The Evictor is expected to run repeatedly by itself normally. This flag is prepared to * make testing easy with repeated_eviction == false, and * stop eviction safely if required. When repeated_eviction is set to false, the Evictor stops itself after it runs the next eviction. When repeated_eviction is set to true, the Evictor runs eviction repeatedly with intervals. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:36: const int64 min_available_disk_space_to_start_eviction_; On 2011/05/23 19:53:47, michaeln wrote: > Do these belong in the public interface? Ah, exactly. Moved them to private. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:50: void OnEvictionCompleted(QuotaStatusCode status); On 2011/05/23 19:53:47, michaeln wrote: > I found these method names to be difficult to follow. They're mostly used as > callbacks to QM methods and it's not clear which is called back from what. Are > these names an improvement? > > void StartEvictionTimer() { > timer_.Start(kEvictionInterval, &ConsiderEviction); > } > > void ConsiderEviction() { > // Get usage and disk space, then continue. > mgr->GetUsageAndQuotaForEviction(callback_factory_. > NewCallback(&OnGotUsageAndQuotaForEviction)) > > } > > void OnGotUsageAndQuotaForEviction( > QuotaStatusCode status, int64 usage, int64 quota, int64 free_disk_space) { > const double kUsageRatioToStartEviction = 0.7; > const int64 kDiskSpaceToKeepFree = 1000 * 1000 * 500; > if (status == kQuotaStatusOk && > (usage > quota * kUsageRatioToStartEviction || > available_disk_space < kDiskSpaceToKeepFree )) { > // Space is getting tight. Get the least recently used origin and continue. > mgr->GetLRUOrigin(kStorageTypeTemporary, callback_factory_.NewCallback( > &OnGotLRUOrigin)); > return; > } > // No action required, sleep for a while check again later. > StartEvictionTimer(); > } > > void OnGotLRUOrigin(const GURL& origin) { > if (origin.is_empty()) { > StartEvictionTimer(); > return; > } > mgr->EvictOriginData(origin, kStorageTypeTemporary, > callback_factory_.NewCallback(&OnEvictionComplete)); > } > > void OnEvictionComplete(QuotaStatusCode status) { > if (status != OK) { > StartEvictionTimer(); > return; > } > // We many need to get rid of more space so reconsider immediately. > ConsiderEviction(); > } Thank you for the suggestion. I chose your method names and some comments. Then I reordered the methods. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:27: int64 e_size = 275; On 2011/05/23 19:53:47, michaeln wrote: > These look like they should be const values? > const int64 kSizeA Done in the test case function. http://codereview.chromium.org/7002024/diff/40003/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:49: task_for_get_usage_and_quota_->Run(); On 2011/05/23 19:53:47, michaeln wrote: > The indirection using a CancelableTask seems overly complicated. Maybe put that > logic directly into your MockHandler instead. As discussing with Kinuko-san, * for RepeatedEvictionTest, we have to intercept the eviction loop * for the other test cases, we don't want to intercept. Prepare another Mock...Handler is an option, but it looks complicated, too. I chose the current design since this looks a little simpler than another Mock...Handler.
A few more comments... http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:66: // No action required, sleep for a while check again later. nit: 'and' before 'check' (... sleep for a while *and* check again...) http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:68: } Maybe we should have a TODO comment about error case handling here? (At least we'll really want to have some simple Stats struct (or variables) to hold several statistics variables: like # of errors, # of eviction, # of skipped-evictions (i.e. how many times GetLRUOrigin returned empty origin), current origin which is being evicted, origins that have repeated errors (nice-to-have) etc. Otherwise debugging or diagnosing what's happening on a bug report will be extremely difficult.) http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:46: void OnEvictionComplete(QuotaStatusCode status); The method names look better/clearer now! http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:127: // Run repeatedly in the single RunAllPending() when interval_ms == 0 nit: in the single -> in a single nit: Run repeatedly -> Run multiple evictions ? http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:140: int e_size, Thanks for parameterizing the expected values. The argument name 'e_size' won't have any meaningful indication to the readers. I think it'd look clearer if we do either: 1. rename this variable to 'size_to_add_between_eviction' or something, or 2. parameterize both of the origin name and size to be added and name it 'origin_to_be_added' or something meaningful (e.g. replace 'int e_size' with 'const std::pair<GURL, int64>& origin_to_be_added'). I slightly prefer option 2. Wdyt? http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:148: quota_eviction_handler()->AccessOrigin(GURL("http://www.c.com")); Here I'd wonder why we need to mix 'eviction-with-access-origin' and 'repeated-eviction' in a single test case? Is this 'accessing origin' a necessary part of the RepeatedEvictionTest? (I might be missing something) I'd imagine if we separate this into two tests like: TEST_F(..., RepeatedEvictionTest) TEST_F(..., EvictionWithAccessOrigin) // (A) We'd be able to easily know something has screwed up around AccessOrigin or GetLRUOrigin if test (B) fails, while we may need to diagnose what part of the test code went wrong when the current version of RepeatedEvictionTest fails.
Thank you. http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:66: // No action required, sleep for a while check again later. On 2011/05/24 09:20:53, kinuko wrote: > nit: 'and' before 'check' (... sleep for a while *and* check again...) Done. http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:68: } On 2011/05/24 09:20:53, kinuko wrote: > Maybe we should have a TODO comment about error case handling here? > > (At least we'll really want to have some simple Stats struct (or variables) to > hold several statistics variables: like # of errors, # of eviction, # of > skipped-evictions (i.e. how many times GetLRUOrigin returned empty origin), > current origin which is being evicted, origins that have repeated errors > (nice-to-have) etc. > Otherwise debugging or diagnosing what's happening on a bug report will be > extremely difficult.) For now, added two TODO comments for stats and error handling. I'll make a change list for stats soon. http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:46: void OnEvictionComplete(QuotaStatusCode status); On 2011/05/24 09:20:53, kinuko wrote: > The method names look better/clearer now! Thanks :) http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:127: // Run repeatedly in the single RunAllPending() when interval_ms == 0 On 2011/05/24 09:20:53, kinuko wrote: > nit: in the single -> in a single > nit: Run repeatedly -> Run multiple evictions ? Done. http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:140: int e_size, On 2011/05/24 09:20:53, kinuko wrote: > Thanks for parameterizing the expected values. > > The argument name 'e_size' won't have any meaningful indication to the readers. > I think it'd look clearer if we do either: > 1. rename this variable to 'size_to_add_between_eviction' or something, or > 2. parameterize both of the origin name and size to be added and name it > 'origin_to_be_added' or something meaningful (e.g. replace 'int e_size' with > 'const std::pair<GURL, int64>& origin_to_be_added'). > I slightly prefer option 2. Wdyt? Make sense. Done 2. http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:148: quota_eviction_handler()->AccessOrigin(GURL("http://www.c.com")); On 2011/05/24 09:20:53, kinuko wrote: > Here I'd wonder why we need to mix 'eviction-with-access-origin' and > 'repeated-eviction' in a single test case? Is this 'accessing origin' a > necessary part of the RepeatedEvictionTest? (I might be missing something) > > I'd imagine if we separate this into two tests like: > > TEST_F(..., RepeatedEvictionTest) > TEST_F(..., EvictionWithAccessOrigin) // (A) > > We'd be able to easily know something has screwed up around AccessOrigin or > GetLRUOrigin if test (B) fails, while we may need to diagnose what part of the > test code went wrong when the current version of RepeatedEvictionTest fails. > Ah, I understand. 1) I've tried separating them. Such an 'eviction-with-access-origin' test looks meaningful, however, only in repeated evictions. AccessOrigin before the first eviction results in a completely identical case with SimpleEvictionTest. 2) I tried separating them into the following two test cases : * RepeatedEvictionWithAddOriginTest * RepeatedEvictionWithAccessOriginTest. The second test 'RepeatedEvictionWithAccessOriginTest' had no meaning since no second eviction occurs without AddOrigin. Finally, I found it is required to do AddOrigin and AccessOrigin in the same case in order to test changing the order in the LRU list (AccessOrigin). My final decision is : * RepeatedEvictionTest : does only AddOrigin(). * RepeatedEvictionWithAccessOriginTest : does AddOrigin() and AccessOrigin(). What do you think?
mostly nits http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/46001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:148: quota_eviction_handler()->AccessOrigin(GURL("http://www.c.com")); On 2011/05/24 11:30:52, Dai Mikurube wrote: > On 2011/05/24 09:20:53, kinuko wrote: > > Here I'd wonder why we need to mix 'eviction-with-access-origin' and > > 'repeated-eviction' in a single test case? Is this 'accessing origin' a > > necessary part of the RepeatedEvictionTest? (I might be missing something) > > > > I'd imagine if we separate this into two tests like: > > > > TEST_F(..., RepeatedEvictionTest) > > TEST_F(..., EvictionWithAccessOrigin) // (A) > > > > We'd be able to easily know something has screwed up around AccessOrigin or > > GetLRUOrigin if test (B) fails, while we may need to diagnose what part of the > > test code went wrong when the current version of RepeatedEvictionTest fails. > > > > Ah, I understand. > > 1) I've tried separating them. Such an 'eviction-with-access-origin' test looks > meaningful, however, only in repeated evictions. AccessOrigin before the first > eviction results in a completely identical case with SimpleEvictionTest. > > 2) I tried separating them into the following two test cases : > * RepeatedEvictionWithAddOriginTest > * RepeatedEvictionWithAccessOriginTest. > > The second test 'RepeatedEvictionWithAccessOriginTest' had no meaning since no > second eviction occurs without AddOrigin. > > Finally, I found it is required to do AddOrigin and AccessOrigin in the same > case in order to test changing the order in the LRU list (AccessOrigin). > > > My final decision is : > * RepeatedEvictionTest : does only AddOrigin(). > * RepeatedEvictionWithAccessOriginTest : does AddOrigin() and AccessOrigin(). > > What do you think? I imagined a separate test doing AccessOrigin between two manually-kicked eviction (without setting repeated_eviction true) but your current approach would work too. http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:70: // TODO(dmikurube): Add simple stats on # of {error, eviction, skipped}. super-nit: no need to declare 'simple' in a TODO comment? http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:9: #include "base/message_loop_proxy.h" nit: not necessary if we forward-declare MessageLoopProxy? http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:12: #include "webkit/quota/quota_manager.h" nit: not necessary if we forward-declare QuotaEvictionHandler? http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:35: friend struct QuotaTemporaryStorageEvictorDeleter; not used now http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:9: #include <utility> No need to include this as long as you have <map>. http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:160: void TaskForRepeatedEvictionWithAccessOriginTest( Probably this method can be merged into the one above? (They are mostly same) (e.g. if (!origin_to_be_accessed.empty()) { AccessOrigin(...); } )
Thank you for the comments. http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:70: // TODO(dmikurube): Add simple stats on # of {error, eviction, skipped}. On 2011/05/24 12:46:48, kinuko wrote: > super-nit: no need to declare 'simple' in a TODO comment? Done. http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:9: #include "base/message_loop_proxy.h" On 2011/05/24 12:46:48, kinuko wrote: > nit: not necessary if we forward-declare MessageLoopProxy? Done. http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:12: #include "webkit/quota/quota_manager.h" On 2011/05/24 12:46:48, kinuko wrote: > nit: not necessary if we forward-declare QuotaEvictionHandler? Done. http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:35: friend struct QuotaTemporaryStorageEvictorDeleter; On 2011/05/24 12:46:48, kinuko wrote: > not used now Done. http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:9: #include <utility> On 2011/05/24 12:46:48, kinuko wrote: > No need to include this as long as you have <map>. Done. http://codereview.chromium.org/7002024/diff/44002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:160: void TaskForRepeatedEvictionWithAccessOriginTest( On 2011/05/24 12:46:48, kinuko wrote: > Probably this method can be merged into the one above? (They are mostly same) > (e.g. if (!origin_to_be_accessed.empty()) { AccessOrigin(...); } ) Done.
I think it lgtm now. http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_manager.... webkit/quota/quota_manager.h:40: virtual ~QuotaEvictionHandler() {} You may want to give your answer why this needs to be public to michael? (Sorry if you've already done) http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_manager.... http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:200: EXPECT_EQ(3700, quota_eviction_handler()->GetUsage()); It might be slightly more readable if you write 3000 + 200 + 500. (This one is easy but sometime I needed some calculation to know where the expected value comes from)
http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_manager.h File webkit/quota/quota_manager.h (right): http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_manager.... webkit/quota/quota_manager.h:40: virtual ~QuotaEvictionHandler() {} On 2011/05/25 06:31:38, kinuko wrote: > You may want to give your answer why this needs to be public to michael? (Sorry > if you've already done) > > http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_manager.... Ah, I moved that because of error when mocking this class in the test. http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor_unittest.cc:200: EXPECT_EQ(3700, quota_eviction_handler()->GetUsage()); On 2011/05/25 06:31:38, kinuko wrote: > It might be slightly more readable if you write 3000 + 200 + 500. > (This one is easy but sometime I needed some calculation to know where the > expected value comes from) Done.
Checked the "Commit" box. On 2011/05/25 07:20:37, Dai Mikurube wrote: > http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_manager.h > File webkit/quota/quota_manager.h (right): > > http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_manager.... > webkit/quota/quota_manager.h:40: virtual ~QuotaEvictionHandler() {} > On 2011/05/25 06:31:38, kinuko wrote: > > You may want to give your answer why this needs to be public to michael? > (Sorry > > if you've already done) > > > > > http://codereview.chromium.org/7002024/diff/36009/webkit/quota/quota_manager.... > > Ah, I moved that because of error when mocking this class in the test. > > http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_temporar... > File webkit/quota/quota_temporary_storage_evictor_unittest.cc (right): > > http://codereview.chromium.org/7002024/diff/50001/webkit/quota/quota_temporar... > webkit/quota/quota_temporary_storage_evictor_unittest.cc:200: EXPECT_EQ(3700, > quota_eviction_handler()->GetUsage()); > On 2011/05/25 06:31:38, kinuko wrote: > > It might be slightly more readable if you write 3000 + 200 + 500. > > (This one is easy but sometime I needed some calculation to know where the > > expected value comes from) > > Done.
Change committed as 86597 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
