SQLite DB for TopSites.

Adding an SQLite database for TopSites.

BUG=None
TEST=TopSitesTest


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

[Nik Shkrob, 2010-05-27 22:28:22]

Please review.

[Paweł Hajdan Jr., 2010-05-28 06:44:52]

Drive-by with some test comments. Generally there's no need to wait for another
review by me, the changes are straightforward.

http://codereview.chromium.org/2249004/diff/43001/44005
File chrome/browser/history/top_sites_unittest.cc (right):

http://codereview.chromium.org/2249004/diff/43001/44005#newcode38
chrome/browser/history/top_sites_unittest.cc:38:
PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
Could you check the return value of this? Additionally, for temporary files it
is preferred to use a ScopedTempDir, so that the risk of leaving temporary files
around is minimized.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode40
chrome/browser/history/top_sites_unittest.cc:40: file_util::Delete(file_name_,
false);
Same here.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode53
chrome/browser/history/top_sites_unittest.cc:53: file_util::Delete(file_name_,
false);
And here.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode403
chrome/browser/history/top_sites_unittest.cc:403: if (!db.Init(file_name())) {
Shouldn't this be ASSERT_TRUE instead? Otherwise we risk silently "nulling" the
entire test code after that.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode428
chrome/browser/history/top_sites_unittest.cc:428: EXPECT_EQ(1u, urls.size());
This should be an ASSERT to avoid a crash.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode438
chrome/browser/history/top_sites_unittest.cc:438: EXPECT_EQ(2u, urls.size());
Similarly here.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode450
chrome/browser/history/top_sites_unittest.cc:450: EXPECT_EQ(3u, urls.size());
And here.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode459
chrome/browser/history/top_sites_unittest.cc:459: EXPECT_EQ(3u, urls.size());
And here.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode467
chrome/browser/history/top_sites_unittest.cc:467: EXPECT_EQ(3u, urls.size());
And here.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode477
chrome/browser/history/top_sites_unittest.cc:477: EXPECT_EQ(2u, urls.size());
And here.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode509
chrome/browser/history/top_sites_unittest.cc:509: EXPECT_EQ(1u, result.size());
And here.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode515
chrome/browser/history/top_sites_unittest.cc:515: EXPECT_TRUE(thumbnail_result
!= NULL);
Possibly here.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode533
chrome/browser/history/top_sites_unittest.cc:533: EXPECT_EQ(2u, result.size());
This should be an ASSERT.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode538
chrome/browser/history/top_sites_unittest.cc:538: EXPECT_EQ(3u,
result[0].redirects.size());
This too.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode556
chrome/browser/history/top_sites_unittest.cc:556: EXPECT_EQ(2u, result.size());
And this.

[Nik Shkrob, 2010-05-28 17:44:05]

http://codereview.chromium.org/2249004/diff/43001/44005
File chrome/browser/history/top_sites_unittest.cc (right):

http://codereview.chromium.org/2249004/diff/43001/44005#newcode38
chrome/browser/history/top_sites_unittest.cc:38:
PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
On 2010/05/28 06:44:52, Paweł Hajdan Jr. wrote:
> Could you check the return value of this? Additionally, for temporary files it
> is preferred to use a ScopedTempDir, so that the risk of leaving temporary
files
> around is minimized.

Done - changed to use ScopedTempDir.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode40
chrome/browser/history/top_sites_unittest.cc:40: file_util::Delete(file_name_,
false);
On 2010/05/28 06:44:52, Paweł Hajdan Jr. wrote:
> Same here.

Done.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode53
chrome/browser/history/top_sites_unittest.cc:53: file_util::Delete(file_name_,
false);
On 2010/05/28 06:44:52, Paweł Hajdan Jr. wrote:
> And here.

Done.

http://codereview.chromium.org/2249004/diff/43001/44005#newcode556
chrome/browser/history/top_sites_unittest.cc:556: EXPECT_EQ(2u, result.size());
On 2010/05/28 06:44:52, Paweł Hajdan Jr. wrote:
> And this.

Done all of the above.

[brettw, 2010-05-28 20:19:35]

http://codereview.chromium.org/2249004/diff/47002/52003
File chrome/browser/history/top_sites_database.cc (right):

http://codereview.chromium.org/2249004/diff/47002/52003#newcode35
chrome/browser/history/top_sites_database.cc:35: "title LONGVARCHAR NOT NULL,"
I wouldn't bother with "NOT NULL" here. Some URLs won't have titles. I guess
we'll set empty strings, which I think is different than NULL, but in any case,
I don't think this does anything for us.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode61
chrome/browser/history/top_sites_database.cc:61: int rank =
select_statement.ColumnInt(1);
I'd just add "ORDER BY url_rank" to the query above, and then the results will
come out in order. The intent with the MostVisitedURLList is that it's always in
rank order. Then you can remove the resize() calls below and just do push_back.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode83
chrome/browser/history/top_sites_database.cc:83: for (uint16 i = 0; i <
url.redirects.size(); i++)
Use size_t here (and in all the uint16 places below).

http://codereview.chromium.org/2249004/diff/47002/52003#newcode100
chrome/browser/history/top_sites_database.cc:100: const TopSites::Images
thumbnail) {
Pass this by const ref to avoid a copy.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode119
chrome/browser/history/top_sites_database.cc:119: if (thumbnail.thumbnail.get())
All these conditionals with >1 line inside should have {}

http://codereview.chromium.org/2249004/diff/47002/52003#newcode128
chrome/browser/history/top_sites_database.cc:128: if (prev_rank == -1)
These should have {} since the're more than one line, but there's actually a
much better way to do this.

When you get the old rank, you don't need to get the entire URL list again.
Instead, you can do

"UPDATE thumbnails SET url_rank = url_rank + 1 WHERE url_rank > ?"

Adjust the +1 and > as necessary to shift things the way you want. This saves a
bunch of reading & writing the DB, and I bet you can remove SetURLRank
completely.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode178
chrome/browser/history/top_sites_database.cc:178: int
TopSitesDatabaseImpl::GetURLsRank(const MostVisitedURL& url) {
You probably means "URLs" as possessive, but it looks like plural here. I'd just
call this "GetURLRank".

http://codereview.chromium.org/2249004/diff/47002/52003#newcode212
chrome/browser/history/top_sites_database.cc:212: for (uint16 i = old_rank + 1;
i < urls.size(); i++)
Use size_t here.

http://codereview.chromium.org/2249004/diff/47002/52004
File chrome/browser/history/top_sites_database.h (right):

http://codereview.chromium.org/2249004/diff/47002/52004#newcode8
chrome/browser/history/top_sites_database.h:8: #include <vector>
Style nit: Swap these two lines

http://codereview.chromium.org/2249004/diff/47002/52004#newcode60
chrome/browser/history/top_sites_database.h:60: // When not INIT_OK, no other
functions should be called.
INIT_OK looks like it should be some enum or something, but isn't defined (I
guess you copied this from somewhere else). Just say when it returns false...

http://codereview.chromium.org/2249004/diff/47002/52004#newcode66
chrome/browser/history/top_sites_database.h:66: virtual MostVisitedURLList
GetTopURLs();
Normally we would have blank lines before the comment above each function.

http://codereview.chromium.org/2249004/diff/47002/52004#newcode86
chrome/browser/history/top_sites_database.h:86: int GetURLsRank(const
MostVisitedURL& url);
Ditto with blank lines here.

[Nik Shkrob, 2010-05-28 22:57:56]

http://codereview.chromium.org/2249004/diff/47002/52003
File chrome/browser/history/top_sites_database.cc (right):

http://codereview.chromium.org/2249004/diff/47002/52003#newcode35
chrome/browser/history/top_sites_database.cc:35: "title LONGVARCHAR NOT NULL,"
On 2010/05/28 20:19:35, brettw wrote:
> I wouldn't bother with "NOT NULL" here. Some URLs won't have titles. I guess
> we'll set empty strings, which I think is different than NULL, but in any
case,
> I don't think this does anything for us.

Done.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode61
chrome/browser/history/top_sites_database.cc:61: int rank =
select_statement.ColumnInt(1);
On 2010/05/28 20:19:35, brettw wrote:
> I'd just add "ORDER BY url_rank" to the query above, and then the results will
> come out in order. The intent with the MostVisitedURLList is that it's always
in
> rank order. Then you can remove the resize() calls below and just do
push_back.

Done.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode83
chrome/browser/history/top_sites_database.cc:83: for (uint16 i = 0; i <
url.redirects.size(); i++)
On 2010/05/28 20:19:35, brettw wrote:
> Use size_t here (and in all the uint16 places below).

Done.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode100
chrome/browser/history/top_sites_database.cc:100: const TopSites::Images
thumbnail) {
On 2010/05/28 20:19:35, brettw wrote:
> Pass this by const ref to avoid a copy.

Done.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode119
chrome/browser/history/top_sites_database.cc:119: if (thumbnail.thumbnail.get())
On 2010/05/28 20:19:35, brettw wrote:
> All these conditionals with >1 line inside should have {}

Done.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode128
chrome/browser/history/top_sites_database.cc:128: if (prev_rank == -1)
On 2010/05/28 20:19:35, brettw wrote:
> These should have {} since the're more than one line, but there's actually a
> much better way to do this.
> 
> When you get the old rank, you don't need to get the entire URL list again.
> Instead, you can do
> 
> "UPDATE thumbnails SET url_rank = url_rank + 1 WHERE url_rank > ?"
> 
> Adjust the +1 and > as necessary to shift things the way you want. This saves
a
> bunch of reading & writing the DB, and I bet you can remove SetURLRank
> completely.

Done.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode178
chrome/browser/history/top_sites_database.cc:178: int
TopSitesDatabaseImpl::GetURLsRank(const MostVisitedURL& url) {
On 2010/05/28 20:19:35, brettw wrote:
> You probably means "URLs" as possessive, but it looks like plural here. I'd
just
> call this "GetURLRank".

Done.

http://codereview.chromium.org/2249004/diff/47002/52003#newcode212
chrome/browser/history/top_sites_database.cc:212: for (uint16 i = old_rank + 1;
i < urls.size(); i++)
On 2010/05/28 20:19:35, brettw wrote:
> Use size_t here.

Replaced with an UPDATE statement.

http://codereview.chromium.org/2249004/diff/47002/52004
File chrome/browser/history/top_sites_database.h (right):

http://codereview.chromium.org/2249004/diff/47002/52004#newcode8
chrome/browser/history/top_sites_database.h:8: #include <vector>
On 2010/05/28 20:19:35, brettw wrote:
> Style nit: Swap these two lines

Done.

http://codereview.chromium.org/2249004/diff/47002/52004#newcode60
chrome/browser/history/top_sites_database.h:60: // When not INIT_OK, no other
functions should be called.
On 2010/05/28 20:19:35, brettw wrote:
> INIT_OK looks like it should be some enum or something, but isn't defined (I
> guess you copied this from somewhere else). Just say when it returns false...

Done.

http://codereview.chromium.org/2249004/diff/47002/52004#newcode66
chrome/browser/history/top_sites_database.h:66: virtual MostVisitedURLList
GetTopURLs();
On 2010/05/28 20:19:35, brettw wrote:
> Normally we would have blank lines before the comment above each function.

Done.

http://codereview.chromium.org/2249004/diff/47002/52004#newcode86
chrome/browser/history/top_sites_database.h:86: int GetURLsRank(const
MostVisitedURL& url);
On 2010/05/28 20:19:35, brettw wrote:
> Ditto with blank lines here.

Done.

[brettw, 2010-05-29 04:06:06]

LGTM

http://codereview.chromium.org/2249004/diff/60001/61003
File chrome/browser/history/top_sites_database.cc (right):

http://codereview.chromium.org/2249004/diff/60001/61003#newcode96
chrome/browser/history/top_sites_database.cc:96: // Must be outside the
transaction: we need this value before we can continue.
Actually, this is wrong. Reading first thing inside the transaction will be
exactly the same as outside of it here. I think it actually makes sense for this
to be below the transaction begin since it can help I/O in some cases (it
doesn't need to get a read lock on the file the extra time).