Adding support for forced URLs to TopSites.

Adding support for forced URLs to TopSites.

This CL makes it possible to add MostVisitedURLs to TopSites that have a non-null |last_forced_time| indicating that these URLs are "forced" and should stay in top-sites even as other non-forced URLs come in. These forced URLs are evicted on a Least Recently Used basis based on |last_forced_time|. Internally, the list of Forced URLs is kept at the beginning of the TopSitesCache list of MostVisitedURLs. These forced URLs are not returned by GetMostVisitedURLs.

Forced URLs make it possible for Chrome to capture thumbnails of sites that may have been suggested through other processes than the TopSitesBackend. They are not used yet but a soon-to-follow CL will link them to thumbnails requested via the chrome-search://thumb2/...

TBR'ing owners for minor API change.

BUG=
TBR=bulach@chromium.org,sky@chromium.org,darin@chromium.org,benwells@chromium.org,estade@chromium.org,

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234394

[beaudoin, 2013-10-31 19:20:23]

Hi Brett,

This is the first follow-up CL to my recent change in the TopSites database
which you reviewed [1]. I'm working on adding unit tests for TopSitesCache but I
think you can start reviewing it now. I'd like your input on my approach.

[1] https://codereview.chromium.org/39053005/

Thanks!

   Philippe

[beaudoin, 2013-10-31 20:35:40]

Added the TopSitesCache unit test, this CL is now fully ready for review I
believe.

Thanks in advance Brett!

[beaudoin, 2013-11-06 14:46:41]

On 2013/10/31 20:35:40, beaudoin wrote:
> Added the TopSitesCache unit test, this CL is now fully ready for review I
> believe.
> 
> Thanks in advance Brett!

Hey Brett,

Friendly ping on this...

Thanks!

[brettw, 2013-11-08 22:56:07]

A few minor comments, I'm checking the logic now.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites.h (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites.h:74: // non-forced URLs. his may be invoked on
any thread.
Spelling "his"

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites.h:75: virtual void GetAllMostVisitedURLs(
I think that the new parameter should just be added to the existing function
rather than adding this new one. I think there are only a couple (one?)
caller(s) to update.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites_cache.cc (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_cache.cc:161: nb_forced_urls_ = 0;
As above, I'd use "num" instead. I've never seen anybody use "Nb"

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_cache.cc:168: // In debug, ensure the cache
user is has no forced URLs pass that point.
Grammar: "is has"

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites_cache.h (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_cache.h:91: size_t GetNbNonForcedURLs() const;
"Nb"?

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:63: include_forced_urls ? &all_urls :
&nonforced_urls;
Indent 2 more spaces.

[brettw, 2013-11-08 23:14:58]

LGTM

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:440: int oldRank = found->second >=
nb_old_forced ?
Style: oldRank -> old_rank

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:664: // Drop older forced URLs if the
list overflows.
It might be worth mentioning somewhere (either here or earlier in this function)
that the forced URLs are sorted with the oldest ones first.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:673: 
Style nit: only one blank line here

[beaudoin, 2013-11-11 21:58:08]

Hi Brett,

Answered your comments save for the one about possibly renaming the function.
I'm sending this to CQ but I'd be happy to implement the function name change in
my next CL if you think it's worth it. (I just didn't want to force this CL on
10 owners. :))

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites.h (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites.h:74: // non-forced URLs. his may be invoked on
any thread.
On 2013/11/08 22:56:07, brettw wrote:
> Spelling "his"

Done.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites.h:75: virtual void GetAllMostVisitedURLs(
On 2013/11/08 22:56:07, brettw wrote:
> I think that the new parameter should just be added to the existing function
> rather than adding this new one. I think there are only a couple (one?)
> caller(s) to update.

That's what I did first but there are surprisingly many callers to
GetMostVisitedURLs (~8 non-test calls). The parameter was a bit opaque too...

Another option would be to have a GetAllMostVisitedURLs that doesn't take a bool
as param, both of them piping into a private GetMostVisitedImpl(callback, bool).

What do you thing?

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites_cache.cc (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_cache.cc:161: nb_forced_urls_ = 0;
On 2013/11/08 22:56:07, brettw wrote:
> As above, I'd use "num" instead. I've never seen anybody use "Nb"

Probably a French thing. Sorry. :)

Done.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_cache.cc:168: // In debug, ensure the cache
user is has no forced URLs pass that point.
On 2013/11/08 22:56:07, brettw wrote:
> Grammar: "is has"

Done.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites_cache.h (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_cache.h:91: size_t GetNbNonForcedURLs() const;
On 2013/11/08 22:56:07, brettw wrote:
> "Nb"?

Done.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:63: include_forced_urls ? &all_urls :
&nonforced_urls;
On 2013/11/08 22:56:07, brettw wrote:
> Indent 2 more spaces.

Done.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:440: int oldRank = found->second >=
nb_old_forced ?
On 2013/11/08 23:14:58, brettw wrote:
> Style: oldRank -> old_rank

Sorry! :|

Done.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:664: // Drop older forced URLs if the
list overflows.
On 2013/11/08 23:14:58, brettw wrote:
> It might be worth mentioning somewhere (either here or earlier in this
function)
> that the forced URLs are sorted with the oldest ones first.

Done.

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:673: 
On 2013/11/08 23:14:58, brettw wrote:
> Style nit: only one blank line here

Done.

[commit-bot: I haz the power, 2013-11-11 21:59:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/53283004/470001

[brettw, 2013-11-11 22:03:30]

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
File chrome/browser/history/top_sites.h (right):

https://codereview.chromium.org/53283004/diff/390001/chrome/browser/history/t...
chrome/browser/history/top_sites.h:75: virtual void GetAllMostVisitedURLs(
8 doesn't sound like many to me. For simple renames you can TBR owners of
calling code. So I'd like to change this in a followup.

[beaudoin1, 2013-11-11 22:05:19]

You know what, I'll do it right away and TBR.


On Mon, Nov 11, 2013 at 5:03 PM, <brettw@chromium.org> wrote:

>
> https://codereview.chromium.org/53283004/diff/390001/
> chrome/browser/history/top_sites.h
> File chrome/browser/history/top_sites.h (right):
>
> https://codereview.chromium.org/53283004/diff/390001/
> chrome/browser/history/top_sites.h#newcode75
> chrome/browser/history/top_sites.h:75: virtual void
> GetAllMostVisitedURLs(
> 8 doesn't sound like many to me. For simple renames you can TBR owners
> of calling code. So I'd like to change this in a followup.
>
> https://codereview.chromium.org/53283004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[beaudoin, 2013-11-11 22:28:37]

TBRing owners for a trivial API change to TopSites::GetMostVisited(...)
recommended by brettw@

[beaudoin, 2013-11-11 22:29:49]

Typo to darin's email.

[darin (slow to review), 2013-11-11 22:32:19]

OK, LGTM

[commit-bot: I haz the power, 2013-11-11 22:36:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/53283004/550001

[commit-bot: I haz the power, 2013-11-12 00:51:47]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-12 01:10:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/53283004/550001

[commit-bot: I haz the power, 2013-11-12 03:50:15]

Change committed as 234394

[bulach, 2013-11-14 13:21:13]

lgtm for android/