Implement QM::GetOriginsModifiedSince for browser data deleter support

Implement QM::GetOriginsModifiedSince for browser data deleter support

BUG=86308
TEST={QuotaDatabaseTest,QuotaManagerTest}.OriginModifiedSince

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91690

[Mike West, 2011-06-16 09:39:33]

Thanks for working on this!

I have a few questions, but I'd very much defer to the other reviewers with
regard to their importance. Don't block on me. :)

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database.cc
File webkit/quota/quota_database.cc (right):

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.cc:222: bool
QuotaDatabase::SetOriginLastModifiedTime(
It's likely that last modified and last accessed will be changing
simultaneously.  You'll still need separate methods for things like `touch`, but
most of the time the values will be the same, right?  Since it requires going
out to the database to change both values, it probably makes sense to have a
mechanism for setting both with one SQL query in the common case.

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.cc:230: if (FindOriginUsedCount(origin, type,
&dummy)) {
This idiom seems heavy.  You do a SQL query to determine if the value exists in
the database, then either update or insert accordingly.  Does the version of
SQLite we bundle support `INSERT OR REPLACE`?  If it does, I'm pretty sure you
can wrap most of this logic up into a single (albeit more complex) SQL query. 
See
http://stackoverflow.com/questions/418898/sqlite-upsert-not-insert-or-replace...
for example.

The same would be true for `SetOriginLastAccessTime` above.

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.cc:274: statement.BindInt64(2, 0);  // last modified
time
Here, last_modified_time is inserted as the third column.  On line 626, it's
read out as the fifth column.  I'd suggest keeping the column order consistent.

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database.h
File webkit/quota/quota_database.h (right):

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.h:55: // registered origin will be initialized to 0.
Isn't the origin being modified at `base::Time::Now()` if we're registering it?

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.h:60: bool DeleteHostQuota(const std::string& host,
StorageType type);
Maybe move this up to live with `GetHostQuota` and `SetHostQuota`, just so
they're all in one place and easy to spot?

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database_...
File webkit/quota/quota_database_unittest.cc (right):

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database_...
webkit/quota/quota_database_unittest.cc:283: kOrigin1, kStorageTypePersistent,
base::Time::FromInternalValue(40)));
This is the only place you do anything with persistent storage.  Would you mind
copy/pasting a few lines to test that getting modified persistent storage
origins behaves the same way as temporary storage?

[kinuko, 2011-06-16 12:09:06]

