New pinning algorithm.

Fix the pinning algorithm so that pinned URLs are not shifted during insertions and deletions.

BUG=none
TEST=TopSitesTest

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

[arv (Not doing code reviews), 2010-07-26 22:12:30]

http://codereview.chromium.org/3039029/diff/7001/8001
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/3039029/diff/7001/8001#newcode386
chrome/browser/history/top_sites.cc:386: int cur_index = IndexOf(urls_copy,
url);
I don't think I understand this. Isn't urls_copy the sites that the user has
visited most? There is no guarantee that the user visits his pinned sites often
enough for them to show up in the most visited sites.

http://codereview.chromium.org/3039029/diff/7001/8001#newcode391
chrome/browser/history/top_sites.cc:391: urls_copy.erase(urls_copy.begin() +
cur_index);
Do you really need to use begin here?

[Nik Shkrob, 2010-07-27 00:03:18]

http://codereview.chromium.org/3039029/diff/7001/8001
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/3039029/diff/7001/8001#newcode386
chrome/browser/history/top_sites.cc:386: int cur_index = IndexOf(urls_copy,
url);
On 2010/07/26 22:12:30, arv wrote:
> I don't think I understand this. Isn't urls_copy the sites that the user has
> visited most? There is no guarantee that the user visits his pinned sites
often
> enough for them to show up in the most visited sites.

Done - changed to handle this case as well as possible.
This should never happen though, since the pinned URLs are stored in top_sites_.
However, I could simulate this case by pinning a URL and clearing history. This
still should not happen, because we need to clear pinned URLs when they are
removed from history - will do another CL for that.

http://codereview.chromium.org/3039029/diff/7001/8001#newcode391
chrome/browser/history/top_sites.cc:391: urls_copy.erase(urls_copy.begin() +
cur_index);
On 2010/07/26 22:12:30, arv wrote:
> Do you really need to use begin here?
Yes, vector.erase takes an iterator, and this is the simplest way to get one.

[arv (Not doing code reviews), 2010-07-27 16:52:53]

lgtm