GTK: Query TopSites for the Most Visited pages and populate fill in the global History menu.

GTK: Query TopSites for the Most Visited pages and populate fill in the global History menu.

Using TopSites means we'll only get updated on New Tab commands which will kick
off the updating procedure, but this should be much less computation heavy than
rebuilding on every history commit.

BUG=30213
TEST=The Most Visited section on the History menu in the global menu bar is filled with the users TopSites, respecting banned websites, etc.

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

[Elliot Glaysher, 2011-04-28 22:18:53]

estade: history menu stuff
sky: top sites usage and modifications

[sky, 2011-04-28 22:31:43]

http://codereview.chromium.org/6901110/diff/4/chrome/browser/history/top_site...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/6901110/diff/4/chrome/browser/history/top_site...
chrome/browser/history/top_sites.cc:871: ResetThreadSafeImageCache();
We need to set the images after the cache.

[Evan Stade, 2011-04-28 22:38:47]

http://codereview.chromium.org/6901110/diff/4/chrome/browser/ui/gtk/global_hi...
File chrome/browser/ui/gtk/global_history_menu.cc (right):

http://codereview.chromium.org/6901110/diff/4/chrome/browser/ui/gtk/global_hi...
chrome/browser/ui/gtk/global_history_menu.cc:146: if (top_sites_) {
should this be a dcheck?

http://codereview.chromium.org/6901110/diff/4/chrome/browser/ui/gtk/global_hi...
chrome/browser/ui/gtk/global_history_menu.cc:161: for (size_t i = 0; i <
visited_list.size(); ++i) {
I think this list may be as long as 20; do we really want to show that many?

[Evan Stade, 2011-04-28 22:40:21]

http://codereview.chromium.org/6901110/diff/4/chrome/browser/history/top_site...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/6901110/diff/4/chrome/browser/history/top_site...
chrome/browser/history/top_sites.cc:923: NotificationType::TOP_SITES_CHANGED,
p.s. this is something the most visited handler for the NTP should be listening
for, but currently isn't, so I'm glad you added this.

[Elliot Glaysher, 2011-04-28 23:08:35]

TAL?

http://codereview.chromium.org/6901110/diff/4/chrome/browser/history/top_site...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/6901110/diff/4/chrome/browser/history/top_site...
chrome/browser/history/top_sites.cc:871: ResetThreadSafeImageCache();
On 2011/04/28 22:31:43, sky wrote:
> We need to set the images after the cache.

Reworked so sending the notification is its own function called from every place
that calls ResetThreadSafeImageCache() isntead.

http://codereview.chromium.org/6901110/diff/4/chrome/browser/history/top_site...
chrome/browser/history/top_sites.cc:923: NotificationType::TOP_SITES_CHANGED,
On 2011/04/28 22:40:21, Evan Stade wrote:
> p.s. this is something the most visited handler for the NTP should be
listening
> for, but currently isn't, so I'm glad you added this.

sky added this notification earlier today. I was soft blocked on this being
added.

[Evan Stade, 2011-04-28 23:19:35]

http://codereview.chromium.org/6901110/diff/6001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_history_menu.cc (right):

http://codereview.chromium.org/6901110/diff/6001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_history_menu.cc:183: added_count++;
isn't added_count the same as i?

[Elliot Glaysher, 2011-04-28 23:25:13]

http://codereview.chromium.org/6901110/diff/6001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_history_menu.cc (right):

http://codereview.chromium.org/6901110/diff/6001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_history_menu.cc:183: added_count++;
On 2011/04/28 23:19:35, Evan Stade wrote:
> isn't added_count the same as i?

The thought process was: ooh, I do this later on in the file, just copy the
counting logic. Yes and fixed.

[sky, 2011-04-28 23:25:36]

TopSites LGTM

[Evan Stade, 2011-04-28 23:30:42]

lgtm