Fix missing thumbnails for new profile with TopSites.

Fix missing thumbnails for new profile with TopSites.

Store a map of temporary thumbnails for unknown URLs.

Also, remove accesses to thumbnail DB after the migration to TopSites.

TEST=TopSitesTest
BUG=none

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

[Nik Shkrob, 2010-07-13 16:47:45]

Please review.

[brettw, 2010-07-13 17:33:43]

http://codereview.chromium.org/2948008/diff/2001/3003
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/2948008/diff/2001/3003#newcode148
chrome/browser/history/thumbnail_database.cc:148: return true;  // Not needed
after migration to TopSites.
Do you still want to set the version number in this case? I wonder if a better
approach to checking if the thumbnails table exists for each function call would
be to set some local var in the class during init that's based on the version
number. Then each function won't have to compile & execute a separate SQL
statement to see if they should return early.

http://codereview.chromium.org/2948008/diff/2001/3004
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/2948008/diff/2001/3004#newcode93
chrome/browser/history/top_sites.cc:93: if(top_sites_.size() < kTopSitesNumber)
{
Style: Need space after "if".

http://codereview.chromium.org/2948008/diff/2001/3004#newcode332
chrome/browser/history/top_sites.cc:332: if (GetCanonicalURL(mv.url) ==
GetCanonicalURL(it->first)) {
Wait, can't we use temp_thumbnails_map_.find(GetCanonicalURL(mv.url)) to avoid
iterating through every item in the map?

[Nik Shkrob, 2010-07-13 18:12:12]

http://codereview.chromium.org/2948008/diff/2001/3003
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/2948008/diff/2001/3003#newcode148
chrome/browser/history/thumbnail_database.cc:148: return true;  // Not needed
after migration to TopSites.
On 2010/07/13 17:33:43, brettw wrote:
> Do you still want to set the version number in this case? I wonder if a better
> approach to checking if the thumbnails table exists for each function call
would
> be to set some local var in the class during init that's based on the version
> number. Then each function won't have to compile & execute a separate SQL
> statement to see if they should return early.

Done.

http://codereview.chromium.org/2948008/diff/2001/3004
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/2948008/diff/2001/3004#newcode93
chrome/browser/history/top_sites.cc:93: if(top_sites_.size() < kTopSitesNumber)
{
On 2010/07/13 17:33:43, brettw wrote:
> Style: Need space after "if".

Done.

http://codereview.chromium.org/2948008/diff/2001/3004#newcode332
chrome/browser/history/top_sites.cc:332: if (GetCanonicalURL(mv.url) ==
GetCanonicalURL(it->first)) {
On 2010/07/13 17:33:43, brettw wrote:
> Wait, can't we use temp_thumbnails_map_.find(GetCanonicalURL(mv.url)) to avoid
> iterating through every item in the map?

No, because temp_thumbnails_map_ has non-canonical URLs. When we add a temp URL,
we don't have the redirect chain yet.
This is not very efficient, but temp_thumbnails_map_ should contain very few
elements anyways.

[brettw, 2010-07-13 18:19:45]

On Tue, Jul 13, 2010 at 11:12 AM,  <nshkrob@chromium.org> wrote:
>
> http://codereview.chromium.org/2948008/diff/2001/3003
> File chrome/browser/history/thumbnail_database.cc (right):
>
> http://codereview.chromium.org/2948008/diff/2001/3003#newcode148
> chrome/browser/history/thumbnail_database.cc:148: return true;  // Not
> needed after migration to TopSites.
> On 2010/07/13 17:33:43, brettw wrote:
>>
>> Do you still want to set the version number in this case? I wonder if
>
> a better
>>
>> approach to checking if the thumbnails table exists for each function
>
> call would
>>
>> be to set some local var in the class during init that's based on the
>
> version
>>
>> number. Then each function won't have to compile & execute a separate
>
> SQL
>>
>> statement to see if they should return early.
>
> Done.
>
> http://codereview.chromium.org/2948008/diff/2001/3004
> File chrome/browser/history/top_sites.cc (right):
>
> http://codereview.chromium.org/2948008/diff/2001/3004#newcode93
> chrome/browser/history/top_sites.cc:93: if(top_sites_.size() <
> kTopSitesNumber) {
> On 2010/07/13 17:33:43, brettw wrote:
>>
>> Style: Need space after "if".
>
> Done.
>
> http://codereview.chromium.org/2948008/diff/2001/3004#newcode332
> chrome/browser/history/top_sites.cc:332: if (GetCanonicalURL(mv.url) ==
> GetCanonicalURL(it->first)) {
> On 2010/07/13 17:33:43, brettw wrote:
>>
>> Wait, can't we use temp_thumbnails_map_.find(GetCanonicalURL(mv.url))
>
> to avoid
>>
>> iterating through every item in the map?
>
> No, because temp_thumbnails_map_ has non-canonical URLs. When we add a
> temp URL, we don't have the redirect chain yet.
> This is not very efficient, but temp_thumbnails_map_ should contain very
> few elements anyways.

I see. Can you add a comment to this effect. How about also computing
the canonical version of mv.url outside the inner loop so you don't
have to recompute it every time.

LGTM with this.

Brett