Add heuristics to skip thumbnail generation when it's unnecessary.

Add heuristics to skip thumbnail generation when it's unnecessary.

More specifically, skip thumbnail generation in the following occasions:
- The browser is in the off-the-record mode.
- The URL is not valid for top sites (ex. new tab pages, etc.)
- The existing thumbnail is new and interesting enough.

This patch should reduce the number of thumbnail generations significantly.

BUG=65936
TEST=add unit tests. confirm that the thumbnails are updated as expected.

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

[satorux1, 2011-01-25 01:56:27]

Today, I'm going to use my local chromium build as the primary browser, with
both in-browser and in-render thumbnailing enabled, to compare how much we can
skip unnecessary thumbnail generations by this patch. :)

[satorux1, 2011-01-26 00:32:06]

Friendly ping.

BTW, the result of the experiment was posted to
http://code.google.com/p/chromium/issues/detail?id=65936#c5

[brettw, 2011-01-26 05:55:20]

LGTM, thanks!

http://codereview.chromium.org/6389001/diff/4001/chrome/browser/history/top_s...
File chrome/browser/history/top_sites.h (right):

http://codereview.chromium.org/6389001/diff/4001/chrome/browser/history/top_s...
chrome/browser/history/top_sites.h:153: virtual bool IsKnownURL(const GURL&
url);
Both IsKnownURL and IsFull return false if TopSites isn't loaded yet. This is
probably OK for your use in the thumbnail service, but the comments here should
probably note this behavior (the result may be wrong if called "too early").

http://codereview.chromium.org/6389001/diff/4001/chrome/browser/history/top_s...
chrome/browser/history/top_sites.h:156: // maximu number of top sites).
spelling: maximum

http://codereview.chromium.org/6389001/diff/4001/chrome/common/thumbnail_scor...
File chrome/common/thumbnail_score.cc (right):

http://codereview.chromium.org/6389001/diff/4001/chrome/common/thumbnail_scor...
chrome/common/thumbnail_score.cc:142: return !new_and_interesting_enough;
Can you add a comment here like "We want to generate a new thumbnail when the
old thumbnail is sufficiently old or uninteresting." It took me a minute to
figure out why you're inverting the condition here.

http://codereview.chromium.org/6389001/diff/4001/chrome/common/thumbnail_score.h
File chrome/common/thumbnail_score.h (right):

http://codereview.chromium.org/6389001/diff/4001/chrome/common/thumbnail_scor...
chrome/common/thumbnail_score.h:94: bool ShouldGenerateThumbnail(const
ThumbnailScore& current);
Can you move this to the ThumbnailScore class? There are already some functions
there and it would be nice to avoid more global functions. You may also want to
consider a new name for the new location, since it has a slightly different
meaning being a class member. Perhaps like ShouldBeReplaced or
ShouldConsiderUpdating?

[satorux1, 2011-01-26 08:14:46]

http://codereview.chromium.org/6389001/diff/4001/chrome/browser/history/top_s...
File chrome/browser/history/top_sites.h (right):

http://codereview.chromium.org/6389001/diff/4001/chrome/browser/history/top_s...
chrome/browser/history/top_sites.h:153: virtual bool IsKnownURL(const GURL&
url);
On 2011/01/26 05:55:20, brettw wrote:
> Both IsKnownURL and IsFull return false if TopSites isn't loaded yet. This is
> probably OK for your use in the thumbnail service, but the comments here
should
> probably note this behavior (the result may be wrong if called "too early").

Good idea. Done.

http://codereview.chromium.org/6389001/diff/4001/chrome/browser/history/top_s...
chrome/browser/history/top_sites.h:156: // maximu number of top sites).
On 2011/01/26 05:55:20, brettw wrote:
> spelling: maximum

Done.

http://codereview.chromium.org/6389001/diff/4001/chrome/common/thumbnail_scor...
File chrome/common/thumbnail_score.cc (right):

http://codereview.chromium.org/6389001/diff/4001/chrome/common/thumbnail_scor...
chrome/common/thumbnail_score.cc:142: return !new_and_interesting_enough;
On 2011/01/26 05:55:20, brettw wrote:
> Can you add a comment here like "We want to generate a new thumbnail when the
> old thumbnail is sufficiently old or uninteresting." It took me a minute to
> figure out why you're inverting the condition here.

Good point. Done.

http://codereview.chromium.org/6389001/diff/4001/chrome/common/thumbnail_score.h
File chrome/common/thumbnail_score.h (right):

http://codereview.chromium.org/6389001/diff/4001/chrome/common/thumbnail_scor...
chrome/common/thumbnail_score.h:94: bool ShouldGenerateThumbnail(const
ThumbnailScore& current);
On 2011/01/26 05:55:20, brettw wrote:
> Can you move this to the ThumbnailScore class? There are already some
functions
> there and it would be nice to avoid more global functions. You may also want
to
> consider a new name for the new location, since it has a slightly different
> meaning being a class member. Perhaps like ShouldBeReplaced or
> ShouldConsiderUpdating?

Agreed. Changed accordingly. Chose ShouldConsiderUpdating.

[satorux1, 2011-01-26 10:53:59]

Turned out that the unit test for ThumbnailGenerator::ShouldUpdateThumbnail()
crashed in TopSitesBackend::~TopSitesBackend() with Debug build.

Hence I had to move instantiation of TopSitesBackend a little bit. Please take
another look at top_sites.cc.

http://codereview.chromium.org/6389001/diff/16001/chrome/browser/history/top_...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/6389001/diff/16001/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:157: backend_ = new TopSitesBackend;
Moved instantiation here.

[brettw, 2011-01-26 16:40:32]

LGTM

http://codereview.chromium.org/6389001/diff/16001/chrome/browser/history/top_...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/6389001/diff/16001/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:157: backend_ = new TopSitesBackend;
Can you add a comment why it's here rather than in the constructor?

[satorux1, 2011-01-27 02:02:11]

http://codereview.chromium.org/6389001/diff/16001/chrome/browser/history/top_...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/6389001/diff/16001/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:157: backend_ = new TopSitesBackend;
On 2011/01/26 16:40:32, brettw wrote:
> Can you add a comment why it's here rather than in the constructor?

Good point. Added a comment.