The implementation of AndroidCacheDatabase.

The implementation of AndroidCacheDatabase.

It is used to support the Android Bookmark Provider;
It caches the favicon id, visit time, created time,
bookmark information from thumbnail database, url
database and BookmarkModel.

The database is created in first access of AndroidProviderBackend,
and deleted after history backend shutdown, synced before
AndroidProviderBackend's Query, Insert, Update or Delete method
called.

BUG=
TEST=


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

[michaelbai, 2012-03-01 04:43:59]

Please review it

[sky, 2012-03-02 15:59:50]

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
File chrome/browser/history/android/android_cache_database.cc (right):

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:24:
AndroidCacheDatabase::AndroidCacheDatabase() {
You should either initialize db_attached_, or nuke it if you don't need it (you
only set it in one place, so I'm not sure if you need it).

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:28: if
(file_util::PathExists(db_name_)) {
Don't you need to close first?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:29: bool result =
file_util::Delete(db_name_, false);
spacing

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:55:
statement.BindInt64(0, ToMilliseconds(created_time));
Why are you using milliseconds instead of InternalValue like the rest of the
code?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:64: return
GetDB().GetLastInsertRowId();
Do you want the return value to be a bool or the ID?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:68: if
(!GetDB().Execute("DELETE FROM android_cache_db.bookmark_cache")) {
Don't you want a cached statement?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:78: std::ostringstream
oss;
Why not just build up a string?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:153: "favicon_id
INTEGER DEFAULT NULL,"   // favicon id.
Don't you want 0 here?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:173:
GetDB().CommitTransaction();
Document why this is needed.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:183:
GetDB().BeginTransaction();
This code is fragile because of having to open the connection before the early
return. How about moving it to a separate function and having it return an error
status. The caller can then be resposible for beginning the transaction after
calling the method.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
File chrome/browser/history/android/android_cache_database.h (right):

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:21: class
AndroidCacheDatabase {
Be sure and document the return value of all methods that aren't void.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:30: // Add a row in
bookmark_cache table.
'Adds a row to the ...'

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:36: bool
ClearAllBookmarkCacheRow();
Either ClearAllBookmarkCacheRows or just ClearBookmarkCache

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:38: // Set the bookmark
column as 1 for the given |url_ids| rows.
Set -> Sets. Document return value.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:39: bool
CacheBookmarkBits(const std::vector<URLID>& url_id);
This description doesn't match that I thought the method would do. How about
MarkURLsAsBookmarked or maybe SetURLsBookmarked

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:41: // Set the favicon
column to |favicon_id| for the given |url_id| row.
Set -> Sets

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:42: bool
CacheFaviconID(URLID url_id, FaviconID favicon_id);
Name this to match your description: SetFaviconID.

[michaelbai, 2012-03-02 20:04:43]

PTAL

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
File chrome/browser/history/android/android_cache_database.cc (right):

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:24:
AndroidCacheDatabase::AndroidCacheDatabase() {
That needs nuked, thanks

On 2012/03/02 15:59:50, sky wrote:
> You should either initialize db_attached_, or nuke it if you don't need it
(you
> only set it in one place, so I'm not sure if you need it).

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:28: if
(file_util::PathExists(db_name_)) {
Thank for finding this, I shouldn't delete the file here because the database
needs close/detach first, then remove the file.

But I can't access the GetDB in destructor as it is virtual method, so the best
way seemes delete file in HistoryBackend's destructor after the db closed.

On 2012/03/02 15:59:50, sky wrote:
> Don't you need to close first?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:29: bool result =
file_util::Delete(db_name_, false);
Code removed.

On 2012/03/02 15:59:50, sky wrote:
> spacing

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:55:
statement.BindInt64(0, ToMilliseconds(created_time));
It because the Android use the milliseconds to query, that's the reason I cache
the time in this table.

On 2012/03/02 15:59:50, sky wrote:
> Why are you using milliseconds instead of InternalValue like the rest of the
> code?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:64: return
GetDB().GetLastInsertRowId();
Return true

On 2012/03/02 15:59:50, sky wrote:
> Do you want the return value to be a bool or the ID?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:68: if
(!GetDB().Execute("DELETE FROM android_cache_db.bookmark_cache")) {
On 2012/03/02 15:59:50, sky wrote:
> Don't you want a cached statement?

Done.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:78: std::ostringstream
oss;
Did you mean use 'string.append()', if so, I need to convert int64 to string
explicitly, using stream seems simple.

On 2012/03/02 15:59:50, sky wrote:
> Why not just build up a string?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:153: "favicon_id
INTEGER DEFAULT NULL,"   // favicon id.
No, this is the tricky thing. The client think the favicon is blob, it could
query the all url without favicon by 'WHERE favicon IS NULL', using the NULL
support this query.

Add more comments in header file.

On 2012/03/02 15:59:50, sky wrote:
> Don't you want 0 here?

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:173:
GetDB().CommitTransaction();
On 2012/03/02 15:59:50, sky wrote:
> Document why this is needed.

Done.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:183:
GetDB().BeginTransaction();
FYI, This code was copied from ThumbnailDatabase. I didn't change it because it
looks mysterious.

On 2012/03/02 15:59:50, sky wrote:
> This code is fragile because of having to open the connection before the early
> return. How about moving it to a separate function and having it return an
error
> status. The caller can then be resposible for beginning the transaction after
> calling the method.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
File chrome/browser/history/android/android_cache_database.h (right):

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:21: class
AndroidCacheDatabase {
On 2012/03/02 15:59:50, sky wrote:
> Be sure and document the return value of all methods that aren't void.

Done.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:30: // Add a row in
bookmark_cache table.
On 2012/03/02 15:59:50, sky wrote:
> 'Adds a row to the ...'

Done.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:36: bool
ClearAllBookmarkCacheRow();
On 2012/03/02 15:59:50, sky wrote:
> Either ClearAllBookmarkCacheRows or just ClearBookmarkCache

Done.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:38: // Set the bookmark
column as 1 for the given |url_ids| rows.
On 2012/03/02 15:59:50, sky wrote:
> Set -> Sets. Document return value.

Done.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:39: bool
CacheBookmarkBits(const std::vector<URLID>& url_id);
On 2012/03/02 15:59:50, sky wrote:
> This description doesn't match that I thought the method would do. How about
> MarkURLsAsBookmarked or maybe SetURLsBookmarked

Done.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:41: // Set the favicon
column to |favicon_id| for the given |url_id| row.
On 2012/03/02 15:59:50, sky wrote:
> Set -> Sets

Done.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.h:42: bool
CacheFaviconID(URLID url_id, FaviconID favicon_id);
On 2012/03/02 15:59:50, sky wrote:
> Name this to match your description: SetFaviconID.

Done.

[sky, 2012-03-02 20:51:28]

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
File chrome/browser/history/android/android_cache_database.cc (right):

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:55:
statement.BindInt64(0, ToMilliseconds(created_time));
On 2012/03/02 20:04:43, michaelbai wrote:
> It because the Android use the milliseconds to query, that's the reason I
cache
> the time in this table.
> 
> On 2012/03/02 15:59:50, sky wrote:
> > Why are you using milliseconds instead of InternalValue like the rest of the
> > code?
> 

Can't you convert when necessary? If not, add a comment as to why you use ms
here.

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:183:
GetDB().BeginTransaction();
On 2012/03/02 20:04:43, michaelbai wrote:
> FYI, This code was copied from ThumbnailDatabase. I didn't change it because
it
> looks mysterious.
> 
> On 2012/03/02 15:59:50, sky wrote:
> > This code is fragile because of having to open the connection before the
early
> > return. How about moving it to a separate function and having it return an
> error
> > status. The caller can then be resposible for beginning the transaction
after
> > calling the method.
> 

Now is your chance to fix it;)

[michaelbai, 2012-03-02 21:16:14]

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
File chrome/browser/history/android/android_cache_database.cc (right):

http://codereview.chromium.org/9549031/diff/2001/chrome/browser/history/andro...
chrome/browser/history/android/android_cache_database.cc:55:
statement.BindInt64(0, ToMilliseconds(created_time));
I can't, as the client could just write a SQL as 'last_visit_time > 1234567'
without using sql arguments, event it uses sql arguements, I don't prefer to try
to know which argument is time.

Added the comment in header file's CreateBookmarkCacheTable


On 2012/03/02 20:51:28, sky wrote:
> On 2012/03/02 20:04:43, michaelbai wrote:
> > It because the Android use the milliseconds to query, that's the reason I
> cache
> > the time in this table.
> > 
> > On 2012/03/02 15:59:50, sky wrote:
> > > Why are you using milliseconds instead of InternalValue like the rest of
the
> > > code?
> > 
> 
> Can't you convert when necessary? If not, add a comment as to why you use ms
> here.

[michaelbai, 2012-03-02 21:16:43]

PTAL

[sky, 2012-03-02 22:56:23]

LGTM

[commit-bot: I haz the power, 2012-03-02 23:05:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/9549031/11004

[commit-bot: I haz the power, 2012-03-03 01:34:27]

Change committed as 124811