Thanks for reviewing!

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database.cc
File webkit/quota/quota_database.cc (right):

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.cc:222: bool
QuotaDatabase::SetOriginLastModifiedTime(
On 2011/06/16 09:39:33, Mike West wrote:
> It's likely that last modified and last accessed will be changing
> simultaneously.  You'll still need separate methods for things like `touch`,
but
> most of the time the values will be the same, right?  Since it requires going
> out to the database to change both values, it probably makes sense to have a
> mechanism for setting both with one SQL query in the common case.

Hmm in the current QuotaManager design we have separate notification methods for
'read (access)' and 'write (modify)', and as far as we want to keep this design
I would be tempted to keep having separate methods for each.

We can change the quota manager<->client messaging design as well, but I'm not
very convinced to do so (yet?) because:
* At least for 'access time' we'll want a separate call since most 'read'
operation won't come with 'write'.
* As for 'modify time' it might be true that we should also update 'access'
time, but there're still some unclear situations--- for example for a long write
which spans multiple write tasks we may want to make multiple 'modify'
notifications (we do this in file write), but probably won't update 'used_count'
for each.

As for database write cost, in general these 'SetXxxTime' calls can be made very
frequently, and to alleviate the cost we try batching multiple database writes
in a fewer transactions.

Does that make sense?  Or if you have more comments/suggestions I'd love to
hear.

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.cc:230: if (FindOriginUsedCount(origin, type,
&dummy)) {
On 2011/06/16 09:39:33, Mike West wrote:
> This idiom seems heavy.  You do a SQL query to determine if the value exists
in
> the database, then either update or insert accordingly.  Does the version of
> SQLite we bundle support `INSERT OR REPLACE`?  If it does, I'm pretty sure you
> can wrap most of this logic up into a single (albeit more complex) SQL query. 
> See
>
http://stackoverflow.com/questions/418898/sqlite-upsert-not-insert-or-replace...
> for example.
> 
> The same would be true for `SetOriginLastAccessTime` above.

(I think I've looked at the same stackoverflow thread... :))

As for SetOriginLastModifiedTime we want to preserve two column values
(used_count and last_access_time) from an existing entry for REPLACE case, as
far as we want to have separate methods for last-mod and last-access time.  In
that case I need to write 'select' subclause twice for each column right?  I
didn't want to do that but I might be missing something.

As for SetOriginLastAccessTime we want to increment used_count if there already
is an entry, and I couldn't find easier/neat sql-only solution.  If there's a
simpler notion I'd love to follow.

(Btw I found that it looks possible to revise SetHostQuota using INSERT OR
REPLACE, so I updated the code.)

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.cc:274: statement.BindInt64(2, 0);  // last modified
time
On 2011/06/16 09:39:33, Mike West wrote:
> Here, last_modified_time is inserted as the third column.  On line 626, it's
> read out as the fifth column.  I'd suggest keeping the column order
consistent.

Done.

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database.h
File webkit/quota/quota_database.h (right):

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.h:55: // registered origin will be initialized to 0.
On 2011/06/16 09:39:33, Mike West wrote:
> Isn't the origin being modified at `base::Time::Now()` if we're registering
it?

The naming might be confusing here, this is a one-time method that is to be
called only for the db initialization after the installation or db schema
update.  Renamed this to RegisterInitialOriginInfo() and slightly changed the
signature, hoping it makes more sense.

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database....
webkit/quota/quota_database.h:60: bool DeleteHostQuota(const std::string& host,
StorageType type);
On 2011/06/16 09:39:33, Mike West wrote:
> Maybe move this up to live with `GetHostQuota` and `SetHostQuota`, just so
> they're all in one place and easy to spot?

Done.

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database_...
File webkit/quota/quota_database_unittest.cc (right):

http://codereview.chromium.org/7168019/diff/7001/webkit/quota/quota_database_...
webkit/quota/quota_database_unittest.cc:283: kOrigin1, kStorageTypePersistent,
base::Time::FromInternalValue(40)));
On 2011/06/16 09:39:33, Mike West wrote:
> This is the only place you do anything with persistent storage.  Would you
mind
> copy/pasting a few lines to test that getting modified persistent storage
> origins behaves the same way as temporary storage?

Done.

[michaeln, 2011-06-16 19:28:53]

Thnx for doing this work!

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_database.cc
File webkit/quota/quota_database.cc (right):

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_database....
webkit/quota/quota_database.cc:493: // TODO(kinuko): Support schema upgrade.
Do we need an upgrade path for this schema change? I'm less concerned about
losing last access times than about losing the host quotas for persistent
storage.

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:650: callback_->Run(kQuotaErrorAbort, space_);
would it be safer to use -1 instead of space_

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:744: callback_->Run(origins_);
I think there may be a race here, what if 'origins_' is being populated on the
FILE thread when Abort() happens?

Might be safer and more clear to Run(std::set<GURL>()) here.

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:779: callback_->Run(entries_);
looks like this may have a similar race?

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:821: callback_->Run(entries_);
ditto race

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:1119: void QuotaManager::GetOriginsModifiedSince(
if the method is moved to the public section, please move the method definition
up accordingly in the .cc file

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.h
File webkit/quota/quota_manager.h (right):

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:255: void GetOriginsModifiedSince(
should this be in the public interface?

[kinuko, 2011-06-23 06:37:40]

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_database.cc
File webkit/quota/quota_database.cc (right):

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_database....
webkit/quota/quota_database.cc:493: // TODO(kinuko): Support schema upgrade.
On 2011/06/16 19:28:53, michaeln wrote:
> Do we need an upgrade path for this schema change? I'm less concerned about
> losing last access times than about losing the host quotas for persistent
> storage.

Done.

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:650: callback_->Run(kQuotaErrorAbort, space_);
On 2011/06/16 19:28:53, michaeln wrote:
> would it be safer to use -1 instead of space_

Done.

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:744: callback_->Run(origins_);
On 2011/06/16 19:28:53, michaeln wrote:
> I think there may be a race here, what if 'origins_' is being populated on the
> FILE thread when Abort() happens?
> 
> Might be safer and more clear to Run(std::set<GURL>()) here.

Done.  Good catch...

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:779: callback_->Run(entries_);
On 2011/06/16 19:28:53, michaeln wrote:
> looks like this may have a similar race?

Done.

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:821: callback_->Run(entries_);
On 2011/06/16 19:28:53, michaeln wrote:
> ditto race

Done.

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:1119: void QuotaManager::GetOriginsModifiedSince(
On 2011/06/16 19:28:53, michaeln wrote:
> if the method is moved to the public section, please move the method
definition
> up accordingly in the .cc file

Done.

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.h
File webkit/quota/quota_manager.h (right):

http://codereview.chromium.org/7168019/diff/4013/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:255: void GetOriginsModifiedSince(
On 2011/06/16 19:28:53, michaeln wrote:
> should this be in the public interface?

Done.

[michaeln, 2011-07-01 00:17:31]

lgtm

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.h (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:43: base::Time GetCurrentMockTime();
Would IncrementMockTime() be more accurate?

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:84: int virtual_time_counter_;
nit: mock_time_counter_ would line up with the method name better

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_database.h
File webkit/quota/quota_database.h (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_database...
webkit/quota/quota_database.h:37: // TODO(kinuko): probably they should be
defined somewhere else as a common
Can these structs be moved to the private section?

Also, I'd vote to remove this TODO comment, if the db schema creation stuff is
ever generalized and put in a shareable place, then relocating these structs
would just happen as an artifact of that effort. Having an explicit TODO about a
small subset of that task is noisy (imo).

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_database...
File webkit/quota/quota_database_unittest.cc (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_database...
webkit/quota/quota_database_unittest.cc:92: void UpgradeSchema(const FilePath&
kDbFile) {
Might be good to be explicit about what versions you're upgrading from and to...
UgradeSchema2to3

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_database...
webkit/quota/quota_database_unittest.cc:111:
EXPECT_TRUE(verifier.table.empty());
it took me a moment to realize how the verifier works, deleting expected entries
as it goes, i saw the test for empty() here and thought... oh that can't be
right... but i see that it is.

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:690: class
QuotaManager::OriginModifyRecordDatabaseTask
naming nit: these names are awkward to me, too many nouns and verbs all in
one... maybe UpdateModifiedTimeTask and GetModifiedSinceTask

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager_...
File webkit/quota/quota_manager_unittest.cc (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager_...
webkit/quota/quota_manager_unittest.cc:306:
modified_origins_.insert(origins.begin(), origins.end());
would an assignment work here?

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager_...
webkit/quota/quota_manager_unittest.cc:1340:
client->ModifyOriginAndNotify(GURL("http://a.com:1/"), kTemp, 10);
maybe modify the kPerm origin here to ensure it doesn't get picked up by the
first call to GetOriginsModifiedSince(kTemp, time1)

[kinuko, 2011-07-04 07:43:52]

All done.  Moved kTables/kIndexes constants from anonymous namespace into
QuotaDatabase class as static const class variables.

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.h (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:43: base::Time GetCurrentMockTime();
On 2011/07/01 00:17:39, michaeln wrote:
> Would IncrementMockTime() be more accurate?

Done.

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:84: int virtual_time_counter_;
On 2011/07/01 00:17:39, michaeln wrote:
> nit: mock_time_counter_ would line up with the method name better

Done.

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_database.h
File webkit/quota/quota_database.h (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_database...
webkit/quota/quota_database.h:37: // TODO(kinuko): probably they should be
defined somewhere else as a common
On 2011/07/01 00:17:39, michaeln wrote:
> Can these structs be moved to the private section?
> 
> Also, I'd vote to remove this TODO comment, if the db schema creation stuff is
> ever generalized and put in a shareable place, then relocating these structs
> would just happen as an artifact of that effort. Having an explicit TODO about
a
> small subset of that task is noisy (imo).

Done.

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_database...
File webkit/quota/quota_database_unittest.cc (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_database...
webkit/quota/quota_database_unittest.cc:92: void UpgradeSchema(const FilePath&
kDbFile) {
On 2011/07/01 00:17:39, michaeln wrote:
> Might be good to be explicit about what versions you're upgrading from and
to...
> UgradeSchema2to3

Done.

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:690: class
QuotaManager::OriginModifyRecordDatabaseTask
On 2011/07/01 00:17:39, michaeln wrote:
> naming nit: these names are awkward to me, too many nouns and verbs all in
> one... maybe UpdateModifiedTimeTask and GetModifiedSinceTask

They are named like that in order to make them distinguishable from method
names, but I know they are awkward... simply renamed most of them in a format
'verb' + 'noun'.

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager_...
File webkit/quota/quota_manager_unittest.cc (right):

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager_...
webkit/quota/quota_manager_unittest.cc:306:
modified_origins_.insert(origins.begin(), origins.end());
On 2011/07/01 00:17:39, michaeln wrote:
> would an assignment work here?

Done.

http://codereview.chromium.org/7168019/diff/15002/webkit/quota/quota_manager_...
webkit/quota/quota_manager_unittest.cc:1340:
client->ModifyOriginAndNotify(GURL("http://a.com:1/"), kTemp, 10);
On 2011/07/01 00:17:39, michaeln wrote:
> maybe modify the kPerm origin here to ensure it doesn't get picked up by the
> first call to GetOriginsModifiedSince(kTemp, time1)

Done.

[michaeln, 2011-07-06 19:37:34]

still LGTM