Change ThumbnailStore to use sqlite instead of individual files.

Change ThumbnailStore to use sqlite instead of individual files.


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

[meelapshah, 2009-07-06 22:42:49]

Updated unit test coming soon...

[brettw, 2009-07-07 02:31:08]

Random comments. I'll check the sqlite stuff tomorrow morning.

http://codereview.chromium.org/149223/diff/1014/31
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/149223/diff/1014/31#newcode1020
Line 1020: thumbnail_store_->Init(GetPath().AppendASCII("ThumbnailDB"), this);
To match the other names in the directory, this should be called "Thumbnails".
Of course, that's taken and we don't want to cause confusion. What do you think
about "Top Thumbnails".

http://codereview.chromium.org/149223/diff/1014/30
File chrome/browser/thumbnail_store.h (right):

http://codereview.chromium.org/149223/diff/1014/30#newcode45
Line 45: bool load_existing = true);   // For debugging only.
Default arguments are disallowed by the style guide, can you make all of them
non-default?

http://codereview.chromium.org/149223/diff/1014/34
File chrome/browser/thumbnail_store_unittest.cc (right):

http://codereview.chromium.org/149223/diff/1014/34#newcode136
Line 136: #if 0
Any reason why these were disabled?

[meelapshah, 2009-07-07 17:45:37]

http://codereview.chromium.org/149223/diff/1014/31
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/149223/diff/1014/31#newcode1020
Line 1020: thumbnail_store_->Init(GetPath().AppendASCII("ThumbnailDB"), this);
On 2009/07/07 02:31:08, brettw wrote:
> To match the other names in the directory, this should be called "Thumbnails".
> Of course, that's taken and we don't want to cause confusion. What do you
think
> about "Top Thumbnails".

That works.
Done.

http://codereview.chromium.org/149223/diff/1014/30
File chrome/browser/thumbnail_store.h (right):

http://codereview.chromium.org/149223/diff/1014/30#newcode45
Line 45: bool load_existing = true);   // For debugging only.
On 2009/07/07 02:31:08, brettw wrote:
> Default arguments are disallowed by the style guide, can you make all of them
> non-default?

Done.

http://codereview.chromium.org/149223/diff/1014/34
File chrome/browser/thumbnail_store_unittest.cc (right):

http://codereview.chromium.org/149223/diff/1014/34#newcode136
Line 136: #if 0
On 2009/07/07 02:31:08, brettw wrote:
> Any reason why these were disabled?

They weren't working since I changed the format of the cache.
It's fixed now so I've re-enabled them.

[brettw, 2009-07-07 19:32:31]

You removed using the file thread. Sqlite is slow (probably moreso than even raw
file access). So you should preserve your previous file_thread stuff and just do
sqlite from the background thread. Sorry if I wasn't clear about this in
advance.

[meelapshah, 2009-07-07 20:19:48]

On 2009/07/07 19:32:31, brettw wrote:
> You removed using the file thread. Sqlite is slow (probably moreso than even
raw
> file access). So you should preserve your previous file_thread stuff and just
do
> sqlite from the background thread. Sorry if I wasn't clear about this in
> advance.

Oops. I didn't mean to do that. It's fixed now.

[brettw, 2009-07-07 20:42:41]

LGTM, I have a suggestion for a follow-on patch I'll email you

http://codereview.chromium.org/149223/diff/1029/1032
File chrome/browser/thumbnail_store.cc (right):

http://codereview.chromium.org/149223/diff/1029/1032#newcode267
Line 267: statement->column_int64(4)));  // Time taken
Can you use Time.To/FromInternalValue instead of TimeT for serialization?

[meelapshah, 2009-07-07 21:01:21]

http://codereview.chromium.org/149223/diff/1029/1032
File chrome/browser/thumbnail_store.cc (right):

http://codereview.chromium.org/149223/diff/1029/1032#newcode267
Line 267: statement->column_int64(4)));  // Time taken
On 2009/07/07 20:42:41, brettw wrote:
> Can you use Time.To/FromInternalValue instead of TimeT for serialization?

Done.

[meelapshah, 2009-07-07 22:40:10]

On 2009/07/07 21:01:21, meelapshah wrote:
> http://codereview.chromium.org/149223/diff/1029/1032
> File chrome/browser/thumbnail_store.cc (right):
> 
> http://codereview.chromium.org/149223/diff/1029/1032#newcode267
> Line 267: statement->column_int64(4)));  // Time taken
> On 2009/07/07 20:42:41, brettw wrote:
> > Can you use Time.To/FromInternalValue instead of TimeT for serialization?
> 
> Done.

committed in r20097.