Implement QuotaTemporaryStorageEvictor.

webkit/quota/quota_temporary_storage_evictor_unittest.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+#include <list>
+#include <map>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/task.h"
+#include "webkit/quota/mock_storage_client.h"
+#include "webkit/quota/quota_temporary_storage_evictor.h"
+
+namespace quota {
+
+class QuotaTemporaryStorageEvictorTest;
+
+namespace {
+
+class MockQuotaEvictionHandler : public quota::QuotaEvictionHandler {
+ public:
+  MockQuotaEvictionHandler(QuotaTemporaryStorageEvictorTest *test)
+      : quota_(0),
+        available_space_(0),
+        test_(test),
+        task_for_get_usage_and_quota_(NULL) {}
+  virtual ~MockQuotaEvictionHandler() {
+    if (task_for_get_usage_and_quota_)
+      delete task_for_get_usage_and_quota_;
+  }
+
+  virtual void EvictOriginData(
+      const GURL& origin,
+      StorageType type,
+      EvictOriginDataCallback* callback) OVERRIDE {
+    RemoveOrigin(origin);
+    callback->Run(quota::kQuotaStatusOk);
+    delete callback;
+  }
+
+  virtual void GetUsageAndQuotaForEviction(
+      GetUsageAndQuotaForEvictionCallback* callback) OVERRIDE {
+    if (task_for_get_usage_and_quota_)
+      task_for_get_usage_and_quota_->Run();

[kinuko, 2011/05/23 04:34:53]

don't we need to delete the task?

[Dai Mikurube (NOT FULLTIME), 2011/05/23 05:53:37]

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.

+    callback->Run(quota::kQuotaStatusOk, GetUsage(), quota_, available_space_);
+    delete callback;
+  }
+
+  virtual void GetLRUOrigin(
+      StorageType type,
+      GetLRUOriginCallback* callback) OVERRIDE {
+    if (origin_order_.empty())
+      callback->Run(GURL());
+    else
+      callback->Run(origin_order_.front());
+    delete callback;
+  }
+
+  int64 GetUsage() const {
+    int64 total_usage = 0;
+    for (std::map<GURL, int64>::const_iterator p = origins_.begin();
+         p != origins_.end();
+         ++p)
+      total_usage += p->second;
+    return total_usage;
+  }
+
+  void set_quota(int64 quota) {
+    quota_ = quota;
+  }
+  void set_available_space(int64 available_space) {
+    available_space_ = available_space;
+  }
+  void set_task_for_get_usage_and_quota(CancelableTask* task) {
+    task_for_get_usage_and_quota_ = task;
+  }
+
+  void AccessOrigin(const GURL& origin) {
+    int64 origin_usage = RemoveOrigin(origin);
+    if (origin_usage >= 0)
+      AddPseudoOrigin(origin, origin_usage);
+  }
+
+  void AddPseudoOrigin(const GURL& origin, int64 usage) {

[kinuko, 2011/05/23 04:34:53]

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)

[Dai Mikurube (NOT FULLTIME), 2011/05/23 05:53:37]

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.

[kinuko, 2011/05/23 07:29:51]

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.
Sounds good, this type of change would improve readability a lot.
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?
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

[Dai Mikurube (NOT FULLTIME), 2011/05/23 09:50:37]

Ok, finally, I added such comments at AccessOrigin() and AddOrigin().
Renamed it.
Really?  I think the new code reorders the list.  If the given origin is not in
the map, reordering has no meaning.
Finally, just added a comment as written above.  But, sorry, I could not
understand the difference between 'insert' and 'add' here...
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...
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.

[kinuko, 2011/05/23 12:10:29]

Apparently this was wrong, please nvm.
 

+    RemoveOrigin(origin);
+    origin_order_.push_back(origin);
+    origins_[origin] = usage;
+  }
+
+ private:
+  int64 RemoveOrigin(const GURL& origin) {
+    int64 origin_usage;
+    if (origins_.find(origin) == origins_.end())

[kinuko, 2011/05/23 04:34:53]

EXPECT_TRUE?

[Dai Mikurube (NOT FULLTIME), 2011/05/23 05:53:37]

As replied above, I designed this method to be called multiple times.

+      return -1;
+    else
+      origin_usage = origins_[origin];
+
+    origins_.erase(origin);
+    origin_order_.remove(origin);
+    return origin_usage;
+  }
+
+  int64 quota_;
+  int64 available_space_;
+  std::list<GURL> origin_order_;
+  std::map<GURL, int64> origins_;
+
+  QuotaTemporaryStorageEvictorTest *test_;
+  CancelableTask* task_for_get_usage_and_quota_;

[kinuko, 2011/05/23 04:34:53]

Can this task be scoped_ptr?

[Dai Mikurube (NOT FULLTIME), 2011/05/23 05:53:37]

Exactly. Done.

+};
+
+}  // anonymous namespace
+
+class QuotaTemporaryStorageEvictorTest : public testing::Test {
+ public:
+  QuotaTemporaryStorageEvictorTest()
+      : num_get_usage_and_quota_for_eviction_(0),
+        runnable_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {}
+
+  void SetUp() {
+    quota_eviction_handler_.reset(new MockQuotaEvictionHandler(this));
+
+    // Run repeatedly in the single RunAllPending() when interval_ms == 0
+    temporary_storage_evictor_.reset(new QuotaTemporaryStorageEvictor(
+        quota_eviction_handler_.get(),
+        0, base::MessageLoopProxy::CreateForCurrentThread()));
+  }
+
+  void TearDown() {
+    temporary_storage_evictor_.reset();
+    quota_eviction_handler_.reset();
+    MessageLoop::current()->RunAllPending();
+  }
+
+  void TaskForRepeatTestAfterGetUsageAndQuota() {
+    switch (num_get_usage_and_quota_for_eviction_) {
+    case 1:
+      EXPECT_EQ(962-292, quota_eviction_handler()->GetUsage());

[kinuko, 2011/05/23 04:34:53]

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).

[Dai Mikurube (NOT FULLTIME), 2011/05/23 05:53:37]

Done.

+      quota_eviction_handler()->AddPseudoOrigin(
+          GURL("http://www.hoge.com"), 280);
+      quota_eviction_handler()->AccessOrigin(GURL("http://www.foo.com"));
+      EXPECT_EQ(962-292+280, quota_eviction_handler()->GetUsage());
+      break;
+    case 2:
+      EXPECT_EQ(962-292+280-120, quota_eviction_handler()->GetUsage());
+      temporary_storage_evictor()->set_repeated_eviction(false);
+      break;
+    }
+    ++num_get_usage_and_quota_for_eviction_;
+  }
+
+ protected:
+  MockQuotaEvictionHandler* quota_eviction_handler() const {
+    return static_cast<MockQuotaEvictionHandler*>(
+        quota_eviction_handler_.get());
+  }
+
+  QuotaTemporaryStorageEvictor* temporary_storage_evictor() const {
+    return temporary_storage_evictor_.get();
+  }
+
+  scoped_ptr<QuotaEvictionHandler> quota_eviction_handler_;
+  scoped_ptr<QuotaTemporaryStorageEvictor> temporary_storage_evictor_;
+
+  int num_get_usage_and_quota_for_eviction_;
+
+  ScopedRunnableMethodFactory<QuotaTemporaryStorageEvictorTest>
+      runnable_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(QuotaTemporaryStorageEvictorTest);
+};
+
+TEST_F(QuotaTemporaryStorageEvictorTest, SimpleEvictionTest) {
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.baz.com"), 3000);
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.bar.com"), 200);
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.foo.com"), 500);
+  quota_eviction_handler()->set_quota(4000);
+  quota_eviction_handler()->set_available_space(1000000000);
+  EXPECT_EQ(3700, quota_eviction_handler()->GetUsage());
+  temporary_storage_evictor()->Start();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(700, quota_eviction_handler()->GetUsage());
+}
+
+TEST_F(QuotaTemporaryStorageEvictorTest, MultipleEvictionTest) {
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.hoge.com"), 20);
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.baz.com"), 2900);
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.bar.com"), 450);
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.foo.com"), 400);
+  quota_eviction_handler()->set_quota(4000);
+  quota_eviction_handler()->set_available_space(1000000000);
+  EXPECT_EQ(3770, quota_eviction_handler()->GetUsage());
+  temporary_storage_evictor()->Start();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(850, quota_eviction_handler()->GetUsage());
+}
+
+TEST_F(QuotaTemporaryStorageEvictorTest, RepeatedEvictionTest) {
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.ban.com"), 292);
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.baz.com"), 120);
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.bar.com"), 150);
+  quota_eviction_handler()->AddPseudoOrigin(GURL("http://www.foo.com"), 400);
+  quota_eviction_handler()->set_quota(1000);
+  quota_eviction_handler()->set_available_space(1000000000);
+  quota_eviction_handler()->set_task_for_get_usage_and_quota(
+      runnable_factory_.NewRunnableMethod(&QuotaTemporaryStorageEvictorTest::
+          TaskForRepeatTestAfterGetUsageAndQuota));
+  EXPECT_EQ(962, quota_eviction_handler()->GetUsage());
+  temporary_storage_evictor()->set_repeated_eviction(true);
+  temporary_storage_evictor()->Start();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(962-292+280-120-150, quota_eviction_handler()->GetUsage());

[kinuko, 2011/05/23 04:34:53]

EXPECT_EQ(2, num_get_usage_and_quota_for_eviction()) or something?

[Dai Mikurube (NOT FULLTIME), 2011/05/23 05:53:37]

Done, but GetUsageAndQuotaForEviction() is called 4 times finally.

+}
+
+// TODO(dmikurube): Add tests for eviction by available disk space.
+
+}  // namespace quota