Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(463)

Issue 2746002: 1. Create and use the TopSites database file.... (Closed)

Created:
10 years, 6 months ago by Nik Shkrob
Modified:
9 years, 7 months ago
Reviewers:
brettw
CC:
chromium-reviews, brettw-cc_chromium.org, ben+cc_chromium.org
Visibility:
Public.

Description

1. Create and use the TopSites database file. 2. Timed updates of the database based on the number of sites changed. BUG=None TEST=TopSitesTest Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49294

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Total comments: 1

Patch Set 7 : '' #

Total comments: 6

Patch Set 8 : '' #

Patch Set 9 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+148 lines, -16 lines) Patch
M chrome/browser/history/top_sites.h View 1 2 3 4 5 6 7 6 chunks +21 lines, -2 lines 0 comments Download
M chrome/browser/history/top_sites.cc View 1 2 3 4 5 6 7 6 chunks +57 lines, -7 lines 0 comments Download
M chrome/browser/history/top_sites_database.h View 1 2 3 4 5 6 7 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/history/top_sites_database.cc View 1 2 2 chunks +1 line, -2 lines 0 comments Download
M chrome/browser/history/top_sites_unittest.cc View 4 5 6 7 5 chunks +61 lines, -0 lines 0 comments Download
M chrome/browser/profile.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/chrome_constants.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_constants.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Nik Shkrob
http://codereview.chromium.org/2746002/diff/40001/4005 File chrome/browser/history/top_sites_unittest.cc (right): http://codereview.chromium.org/2746002/diff/40001/4005#newcode645 chrome/browser/history/top_sites_unittest.cc:645: (const NotificationDetails&)details); I couldn't find an easy way to ...
10 years, 6 months ago (2010-06-08 18:24:56 UTC) #1
Nik Shkrob
Uploaded a new patch - should fix compile issues on win and runtime errors for ...
10 years, 6 months ago (2010-06-08 20:50:41 UTC) #2
brettw
10 years, 6 months ago (2010-06-08 23:22:42 UTC) #3
LGTM, just some style nits.

http://codereview.chromium.org/2746002/diff/27011/13002
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/2746002/diff/27011/13002#newcode5
chrome/browser/history/top_sites.cc:5: #include <algorithm>
This should go after top_sites.h (the corresponding h for the cc file should
always be first).

http://codereview.chromium.org/2746002/diff/27011/13002#newcode335
chrome/browser/history/top_sites.cc:335: // Delete all
This comment probably isn't needed.

http://codereview.chromium.org/2746002/diff/27011/13002#newcode338
chrome/browser/history/top_sites.cc:338: file_util::Delete(db_path_, false);
Note that when you do threading, you'll need to put this on the background
thread. Can you add a TODO for now?

http://codereview.chromium.org/2746002/diff/27011/13003
File chrome/browser/history/top_sites.h (right):

http://codereview.chromium.org/2746002/diff/27011/13003#newcode195
chrome/browser/history/top_sites.h:195: size_t num_urls_changed;
Should have an underscore at the end of this. It would make more sense to me if
you called this "last_num_urls_changed_" instead

http://codereview.chromium.org/2746002/diff/27011/13004
File chrome/browser/history/top_sites_database.h (right):

http://codereview.chromium.org/2746002/diff/27011/13004#newcode57
chrome/browser/history/top_sites_database.h:57: class TopSitesDatabaseImpl:
public TopSitesDatabase {
Can you put an extra space before the colon?

http://codereview.chromium.org/2746002/diff/27011/13007
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/2746002/diff/27011/13007#newcode102
chrome/common/chrome_constants.cc:102: const FilePath::CharType
kTopSitesFilename[] = FPL("TopSites");
I'd put a space in here like we do for the rest of the filenames.

Powered by Google App Engine
This is Rietveld 408576698